All of lore.kernel.org
 help / color / mirror / Atom feed
* [Xen-devel] [PATCH 0/4] x86/svm: (Post TASK_SWITCH) cleanup
@ 2019-12-04  9:43 Andrew Cooper
  2019-12-04  9:43 ` [Xen-devel] [PATCH 1/4] x86/svm: Clean up construct_vmcb() Andrew Cooper
                   ` (4 more replies)
  0 siblings, 5 replies; 18+ messages in thread
From: Andrew Cooper @ 2019-12-04  9:43 UTC (permalink / raw)
  To: Xen-devel; +Cc: Andrew Cooper, Wei Liu, Jan Beulich, Roger Pau Monné

Various bits of cleanup stemming from the recent TASK_SWITCH fixes.

This series depends on both
  x86/svm: Correct vm_event API for descriptor accesses
  x86/svm: Fix handling of EFLAGS.RF on task switch

posted separately due to their bugfix nature, but neither are overly important
for review purposes.

Andrew Cooper (4):
  x86/svm: Clean up construct_vmcb()
  x86/svm: Don't shadow variables in svm_vmexit_handler()
  x86/svm: Clean up intinfo_t variables
  x86/svm: Use named (bit)fields for task switch exit info

 xen/arch/x86/hvm/svm/intr.c        |  32 ++++----
 xen/arch/x86/hvm/svm/nestedsvm.c   |  28 +++----
 xen/arch/x86/hvm/svm/svm.c         | 145 ++++++++++++++++---------------------
 xen/arch/x86/hvm/svm/svmdebug.c    |  12 +--
 xen/arch/x86/hvm/svm/vmcb.c        |  75 ++++++-------------
 xen/include/asm-x86/hvm/svm/vmcb.h |  55 +++++++++-----
 6 files changed, 153 insertions(+), 194 deletions(-)

-- 
2.11.0


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

^ permalink raw reply	[flat|nested] 18+ messages in thread
* [Xen-devel] [PATCH 1/4] x86/svm: Clean up construct_vmcb()
  2019-12-04  9:43 [Xen-devel] [PATCH 0/4] x86/svm: (Post TASK_SWITCH) cleanup Andrew Cooper
@ 2019-12-04  9:43 ` Andrew Cooper
  2019-12-04 10:06   ` Jan Beulich
  2019-12-04  9:43 ` [Xen-devel] [PATCH 2/4] x86/svm: Don't shadow variables in svm_vmexit_handler() Andrew Cooper
                   ` (3 subsequent siblings)
  4 siblings, 1 reply; 18+ messages in thread
From: Andrew Cooper @ 2019-12-04  9:43 UTC (permalink / raw)
  To: Xen-devel; +Cc: Andrew Cooper, Wei Liu, Jan Beulich, Roger Pau Monné

The vmcb is zeroed on allocate - drop all explicit writes of 0.  Move
hvm_update_guest_efer() to co-locate it with the other control register
updates.

Move the BUILD_BUG_ON() into build_assertions(), and add some offset checks
for fields after the large blocks of reserved fields (as these are the most
likely to trigger from a mis-edit).  Take the opportunity to fold 6 adjacent
res* fields into one.

Finally, drop all trailing whitespace in the file.

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/vmcb.c        | 75 ++++++++++++--------------------------
 xen/include/asm-x86/hvm/svm/vmcb.h |  7 +---
 2 files changed, 25 insertions(+), 57 deletions(-)

diff --git a/xen/arch/x86/hvm/svm/vmcb.c b/xen/arch/x86/hvm/svm/vmcb.c
index 1fef0da22c..fa13fc0b6b 100644
--- a/xen/arch/x86/hvm/svm/vmcb.c
+++ b/xen/arch/x86/hvm/svm/vmcb.c
@@ -30,7 +30,7 @@
 #include <asm/hvm/svm/svm.h>
 #include <asm/hvm/svm/svmdebug.h>
 
-struct vmcb_struct *alloc_vmcb(void) 
+struct vmcb_struct *alloc_vmcb(void)
 {
     struct vmcb_struct *vmcb;
 
@@ -56,18 +56,15 @@ static int construct_vmcb(struct vcpu *v)
     struct svm_vcpu *svm = &v->arch.hvm.svm;
     struct vmcb_struct *vmcb = svm->vmcb;
 
-    /* Build-time check of the size of VMCB AMD structure. */
-    BUILD_BUG_ON(sizeof(*vmcb) != PAGE_SIZE);
-
-    vmcb->_general1_intercepts = 
+    vmcb->_general1_intercepts =
         GENERAL1_INTERCEPT_INTR        | GENERAL1_INTERCEPT_NMI         |
         GENERAL1_INTERCEPT_SMI         | GENERAL1_INTERCEPT_INIT        |
         GENERAL1_INTERCEPT_CPUID       | GENERAL1_INTERCEPT_INVD        |
-        GENERAL1_INTERCEPT_HLT         | GENERAL1_INTERCEPT_INVLPG      | 
+        GENERAL1_INTERCEPT_HLT         | GENERAL1_INTERCEPT_INVLPG      |
         GENERAL1_INTERCEPT_INVLPGA     | GENERAL1_INTERCEPT_IOIO_PROT   |
         GENERAL1_INTERCEPT_MSR_PROT    | GENERAL1_INTERCEPT_SHUTDOWN_EVT|
         GENERAL1_INTERCEPT_TASK_SWITCH;
-    vmcb->_general2_intercepts = 
+    vmcb->_general2_intercepts =
         GENERAL2_INTERCEPT_VMRUN       | GENERAL2_INTERCEPT_VMMCALL     |
         GENERAL2_INTERCEPT_VMLOAD      | GENERAL2_INTERCEPT_VMSAVE      |
         GENERAL2_INTERCEPT_STGI        | GENERAL2_INTERCEPT_CLGI        |
@@ -105,12 +102,6 @@ static int construct_vmcb(struct vcpu *v)
 
     /* Virtualise EFLAGS.IF and LAPIC TPR (CR8). */
     vmcb->_vintr.fields.intr_masking = 1;
-  
-    /* Initialise event injection to no-op. */
-    vmcb->eventinj.bytes = 0;
-
-    /* TSC. */
-    vmcb->_tsc_offset = 0;
 
     /* Don't need to intercept RDTSC if CPU supports TSC rate scaling */
     if ( v->domain->arch.vtsc && !cpu_has_tsc_ratio )
@@ -119,10 +110,6 @@ static int construct_vmcb(struct vcpu *v)
         vmcb->_general2_intercepts |= GENERAL2_INTERCEPT_RDTSCP;
     }
 
-    /* Guest EFER. */
-    v->arch.hvm.guest_efer = 0;
-    hvm_update_guest_efer(v);
-
     /* Guest segment limits. */
     vmcb->cs.limit = ~0u;
     vmcb->es.limit = ~0u;
@@ -131,14 +118,6 @@ static int construct_vmcb(struct vcpu *v)
     vmcb->fs.limit = ~0u;
     vmcb->gs.limit = ~0u;
 
-    /* Guest segment bases. */
-    vmcb->cs.base = 0;
-    vmcb->es.base = 0;
-    vmcb->ss.base = 0;
-    vmcb->ds.base = 0;
-    vmcb->fs.base = 0;
-    vmcb->gs.base = 0;
-
     /* Guest segment AR bytes. */
     vmcb->es.attr = 0xc93; /* read/write, accessed */
     vmcb->ss.attr = 0xc93;
@@ -147,29 +126,13 @@ static int construct_vmcb(struct vcpu *v)
     vmcb->gs.attr = 0xc93;
     vmcb->cs.attr = 0xc9b; /* exec/read, accessed */
 
-    /* Guest IDT. */
-    vmcb->idtr.base = 0;
-    vmcb->idtr.limit = 0;
-
-    /* Guest GDT. */
-    vmcb->gdtr.base = 0;
-    vmcb->gdtr.limit = 0;
-
-    /* Guest LDT. */
-    vmcb->ldtr.sel = 0;
-    vmcb->ldtr.base = 0;
-    vmcb->ldtr.limit = 0;
-    vmcb->ldtr.attr = 0;
-
     /* Guest TSS. */
     vmcb->tr.attr = 0x08b; /* 32-bit TSS (busy) */
-    vmcb->tr.base = 0;
     vmcb->tr.limit = 0xff;
 
     v->arch.hvm.guest_cr[0] = X86_CR0_PE | X86_CR0_ET;
+    hvm_update_guest_efer(v);
     hvm_update_guest_cr(v, 0);
-
-    v->arch.hvm.guest_cr[4] = 0;
     hvm_update_guest_cr(v, 4);
 
     paging_update_paging_modes(v);
@@ -212,8 +175,6 @@ static int construct_vmcb(struct vcpu *v)
             vmcb->_pause_filter_thresh = SVM_PAUSETHRESH_INIT;
     }
 
-    vmcb->cleanbits.bytes = 0;
-
     return 0;
 }
 
@@ -268,7 +229,7 @@ static void vmcb_dump(unsigned char ch)
 {
     struct domain *d;
     struct vcpu *v;
-    
+
     printk("*********** VMCB Areas **************\n");
 
     rcu_read_lock(&domlist_read_lock);
@@ -297,14 +258,26 @@ void __init setup_vmcb_dump(void)
 
 static void __init __maybe_unused build_assertions(void)
 {
-    struct segment_register sreg;
+    struct vmcb_struct vmcb;
+
+    /* Build-time check of the VMCB layout. */
+    BUILD_BUG_ON(sizeof(vmcb) != PAGE_SIZE);
+    BUILD_BUG_ON(offsetof(struct vmcb_struct, _pause_filter_thresh) != 0x03c);
+    BUILD_BUG_ON(offsetof(struct vmcb_struct, _vintr)               != 0x060);
+    BUILD_BUG_ON(offsetof(struct vmcb_struct, eventinj)             != 0x0a8);
+    BUILD_BUG_ON(offsetof(struct vmcb_struct, es)                   != 0x400);
+    BUILD_BUG_ON(offsetof(struct vmcb_struct, _cpl)                 != 0x4cb);
+    BUILD_BUG_ON(offsetof(struct vmcb_struct, _cr4)                 != 0x548);
+    BUILD_BUG_ON(offsetof(struct vmcb_struct, rsp)                  != 0x5d8);
+    BUILD_BUG_ON(offsetof(struct vmcb_struct, rax)                  != 0x5f8);
+    BUILD_BUG_ON(offsetof(struct vmcb_struct, _g_pat)               != 0x668);
 
     /* Check struct segment_register against the VMCB segment layout. */
-    BUILD_BUG_ON(sizeof(sreg)       != 16);
-    BUILD_BUG_ON(sizeof(sreg.sel)   != 2);
-    BUILD_BUG_ON(sizeof(sreg.attr)  != 2);
-    BUILD_BUG_ON(sizeof(sreg.limit) != 4);
-    BUILD_BUG_ON(sizeof(sreg.base)  != 8);
+    BUILD_BUG_ON(sizeof(vmcb.es)       != 16);
+    BUILD_BUG_ON(sizeof(vmcb.es.sel)   != 2);
+    BUILD_BUG_ON(sizeof(vmcb.es.attr)  != 2);
+    BUILD_BUG_ON(sizeof(vmcb.es.limit) != 4);
+    BUILD_BUG_ON(sizeof(vmcb.es.base)  != 8);
     BUILD_BUG_ON(offsetof(struct segment_register, sel)   != 0);
     BUILD_BUG_ON(offsetof(struct segment_register, attr)  != 2);
     BUILD_BUG_ON(offsetof(struct segment_register, limit) != 4);
diff --git a/xen/include/asm-x86/hvm/svm/vmcb.h b/xen/include/asm-x86/hvm/svm/vmcb.h
index 5c710286f7..e37220edf2 100644
--- a/xen/include/asm-x86/hvm/svm/vmcb.h
+++ b/xen/include/asm-x86/hvm/svm/vmcb.h
@@ -406,12 +406,7 @@ struct vmcb_struct {
     u32 _exception_intercepts;  /* offset 0x08 - cleanbit 0 */
     u32 _general1_intercepts;   /* offset 0x0C - cleanbit 0 */
     u32 _general2_intercepts;   /* offset 0x10 - cleanbit 0 */
-    u32 res01;                  /* offset 0x14 */
-    u64 res02;                  /* offset 0x18 */
-    u64 res03;                  /* offset 0x20 */
-    u64 res04;                  /* offset 0x28 */
-    u64 res05;                  /* offset 0x30 */
-    u32 res06;                  /* offset 0x38 */
+    u32 res01[10];
     u16 _pause_filter_thresh;   /* offset 0x3C - cleanbit 0 */
     u16 _pause_filter_count;    /* offset 0x3E - cleanbit 0 */
     u64 _iopm_base_pa;          /* offset 0x40 - cleanbit 1 */
-- 
2.11.0


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

^ permalink raw reply related	[flat|nested] 18+ messages in thread
* [Xen-devel] [PATCH 2/4] x86/svm: Don't shadow variables in svm_vmexit_handler()
  2019-12-04  9:43 [Xen-devel] [PATCH 0/4] x86/svm: (Post TASK_SWITCH) cleanup Andrew Cooper
  2019-12-04  9:43 ` [Xen-devel] [PATCH 1/4] x86/svm: Clean up construct_vmcb() Andrew Cooper
@ 2019-12-04  9:43 ` Andrew Cooper
  2019-12-04 10:10   ` Jan Beulich
  2019-12-04  9:43 ` [Xen-devel] [PATCH 3/4] x86/svm: Clean up intinfo_t variables Andrew Cooper
                   ` (2 subsequent siblings)
  4 siblings, 1 reply; 18+ messages in thread
From: Andrew Cooper @ 2019-12-04  9:43 UTC (permalink / raw)
  To: Xen-devel; +Cc: Andrew Cooper, Wei Liu, Jan Beulich, Roger Pau Monné

The local variable eventinj is set to the value of vmcb->exitintinfo which is
confusing considering that it isn't vmcb->eventinj.  The variable isn't
necessary to begin with, so drop it to avoid confusion.

A local rc variable is shadowed in the CPUID, #DB and #BP handlers.

There is a mix of spelling of inst_len and insn_len, all of which are
logically the same value.  Consolidate on insn_len which also matches the name
of the emulation functions for obtaining instruction lengths, and avoid
shadowing it in the CPUID and TASK_SWITCH handlers.

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 | 63 ++++++++++++++++++++--------------------------
 1 file changed, 27 insertions(+), 36 deletions(-)

diff --git a/xen/arch/x86/hvm/svm/svm.c b/xen/arch/x86/hvm/svm/svm.c
index 518eaefe68..c5ac03b0b1 100644
--- a/xen/arch/x86/hvm/svm/svm.c
+++ b/xen/arch/x86/hvm/svm/svm.c
@@ -2480,8 +2480,7 @@ void svm_vmexit_handler(struct cpu_user_regs *regs)
     uint64_t exit_reason;
     struct vcpu *v = current;
     struct vmcb_struct *vmcb = v->arch.hvm.svm.vmcb;
-    eventinj_t eventinj;
-    int inst_len, rc;
+    int insn_len, rc;
     vintr_t intr;
     bool_t vcpu_guestmode = 0;
     struct vlapic *vlapic = vcpu_vlapic(v);
@@ -2603,11 +2602,10 @@ void svm_vmexit_handler(struct cpu_user_regs *regs)
     vmcb->cleanbits.bytes = cpu_has_svm_cleanbits ? ~0u : 0u;
 
     /* Event delivery caused this intercept? Queue for redelivery. */
-    eventinj = vmcb->exitintinfo;
-    if ( unlikely(eventinj.fields.v) &&
-         hvm_event_needs_reinjection(eventinj.fields.type,
-                                     eventinj.fields.vector) )
-        vmcb->eventinj = eventinj;
+    if ( unlikely(vmcb->exitintinfo.fields.v) &&
+         hvm_event_needs_reinjection(vmcb->exitintinfo.fields.type,
+                                     vmcb->exitintinfo.fields.vector) )
+        vmcb->eventinj = vmcb->exitintinfo;
 
     switch ( exit_reason )
     {
@@ -2630,63 +2628,60 @@ void svm_vmexit_handler(struct cpu_user_regs *regs)
     case VMEXIT_EXCEPTION_DB:
         if ( !v->domain->debugger_attached )
         {
-            int rc;
             unsigned int trap_type;
 
             if ( likely(exit_reason != VMEXIT_ICEBP) )
             {
                 trap_type = X86_EVENTTYPE_HW_EXCEPTION;
-                inst_len = 0;
+                insn_len = 0;
             }
             else
             {
                 trap_type = X86_EVENTTYPE_PRI_SW_EXCEPTION;
-                inst_len = svm_get_insn_len(v, INSTR_ICEBP);
+                insn_len = svm_get_insn_len(v, INSTR_ICEBP);
 
-                if ( !inst_len )
+                if ( !insn_len )
                     break;
             }
 
             rc = hvm_monitor_debug(regs->rip,
                                    HVM_MONITOR_DEBUG_EXCEPTION,
-                                   trap_type, inst_len);
+                                   trap_type, insn_len);
             if ( rc < 0 )
                 goto unexpected_exit_type;
             if ( !rc )
                 hvm_inject_exception(TRAP_debug,
-                                     trap_type, inst_len, X86_EVENT_NO_EC);
+                                     trap_type, insn_len, X86_EVENT_NO_EC);
         }
         else
             domain_pause_for_debugger();
         break;
 
     case VMEXIT_EXCEPTION_BP:
-        inst_len = svm_get_insn_len(v, INSTR_INT3);
+        insn_len = svm_get_insn_len(v, INSTR_INT3);
 
-        if ( inst_len == 0 )
+        if ( insn_len == 0 )
              break;
 
         if ( v->domain->debugger_attached )
         {
             /* AMD Vol2, 15.11: INT3, INTO, BOUND intercepts do not update RIP. */
-            __update_guest_eip(regs, inst_len);
+            __update_guest_eip(regs, insn_len);
             current->arch.gdbsx_vcpu_event = TRAP_int3;
             domain_pause_for_debugger();
         }
         else
         {
-           int rc;
-
            rc = hvm_monitor_debug(regs->rip,
                                   HVM_MONITOR_SOFTWARE_BREAKPOINT,
                                   X86_EVENTTYPE_SW_EXCEPTION,
-                                  inst_len);
+                                  insn_len);
            if ( rc < 0 )
                goto unexpected_exit_type;
            if ( !rc )
                hvm_inject_exception(TRAP_int3,
                                     X86_EVENTTYPE_SW_EXCEPTION,
-                                    inst_len, X86_EVENT_NO_EC);
+                                    insn_len, X86_EVENT_NO_EC);
         }
         break;
 
@@ -2757,7 +2752,7 @@ void svm_vmexit_handler(struct cpu_user_regs *regs)
 
     case VMEXIT_TASK_SWITCH: {
         enum hvm_task_switch_reason reason;
-        int32_t errcode = -1, insn_len = -1;
+        int32_t errcode = -1;
 
         /*
          * All TASK_SWITCH intercepts have fault-like semantics.  NRIP is
@@ -2769,6 +2764,7 @@ void svm_vmexit_handler(struct cpu_user_regs *regs)
          * to distinguish interrupts/exceptions from instruction based
          * switches.
          */
+        insn_len = -1;
         if ( vmcb->exitintinfo.fields.v )
         {
             switch ( vmcb->exitintinfo.fields.type )
@@ -2818,22 +2814,17 @@ void svm_vmexit_handler(struct cpu_user_regs *regs)
     }
 
     case VMEXIT_CPUID:
-    {
-        unsigned int inst_len = svm_get_insn_len(v, INSTR_CPUID);
-        int rc = 0;
-
-        if ( inst_len == 0 )
+        if ( (insn_len = svm_get_insn_len(v, INSTR_CPUID)) == 0 )
             break;
 
-        rc = hvm_vmexit_cpuid(regs, inst_len);
+        rc = hvm_vmexit_cpuid(regs, insn_len);
 
         if ( rc < 0 )
             goto unexpected_exit_type;
         if ( !rc )
-            __update_guest_eip(regs, inst_len); /* Safe: CPUID */
-
+            __update_guest_eip(regs, insn_len);
         break;
-    }
+
     case VMEXIT_HLT:
         svm_vmexit_do_hlt(vmcb, regs);
         break;
@@ -2875,20 +2866,20 @@ void svm_vmexit_handler(struct cpu_user_regs *regs)
             hvm_inject_hw_exception(TRAP_invalid_op, X86_EVENT_NO_EC);
             break;
         }
-        if ( (inst_len = svm_get_insn_len(v, INSTR_INVLPGA)) == 0 )
+        if ( (insn_len = svm_get_insn_len(v, INSTR_INVLPGA)) == 0 )
             break;
         svm_invlpga_intercept(v, regs->rax, regs->ecx);
-        __update_guest_eip(regs, inst_len);
+        __update_guest_eip(regs, insn_len);
         break;
 
     case VMEXIT_VMMCALL:
-        if ( (inst_len = svm_get_insn_len(v, INSTR_VMCALL)) == 0 )
+        if ( (insn_len = svm_get_insn_len(v, INSTR_VMCALL)) == 0 )
             break;
         BUG_ON(vcpu_guestmode);
         HVMTRACE_1D(VMMCALL, regs->eax);
 
         if ( hvm_hypercall(regs) == HVM_HCALL_completed )
-            __update_guest_eip(regs, inst_len);
+            __update_guest_eip(regs, insn_len);
         break;
 
     case VMEXIT_DR0_READ ... VMEXIT_DR7_READ:
@@ -2936,9 +2927,9 @@ void svm_vmexit_handler(struct cpu_user_regs *regs)
     case VMEXIT_XSETBV:
         if ( vmcb_get_cpl(vmcb) )
             hvm_inject_hw_exception(TRAP_gp_fault, 0);
-        else if ( (inst_len = svm_get_insn_len(v, INSTR_XSETBV)) &&
+        else if ( (insn_len = svm_get_insn_len(v, INSTR_XSETBV)) &&
                   hvm_handle_xsetbv(regs->ecx, msr_fold(regs)) == X86EMUL_OKAY )
-            __update_guest_eip(regs, inst_len);
+            __update_guest_eip(regs, insn_len);
         break;
 
     case VMEXIT_NPF:
-- 
2.11.0


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

^ permalink raw reply related	[flat|nested] 18+ messages in thread
* [Xen-devel] [PATCH 3/4] x86/svm: Clean up intinfo_t variables
  2019-12-04  9:43 [Xen-devel] [PATCH 0/4] x86/svm: (Post TASK_SWITCH) cleanup Andrew Cooper
  2019-12-04  9:43 ` [Xen-devel] [PATCH 1/4] x86/svm: Clean up construct_vmcb() Andrew Cooper
  2019-12-04  9:43 ` [Xen-devel] [PATCH 2/4] x86/svm: Don't shadow variables in svm_vmexit_handler() Andrew Cooper
@ 2019-12-04  9:43 ` Andrew Cooper
  2019-12-04 10:19   ` Jan Beulich
                     ` (2 more replies)
  2019-12-04  9:43 ` [Xen-devel] [PATCH 4/4] x86/svm: Use named (bit)fields for task switch exit info Andrew Cooper
  2019-12-05 10:51 ` [Xen-devel] [PATCH 5/4] x86/svm: Minor cleanup to start_svm() Andrew Cooper
  4 siblings, 3 replies; 18+ messages in thread
From: Andrew Cooper @ 2019-12-04  9:43 UTC (permalink / raw)
  To: Xen-devel; +Cc: Andrew Cooper, Wei Liu, Jan Beulich, Roger Pau Monné

The type name is poor because the type is also used for the IDT vectoring
field, not just for the event injection field.  Rename it to intinfo_t which
is how the APM refers to the data.

Rearrange the union to drop the .fields infix, and rename bytes to the more
common raw.

While adjusting all call sites, fix up style issues and make use of structure
assignments where applicable.

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/intr.c        | 32 ++++++++----------
 xen/arch/x86/hvm/svm/nestedsvm.c   | 28 +++++++---------
 xen/arch/x86/hvm/svm/svm.c         | 68 ++++++++++++++++++--------------------
 xen/arch/x86/hvm/svm/svmdebug.c    | 12 +++----
 xen/include/asm-x86/hvm/svm/vmcb.h | 22 ++++++------
 5 files changed, 75 insertions(+), 87 deletions(-)

diff --git a/xen/arch/x86/hvm/svm/intr.c b/xen/arch/x86/hvm/svm/intr.c
index ff755165cd..4eede5cc23 100644
--- a/xen/arch/x86/hvm/svm/intr.c
+++ b/xen/arch/x86/hvm/svm/intr.c
@@ -43,15 +43,13 @@ static void svm_inject_nmi(struct vcpu *v)
 {
     struct vmcb_struct *vmcb = v->arch.hvm.svm.vmcb;
     u32 general1_intercepts = vmcb_get_general1_intercepts(vmcb);
-    eventinj_t event;
 
-    event.bytes = 0;
-    event.fields.v = 1;
-    event.fields.type = X86_EVENTTYPE_NMI;
-    event.fields.vector = 2;
-
-    ASSERT(vmcb->eventinj.fields.v == 0);
-    vmcb->eventinj = event;
+    ASSERT(!vmcb->eventinj.v);
+    vmcb->eventinj = (intinfo_t){
+        .vector = 2,
+        .type = X86_EVENTTYPE_NMI,
+        .v = true,
+    };
 
     /*
      * SVM does not virtualise the NMI mask, so we emulate it by intercepting
@@ -64,15 +62,13 @@ static void svm_inject_nmi(struct vcpu *v)
 static void svm_inject_extint(struct vcpu *v, int vector)
 {
     struct vmcb_struct *vmcb = v->arch.hvm.svm.vmcb;
-    eventinj_t event;
-
-    event.bytes = 0;
-    event.fields.v = 1;
-    event.fields.type = X86_EVENTTYPE_EXT_INTR;
-    event.fields.vector = vector;
 
-    ASSERT(vmcb->eventinj.fields.v == 0);
-    vmcb->eventinj = event;
+    ASSERT(!vmcb->eventinj.v);
+    vmcb->eventinj = (intinfo_t){
+        .vector = vector,
+        .type = X86_EVENTTYPE_EXT_INTR,
+        .v = true,
+    };
 }
 
 static void svm_enable_intr_window(struct vcpu *v, struct hvm_intack intack)
@@ -99,7 +95,7 @@ static void svm_enable_intr_window(struct vcpu *v, struct hvm_intack intack)
     }
 
     HVMTRACE_3D(INTR_WINDOW, intack.vector, intack.source,
-                vmcb->eventinj.fields.v?vmcb->eventinj.fields.vector:-1);
+                vmcb->eventinj.v ? vmcb->eventinj.vector : -1);
 
     /*
      * Create a dummy virtual interrupt to intercept as soon as the
@@ -197,7 +193,7 @@ void svm_intr_assist(void)
          *      have cleared the interrupt out of the IRR.
          * 2. The IRQ is masked.
          */
-        if ( unlikely(vmcb->eventinj.fields.v) || intblk )
+        if ( unlikely(vmcb->eventinj.v) || intblk )
         {
             svm_enable_intr_window(v, intack);
             return;
diff --git a/xen/arch/x86/hvm/svm/nestedsvm.c b/xen/arch/x86/hvm/svm/nestedsvm.c
index fef124fb11..d279a50e5c 100644
--- a/xen/arch/x86/hvm/svm/nestedsvm.c
+++ b/xen/arch/x86/hvm/svm/nestedsvm.c
@@ -340,7 +340,7 @@ static int nsvm_vcpu_hostrestore(struct vcpu *v, struct cpu_user_regs *regs)
     /* Clear exitintinfo to prevent a fault loop of re-injecting
      * exceptions forever.
      */
-    n1vmcb->exitintinfo.bytes = 0;
+    n1vmcb->exitintinfo.raw = 0;
 
     /* Cleanbits */
     n1vmcb->cleanbits.bytes = 0;
@@ -806,13 +806,10 @@ nsvm_vcpu_vmexit_inject(struct vcpu *v, struct cpu_user_regs *regs,
 
         switch (exitcode) {
         case VMEXIT_INTR:
-            if ( unlikely(ns_vmcb->eventinj.fields.v)
-                && nv->nv_vmentry_pending
-                && hvm_event_needs_reinjection(ns_vmcb->eventinj.fields.type,
-                    ns_vmcb->eventinj.fields.vector) )
-            {
-                ns_vmcb->exitintinfo.bytes = ns_vmcb->eventinj.bytes;
-            }
+            if ( unlikely(ns_vmcb->eventinj.v) && nv->nv_vmentry_pending &&
+                 hvm_event_needs_reinjection(ns_vmcb->eventinj.type,
+                                             ns_vmcb->eventinj.vector) )
+                ns_vmcb->exitintinfo = ns_vmcb->eventinj;
             break;
         case VMEXIT_EXCEPTION_PF:
             ns_vmcb->_cr2 = ns_vmcb->exitinfo2;
@@ -837,7 +834,7 @@ nsvm_vcpu_vmexit_inject(struct vcpu *v, struct cpu_user_regs *regs,
     }
 
     ns_vmcb->exitcode = exitcode;
-    ns_vmcb->eventinj.bytes = 0;
+    ns_vmcb->eventinj.raw = 0;
     return 0;
 }
 
@@ -1077,14 +1074,12 @@ nsvm_vmcb_prepare4vmexit(struct vcpu *v, struct cpu_user_regs *regs)
      * only happens on a VMRUN instruction intercept which has no valid
      * exitintinfo set.
      */
-    if ( unlikely(n2vmcb->eventinj.fields.v) &&
-         hvm_event_needs_reinjection(n2vmcb->eventinj.fields.type,
-                                     n2vmcb->eventinj.fields.vector) )
-    {
+    if ( unlikely(n2vmcb->eventinj.v) &&
+         hvm_event_needs_reinjection(n2vmcb->eventinj.type,
+                                     n2vmcb->eventinj.vector) )
         ns_vmcb->exitintinfo = n2vmcb->eventinj;
-    }
 
-    ns_vmcb->eventinj.bytes = 0;
+    ns_vmcb->eventinj.raw = 0;
 
     /* Nested paging mode */
     if (nestedhvm_paging_mode_hap(v)) {
@@ -1249,7 +1244,8 @@ enum hvm_intblk nsvm_intr_blocked(struct vcpu *v)
         if ( v->arch.hvm.hvm_io.io_req.state != STATE_IOREQ_NONE )
             return hvm_intblk_shadow;
 
-        if ( !nv->nv_vmexit_pending && n2vmcb->exitintinfo.bytes != 0 ) {
+        if ( !nv->nv_vmexit_pending && n2vmcb->exitintinfo.v )
+        {
             /* Give the l2 guest a chance to finish the delivery of
              * the last injected interrupt or exception before we
              * emulate a VMEXIT (e.g. VMEXIT(INTR) ).
diff --git a/xen/arch/x86/hvm/svm/svm.c b/xen/arch/x86/hvm/svm/svm.c
index c5ac03b0b1..263ae03bfd 100644
--- a/xen/arch/x86/hvm/svm/svm.c
+++ b/xen/arch/x86/hvm/svm/svm.c
@@ -259,12 +259,12 @@ static int svm_vmcb_save(struct vcpu *v, struct hvm_hw_cpu *c)
     c->sysenter_esp = v->arch.hvm.svm.guest_sysenter_esp;
     c->sysenter_eip = v->arch.hvm.svm.guest_sysenter_eip;
 
-    if ( vmcb->eventinj.fields.v &&
-         hvm_event_needs_reinjection(vmcb->eventinj.fields.type,
-                                     vmcb->eventinj.fields.vector) )
+    if ( vmcb->eventinj.v &&
+         hvm_event_needs_reinjection(vmcb->eventinj.type,
+                                     vmcb->eventinj.vector) )
     {
-        c->pending_event = (uint32_t)vmcb->eventinj.bytes;
-        c->error_code = vmcb->eventinj.fields.errorcode;
+        c->pending_event = vmcb->eventinj.raw;
+        c->error_code = vmcb->eventinj.ec;
     }
 
     return 1;
@@ -339,11 +339,11 @@ static int svm_vmcb_restore(struct vcpu *v, struct hvm_hw_cpu *c)
     {
         gdprintk(XENLOG_INFO, "Re-injecting %#"PRIx32", %#"PRIx32"\n",
                  c->pending_event, c->error_code);
-        vmcb->eventinj.bytes = c->pending_event;
-        vmcb->eventinj.fields.errorcode = c->error_code;
+        vmcb->eventinj.raw = c->pending_event;
+        vmcb->eventinj.ec = c->error_code;
     }
     else
-        vmcb->eventinj.bytes = 0;
+        vmcb->eventinj.raw = 0;
 
     vmcb->cleanbits.bytes = 0;
     paging_update_paging_modes(v);
@@ -1301,7 +1301,7 @@ static void svm_inject_event(const struct x86_event *event)
 {
     struct vcpu *curr = current;
     struct vmcb_struct *vmcb = curr->arch.hvm.svm.vmcb;
-    eventinj_t eventinj = vmcb->eventinj;
+    intinfo_t eventinj = vmcb->eventinj;
     struct x86_event _event = *event;
     struct cpu_user_regs *regs = guest_cpu_user_regs();
 
@@ -1342,18 +1342,15 @@ static void svm_inject_event(const struct x86_event *event)
         break;
     }
 
-    if ( unlikely(eventinj.fields.v) &&
-         (eventinj.fields.type == X86_EVENTTYPE_HW_EXCEPTION) )
+    if ( eventinj.v && (eventinj.type == X86_EVENTTYPE_HW_EXCEPTION) )
     {
         _event.vector = hvm_combine_hw_exceptions(
-            eventinj.fields.vector, _event.vector);
+            eventinj.vector, _event.vector);
         if ( _event.vector == TRAP_double_fault )
             _event.error_code = 0;
     }
 
-    eventinj.bytes = 0;
-    eventinj.fields.v = 1;
-    eventinj.fields.vector = _event.vector;
+    eventinj = (intinfo_t){ .vector = _event.vector, .v = true };
 
     /*
      * Refer to AMD Vol 2: System Programming, 15.20 Event Injection.
@@ -1373,7 +1370,7 @@ static void svm_inject_event(const struct x86_event *event)
             vmcb->nextrip = regs->rip + _event.insn_len;
         else
             regs->rip += _event.insn_len;
-        eventinj.fields.type = X86_EVENTTYPE_SW_INTERRUPT;
+        eventinj.type = X86_EVENTTYPE_SW_INTERRUPT;
         break;
 
     case X86_EVENTTYPE_PRI_SW_EXCEPTION: /* icebp */
@@ -1385,7 +1382,7 @@ static void svm_inject_event(const struct x86_event *event)
         regs->rip += _event.insn_len;
         if ( cpu_has_svm_nrips )
             vmcb->nextrip = regs->rip;
-        eventinj.fields.type = X86_EVENTTYPE_HW_EXCEPTION;
+        eventinj.type = X86_EVENTTYPE_HW_EXCEPTION;
         break;
 
     case X86_EVENTTYPE_SW_EXCEPTION: /* int3, into */
@@ -1397,13 +1394,13 @@ static void svm_inject_event(const struct x86_event *event)
             vmcb->nextrip = regs->rip + _event.insn_len;
         else
             regs->rip += _event.insn_len;
-        eventinj.fields.type = X86_EVENTTYPE_HW_EXCEPTION;
+        eventinj.type = X86_EVENTTYPE_HW_EXCEPTION;
         break;
 
     default:
-        eventinj.fields.type = X86_EVENTTYPE_HW_EXCEPTION;
-        eventinj.fields.ev = (_event.error_code != X86_EVENT_NO_EC);
-        eventinj.fields.errorcode = _event.error_code;
+        eventinj.type = X86_EVENTTYPE_HW_EXCEPTION;
+        eventinj.ev = (_event.error_code != X86_EVENT_NO_EC);
+        eventinj.ec = _event.error_code;
         break;
     }
 
@@ -1417,8 +1414,7 @@ static void svm_inject_event(const struct x86_event *event)
         vmcb->nextrip = (uint32_t)vmcb->nextrip;
     }
 
-    ASSERT(!eventinj.fields.ev ||
-           eventinj.fields.errorcode == (uint16_t)eventinj.fields.errorcode);
+    ASSERT(!eventinj.ev || eventinj.ec == (uint16_t)eventinj.ec);
     vmcb->eventinj = eventinj;
 
     if ( _event.vector == TRAP_page_fault &&
@@ -1431,7 +1427,7 @@ static void svm_inject_event(const struct x86_event *event)
 
 static bool svm_event_pending(const struct vcpu *v)
 {
-    return v->arch.hvm.svm.vmcb->eventinj.fields.v;
+    return v->arch.hvm.svm.vmcb->eventinj.v;
 }
 
 static void svm_cpu_dead(unsigned int cpu)
@@ -2410,12 +2406,12 @@ static bool svm_get_pending_event(struct vcpu *v, struct x86_event *info)
 {
     const struct vmcb_struct *vmcb = v->arch.hvm.svm.vmcb;
 
-    if ( vmcb->eventinj.fields.v )
+    if ( vmcb->eventinj.v )
         return false;
 
-    info->vector = vmcb->eventinj.fields.vector;
-    info->type = vmcb->eventinj.fields.type;
-    info->error_code = vmcb->eventinj.fields.errorcode;
+    info->vector = vmcb->eventinj.vector;
+    info->type = vmcb->eventinj.type;
+    info->error_code = vmcb->eventinj.ec;
 
     return true;
 }
@@ -2602,9 +2598,9 @@ void svm_vmexit_handler(struct cpu_user_regs *regs)
     vmcb->cleanbits.bytes = cpu_has_svm_cleanbits ? ~0u : 0u;
 
     /* Event delivery caused this intercept? Queue for redelivery. */
-    if ( unlikely(vmcb->exitintinfo.fields.v) &&
-         hvm_event_needs_reinjection(vmcb->exitintinfo.fields.type,
-                                     vmcb->exitintinfo.fields.vector) )
+    if ( unlikely(vmcb->exitintinfo.v) &&
+         hvm_event_needs_reinjection(vmcb->exitintinfo.type,
+                                     vmcb->exitintinfo.vector) )
         vmcb->eventinj = vmcb->exitintinfo;
 
     switch ( exit_reason )
@@ -2765,9 +2761,9 @@ void svm_vmexit_handler(struct cpu_user_regs *regs)
          * switches.
          */
         insn_len = -1;
-        if ( vmcb->exitintinfo.fields.v )
+        if ( vmcb->exitintinfo.v )
         {
-            switch ( vmcb->exitintinfo.fields.type )
+            switch ( vmcb->exitintinfo.type )
             {
                 /*
                  * #BP and #OF are from INT3/INTO respectively.  #DB from
@@ -2775,8 +2771,8 @@ void svm_vmexit_handler(struct cpu_user_regs *regs)
                  * semantics.
                  */
             case X86_EVENTTYPE_HW_EXCEPTION:
-                if ( vmcb->exitintinfo.fields.vector == TRAP_int3 ||
-                     vmcb->exitintinfo.fields.vector == TRAP_overflow )
+                if ( vmcb->exitintinfo.vector == TRAP_int3 ||
+                     vmcb->exitintinfo.vector == TRAP_overflow )
                     break;
                 /* Fallthrough */
             case X86_EVENTTYPE_EXT_INTR:
@@ -2789,7 +2785,7 @@ void svm_vmexit_handler(struct cpu_user_regs *regs)
              * The common logic above will have forwarded the vectoring
              * information.  Undo this as we are going to emulate.
              */
-            vmcb->eventinj.bytes = 0;
+            vmcb->eventinj.raw = 0;
         }
 
         /*
diff --git a/xen/arch/x86/hvm/svm/svmdebug.c b/xen/arch/x86/hvm/svm/svmdebug.c
index 4293d8dba5..26e4b9d7bb 100644
--- a/xen/arch/x86/hvm/svm/svmdebug.c
+++ b/xen/arch/x86/hvm/svm/svmdebug.c
@@ -55,11 +55,11 @@ void svm_vmcb_dump(const char *from, const struct vmcb_struct *vmcb)
            vmcb->tlb_control, vmcb_get_vintr(vmcb).bytes,
            vmcb->interrupt_shadow);
     printk("eventinj %016"PRIx64", valid? %d, ec? %d, type %u, vector %#x\n",
-           vmcb->eventinj.bytes, vmcb->eventinj.fields.v,
-           vmcb->eventinj.fields.ev, vmcb->eventinj.fields.type,
-           vmcb->eventinj.fields.vector);
+           vmcb->eventinj.raw, vmcb->eventinj.v,
+           vmcb->eventinj.ev, vmcb->eventinj.type,
+           vmcb->eventinj.vector);
     printk("exitcode = %#"PRIx64" exitintinfo = %#"PRIx64"\n",
-           vmcb->exitcode, vmcb->exitintinfo.bytes);
+           vmcb->exitcode, vmcb->exitintinfo.raw);
     printk("exitinfo1 = %#"PRIx64" exitinfo2 = %#"PRIx64"\n",
            vmcb->exitinfo1, vmcb->exitinfo2);
     printk("np_enable = %#"PRIx64" guest_asid = %#x\n",
@@ -164,9 +164,9 @@ bool svm_vmcb_isvalid(const char *from, const struct vmcb_struct *vmcb,
         PRINTF("GENERAL2_INTERCEPT: VMRUN intercept bit is clear (%#"PRIx32")\n",
                vmcb_get_general2_intercepts(vmcb));
 
-    if ( vmcb->eventinj.fields.resvd1 )
+    if ( vmcb->eventinj.resvd1 )
         PRINTF("eventinj: MBZ bits are set (%#"PRIx64")\n",
-               vmcb->eventinj.bytes);
+               vmcb->eventinj.raw);
 
 #undef PRINTF
     return ret;
diff --git a/xen/include/asm-x86/hvm/svm/vmcb.h b/xen/include/asm-x86/hvm/svm/vmcb.h
index e37220edf2..fc67a88660 100644
--- a/xen/include/asm-x86/hvm/svm/vmcb.h
+++ b/xen/include/asm-x86/hvm/svm/vmcb.h
@@ -306,17 +306,17 @@ enum VMEXIT_EXITCODE
 
 typedef union
 {
-    u64 bytes;
     struct
     {
-        u64 vector:    8;
-        u64 type:      3;
-        u64 ev:        1;
-        u64 resvd1:   19;
-        u64 v:         1;
-        u64 errorcode:32;
-    } fields;
-} eventinj_t;
+        uint8_t  vector;
+        uint8_t  type:3;
+        bool     ev:1;
+        uint32_t resvd1:19;
+        bool     v:1;
+        uint32_t ec;
+    };
+    uint64_t raw;
+} intinfo_t;
 
 typedef union
 {
@@ -420,10 +420,10 @@ struct vmcb_struct {
     u64 exitcode;               /* offset 0x70 */
     u64 exitinfo1;              /* offset 0x78 */
     u64 exitinfo2;              /* offset 0x80 */
-    eventinj_t  exitintinfo;    /* offset 0x88 */
+    intinfo_t exitintinfo;      /* offset 0x88 */
     u64 _np_enable;             /* offset 0x90 - cleanbit 4 */
     u64 res08[2];
-    eventinj_t  eventinj;       /* offset 0xA8 */
+    intinfo_t eventinj;         /* offset 0xA8 */
     u64 _h_cr3;                 /* offset 0xB0 - cleanbit 4 */
     virt_ext_t virt_ext;        /* offset 0xB8 */
     vmcbcleanbits_t cleanbits;  /* offset 0xC0 */
-- 
2.11.0


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

^ permalink raw reply related	[flat|nested] 18+ messages in thread
* [Xen-devel] [PATCH 4/4] x86/svm: Use named (bit)fields for task switch exit info
  2019-12-04  9:43 [Xen-devel] [PATCH 0/4] x86/svm: (Post TASK_SWITCH) cleanup Andrew Cooper
                   ` (2 preceding siblings ...)
  2019-12-04  9:43 ` [Xen-devel] [PATCH 3/4] x86/svm: Clean up intinfo_t variables Andrew Cooper
@ 2019-12-04  9:43 ` Andrew Cooper
  2019-12-04 10:24   ` Jan Beulich
  2019-12-05 10:51 ` [Xen-devel] [PATCH 5/4] x86/svm: Minor cleanup to start_svm() Andrew Cooper
  4 siblings, 1 reply; 18+ messages in thread
From: Andrew Cooper @ 2019-12-04  9:43 UTC (permalink / raw)
  To: Xen-devel; +Cc: Andrew Cooper, Wei Liu, Jan Beulich, Roger Pau Monné

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(-)

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];
-- 
2.11.0


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

^ permalink raw reply related	[flat|nested] 18+ messages in thread
* Re: [Xen-devel] [PATCH 1/4] x86/svm: Clean up construct_vmcb()
  2019-12-04  9:43 ` [Xen-devel] [PATCH 1/4] x86/svm: Clean up construct_vmcb() Andrew Cooper
@ 2019-12-04 10:06   ` Jan Beulich
  2019-12-04 19:21     ` Andrew Cooper
  0 siblings, 1 reply; 18+ messages in thread
From: Jan Beulich @ 2019-12-04 10:06 UTC (permalink / raw)
  To: Andrew Cooper; +Cc: Xen-devel, Wei Liu, Roger Pau Monné

On 04.12.2019 10:43, Andrew Cooper wrote:
> The vmcb is zeroed on allocate - drop all explicit writes of 0.  Move
> hvm_update_guest_efer() to co-locate it with the other control register
> updates.
> 
> Move the BUILD_BUG_ON() into build_assertions(), and add some offset checks
> for fields after the large blocks of reserved fields (as these are the most
> likely to trigger from a mis-edit).  Take the opportunity to fold 6 adjacent
> res* fields into one.
> 
> Finally, drop all trailing whitespace in the file.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Reviewed-by: Jan Beulich <jbeulich@suse.com>
albeit with two (optional) suggestions:

> @@ -297,14 +258,26 @@ void __init setup_vmcb_dump(void)
>  
>  static void __init __maybe_unused build_assertions(void)
>  {
> -    struct segment_register sreg;
> +    struct vmcb_struct vmcb;
> +
> +    /* Build-time check of the VMCB layout. */
> +    BUILD_BUG_ON(sizeof(vmcb) != PAGE_SIZE);
> +    BUILD_BUG_ON(offsetof(struct vmcb_struct, _pause_filter_thresh) != 0x03c);
> +    BUILD_BUG_ON(offsetof(struct vmcb_struct, _vintr)               != 0x060);
> +    BUILD_BUG_ON(offsetof(struct vmcb_struct, eventinj)             != 0x0a8);
> +    BUILD_BUG_ON(offsetof(struct vmcb_struct, es)                   != 0x400);
> +    BUILD_BUG_ON(offsetof(struct vmcb_struct, _cpl)                 != 0x4cb);
> +    BUILD_BUG_ON(offsetof(struct vmcb_struct, _cr4)                 != 0x548);
> +    BUILD_BUG_ON(offsetof(struct vmcb_struct, rsp)                  != 0x5d8);
> +    BUILD_BUG_ON(offsetof(struct vmcb_struct, rax)                  != 0x5f8);
> +    BUILD_BUG_ON(offsetof(struct vmcb_struct, _g_pat)               != 0x668);
>  
>      /* Check struct segment_register against the VMCB segment layout. */
> -    BUILD_BUG_ON(sizeof(sreg)       != 16);
> -    BUILD_BUG_ON(sizeof(sreg.sel)   != 2);
> -    BUILD_BUG_ON(sizeof(sreg.attr)  != 2);
> -    BUILD_BUG_ON(sizeof(sreg.limit) != 4);
> -    BUILD_BUG_ON(sizeof(sreg.base)  != 8);
> +    BUILD_BUG_ON(sizeof(vmcb.es)       != 16);
> +    BUILD_BUG_ON(sizeof(vmcb.es.sel)   != 2);
> +    BUILD_BUG_ON(sizeof(vmcb.es.attr)  != 2);
> +    BUILD_BUG_ON(sizeof(vmcb.es.limit) != 4);
> +    BUILD_BUG_ON(sizeof(vmcb.es.base)  != 8);
>      BUILD_BUG_ON(offsetof(struct segment_register, sel)   != 0);
>      BUILD_BUG_ON(offsetof(struct segment_register, attr)  != 2);
>      BUILD_BUG_ON(offsetof(struct segment_register, limit) != 4);

For the ones only supplying context here, how about using the
shorter offsetof(typeof(vmcb.es), ...), also tying things better
to the prior sizeof() checks? The same, albeit to a lesser degree,
might then go for the earlier block, which could use the shorter
typeof(vmcb).

> --- a/xen/include/asm-x86/hvm/svm/vmcb.h
> +++ b/xen/include/asm-x86/hvm/svm/vmcb.h
> @@ -406,12 +406,7 @@ struct vmcb_struct {
>      u32 _exception_intercepts;  /* offset 0x08 - cleanbit 0 */
>      u32 _general1_intercepts;   /* offset 0x0C - cleanbit 0 */
>      u32 _general2_intercepts;   /* offset 0x10 - cleanbit 0 */
> -    u32 res01;                  /* offset 0x14 */
> -    u64 res02;                  /* offset 0x18 */
> -    u64 res03;                  /* offset 0x20 */
> -    u64 res04;                  /* offset 0x28 */
> -    u64 res05;                  /* offset 0x30 */
> -    u32 res06;                  /* offset 0x38 */
> +    u32 res01[10];

Was it intentional for the comment to be lost altogether?

Jan

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

^ permalink raw reply	[flat|nested] 18+ messages in thread
* Re: [Xen-devel] [PATCH 2/4] x86/svm: Don't shadow variables in svm_vmexit_handler()
  2019-12-04  9:43 ` [Xen-devel] [PATCH 2/4] x86/svm: Don't shadow variables in svm_vmexit_handler() Andrew Cooper
@ 2019-12-04 10:10   ` Jan Beulich
  0 siblings, 0 replies; 18+ messages in thread
From: Jan Beulich @ 2019-12-04 10:10 UTC (permalink / raw)
  To: Andrew Cooper; +Cc: Xen-devel, Wei Liu, Roger Pau Monné

On 04.12.2019 10:43, Andrew Cooper wrote:
> The local variable eventinj is set to the value of vmcb->exitintinfo which is
> confusing considering that it isn't vmcb->eventinj.  The variable isn't
> necessary to begin with, so drop it to avoid confusion.
> 
> A local rc variable is shadowed in the CPUID, #DB and #BP handlers.
> 
> There is a mix of spelling of inst_len and insn_len, all of which are
> logically the same value.  Consolidate on insn_len which also matches the name
> of the emulation functions for obtaining instruction lengths, and avoid
> shadowing it in the CPUID and TASK_SWITCH handlers.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

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

> --- a/xen/arch/x86/hvm/svm/svm.c
> +++ b/xen/arch/x86/hvm/svm/svm.c
> @@ -2480,8 +2480,7 @@ void svm_vmexit_handler(struct cpu_user_regs *regs)
>      uint64_t exit_reason;
>      struct vcpu *v = current;
>      struct vmcb_struct *vmcb = v->arch.hvm.svm.vmcb;
> -    eventinj_t eventinj;
> -    int inst_len, rc;
> +    int insn_len, rc;

I'm not really happy to see insn_len be plain int, but the task switch
case requires it to be so (at least for the time being).

Jan

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

^ permalink raw reply	[flat|nested] 18+ messages in thread
* Re: [Xen-devel] [PATCH 3/4] x86/svm: Clean up intinfo_t variables
  2019-12-04  9:43 ` [Xen-devel] [PATCH 3/4] x86/svm: Clean up intinfo_t variables Andrew Cooper
@ 2019-12-04 10:19   ` Jan Beulich
  2019-12-04 19:22     ` Andrew Cooper
  2019-12-04 19:38   ` Andrew Cooper
  2019-12-05 12:33   ` Andrew Cooper
  2 siblings, 1 reply; 18+ messages in thread
From: Jan Beulich @ 2019-12-04 10:19 UTC (permalink / raw)
  To: Andrew Cooper; +Cc: Xen-devel, Wei Liu, Roger Pau Monné

On 04.12.2019 10:43, Andrew Cooper wrote:
> --- a/xen/arch/x86/hvm/svm/intr.c
> +++ b/xen/arch/x86/hvm/svm/intr.c
> @@ -43,15 +43,13 @@ static void svm_inject_nmi(struct vcpu *v)
>  {
>      struct vmcb_struct *vmcb = v->arch.hvm.svm.vmcb;
>      u32 general1_intercepts = vmcb_get_general1_intercepts(vmcb);
> -    eventinj_t event;
>  
> -    event.bytes = 0;
> -    event.fields.v = 1;
> -    event.fields.type = X86_EVENTTYPE_NMI;
> -    event.fields.vector = 2;
> -
> -    ASSERT(vmcb->eventinj.fields.v == 0);
> -    vmcb->eventinj = event;
> +    ASSERT(!vmcb->eventinj.v);
> +    vmcb->eventinj = (intinfo_t){
> +        .vector = 2,

Perhaps TRAP_nmi here, seeing that TRAP_* are used elsewhere as well?
In any event
Reviewed-by: Jan Beulich <jbeulich@suse.com>

Jan

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

^ permalink raw reply	[flat|nested] 18+ messages in thread
* Re: [Xen-devel] [PATCH 4/4] x86/svm: Use named (bit)fields for task switch exit info
  2019-12-04  9:43 ` [Xen-devel] [PATCH 4/4] x86/svm: Use named (bit)fields for task switch exit info Andrew Cooper
@ 2019-12-04 10:24   ` Jan Beulich
  2019-12-04 20:04     ` Andrew Cooper
  0 siblings, 1 reply; 18+ messages in thread
From: Jan Beulich @ 2019-12-04 10:24 UTC (permalink / raw)
  To: Andrew Cooper; +Cc: Xen-devel, Wei Liu, Roger Pau Monné

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

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

^ permalink raw reply	[flat|nested] 18+ messages in thread
* Re: [Xen-devel] [PATCH 1/4] x86/svm: Clean up construct_vmcb()
  2019-12-04 10:06   ` Jan Beulich
@ 2019-12-04 19:21     ` Andrew Cooper
  0 siblings, 0 replies; 18+ messages in thread
From: Andrew Cooper @ 2019-12-04 19:21 UTC (permalink / raw)
  To: Jan Beulich; +Cc: Xen-devel, Wei Liu, Roger Pau Monné

On 04/12/2019 10:06, Jan Beulich wrote:
> On 04.12.2019 10:43, Andrew Cooper wrote:
>> The vmcb is zeroed on allocate - drop all explicit writes of 0.  Move
>> hvm_update_guest_efer() to co-locate it with the other control register
>> updates.
>>
>> Move the BUILD_BUG_ON() into build_assertions(), and add some offset checks
>> for fields after the large blocks of reserved fields (as these are the most
>> likely to trigger from a mis-edit).  Take the opportunity to fold 6 adjacent
>> res* fields into one.
>>
>> Finally, drop all trailing whitespace in the file.
>>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> Reviewed-by: Jan Beulich <jbeulich@suse.com>
> albeit with two (optional) suggestions:
>
>> @@ -297,14 +258,26 @@ void __init setup_vmcb_dump(void)
>>  
>>  static void __init __maybe_unused build_assertions(void)
>>  {
>> -    struct segment_register sreg;
>> +    struct vmcb_struct vmcb;
>> +
>> +    /* Build-time check of the VMCB layout. */
>> +    BUILD_BUG_ON(sizeof(vmcb) != PAGE_SIZE);
>> +    BUILD_BUG_ON(offsetof(struct vmcb_struct, _pause_filter_thresh) != 0x03c);
>> +    BUILD_BUG_ON(offsetof(struct vmcb_struct, _vintr)               != 0x060);
>> +    BUILD_BUG_ON(offsetof(struct vmcb_struct, eventinj)             != 0x0a8);
>> +    BUILD_BUG_ON(offsetof(struct vmcb_struct, es)                   != 0x400);
>> +    BUILD_BUG_ON(offsetof(struct vmcb_struct, _cpl)                 != 0x4cb);
>> +    BUILD_BUG_ON(offsetof(struct vmcb_struct, _cr4)                 != 0x548);
>> +    BUILD_BUG_ON(offsetof(struct vmcb_struct, rsp)                  != 0x5d8);
>> +    BUILD_BUG_ON(offsetof(struct vmcb_struct, rax)                  != 0x5f8);
>> +    BUILD_BUG_ON(offsetof(struct vmcb_struct, _g_pat)               != 0x668);
>>  
>>      /* Check struct segment_register against the VMCB segment layout. */
>> -    BUILD_BUG_ON(sizeof(sreg)       != 16);
>> -    BUILD_BUG_ON(sizeof(sreg.sel)   != 2);
>> -    BUILD_BUG_ON(sizeof(sreg.attr)  != 2);
>> -    BUILD_BUG_ON(sizeof(sreg.limit) != 4);
>> -    BUILD_BUG_ON(sizeof(sreg.base)  != 8);
>> +    BUILD_BUG_ON(sizeof(vmcb.es)       != 16);
>> +    BUILD_BUG_ON(sizeof(vmcb.es.sel)   != 2);
>> +    BUILD_BUG_ON(sizeof(vmcb.es.attr)  != 2);
>> +    BUILD_BUG_ON(sizeof(vmcb.es.limit) != 4);
>> +    BUILD_BUG_ON(sizeof(vmcb.es.base)  != 8);
>>      BUILD_BUG_ON(offsetof(struct segment_register, sel)   != 0);
>>      BUILD_BUG_ON(offsetof(struct segment_register, attr)  != 2);
>>      BUILD_BUG_ON(offsetof(struct segment_register, limit) != 4);
> For the ones only supplying context here, how about using the
> shorter offsetof(typeof(vmcb.es), ...), also tying things better
> to the prior sizeof() checks? The same, albeit to a lesser degree,
> might then go for the earlier block, which could use the shorter
> typeof(vmcb).

Fixed.

>
>> --- a/xen/include/asm-x86/hvm/svm/vmcb.h
>> +++ b/xen/include/asm-x86/hvm/svm/vmcb.h
>> @@ -406,12 +406,7 @@ struct vmcb_struct {
>>      u32 _exception_intercepts;  /* offset 0x08 - cleanbit 0 */
>>      u32 _general1_intercepts;   /* offset 0x0C - cleanbit 0 */
>>      u32 _general2_intercepts;   /* offset 0x10 - cleanbit 0 */
>> -    u32 res01;                  /* offset 0x14 */
>> -    u64 res02;                  /* offset 0x18 */
>> -    u64 res03;                  /* offset 0x20 */
>> -    u64 res04;                  /* offset 0x28 */
>> -    u64 res05;                  /* offset 0x30 */
>> -    u32 res06;                  /* offset 0x38 */
>> +    u32 res01[10];
> Was it intentional for the comment to be lost altogether?

Yes.  The offset is trivial (0x10 + sizeof(u32)) and of no interest.

Omitting it increases readability by helping to highlight where the
reserved blocks are.

~Andrew

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

^ permalink raw reply	[flat|nested] 18+ messages in thread
* Re: [Xen-devel] [PATCH 3/4] x86/svm: Clean up intinfo_t variables
  2019-12-04 10:19   ` Jan Beulich
@ 2019-12-04 19:22     ` Andrew Cooper
  0 siblings, 0 replies; 18+ messages in thread
From: Andrew Cooper @ 2019-12-04 19:22 UTC (permalink / raw)
  To: Jan Beulich; +Cc: Xen-devel, Wei Liu, Roger Pau Monné

On 04/12/2019 10:19, Jan Beulich wrote:
> On 04.12.2019 10:43, Andrew Cooper wrote:
>> --- a/xen/arch/x86/hvm/svm/intr.c
>> +++ b/xen/arch/x86/hvm/svm/intr.c
>> @@ -43,15 +43,13 @@ static void svm_inject_nmi(struct vcpu *v)
>>  {
>>      struct vmcb_struct *vmcb = v->arch.hvm.svm.vmcb;
>>      u32 general1_intercepts = vmcb_get_general1_intercepts(vmcb);
>> -    eventinj_t event;
>>  
>> -    event.bytes = 0;
>> -    event.fields.v = 1;
>> -    event.fields.type = X86_EVENTTYPE_NMI;
>> -    event.fields.vector = 2;
>> -
>> -    ASSERT(vmcb->eventinj.fields.v == 0);
>> -    vmcb->eventinj = event;
>> +    ASSERT(!vmcb->eventinj.v);
>> +    vmcb->eventinj = (intinfo_t){
>> +        .vector = 2,
> Perhaps TRAP_nmi here, seeing that TRAP_* are used elsewhere as well?

Fixed.

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

Thanks.

~Andrew

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

^ permalink raw reply	[flat|nested] 18+ messages in thread
* Re: [Xen-devel] [PATCH 3/4] x86/svm: Clean up intinfo_t variables
  2019-12-04  9:43 ` [Xen-devel] [PATCH 3/4] x86/svm: Clean up intinfo_t variables Andrew Cooper
  2019-12-04 10:19   ` Jan Beulich
@ 2019-12-04 19:38   ` Andrew Cooper
  2019-12-05  9:11     ` Jan Beulich
  2019-12-05 12:33   ` Andrew Cooper
  2 siblings, 1 reply; 18+ messages in thread
From: Andrew Cooper @ 2019-12-04 19:38 UTC (permalink / raw)
  To: Xen-devel; +Cc: Wei Liu, Jan Beulich, Roger Pau Monné

On 04/12/2019 09:43, Andrew Cooper wrote:
> @@ -420,10 +420,10 @@ struct vmcb_struct {
>      u64 exitcode;               /* offset 0x70 */
>      u64 exitinfo1;              /* offset 0x78 */
>      u64 exitinfo2;              /* offset 0x80 */
> -    eventinj_t  exitintinfo;    /* offset 0x88 */
> +    intinfo_t exitintinfo;      /* offset 0x88 */
>      u64 _np_enable;             /* offset 0x90 - cleanbit 4 */
>      u64 res08[2];
> -    eventinj_t  eventinj;       /* offset 0xA8 */
> +    intinfo_t eventinj;         /* offset 0xA8 */
>      u64 _h_cr3;                 /* offset 0xB0 - cleanbit 4 */
>      virt_ext_t virt_ext;        /* offset 0xB8 */
>      vmcbcleanbits_t cleanbits;  /* offset 0xC0 */

On second thoughts, I'm considering using this opportunity to switch to
exit_int_info and event_inj.

There are a lot of exit-prefixed names which are easy to confuse at a
glance.

~Andrew

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

^ permalink raw reply	[flat|nested] 18+ messages in thread
* Re: [Xen-devel] [PATCH 4/4] x86/svm: Use named (bit)fields for task switch exit info
  2019-12-04 10:24   ` Jan Beulich
@ 2019-12-04 20:04     ` Andrew Cooper
  2019-12-05  9:05       ` Jan Beulich
  0 siblings, 1 reply; 18+ messages in thread
From: Andrew Cooper @ 2019-12-04 20:04 UTC (permalink / raw)
  To: Jan Beulich; +Cc: Xen-devel, Wei Liu, Roger Pau Monné

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

^ permalink raw reply related	[flat|nested] 18+ messages in thread
* Re: [Xen-devel] [PATCH 4/4] x86/svm: Use named (bit)fields for task switch exit info
  2019-12-04 20:04     ` Andrew Cooper
@ 2019-12-05  9:05       ` Jan Beulich
  0 siblings, 0 replies; 18+ messages in thread
From: Jan Beulich @ 2019-12-05  9:05 UTC (permalink / raw)
  To: Andrew Cooper; +Cc: Xen-devel, Wei Liu, Roger Pau Monné

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

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

^ permalink raw reply	[flat|nested] 18+ messages in thread
* Re: [Xen-devel] [PATCH 3/4] x86/svm: Clean up intinfo_t variables
  2019-12-04 19:38   ` Andrew Cooper
@ 2019-12-05  9:11     ` Jan Beulich
  0 siblings, 0 replies; 18+ messages in thread
From: Jan Beulich @ 2019-12-05  9:11 UTC (permalink / raw)
  To: Andrew Cooper; +Cc: Xen-devel, Wei Liu, Roger Pau Monné

On 04.12.2019 20:38, Andrew Cooper wrote:
> On 04/12/2019 09:43, Andrew Cooper wrote:
>> @@ -420,10 +420,10 @@ struct vmcb_struct {
>>      u64 exitcode;               /* offset 0x70 */
>>      u64 exitinfo1;              /* offset 0x78 */
>>      u64 exitinfo2;              /* offset 0x80 */
>> -    eventinj_t  exitintinfo;    /* offset 0x88 */
>> +    intinfo_t exitintinfo;      /* offset 0x88 */
>>      u64 _np_enable;             /* offset 0x90 - cleanbit 4 */
>>      u64 res08[2];
>> -    eventinj_t  eventinj;       /* offset 0xA8 */
>> +    intinfo_t eventinj;         /* offset 0xA8 */
>>      u64 _h_cr3;                 /* offset 0xB0 - cleanbit 4 */
>>      virt_ext_t virt_ext;        /* offset 0xB8 */
>>      vmcbcleanbits_t cleanbits;  /* offset 0xC0 */
> 
> On second thoughts, I'm considering using this opportunity to switch to
> exit_int_info and event_inj.
> 
> There are a lot of exit-prefixed names which are easy to confuse at a
> glance.

Fine with me, my R-b stands with this extra purely mechanical
adjustment.

Jan

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

^ permalink raw reply	[flat|nested] 18+ messages in thread
* [Xen-devel] [PATCH 5/4] x86/svm: Minor cleanup to start_svm()
  2019-12-04  9:43 [Xen-devel] [PATCH 0/4] x86/svm: (Post TASK_SWITCH) cleanup Andrew Cooper
                   ` (3 preceding siblings ...)
  2019-12-04  9:43 ` [Xen-devel] [PATCH 4/4] x86/svm: Use named (bit)fields for task switch exit info Andrew Cooper
@ 2019-12-05 10:51 ` Andrew Cooper
  2019-12-05 10:53   ` Jan Beulich
  4 siblings, 1 reply; 18+ messages in thread
From: Andrew Cooper @ 2019-12-05 10:51 UTC (permalink / raw)
  To: Xen-devel; +Cc: Andrew Cooper, Wei Liu, Jan Beulich, Roger Pau Monné

The function is init, so can use boot_cpu_data directly.

There is no need to write 0 to svm_feature_flags in the case of a CPUID
mismatch (not least because this is dead code on real hardware), and no need
to use locked atomic operations.

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 | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/xen/arch/x86/hvm/svm/svm.c b/xen/arch/x86/hvm/svm/svm.c
index af3d45fe56..806bf91171 100644
--- a/xen/arch/x86/hvm/svm/svm.c
+++ b/xen/arch/x86/hvm/svm/svm.c
@@ -1632,14 +1632,14 @@ const struct hvm_function_table * __init start_svm(void)
 
     setup_vmcb_dump();
 
-    svm_feature_flags = (current_cpu_data.extended_cpuid_level >= 0x8000000A ?
-                         cpuid_edx(0x8000000A) : 0);
+    if ( boot_cpu_data.extended_cpuid_level >= 0x8000000a )
+        svm_feature_flags = cpuid_edx(0x8000000a);
 
     printk("SVM: Supported advanced features:\n");
 
     /* DecodeAssists fast paths assume nextrip is valid for fast rIP update. */
     if ( !cpu_has_svm_nrips )
-        clear_bit(SVM_FEATURE_DECODEASSISTS, &svm_feature_flags);
+        __clear_bit(SVM_FEATURE_DECODEASSISTS, &svm_feature_flags);
 
     if ( cpu_has_tsc_ratio )
         svm_function_table.tsc_scaling.ratio_frac_bits = 32;
-- 
2.11.0


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

^ permalink raw reply related	[flat|nested] 18+ messages in thread
* Re: [Xen-devel] [PATCH 5/4] x86/svm: Minor cleanup to start_svm()
  2019-12-05 10:51 ` [Xen-devel] [PATCH 5/4] x86/svm: Minor cleanup to start_svm() Andrew Cooper
@ 2019-12-05 10:53   ` Jan Beulich
  0 siblings, 0 replies; 18+ messages in thread
From: Jan Beulich @ 2019-12-05 10:53 UTC (permalink / raw)
  To: Andrew Cooper; +Cc: Xen-devel, Wei Liu, Roger Pau Monné

On 05.12.2019 11:51, Andrew Cooper wrote:
> The function is init, so can use boot_cpu_data directly.
> 
> There is no need to write 0 to svm_feature_flags in the case of a CPUID
> mismatch (not least because this is dead code on real hardware), and no need
> to use locked atomic operations.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

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

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

^ permalink raw reply	[flat|nested] 18+ messages in thread
* Re: [Xen-devel] [PATCH 3/4] x86/svm: Clean up intinfo_t variables
  2019-12-04  9:43 ` [Xen-devel] [PATCH 3/4] x86/svm: Clean up intinfo_t variables Andrew Cooper
  2019-12-04 10:19   ` Jan Beulich
  2019-12-04 19:38   ` Andrew Cooper
@ 2019-12-05 12:33   ` Andrew Cooper
  2 siblings, 0 replies; 18+ messages in thread
From: Andrew Cooper @ 2019-12-05 12:33 UTC (permalink / raw)
  To: Xen-devel; +Cc: Wei Liu, Jan Beulich, Roger Pau Monné

On 04/12/2019 09:43, Andrew Cooper wrote:
> The type name is poor because the type is also used for the IDT vectoring
> field, not just for the event injection field.  Rename it to intinfo_t which
> is how the APM refers to the data.
>
> Rearrange the union to drop the .fields infix, and rename bytes to the more
> common raw.
>
> While adjusting all call sites, fix up style issues and make use of structure
> assignments where applicable.
>
> 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/intr.c        | 32 ++++++++----------
>  xen/arch/x86/hvm/svm/nestedsvm.c   | 28 +++++++---------
>  xen/arch/x86/hvm/svm/svm.c         | 68 ++++++++++++++++++--------------------
>  xen/arch/x86/hvm/svm/svmdebug.c    | 12 +++----
>  xen/include/asm-x86/hvm/svm/vmcb.h | 22 ++++++------
>  5 files changed, 75 insertions(+), 87 deletions(-)
>
> diff --git a/xen/arch/x86/hvm/svm/intr.c b/xen/arch/x86/hvm/svm/intr.c
> index ff755165cd..4eede5cc23 100644
> --- a/xen/arch/x86/hvm/svm/intr.c
> +++ b/xen/arch/x86/hvm/svm/intr.c
> @@ -43,15 +43,13 @@ static void svm_inject_nmi(struct vcpu *v)
>  {
>      struct vmcb_struct *vmcb = v->arch.hvm.svm.vmcb;
>      u32 general1_intercepts = vmcb_get_general1_intercepts(vmcb);
> -    eventinj_t event;
>  
> -    event.bytes = 0;
> -    event.fields.v = 1;
> -    event.fields.type = X86_EVENTTYPE_NMI;
> -    event.fields.vector = 2;
> -
> -    ASSERT(vmcb->eventinj.fields.v == 0);
> -    vmcb->eventinj = event;
> +    ASSERT(!vmcb->eventinj.v);
> +    vmcb->eventinj = (intinfo_t){
> +        .vector = 2,
> +        .type = X86_EVENTTYPE_NMI,
> +        .v = true,
> +    };

And in a surprise move (not really), CentOS 6 isn't happy with this.

I'll revert back to the previous way of filling in eventinj (until we
can actually decide on a newer tools baseline).

~Andrew

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

^ permalink raw reply	[flat|nested] 18+ messages in thread
end of thread, other threads:[~2019-12-05 12:33 UTC | newest]

Thread overview: 18+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-12-04  9:43 [Xen-devel] [PATCH 0/4] x86/svm: (Post TASK_SWITCH) cleanup Andrew Cooper
2019-12-04  9:43 ` [Xen-devel] [PATCH 1/4] x86/svm: Clean up construct_vmcb() Andrew Cooper
2019-12-04 10:06   ` Jan Beulich
2019-12-04 19:21     ` Andrew Cooper
2019-12-04  9:43 ` [Xen-devel] [PATCH 2/4] x86/svm: Don't shadow variables in svm_vmexit_handler() Andrew Cooper
2019-12-04 10:10   ` Jan Beulich
2019-12-04  9:43 ` [Xen-devel] [PATCH 3/4] x86/svm: Clean up intinfo_t variables Andrew Cooper
2019-12-04 10:19   ` Jan Beulich
2019-12-04 19:22     ` Andrew Cooper
2019-12-04 19:38   ` Andrew Cooper
2019-12-05  9:11     ` Jan Beulich
2019-12-05 12:33   ` Andrew Cooper
2019-12-04  9:43 ` [Xen-devel] [PATCH 4/4] x86/svm: Use named (bit)fields for task switch exit info Andrew Cooper
2019-12-04 10:24   ` Jan Beulich
2019-12-04 20:04     ` Andrew Cooper
2019-12-05  9:05       ` Jan Beulich
2019-12-05 10:51 ` [Xen-devel] [PATCH 5/4] x86/svm: Minor cleanup to start_svm() Andrew Cooper
2019-12-05 10:53   ` Jan Beulich

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.