Re: [Xen-devel] [PATCH 4/4] x86/svm: Use named (bit)fields for task switch exit info

[Andrew Cooper <andrew.cooper3@citrix.com>]

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

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel