x86/svm: Provide EXITINFO decodes for MOV CR/DR intercepts

[PATCH] x86/svm: Provide EXITINFO decodes for MOV CR/DR intercepts

Posted by Andrew Cooper 1 year ago

This removes raw number manipulation, and makes the logic easier to follow.

No functional change.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Roger Pau Monné <roger.pau@citrix.com>
CC: Wei Liu <wl@xen.org>
---
 xen/arch/x86/hvm/svm/svm.c              | 4 ++--
 xen/arch/x86/include/asm/hvm/svm/vmcb.h | 5 +++++
 2 files changed, 7 insertions(+), 2 deletions(-)

diff --git a/xen/arch/x86/hvm/svm/svm.c b/xen/arch/x86/hvm/svm/svm.c
index 8d8b250101ce..90c2f89c1b0d 100644
--- a/xen/arch/x86/hvm/svm/svm.c
+++ b/xen/arch/x86/hvm/svm/svm.c
@@ -1740,7 +1740,7 @@ static void svm_vmexit_do_cr_access(
     cr = vmcb->exitcode - VMEXIT_CR0_READ;
     dir = (cr > 15);
     cr &= 0xf;
-    gp = vmcb->exitinfo1 & 0xf;
+    gp = vmcb->ei.mov.gpr;
 
     rc = dir ? hvm_mov_to_cr(cr, gp) : hvm_mov_from_cr(cr, gp);
 
@@ -2961,7 +2961,7 @@ void svm_vmexit_handler(void)
 
     case VMEXIT_CR0_READ ... VMEXIT_CR15_READ:
     case VMEXIT_CR0_WRITE ... VMEXIT_CR15_WRITE:
-        if ( cpu_has_svm_decode && (vmcb->exitinfo1 & (1ULL << 63)) )
+        if ( cpu_has_svm_decode && vmcb->ei.mov.mov_insn )
             svm_vmexit_do_cr_access(vmcb, regs);
         else if ( !hvm_emulate_one_insn(x86_insn_is_cr_access, "CR access") )
             hvm_inject_hw_exception(X86_EXC_GP, 0);
diff --git a/xen/arch/x86/include/asm/hvm/svm/vmcb.h b/xen/arch/x86/include/asm/hvm/svm/vmcb.h
index b809e85507aa..77e3bd9aa048 100644
--- a/xen/arch/x86/include/asm/hvm/svm/vmcb.h
+++ b/xen/arch/x86/include/asm/hvm/svm/vmcb.h
@@ -450,6 +450,11 @@ struct vmcb_struct {
 
                 uint64_t nrip;
             } io;
+            struct {
+                uint64_t gpr:4;
+                uint64_t :59;
+                bool     mov_insn:1; /* MOV, as opposed to LMSW, CLTS, etc */
+            } mov;
             struct {
                 uint16_t sel;
                 uint64_t :48;
-- 
2.30.2

Re: [PATCH] x86/svm: Provide EXITINFO decodes for MOV CR/DR intercepts

Posted by Jan Beulich 1 year ago

On 05.04.2023 22:44, Andrew Cooper wrote:
> This removes raw number manipulation, and makes the logic easier to follow.
> 
> No functional change.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Reviewed-by: Jan Beulich <jbeulich@suse.com>

One remark though:

> --- a/xen/arch/x86/include/asm/hvm/svm/vmcb.h
> +++ b/xen/arch/x86/include/asm/hvm/svm/vmcb.h
> @@ -450,6 +450,11 @@ struct vmcb_struct {
>  
>                  uint64_t nrip;
>              } io;
> +            struct {
> +                uint64_t gpr:4;
> +                uint64_t :59;
> +                bool     mov_insn:1; /* MOV, as opposed to LMSW, CLTS, etc */
> +            } mov;

The field being named just "mov" makes it apparently applicable to DRn
moves, too (and the title supports this), yet the top bit doesn't have
this meaning there. So perhaps say "MOV-CR" (or alike) in the comment?

Jan

Re: [PATCH] x86/svm: Provide EXITINFO decodes for MOV CR/DR intercepts

Posted by Andrew Cooper 1 year ago

On 06/04/2023 10:31 am, Jan Beulich wrote:
> On 05.04.2023 22:44, Andrew Cooper wrote:
>> This removes raw number manipulation, and makes the logic easier to follow.
>>
>> No functional change.
>>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> Reviewed-by: Jan Beulich <jbeulich@suse.com>

Thanks.

>
> One remark though:
>
>> --- a/xen/arch/x86/include/asm/hvm/svm/vmcb.h
>> +++ b/xen/arch/x86/include/asm/hvm/svm/vmcb.h
>> @@ -450,6 +450,11 @@ struct vmcb_struct {
>>  
>>                  uint64_t nrip;
>>              } io;
>> +            struct {
>> +                uint64_t gpr:4;
>> +                uint64_t :59;
>> +                bool     mov_insn:1; /* MOV, as opposed to LMSW, CLTS, etc */
>> +            } mov;
> The field being named just "mov" makes it apparently applicable to DRn
> moves, too (and the title supports this), yet the top bit doesn't have
> this meaning there. So perhaps say "MOV-CR" (or alike) in the comment?

Hmm - I'd not spotted that distinction.

Xen never decodes the exitinfo for a DR access - we just resync dr
state, drop the intercept and reenter the guest.

Therefore I think it would be better to rename "mov" to "mov_cr" so you
can't use the mov_insn field in a context that plausibly looks like a dr
access.

Thoughts?

~Andrew

Re: [PATCH] x86/svm: Provide EXITINFO decodes for MOV CR/DR intercepts

Posted by Jan Beulich 1 year ago

On 06.04.2023 11:52, Andrew Cooper wrote:
> On 06/04/2023 10:31 am, Jan Beulich wrote:
>> On 05.04.2023 22:44, Andrew Cooper wrote:
>>> --- a/xen/arch/x86/include/asm/hvm/svm/vmcb.h
>>> +++ b/xen/arch/x86/include/asm/hvm/svm/vmcb.h
>>> @@ -450,6 +450,11 @@ struct vmcb_struct {
>>>  
>>>                  uint64_t nrip;
>>>              } io;
>>> +            struct {
>>> +                uint64_t gpr:4;
>>> +                uint64_t :59;
>>> +                bool     mov_insn:1; /* MOV, as opposed to LMSW, CLTS, etc */
>>> +            } mov;
>> The field being named just "mov" makes it apparently applicable to DRn
>> moves, too (and the title supports this), yet the top bit doesn't have
>> this meaning there. So perhaps say "MOV-CR" (or alike) in the comment?
> 
> Hmm - I'd not spotted that distinction.
> 
> Xen never decodes the exitinfo for a DR access - we just resync dr
> state, drop the intercept and reenter the guest.
> 
> Therefore I think it would be better to rename "mov" to "mov_cr" so you
> can't use the mov_insn field in a context that plausibly looks like a dr
> access.
> 
> Thoughts?

That was my other thought; it merely seemed to me that updating the comment
would allow future (if ever) use with MOV-DR. So yes, if you prefer going
that route: Fine with me.

Jan

Re: [PATCH] x86/svm: Provide EXITINFO decodes for MOV CR/DR intercepts

Posted by Andrew Cooper 1 year ago

On 06/04/2023 10:59 am, Jan Beulich wrote:
> On 06.04.2023 11:52, Andrew Cooper wrote:
>> On 06/04/2023 10:31 am, Jan Beulich wrote:
>>> On 05.04.2023 22:44, Andrew Cooper wrote:
>>>> --- a/xen/arch/x86/include/asm/hvm/svm/vmcb.h
>>>> +++ b/xen/arch/x86/include/asm/hvm/svm/vmcb.h
>>>> @@ -450,6 +450,11 @@ struct vmcb_struct {
>>>>  
>>>>                  uint64_t nrip;
>>>>              } io;
>>>> +            struct {
>>>> +                uint64_t gpr:4;
>>>> +                uint64_t :59;
>>>> +                bool     mov_insn:1; /* MOV, as opposed to LMSW, CLTS, etc */
>>>> +            } mov;
>>> The field being named just "mov" makes it apparently applicable to DRn
>>> moves, too (and the title supports this), yet the top bit doesn't have
>>> this meaning there. So perhaps say "MOV-CR" (or alike) in the comment?
>> Hmm - I'd not spotted that distinction.
>>
>> Xen never decodes the exitinfo for a DR access - we just resync dr
>> state, drop the intercept and reenter the guest.
>>
>> Therefore I think it would be better to rename "mov" to "mov_cr" so you
>> can't use the mov_insn field in a context that plausibly looks like a dr
>> access.
>>
>> Thoughts?
> That was my other thought; it merely seemed to me that updating the comment
> would allow future (if ever) use with MOV-DR. So yes, if you prefer going
> that route: Fine with me.

Will do.  I don't foresee us ever needing to inspect the dr decode
information.

~Andrew