[4/4] x86/svm: Use named (bit)fields for task switch exit info
diff mbox series

Message ID 20191204094335.24603-5-andrew.cooper3@citrix.com
State New
Headers show
Series
  • x86/svm: (Post TASK_SWITCH) cleanup
Related show

Commit Message

Andrew Cooper Dec. 4, 2019, 9:43 a.m. UTC
Introduce vmcb.e1.* and vmcb.e2.* to provide names to fields in exitinfo{1,2}
respectively.  Implement the task switch names for now, and clean up the
TASK_SWITCH handler.

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

Comments

Jan Beulich Dec. 4, 2019, 10:24 a.m. UTC | #1
On 04.12.2019 10:43, Andrew Cooper wrote:
> Introduce vmcb.e1.* and vmcb.e2.* to provide names to fields in exitinfo{1,2}
> respectively.  Implement the task switch names for now, and clean up the
> TASK_SWITCH handler.

"e1" and "e2" look overly short - and hence possibly ambiguous -
to me. Make them perhaps "ei1" and "ei2"? Furthermore, seeing ...

> @@ -2795,19 +2792,12 @@ void svm_vmexit_handler(struct cpu_user_regs *regs)
>          if ( insn_len < 0 && (insn_len = svm_get_task_switch_insn_len()) == 0 )
>              goto crash_or_fault;
>  
> -        if ( (vmcb->exitinfo2 >> 36) & 1 )
> -            reason = TSW_iret;
> -        else if ( (vmcb->exitinfo2 >> 38) & 1 )
> -            reason = TSW_jmp;
> -        else
> -            reason = TSW_call_or_int;
> -        if ( (vmcb->exitinfo2 >> 44) & 1 )
> -            errcode = (uint32_t)vmcb->exitinfo2;
> -
> -        hvm_task_switch(vmcb->exitinfo1, reason, errcode, insn_len,
> -                        (vmcb->exitinfo2 & (1ul << 48)) ? X86_EFLAGS_RF : 0);
> +        hvm_task_switch(vmcb->e1.task_switch.sel,
> +                        vmcb->e2.task_switch.iret ? TSW_iret :
> +                        vmcb->e2.task_switch.jmp  ? TSW_jmp : TSW_call_or_int,
> +                        vmcb->e2.task_switch.ev ? vmcb->e2.task_switch.ec : -1,
> +                        insn_len, vmcb->e2.task_switch.rf ? X86_EFLAGS_RF : 0);

... this, wouldn't it make sense to simply have "ei" covering both
parts, no longer making it a requirement to use (and hence look up)
the numeric suffixes at use sites?

> --- a/xen/include/asm-x86/hvm/svm/vmcb.h
> +++ b/xen/include/asm-x86/hvm/svm/vmcb.h
> @@ -418,8 +418,30 @@ struct vmcb_struct {
>      vintr_t _vintr;             /* offset 0x60 - cleanbit 3 */
>      u64 interrupt_shadow;       /* offset 0x68 */
>      u64 exitcode;               /* offset 0x70 */
> -    u64 exitinfo1;              /* offset 0x78 */
> -    u64 exitinfo2;              /* offset 0x80 */
> +    union {
> +        u64 exitinfo1;          /* offset 0x78 */

uint64_t (also below)?

Jan
Andrew Cooper Dec. 4, 2019, 8:04 p.m. UTC | #2
On 04/12/2019 10:24, Jan Beulich wrote:
> On 04.12.2019 10:43, Andrew Cooper wrote:
>> Introduce vmcb.e1.* and vmcb.e2.* to provide names to fields in exitinfo{1,2}
>> respectively.  Implement the task switch names for now, and clean up the
>> TASK_SWITCH handler.
> "e1" and "e2" look overly short - and hence possibly ambiguous -
> to me. Make them perhaps "ei1" and "ei2"?

Written on their own like that perhaps, but the ei[12] versions are
equally ambiguous.

However, they are only ever used with vmcb-> in code, so there is no
issue with ambiguity.

>  Furthermore, seeing ...
>
>> @@ -2795,19 +2792,12 @@ void svm_vmexit_handler(struct cpu_user_regs *regs)
>>          if ( insn_len < 0 && (insn_len = svm_get_task_switch_insn_len()) == 0 )
>>              goto crash_or_fault;
>>  
>> -        if ( (vmcb->exitinfo2 >> 36) & 1 )
>> -            reason = TSW_iret;
>> -        else if ( (vmcb->exitinfo2 >> 38) & 1 )
>> -            reason = TSW_jmp;
>> -        else
>> -            reason = TSW_call_or_int;
>> -        if ( (vmcb->exitinfo2 >> 44) & 1 )
>> -            errcode = (uint32_t)vmcb->exitinfo2;
>> -
>> -        hvm_task_switch(vmcb->exitinfo1, reason, errcode, insn_len,
>> -                        (vmcb->exitinfo2 & (1ul << 48)) ? X86_EFLAGS_RF : 0);
>> +        hvm_task_switch(vmcb->e1.task_switch.sel,
>> +                        vmcb->e2.task_switch.iret ? TSW_iret :
>> +                        vmcb->e2.task_switch.jmp  ? TSW_jmp : TSW_call_or_int,
>> +                        vmcb->e2.task_switch.ev ? vmcb->e2.task_switch.ec : -1,
>> +                        insn_len, vmcb->e2.task_switch.rf ? X86_EFLAGS_RF : 0);
> ... this, wouldn't it make sense to simply have "ei" covering both
> parts, no longer making it a requirement to use (and hence look up)
> the numeric suffixes at use sites?

Net delta is:

diff --git a/xen/include/asm-x86/hvm/svm/vmcb.h
b/xen/include/asm-x86/hvm/svm/vmcb.h
index 02b5e86b49..864618ccf9 100644
--- a/xen/include/asm-x86/hvm/svm/vmcb.h
+++ b/xen/include/asm-x86/hvm/svm/vmcb.h
@@ -419,17 +419,15 @@ struct vmcb_struct {
     u64 interrupt_shadow;       /* offset 0x68 */
     u64 exitcode;               /* offset 0x70 */
     union {
-        u64 exitinfo1;          /* offset 0x78 */
+        struct {
+            uint64_t exitinfo1; /* offset 0x78 */
+            uint64_t exitinfo2; /* offset 0x80 */
+        };
         union {
             struct {
                 uint16_t sel;
-            } task_switch;
-        } e1;
-    };
-    union {
-        u64 exitinfo2;          /* offset 0x80 */
-        union {
-            struct {
+                uint64_t :48;
+
                 uint32_t ec;
                 uint32_t :4;
                 bool     iret:1;
@@ -440,7 +438,7 @@ struct vmcb_struct {
                 uint32_t :3;
                 bool     rf:1;
             } task_switch;
-        } e2;
+        } ei;
     };
     intinfo_t exitintinfo;      /* offset 0x88 */
     u64 _np_enable;             /* offset 0x90 - cleanbit 4 */

LGTM.

~Andrew
Jan Beulich Dec. 5, 2019, 9:05 a.m. UTC | #3
On 04.12.2019 21:04, Andrew Cooper wrote:
> Net delta is:
> 
> diff --git a/xen/include/asm-x86/hvm/svm/vmcb.h
> b/xen/include/asm-x86/hvm/svm/vmcb.h
> index 02b5e86b49..864618ccf9 100644
> --- a/xen/include/asm-x86/hvm/svm/vmcb.h
> +++ b/xen/include/asm-x86/hvm/svm/vmcb.h
> @@ -419,17 +419,15 @@ struct vmcb_struct {
>      u64 interrupt_shadow;       /* offset 0x68 */
>      u64 exitcode;               /* offset 0x70 */
>      union {
> -        u64 exitinfo1;          /* offset 0x78 */
> +        struct {
> +            uint64_t exitinfo1; /* offset 0x78 */
> +            uint64_t exitinfo2; /* offset 0x80 */
> +        };
>          union {
>              struct {
>                  uint16_t sel;
> -            } task_switch;
> -        } e1;
> -    };
> -    union {
> -        u64 exitinfo2;          /* offset 0x80 */
> -        union {
> -            struct {
> +                uint64_t :48;
> +
>                  uint32_t ec;
>                  uint32_t :4;
>                  bool     iret:1;
> @@ -440,7 +438,7 @@ struct vmcb_struct {
>                  uint32_t :3;
>                  bool     rf:1;
>              } task_switch;
> -        } e2;
> +        } ei;
>      };
>      intinfo_t exitintinfo;      /* offset 0x88 */
>      u64 _np_enable;             /* offset 0x90 - cleanbit 4 */
> 
> LGTM.

And the result then
Reviewed-by: Jan Beulich <jbeulich@suse.com>

Jan

Patch
diff mbox series

diff --git a/xen/arch/x86/hvm/svm/svm.c b/xen/arch/x86/hvm/svm/svm.c
index 263ae03bfd..6c68bcee59 100644
--- a/xen/arch/x86/hvm/svm/svm.c
+++ b/xen/arch/x86/hvm/svm/svm.c
@@ -2746,10 +2746,7 @@  void svm_vmexit_handler(struct cpu_user_regs *regs)
         svm_vmexit_do_invalidate_cache(regs, exit_reason == VMEXIT_INVD);
         break;
 
-    case VMEXIT_TASK_SWITCH: {
-        enum hvm_task_switch_reason reason;
-        int32_t errcode = -1;
-
+    case VMEXIT_TASK_SWITCH:
         /*
          * All TASK_SWITCH intercepts have fault-like semantics.  NRIP is
          * never provided, even for instruction-induced task switches, but we
@@ -2795,19 +2792,12 @@  void svm_vmexit_handler(struct cpu_user_regs *regs)
         if ( insn_len < 0 && (insn_len = svm_get_task_switch_insn_len()) == 0 )
             goto crash_or_fault;
 
-        if ( (vmcb->exitinfo2 >> 36) & 1 )
-            reason = TSW_iret;
-        else if ( (vmcb->exitinfo2 >> 38) & 1 )
-            reason = TSW_jmp;
-        else
-            reason = TSW_call_or_int;
-        if ( (vmcb->exitinfo2 >> 44) & 1 )
-            errcode = (uint32_t)vmcb->exitinfo2;
-
-        hvm_task_switch(vmcb->exitinfo1, reason, errcode, insn_len,
-                        (vmcb->exitinfo2 & (1ul << 48)) ? X86_EFLAGS_RF : 0);
+        hvm_task_switch(vmcb->e1.task_switch.sel,
+                        vmcb->e2.task_switch.iret ? TSW_iret :
+                        vmcb->e2.task_switch.jmp  ? TSW_jmp : TSW_call_or_int,
+                        vmcb->e2.task_switch.ev ? vmcb->e2.task_switch.ec : -1,
+                        insn_len, vmcb->e2.task_switch.rf ? X86_EFLAGS_RF : 0);
         break;
-    }
 
     case VMEXIT_CPUID:
         if ( (insn_len = svm_get_insn_len(v, INSTR_CPUID)) == 0 )
diff --git a/xen/include/asm-x86/hvm/svm/vmcb.h b/xen/include/asm-x86/hvm/svm/vmcb.h
index fc67a88660..02b5e86b49 100644
--- a/xen/include/asm-x86/hvm/svm/vmcb.h
+++ b/xen/include/asm-x86/hvm/svm/vmcb.h
@@ -418,8 +418,30 @@  struct vmcb_struct {
     vintr_t _vintr;             /* offset 0x60 - cleanbit 3 */
     u64 interrupt_shadow;       /* offset 0x68 */
     u64 exitcode;               /* offset 0x70 */
-    u64 exitinfo1;              /* offset 0x78 */
-    u64 exitinfo2;              /* offset 0x80 */
+    union {
+        u64 exitinfo1;          /* offset 0x78 */
+        union {
+            struct {
+                uint16_t sel;
+            } task_switch;
+        } e1;
+    };
+    union {
+        u64 exitinfo2;          /* offset 0x80 */
+        union {
+            struct {
+                uint32_t ec;
+                uint32_t :4;
+                bool     iret:1;
+                uint32_t :1;
+                bool     jmp:1;
+                uint32_t :5;
+                bool     ev:1;
+                uint32_t :3;
+                bool     rf:1;
+            } task_switch;
+        } e2;
+    };
     intinfo_t exitintinfo;      /* offset 0x88 */
     u64 _np_enable;             /* offset 0x90 - cleanbit 4 */
     u64 res08[2];