[PATCH 0/6] x86/vmx: Misc fixes and improvements

[PATCH 0/6] x86/vmx: Misc fixes and improvements

[Andrew Cooper]

Patch 1 might want to be considered for 4.11.  All others are 4.12 material at
this point.

Andrew Cooper (6):
  x86/vmx: Fix handing of MSR_DEBUGCTL on VMExit
  x86: Improvements to ler debugging
  x86/pat: Simplify host PAT handling
  x86/vmx: Simplify PAT handling during vcpu construction
  x86/vmx: Defer vmx_vmcs_exit() as long as possible in construct_vmcs()
  x86/vmx: Drop VMX signal for full real-mode

 docs/misc/xen-command-line.markdown |  6 ++++
 xen/arch/x86/acpi/suspend.c         |  3 +-
 xen/arch/x86/cpu/common.c           |  9 +-----
 xen/arch/x86/hvm/mtrr.c             |  2 +-
 xen/arch/x86/hvm/vmx/entry.S        |  9 ++++++
 xen/arch/x86/hvm/vmx/vmcs.c         | 22 +++++---------
 xen/arch/x86/hvm/vmx/vmx.c          | 14 ++-------
 xen/arch/x86/traps.c                | 58 ++++++++++++++++++-------------------
 xen/arch/x86/x86_64/traps.c         | 14 ++++++---
 xen/include/asm-x86/cpufeature.h    |  2 +-
 xen/include/asm-x86/cpufeatures.h   |  1 +
 xen/include/asm-x86/msr.h           |  2 +-
 xen/include/asm-x86/processor.h     |  7 ++++-
 xen/include/asm-x86/x86_64/page.h   |  8 +++++
 14 files changed, 83 insertions(+), 74 deletions(-)

-- 
2.1.4


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

[PATCH 1/6] x86/vmx: Fix handing of MSR_DEBUGCTL on VMExit

[Andrew Cooper]

Currently, whenever the guest writes a nonzero value to MSR_DEBUGCTL, Xen
updates a host MSR load list entry with the current hardware value of
MSR_DEBUGCTL.  This is wrong.

On VMExit, hardware automatically resets MSR_DEBUGCTL to 0.  The only case
where different behaviour is needed is if Xen is debugging itself, and this
needs setting up unconditionally for the lifetime of the VM, rather than based
on guest actions.

This could be fixed by using a host MSR load list entry set up during
construct_vmcs().  However, a more efficient option is to use an alternative
block in the VMExit path, keyed on whether hypervisor debugging has been
enabled.

In order to set this up, drop the per cpu ler_msr variable (as there is no
point having it per cpu when it will be the same everywhere), and use a single
read_mostly variable instead.  Split calc_ler_msr() out of percpu_traps_init()
for clarity, and only perform the calculation on the first call.

Finally, clean up do_debug().  Rearrange it to have a common tail, and call
out the LBR behaviour specifically.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Jun Nakajima <jun.nakajima@intel.com>
CC: Kevin Tian <kevin.tian@intel.com>
CC: Wei Liu <wei.liu2@citrix.com>
CC: Roger Pau Monné <roger.pau@citrix.com>

Initially, I tried to have a common xen_msr_debugctl variable, but
rip-relative addresses don't resolve correctly in alternative blocks.
LBR-only has been fine for ages, and I don't see that changing any time soon.
---
 xen/arch/x86/hvm/vmx/entry.S      |  9 ++++++
 xen/arch/x86/hvm/vmx/vmx.c        |  3 +-
 xen/arch/x86/traps.c              | 58 +++++++++++++++++++--------------------
 xen/arch/x86/x86_64/traps.c       |  7 +++--
 xen/include/asm-x86/cpufeature.h  |  1 +
 xen/include/asm-x86/cpufeatures.h |  1 +
 xen/include/asm-x86/msr.h         |  2 +-
 7 files changed, 45 insertions(+), 36 deletions(-)

diff --git a/xen/arch/x86/hvm/vmx/entry.S b/xen/arch/x86/hvm/vmx/entry.S
index aa2f103..afd552f 100644
--- a/xen/arch/x86/hvm/vmx/entry.S
+++ b/xen/arch/x86/hvm/vmx/entry.S
@@ -41,6 +41,15 @@ ENTRY(vmx_asm_vmexit_handler)
         SPEC_CTRL_ENTRY_FROM_HVM    /* Req: b=curr %rsp=regs/cpuinfo, Clob: acd */
         /* WARNING! `ret`, `call *`, `jmp *` not safe before this point. */
 
+        /* Hardware clears MSR_DEBUGCTL on VMExit.  Reinstate it if debugging Xen. */
+        .macro restore_lbr
+            mov $IA32_DEBUGCTLMSR_LBR, %eax
+            mov $MSR_IA32_DEBUGCTLMSR, %ecx
+            xor %edx, %edx
+            wrmsr
+        .endm
+        ALTERNATIVE "", restore_lbr, X86_FEATURE_XEN_LBR
+
         mov  %rsp,%rdi
         call vmx_vmexit_handler
 
diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c
index 9707514..33d39f6 100644
--- a/xen/arch/x86/hvm/vmx/vmx.c
+++ b/xen/arch/x86/hvm/vmx/vmx.c
@@ -3120,8 +3120,7 @@ static int vmx_msr_write_intercept(unsigned int msr, uint64_t msr_content)
                     }
         }
 
-        if ( (rc < 0) ||
-             (msr_content && (vmx_add_host_load_msr(msr) < 0)) )
+        if ( rc < 0 )
             hvm_inject_hw_exception(TRAP_machine_check, X86_EVENT_NO_EC);
         else
             __vmwrite(GUEST_IA32_DEBUGCTL, msr_content);
diff --git a/xen/arch/x86/traps.c b/xen/arch/x86/traps.c
index 8a99174..74784a2 100644
--- a/xen/arch/x86/traps.c
+++ b/xen/arch/x86/traps.c
@@ -96,8 +96,6 @@ string_param("nmi", opt_nmi);
 DEFINE_PER_CPU(uint64_t, efer);
 static DEFINE_PER_CPU(unsigned long, last_extable_addr);
 
-DEFINE_PER_CPU_READ_MOSTLY(u32, ler_msr);
-
 DEFINE_PER_CPU_READ_MOSTLY(struct desc_struct *, gdt_table);
 DEFINE_PER_CPU_READ_MOSTLY(struct desc_struct *, compat_gdt_table);
 
@@ -117,6 +115,9 @@ integer_param("debug_stack_lines", debug_stack_lines);
 static bool opt_ler;
 boolean_param("ler", opt_ler);
 
+/* LastExceptionFromIP on this hardware.  Zero if LER is not in use. */
+uint32_t __read_mostly ler_msr;
+
 #define stack_words_per_line 4
 #define ESP_BEFORE_EXCEPTION(regs) ((unsigned long *)regs->rsp)
 
@@ -1764,17 +1765,6 @@ void do_device_not_available(struct cpu_user_regs *regs)
     return;
 }
 
-static void ler_enable(void)
-{
-    u64 debugctl;
-
-    if ( !this_cpu(ler_msr) )
-        return;
-
-    rdmsrl(MSR_IA32_DEBUGCTLMSR, debugctl);
-    wrmsrl(MSR_IA32_DEBUGCTLMSR, debugctl | IA32_DEBUGCTLMSR_LBR);
-}
-
 void do_debug(struct cpu_user_regs *regs)
 {
     unsigned long dr6;
@@ -1870,13 +1860,13 @@ void do_debug(struct cpu_user_regs *regs)
     v->arch.debugreg[6] |= (dr6 & ~X86_DR6_DEFAULT);
     v->arch.debugreg[6] &= (dr6 | ~X86_DR6_DEFAULT);
 
-    ler_enable();
     pv_inject_hw_exception(TRAP_debug, X86_EVENT_NO_EC);
-    return;
 
  out:
-    ler_enable();
-    return;
+
+    /* #DB automatically disabled LBR.  Reinstate it if debugging Xen. */
+    if ( cpu_has_xen_lbr )
+        wrmsrl(MSR_IA32_DEBUGCTLMSR, IA32_DEBUGCTLMSR_LBR);
 }
 
 static void __init noinline __set_intr_gate(unsigned int n,
@@ -1920,38 +1910,46 @@ void load_TR(void)
         : "=m" (old_gdt) : "rm" (TSS_ENTRY << 3), "m" (tss_gdt) : "memory" );
 }
 
-void percpu_traps_init(void)
+static uint32_t calc_ler_msr(void)
 {
-    subarch_percpu_traps_init();
-
-    if ( !opt_ler )
-        return;
-
     switch ( boot_cpu_data.x86_vendor )
     {
     case X86_VENDOR_INTEL:
         switch ( boot_cpu_data.x86 )
         {
         case 6:
-            this_cpu(ler_msr) = MSR_IA32_LASTINTFROMIP;
-            break;
+            return MSR_IA32_LASTINTFROMIP;
+
         case 15:
-            this_cpu(ler_msr) = MSR_P4_LER_FROM_LIP;
-            break;
+            return MSR_P4_LER_FROM_LIP;
         }
         break;
+
     case X86_VENDOR_AMD:
         switch ( boot_cpu_data.x86 )
         {
         case 6:
         case 0xf ... 0x17:
-            this_cpu(ler_msr) = MSR_IA32_LASTINTFROMIP;
-            break;
+            return MSR_IA32_LASTINTFROMIP;
         }
         break;
     }
 
-    ler_enable();
+    return 0;
+}
+
+void percpu_traps_init(void)
+{
+    subarch_percpu_traps_init();
+
+    if ( !opt_ler )
+        return;
+
+    if ( !ler_msr && (ler_msr = calc_ler_msr()) )
+        setup_force_cpu_cap(X86_FEATURE_XEN_LBR);
+
+    if ( cpu_has_xen_lbr )
+        wrmsrl(MSR_IA32_DEBUGCTLMSR, IA32_DEBUGCTLMSR_LBR);
 }
 
 void __init init_idt_traps(void)
diff --git a/xen/arch/x86/x86_64/traps.c b/xen/arch/x86/x86_64/traps.c
index f7f6928..b040185 100644
--- a/xen/arch/x86/x86_64/traps.c
+++ b/xen/arch/x86/x86_64/traps.c
@@ -144,11 +144,12 @@ void show_registers(const struct cpu_user_regs *regs)
     printk("CPU:    %d\n", smp_processor_id());
     _show_registers(&fault_regs, fault_crs, context, v);
 
-    if ( this_cpu(ler_msr) && !guest_mode(regs) )
+    if ( ler_msr && !guest_mode(regs) )
     {
         u64 from, to;
-        rdmsrl(this_cpu(ler_msr), from);
-        rdmsrl(this_cpu(ler_msr) + 1, to);
+
+        rdmsrl(ler_msr, from);
+        rdmsrl(ler_msr + 1, to);
         printk("ler: %016lx -> %016lx\n", from, to);
     }
 }
diff --git a/xen/include/asm-x86/cpufeature.h b/xen/include/asm-x86/cpufeature.h
index 2cf8f7e..b237da1 100644
--- a/xen/include/asm-x86/cpufeature.h
+++ b/xen/include/asm-x86/cpufeature.h
@@ -113,6 +113,7 @@
 #define cpu_has_aperfmperf      boot_cpu_has(X86_FEATURE_APERFMPERF)
 #define cpu_has_lfence_dispatch boot_cpu_has(X86_FEATURE_LFENCE_DISPATCH)
 #define cpu_has_no_xpti         boot_cpu_has(X86_FEATURE_NO_XPTI)
+#define cpu_has_xen_lbr         boot_cpu_has(X86_FEATURE_XEN_LBR)
 
 enum _cache_type {
     CACHE_TYPE_NULL = 0,
diff --git a/xen/include/asm-x86/cpufeatures.h b/xen/include/asm-x86/cpufeatures.h
index b90aa2d..8e5cc53 100644
--- a/xen/include/asm-x86/cpufeatures.h
+++ b/xen/include/asm-x86/cpufeatures.h
@@ -32,3 +32,4 @@ XEN_CPUFEATURE(SC_RSB_PV,       (FSCAPINTS+0)*32+18) /* RSB overwrite needed for
 XEN_CPUFEATURE(SC_RSB_HVM,      (FSCAPINTS+0)*32+19) /* RSB overwrite needed for HVM */
 XEN_CPUFEATURE(NO_XPTI,         (FSCAPINTS+0)*32+20) /* XPTI mitigation not in use */
 XEN_CPUFEATURE(SC_MSR_IDLE,     (FSCAPINTS+0)*32+21) /* (SC_MSR_PV || SC_MSR_HVM) && default_xen_spec_ctrl */
+XEN_CPUFEATURE(XEN_LBR,         (FSCAPINTS+0)*32+22) /* Xen uses MSR_DEBUGCTL.LBR */
diff --git a/xen/include/asm-x86/msr.h b/xen/include/asm-x86/msr.h
index f14f265..9f6d3b2 100644
--- a/xen/include/asm-x86/msr.h
+++ b/xen/include/asm-x86/msr.h
@@ -241,7 +241,7 @@ static inline void write_efer(uint64_t val)
     wrmsrl(MSR_EFER, val);
 }
 
-DECLARE_PER_CPU(u32, ler_msr);
+extern uint32_t ler_msr;
 
 DECLARE_PER_CPU(uint32_t, tsc_aux);
 
-- 
2.1.4


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

[PATCH 2/6] x86: Improvements to ler debugging

[Andrew Cooper]

 * Command line documentation for what the option does.
 * Implement a canonicalise_addr() helper and replace the opencoded use in
   sign_extend_msr()
 * Canonicalise the ler pointers and print symbol information.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Jun Nakajima <jun.nakajima@intel.com>
CC: Kevin Tian <kevin.tian@intel.com>
CC: Wei Liu <wei.liu2@citrix.com>
CC: Roger Pau Monné <roger.pau@citrix.com>
---
 docs/misc/xen-command-line.markdown | 6 ++++++
 xen/arch/x86/hvm/vmx/vmx.c          | 7 +------
 xen/arch/x86/x86_64/traps.c         | 7 ++++++-
 xen/include/asm-x86/x86_64/page.h   | 8 ++++++++
 4 files changed, 21 insertions(+), 7 deletions(-)

diff --git a/docs/misc/xen-command-line.markdown b/docs/misc/xen-command-line.markdown
index 8712a83..1212ebd 100644
--- a/docs/misc/xen-command-line.markdown
+++ b/docs/misc/xen-command-line.markdown
@@ -1245,6 +1245,12 @@ if left disabled by the BIOS.
 ### ler (x86)
 > `= <boolean>`
 
+> Default: 0
+
+This option is intended for debugging purposes only.  Enable MSR_DEBUGCTL.LBR
+in hypervisor context to be able to dump the Last Interrupt/Exception To/From
+record with other registers.
+
 ### loglvl
 > `= <level>[/<rate-limited level>]` where level is `none | error | warning | info | debug | all`
 
diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c
index 33d39f6..8049264 100644
--- a/xen/arch/x86/hvm/vmx/vmx.c
+++ b/xen/arch/x86/hvm/vmx/vmx.c
@@ -4183,12 +4183,7 @@ static void sign_extend_msr(u32 msr, int type)
     struct vmx_msr_entry *entry;
 
     if ( (entry = vmx_find_msr(msr, type)) != NULL )
-    {
-        if ( entry->data & VADDR_TOP_BIT )
-            entry->data |= CANONICAL_MASK;
-        else
-            entry->data &= ~CANONICAL_MASK;
-    }
+        entry->data = canonicalise_addr(entry->data);
 }
 
 static void bdw_erratum_bdf14_fixup(void)
diff --git a/xen/arch/x86/x86_64/traps.c b/xen/arch/x86/x86_64/traps.c
index b040185..ed02b78 100644
--- a/xen/arch/x86/x86_64/traps.c
+++ b/xen/arch/x86/x86_64/traps.c
@@ -150,7 +150,12 @@ void show_registers(const struct cpu_user_regs *regs)
 
         rdmsrl(ler_msr, from);
         rdmsrl(ler_msr + 1, to);
-        printk("ler: %016lx -> %016lx\n", from, to);
+
+        /* Upper bits may store metadata.  Re-canonicalise for printing. */
+        printk("ler: from %016"PRIx64" [%ps]\n",
+               from, _p(canonicalise_addr(from)));
+        printk("       to %016"PRIx64" [%ps]\n",
+               to, _p(canonicalise_addr(to)));
     }
 }
 
diff --git a/xen/include/asm-x86/x86_64/page.h b/xen/include/asm-x86/x86_64/page.h
index 05a0334..4fe0205 100644
--- a/xen/include/asm-x86/x86_64/page.h
+++ b/xen/include/asm-x86/x86_64/page.h
@@ -34,6 +34,14 @@
 
 #ifndef __ASSEMBLY__
 
+static inline unsigned long canonicalise_addr(unsigned long addr)
+{
+    if ( addr & VADDR_TOP_BIT )
+        return addr | CANONICAL_MASK;
+    else
+        return addr & ~CANONICAL_MASK;
+}
+
 #include <asm/types.h>
 
 #include <xen/pdx.h>
-- 
2.1.4


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

[PATCH 3/6] x86/pat: Simplify host PAT handling

[Andrew Cooper]

With the removal of the 32bit hypervisor build, host_pat is a constant value.
Drop the variable and the redundant cpu_has_pat predicate, and use a define
instead.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Wei Liu <wei.liu2@citrix.com>
CC: Roger Pau Monné <roger.pau@citrix.com>
---
 xen/arch/x86/acpi/suspend.c      | 3 +--
 xen/arch/x86/cpu/common.c        | 9 +--------
 xen/arch/x86/hvm/mtrr.c          | 2 +-
 xen/include/asm-x86/cpufeature.h | 1 -
 xen/include/asm-x86/processor.h  | 7 ++++++-
 5 files changed, 9 insertions(+), 13 deletions(-)

diff --git a/xen/arch/x86/acpi/suspend.c b/xen/arch/x86/acpi/suspend.c
index 044bd81..eecf357 100644
--- a/xen/arch/x86/acpi/suspend.c
+++ b/xen/arch/x86/acpi/suspend.c
@@ -95,8 +95,7 @@ void restore_rest_processor_state(void)
     /* Reload FPU state on next FPU use. */
     stts();
 
-    if (cpu_has_pat)
-        wrmsrl(MSR_IA32_CR_PAT, host_pat);
+    wrmsrl(MSR_IA32_CR_PAT, XEN_MSR_PAT);
 
     mtrr_bp_restore();
 }
diff --git a/xen/arch/x86/cpu/common.c b/xen/arch/x86/cpu/common.c
index 528aff1..3548b12 100644
--- a/xen/arch/x86/cpu/common.c
+++ b/xen/arch/x86/cpu/common.c
@@ -47,12 +47,6 @@ unsigned int paddr_bits __read_mostly = 36;
 unsigned int hap_paddr_bits __read_mostly = 36;
 unsigned int vaddr_bits __read_mostly = VADDR_BITS;
 
-/*
- * Default host IA32_CR_PAT value to cover all memory types.
- * BIOS usually sets it to 0x07040600070406.
- */
-u64 host_pat = 0x050100070406;
-
 static unsigned int cleared_caps[NCAPINTS];
 static unsigned int forced_caps[NCAPINTS];
 
@@ -814,8 +808,7 @@ void cpu_init(void)
 	if (opt_cpu_info)
 		printk("Initializing CPU#%d\n", cpu);
 
-	if (cpu_has_pat)
-		wrmsrl(MSR_IA32_CR_PAT, host_pat);
+	wrmsrl(MSR_IA32_CR_PAT, XEN_MSR_PAT);
 
 	/* Install correct page table. */
 	write_ptbase(current);
diff --git a/xen/arch/x86/hvm/mtrr.c b/xen/arch/x86/hvm/mtrr.c
index a61cc1e..c78e5c1 100644
--- a/xen/arch/x86/hvm/mtrr.c
+++ b/xen/arch/x86/hvm/mtrr.c
@@ -125,7 +125,7 @@ static int __init hvm_mtrr_pat_init(void)
     {
         for ( j = 0; j < PAT_TYPE_NUMS; j++ )
         {
-            if ( pat_cr_2_paf(host_pat, j) == i )
+            if ( pat_cr_2_paf(XEN_MSR_PAT, j) == i )
             {
                 pat_entry_tbl[i] = j;
                 break;
diff --git a/xen/include/asm-x86/cpufeature.h b/xen/include/asm-x86/cpufeature.h
index b237da1..4bc6c91 100644
--- a/xen/include/asm-x86/cpufeature.h
+++ b/xen/include/asm-x86/cpufeature.h
@@ -37,7 +37,6 @@
 #define cpu_has_sep             boot_cpu_has(X86_FEATURE_SEP)
 #define cpu_has_mtrr            1
 #define cpu_has_pge             1
-#define cpu_has_pat             1
 #define cpu_has_pse36           boot_cpu_has(X86_FEATURE_PSE36)
 #define cpu_has_clflush         boot_cpu_has(X86_FEATURE_CLFLUSH)
 #define cpu_has_mmx             1
diff --git a/xen/include/asm-x86/processor.h b/xen/include/asm-x86/processor.h
index 9924cdf..ac1577c 100644
--- a/xen/include/asm-x86/processor.h
+++ b/xen/include/asm-x86/processor.h
@@ -97,6 +97,12 @@
                           X86_EFLAGS_NT|X86_EFLAGS_DF|X86_EFLAGS_IF|    \
                           X86_EFLAGS_TF)
 
+/*
+ * Host IA32_CR_PAT value to cover all memory types.  This is not the default
+ * MSR_PAT value, and is an ABI with PV guests.
+ */
+#define XEN_MSR_PAT 0x050100070406ul
+
 #ifndef __ASSEMBLY__
 
 struct domain;
@@ -145,7 +151,6 @@ extern bool probe_cpuid_faulting(void);
 extern void ctxt_switch_levelling(const struct vcpu *next);
 extern void (*ctxt_switch_masking)(const struct vcpu *next);
 
-extern u64 host_pat;
 extern bool_t opt_cpu_info;
 extern u32 cpuid_ext_features;
 extern u64 trampoline_misc_enable_off;
-- 
2.1.4


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

[PATCH 4/6] x86/vmx: Simplify PAT handling during vcpu construction

[Andrew Cooper]

The host PAT value is a compile time constant, and doesn't need to be read out
of hardware.  Merge this if block into the previous block, which has an
identical condition.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Jun Nakajima <jun.nakajima@intel.com>
CC: Kevin Tian <kevin.tian@intel.com>
CC: Wei Liu <wei.liu2@citrix.com>
CC: Roger Pau Monné <roger.pau@citrix.com>
---
 xen/arch/x86/hvm/vmx/vmcs.c | 12 ++----------
 1 file changed, 2 insertions(+), 10 deletions(-)

diff --git a/xen/arch/x86/hvm/vmx/vmcs.c b/xen/arch/x86/hvm/vmx/vmcs.c
index 70c2fb7..be02be1 100644
--- a/xen/arch/x86/hvm/vmx/vmcs.c
+++ b/xen/arch/x86/hvm/vmx/vmcs.c
@@ -1246,17 +1246,9 @@ static int construct_vmcs(struct vcpu *v)
 
         ept->mfn = pagetable_get_pfn(p2m_get_pagetable(p2m));
         __vmwrite(EPT_POINTER, ept->eptp);
-    }
-
-    if ( paging_mode_hap(d) )
-    {
-        u64 host_pat, guest_pat;
-
-        rdmsrl(MSR_IA32_CR_PAT, host_pat);
-        guest_pat = MSR_IA32_CR_PAT_RESET;
 
-        __vmwrite(HOST_PAT, host_pat);
-        __vmwrite(GUEST_PAT, guest_pat);
+        __vmwrite(HOST_PAT, XEN_MSR_PAT);
+        __vmwrite(GUEST_PAT, MSR_IA32_CR_PAT_RESET);
     }
     if ( cpu_has_vmx_mpx )
         __vmwrite(GUEST_BNDCFGS, 0);
-- 
2.1.4


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

[PATCH 5/6] x86/vmx: Defer vmx_vmcs_exit() as long as possible in construct_vmcs()

[Andrew Cooper]

paging_update_paging_modes() and vmx_vlapic_msr_changed() both operate on the
VMCS being constructed.  Avoid dropping and re-acquiring the reference
multiple times.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Jun Nakajima <jun.nakajima@intel.com>
CC: Kevin Tian <kevin.tian@intel.com>
CC: Wei Liu <wei.liu2@citrix.com>
CC: Roger Pau Monné <roger.pau@citrix.com>
---
 xen/arch/x86/hvm/vmx/vmcs.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/xen/arch/x86/hvm/vmx/vmcs.c b/xen/arch/x86/hvm/vmx/vmcs.c
index be02be1..ce78f19 100644
--- a/xen/arch/x86/hvm/vmx/vmcs.c
+++ b/xen/arch/x86/hvm/vmx/vmcs.c
@@ -996,6 +996,7 @@ static int construct_vmcs(struct vcpu *v)
     struct domain *d = v->domain;
     u32 vmexit_ctl = vmx_vmexit_control;
     u32 vmentry_ctl = vmx_vmentry_control;
+    int rc;
 
     vmx_vmcs_enter(v);
 
@@ -1083,8 +1084,8 @@ static int construct_vmcs(struct vcpu *v)
 
         if ( msr_bitmap == NULL )
         {
-            vmx_vmcs_exit(v);
-            return -ENOMEM;
+            rc = -ENOMEM;
+            goto out;
         }
 
         memset(msr_bitmap, ~0, PAGE_SIZE);
@@ -1258,13 +1259,14 @@ static int construct_vmcs(struct vcpu *v)
     if ( cpu_has_vmx_tsc_scaling )
         __vmwrite(TSC_MULTIPLIER, d->arch.hvm_domain.tsc_scaling_ratio);
 
-    vmx_vmcs_exit(v);
-
     /* will update HOST & GUEST_CR3 as reqd */
     paging_update_paging_modes(v);
 
     vmx_vlapic_msr_changed(v);
 
+ out:
+    vmx_vmcs_exit(v);
+
     return 0;
 }
 
-- 
2.1.4


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

[PATCH 6/6] x86/vmx: Drop VMX signal for full real-mode

[Andrew Cooper]

The hvmloader code which used this signal was deleted 10 years ago (c/s
50b12df83 "x86 vmx: Remove vmxassist").  Furthermore, the value gets discarded
anyway because the HVM domain builder unconditionally sets %rax to 0 in the
same action it uses to set %rip to the appropriate entrypoint.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Jun Nakajima <jun.nakajima@intel.com>
CC: Kevin Tian <kevin.tian@intel.com>
CC: Roger Pau Monné <roger.pau@citrix.com>
CC: Wei Liu <wei.liu2@citrix.com>
---
 xen/arch/x86/hvm/vmx/vmx.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c
index 8049264..4318b15 100644
--- a/xen/arch/x86/hvm/vmx/vmx.c
+++ b/xen/arch/x86/hvm/vmx/vmx.c
@@ -462,10 +462,6 @@ static int vmx_vcpu_initialise(struct vcpu *v)
 
     vmx_install_vlapic_mapping(v);
 
-    /* %eax == 1 signals full real-mode support to the guest loader. */
-    if ( v->vcpu_id == 0 )
-        v->arch.user_regs.rax = 1;
-
     return 0;
 }
 
-- 
2.1.4


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

Re: [PATCH 1/6] x86/vmx: Fix handing of MSR_DEBUGCTL on VMExit

[Jan Beulich]

>>> On 28.05.18 at 16:27, <andrew.cooper3@citrix.com> wrote:
> Currently, whenever the guest writes a nonzero value to MSR_DEBUGCTL, Xen
> updates a host MSR load list entry with the current hardware value of
> MSR_DEBUGCTL.  This is wrong.

"This is wrong" goes too far for my taste: It is not very efficient to do it that
way, but it's still correct. Unless, of course, the zeroing of the register
happens after the processing of the MSR load list (which I doubt it does).

> Initially, I tried to have a common xen_msr_debugctl variable, but
> rip-relative addresses don't resolve correctly in alternative blocks.
> LBR-only has been fine for ages, and I don't see that changing any time 
> soon.

The chosen solution is certainly fine, but the issue could have been
avoided by doing the load from memory ahead of the alternative block
(accepting that it also happens when the value isn't actually needed).

Another option would be to invert the sense of the feature flag,
patching NOPs over the register setup plus WRMSR.

> @@ -1764,17 +1765,6 @@ void do_device_not_available(struct cpu_user_regs *regs)
>      return;
>  }
>  
> -static void ler_enable(void)
> -{
> -    u64 debugctl;
> -
> -    if ( !this_cpu(ler_msr) )
> -        return;
> -
> -    rdmsrl(MSR_IA32_DEBUGCTLMSR, debugctl);
> -    wrmsrl(MSR_IA32_DEBUGCTLMSR, debugctl | IA32_DEBUGCTLMSR_LBR);
> -}
> -
>  void do_debug(struct cpu_user_regs *regs)
>  {
>      unsigned long dr6;
> @@ -1870,13 +1860,13 @@ void do_debug(struct cpu_user_regs *regs)
>      v->arch.debugreg[6] |= (dr6 & ~X86_DR6_DEFAULT);
>      v->arch.debugreg[6] &= (dr6 | ~X86_DR6_DEFAULT);
>  
> -    ler_enable();
>      pv_inject_hw_exception(TRAP_debug, X86_EVENT_NO_EC);
> -    return;
>  
>   out:
> -    ler_enable();
> -    return;
> +
> +    /* #DB automatically disabled LBR.  Reinstate it if debugging Xen. */
> +    if ( cpu_has_xen_lbr )
> +        wrmsrl(MSR_IA32_DEBUGCTLMSR, IA32_DEBUGCTLMSR_LBR);

While I can see that we don't currently need anything more than this one
bit, it still doesn't feel overly well to not do a read-modify-write cycle here.

In any event, rather than moving the write further towards the end of
the function, could I ask you to move it further up, so that in the (unlikely)
event of do_debug() itself triggering an exception we'd get a proper
indication of the last branch before that?

> @@ -1920,38 +1910,46 @@ void load_TR(void)
>          : "=m" (old_gdt) : "rm" (TSS_ENTRY << 3), "m" (tss_gdt) : "memory" );
>  }
>  
> -void percpu_traps_init(void)
> +static uint32_t calc_ler_msr(void)

Here and elsewhere "unsigned int" would be more appropriate to use.
We don't require MSR indexes to be exactly 32 bits wide, but only at
least as wide.

> +void percpu_traps_init(void)
> +{
> +    subarch_percpu_traps_init();
> +
> +    if ( !opt_ler )
> +        return;
> +
> +    if ( !ler_msr && (ler_msr = calc_ler_msr()) )
> +        setup_force_cpu_cap(X86_FEATURE_XEN_LBR);

This does not hold up with the promise the description makes: If running
on an unrecognized model, calc_ler_msr() is going to be called more than
once. If it really was called just once, it could also become __init. With
the inverted sense of the feature flag (as suggested above) you could
check whether the flag bit is set or ler_msr is non-zero.

Jan



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

Re: [PATCH 2/6] x86: Improvements to ler debugging

[Jan Beulich]

 >>> On 28.05.18 at 16:27, <andrew.cooper3@citrix.com> wrote:
> * Command line documentation for what the option does.
>  * Implement a canonicalise_addr() helper and replace the opencoded use in
>    sign_extend_msr()
>  * Canonicalise the ler pointers and print symbol information.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

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

> --- a/docs/misc/xen-command-line.markdown
> +++ b/docs/misc/xen-command-line.markdown
> @@ -1245,6 +1245,12 @@ if left disabled by the BIOS.
>  ### ler (x86)
>  > `= <boolean>`
>  
> +> Default: 0

Perhaps better `false`?

Jan



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

Re: [PATCH 3/6] x86/pat: Simplify host PAT handling

[Jan Beulich]

>>> On 28.05.18 at 16:27, <andrew.cooper3@citrix.com> wrote:
> With the removal of the 32bit hypervisor build, host_pat is a constant value.
> Drop the variable and the redundant cpu_has_pat predicate, and use a define
> instead.
> 
> 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

Re: [PATCH 4/6] x86/vmx: Simplify PAT handling during vcpu construction

[Jan Beulich]

>>> On 28.05.18 at 16:27, <andrew.cooper3@citrix.com> wrote:
> The host PAT value is a compile time constant, and doesn't need to be read out
> of hardware.  Merge this if block into the previous block, which has an
> identical condition.
> 
> 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

Re: [PATCH 5/6] x86/vmx: Defer vmx_vmcs_exit() as long as possible in construct_vmcs()

[Jan Beulich]

>>> On 28.05.18 at 16:27, <andrew.cooper3@citrix.com> wrote:
> paging_update_paging_modes() and vmx_vlapic_msr_changed() both operate on the
> VMCS being constructed.  Avoid dropping and re-acquiring the reference
> multiple times.
> 
> 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

Re: [PATCH 1/6] x86/vmx: Fix handing of MSR_DEBUGCTL on VMExit

[Andrew Cooper]

On 29/05/18 11:33, Jan Beulich wrote:
>>>> On 28.05.18 at 16:27, <andrew.cooper3@citrix.com> wrote:
>> Currently, whenever the guest writes a nonzero value to MSR_DEBUGCTL, Xen
>> updates a host MSR load list entry with the current hardware value of
>> MSR_DEBUGCTL.  This is wrong.
> "This is wrong" goes too far for my taste: It is not very efficient to do it that
> way, but it's still correct. Unless, of course, the zeroing of the register
> happens after the processing of the MSR load list (which I doubt it does).

It is functionally broken.  Restoration of Xen's debugging setting must
happen from the first vmexit, not the first vmexit after the guest plays
with MSR_DEBUGCTL.

With the current behaviour, Xen looses its MSR_DEBUGCTL setting on any
pcpu where an HVM guest has been scheduled, and then feeds the current
value (0) into the host load list, even when it was attempting to set a
non-zero value.

>
>> Initially, I tried to have a common xen_msr_debugctl variable, but
>> rip-relative addresses don't resolve correctly in alternative blocks.
>> LBR-only has been fine for ages, and I don't see that changing any time 
>> soon.
> The chosen solution is certainly fine, but the issue could have been
> avoided by doing the load from memory ahead of the alternative block
> (accepting that it also happens when the value isn't actually needed).
>
> Another option would be to invert the sense of the feature flag,
> patching NOPs over the register setup plus WRMSR.

I considered both, but until it is necessary, there is little point.

>
>> @@ -1764,17 +1765,6 @@ void do_device_not_available(struct cpu_user_regs *regs)
>>      return;
>>  }
>>  
>> -static void ler_enable(void)
>> -{
>> -    u64 debugctl;
>> -
>> -    if ( !this_cpu(ler_msr) )
>> -        return;
>> -
>> -    rdmsrl(MSR_IA32_DEBUGCTLMSR, debugctl);
>> -    wrmsrl(MSR_IA32_DEBUGCTLMSR, debugctl | IA32_DEBUGCTLMSR_LBR);
>> -}
>> -
>>  void do_debug(struct cpu_user_regs *regs)
>>  {
>>      unsigned long dr6;
>> @@ -1870,13 +1860,13 @@ void do_debug(struct cpu_user_regs *regs)
>>      v->arch.debugreg[6] |= (dr6 & ~X86_DR6_DEFAULT);
>>      v->arch.debugreg[6] &= (dr6 | ~X86_DR6_DEFAULT);
>>  
>> -    ler_enable();
>>      pv_inject_hw_exception(TRAP_debug, X86_EVENT_NO_EC);
>> -    return;
>>  
>>   out:
>> -    ler_enable();
>> -    return;
>> +
>> +    /* #DB automatically disabled LBR.  Reinstate it if debugging Xen. */
>> +    if ( cpu_has_xen_lbr )
>> +        wrmsrl(MSR_IA32_DEBUGCTLMSR, IA32_DEBUGCTLMSR_LBR);
> While I can see that we don't currently need anything more than this one
> bit, it still doesn't feel overly well to not do a read-modify-write cycle here.

We should never be using a RMW cycle.  All that risks doing is
accumulating unexpected debugging controls.

If/when it becomes a variable, the correct code here is:

if ( xen_debugctl_val & IA32_DEBUGCTLMSR_LBR )
    wrmsrl(MSR_IA32_DEBUGCTLMSR, xen_debugctl_val);

(except that since writing this patch, I've found that BTF is also
cleared on AMD hardware, so that probably wants to be taken into account).

> In any event, rather than moving the write further towards the end of
> the function, could I ask you to move it further up, so that in the (unlikely)
> event of do_debug() itself triggering an exception we'd get a proper
> indication of the last branch before that?

Ok.  It can move to immediately after resetting %dr6.

>
>> @@ -1920,38 +1910,46 @@ void load_TR(void)
>>          : "=m" (old_gdt) : "rm" (TSS_ENTRY << 3), "m" (tss_gdt) : "memory" );
>>  }
>>  
>> -void percpu_traps_init(void)
>> +static uint32_t calc_ler_msr(void)
> Here and elsewhere "unsigned int" would be more appropriate to use.
> We don't require MSR indexes to be exactly 32 bits wide, but only at
> least as wide.

MSR indices are architecturally 32 bits wide.

>
>> +void percpu_traps_init(void)
>> +{
>> +    subarch_percpu_traps_init();
>> +
>> +    if ( !opt_ler )
>> +        return;
>> +
>> +    if ( !ler_msr && (ler_msr = calc_ler_msr()) )
>> +        setup_force_cpu_cap(X86_FEATURE_XEN_LBR);
> This does not hold up with the promise the description makes: If running
> on an unrecognized model, calc_ler_msr() is going to be called more than
> once. If it really was called just once, it could also become __init. With
> the inverted sense of the feature flag (as suggested above) you could
> check whether the flag bit is set or ler_msr is non-zero.

Hmm - I suppose it doesn't quite match the description, but does it
matter (if I tweak the description)?  It is debugging functionality, and
I don't see any 64bit models missing from the list.

~Andrew

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

Re: [PATCH 2/6] x86: Improvements to ler debugging

[Andrew Cooper]

On 29/05/18 12:39, Jan Beulich wrote:
>  >>> On 28.05.18 at 16:27, <andrew.cooper3@citrix.com> wrote:
>> * Command line documentation for what the option does.
>>  * Implement a canonicalise_addr() helper and replace the opencoded use in
>>    sign_extend_msr()
>>  * Canonicalise the ler pointers and print symbol information.
>>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> Reviewed-by: Jan Beulich <jbeulich@suse.com>

Thanks,

> with one remark:
>
>> --- a/docs/misc/xen-command-line.markdown
>> +++ b/docs/misc/xen-command-line.markdown
>> @@ -1245,6 +1245,12 @@ if left disabled by the BIOS.
>>  ### ler (x86)
>>  > `= <boolean>`
>>  
>> +> Default: 0
> Perhaps better `false`?

Fixed.

~Andrew

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

Re: [PATCH 1/6] x86/vmx: Fix handing of MSR_DEBUGCTL on VMExit

[Jan Beulich]

>>> On 29.05.18 at 20:08, <andrew.cooper3@citrix.com> wrote:
> On 29/05/18 11:33, Jan Beulich wrote:
>>>>> On 28.05.18 at 16:27, <andrew.cooper3@citrix.com> wrote:
>>> Currently, whenever the guest writes a nonzero value to MSR_DEBUGCTL, Xen
>>> updates a host MSR load list entry with the current hardware value of
>>> MSR_DEBUGCTL.  This is wrong.
>> "This is wrong" goes too far for my taste: It is not very efficient to do it that
>> way, but it's still correct. Unless, of course, the zeroing of the register
>> happens after the processing of the MSR load list (which I doubt it does).
> 
> It is functionally broken.  Restoration of Xen's debugging setting must
> happen from the first vmexit, not the first vmexit after the guest plays
> with MSR_DEBUGCTL.
> 
> With the current behaviour, Xen looses its MSR_DEBUGCTL setting on any
> pcpu where an HVM guest has been scheduled, and then feeds the current
> value (0) into the host load list, even when it was attempting to set a
> non-zero value.

Oh, indeed, you're right.

>>> @@ -1920,38 +1910,46 @@ void load_TR(void)
>>>          : "=m" (old_gdt) : "rm" (TSS_ENTRY << 3), "m" (tss_gdt) : "memory" );
>>>  }
>>>  
>>> -void percpu_traps_init(void)
>>> +static uint32_t calc_ler_msr(void)
>> Here and elsewhere "unsigned int" would be more appropriate to use.
>> We don't require MSR indexes to be exactly 32 bits wide, but only at
>> least as wide.
> 
> MSR indices are architecturally 32 bits wide.

Correct. Hence for communicating such values between functions we need
a type at least as wide as 32 bits, not exactly as wide. There's a reason the
standard also defined uint_least32_t et al; it's just that I think unsigned int
is less ugly to read and fulfills the same purpose (with the prereq that we
assume to run only on architectures where unsigned int is at least 32 bits
wide, just like we make an even more strict assumption on unsigned long).

>>> +void percpu_traps_init(void)
>>> +{
>>> +    subarch_percpu_traps_init();
>>> +
>>> +    if ( !opt_ler )
>>> +        return;
>>> +
>>> +    if ( !ler_msr && (ler_msr = calc_ler_msr()) )
>>> +        setup_force_cpu_cap(X86_FEATURE_XEN_LBR);
>> This does not hold up with the promise the description makes: If running
>> on an unrecognized model, calc_ler_msr() is going to be called more than
>> once. If it really was called just once, it could also become __init. With
>> the inverted sense of the feature flag (as suggested above) you could
>> check whether the flag bit is set or ler_msr is non-zero.
> 
> Hmm - I suppose it doesn't quite match the description, but does it
> matter (if I tweak the description)?  It is debugging functionality, and
> I don't see any 64bit models missing from the list.

Non-Intel, non-AMD CPUs are clearly missing. We have Centaur (VIA)
support, and we're going to gain support for one more right after the
tree was branched for 4.11.

Jan



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

Re: [PATCH 1/6] x86/vmx: Fix handing of MSR_DEBUGCTL on VMExit

[Andrew Cooper]

On 30/05/18 08:32, Jan Beulich wrote:
>>>> On 29.05.18 at 20:08, <andrew.cooper3@citrix.com> wrote:
>> On 29/05/18 11:33, Jan Beulich wrote:
>>>>>> On 28.05.18 at 16:27, <andrew.cooper3@citrix.com> wrote:
>>>> Currently, whenever the guest writes a nonzero value to MSR_DEBUGCTL, Xen
>>>> updates a host MSR load list entry with the current hardware value of
>>>> MSR_DEBUGCTL.  This is wrong.
>>> "This is wrong" goes too far for my taste: It is not very efficient to do it that
>>> way, but it's still correct. Unless, of course, the zeroing of the register
>>> happens after the processing of the MSR load list (which I doubt it does).
>> It is functionally broken.  Restoration of Xen's debugging setting must
>> happen from the first vmexit, not the first vmexit after the guest plays
>> with MSR_DEBUGCTL.
>>
>> With the current behaviour, Xen looses its MSR_DEBUGCTL setting on any
>> pcpu where an HVM guest has been scheduled, and then feeds the current
>> value (0) into the host load list, even when it was attempting to set a
>> non-zero value.
> Oh, indeed, you're right.

I've rewritten this bit of the commit message.  How about:

Currently, whenever the guest writes a nonzero value to MSR_DEBUGCTL, Xen
updates a host MSR load list entry with the current hardware value of
MSR_DEBUGCTL.

On VMExit, hardware automatically resets MSR_DEBUGCTL to 0.  Later, when the
guest writes to MSR_DEBUGCTL, the current value in hardware (0) is fed back
into guest load list.  As a practical result, `ler` debugging gets lost on any
PCPU which has ever scheduled an HVM vcpu, and the common case when `ler`
debugging isn't active, guest actions result in an unnecessary load list entry
repeating the MSR_DEBUGCTL reset.

Restoration of Xen's debugging setting needs to happen from the very first
vmexit.  Due to the automatic reset, Xen need take no action in the general
case, and only needs to load a value when debugging is active.

>>>> +void percpu_traps_init(void)
>>>> +{
>>>> +    subarch_percpu_traps_init();
>>>> +
>>>> +    if ( !opt_ler )
>>>> +        return;
>>>> +
>>>> +    if ( !ler_msr && (ler_msr = calc_ler_msr()) )
>>>> +        setup_force_cpu_cap(X86_FEATURE_XEN_LBR);
>>> This does not hold up with the promise the description makes: If running
>>> on an unrecognized model, calc_ler_msr() is going to be called more than
>>> once. If it really was called just once, it could also become __init. With
>>> the inverted sense of the feature flag (as suggested above) you could
>>> check whether the flag bit is set or ler_msr is non-zero.
>> Hmm - I suppose it doesn't quite match the description, but does it
>> matter (if I tweak the description)?  It is debugging functionality, and
>> I don't see any 64bit models missing from the list.
> Non-Intel, non-AMD CPUs are clearly missing. We have Centaur (VIA)
> support, and we're going to gain support for one more right after the
> tree was branched for 4.11.

Ok, but all of this is behind !opt_ler which means it doesn't get
executed in the general case.

~Andrew

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

Re: [PATCH 1/6] x86/vmx: Fix handing of MSR_DEBUGCTL on VMExit

[Jan Beulich]

>>> On 30.05.18 at 12:28, <andrew.cooper3@citrix.com> wrote:
> On 30/05/18 08:32, Jan Beulich wrote:
>>>>> On 29.05.18 at 20:08, <andrew.cooper3@citrix.com> wrote:
>>> On 29/05/18 11:33, Jan Beulich wrote:
>>>>>>> On 28.05.18 at 16:27, <andrew.cooper3@citrix.com> wrote:
>>>>> Currently, whenever the guest writes a nonzero value to MSR_DEBUGCTL, Xen
>>>>> updates a host MSR load list entry with the current hardware value of
>>>>> MSR_DEBUGCTL.  This is wrong.
>>>> "This is wrong" goes too far for my taste: It is not very efficient to do it 
> that
>>>> way, but it's still correct. Unless, of course, the zeroing of the register
>>>> happens after the processing of the MSR load list (which I doubt it does).
>>> It is functionally broken.  Restoration of Xen's debugging setting must
>>> happen from the first vmexit, not the first vmexit after the guest plays
>>> with MSR_DEBUGCTL.
>>>
>>> With the current behaviour, Xen looses its MSR_DEBUGCTL setting on any
>>> pcpu where an HVM guest has been scheduled, and then feeds the current
>>> value (0) into the host load list, even when it was attempting to set a
>>> non-zero value.
>> Oh, indeed, you're right.
> 
> I've rewritten this bit of the commit message.  How about:
> 
> Currently, whenever the guest writes a nonzero value to MSR_DEBUGCTL, Xen
> updates a host MSR load list entry with the current hardware value of
> MSR_DEBUGCTL.
> 
> On VMExit, hardware automatically resets MSR_DEBUGCTL to 0.  Later, when the
> guest writes to MSR_DEBUGCTL, the current value in hardware (0) is fed back
> into guest load list.  As a practical result, `ler` debugging gets lost on any
> PCPU which has ever scheduled an HVM vcpu, and the common case when `ler`
> debugging isn't active, guest actions result in an unnecessary load list entry
> repeating the MSR_DEBUGCTL reset.
> 
> Restoration of Xen's debugging setting needs to happen from the very first
> vmexit.  Due to the automatic reset, Xen need take no action in the general
> case, and only needs to load a value when debugging is active.

Thanks, this makes things more explicit imo.

>>>>> +void percpu_traps_init(void)
>>>>> +{
>>>>> +    subarch_percpu_traps_init();
>>>>> +
>>>>> +    if ( !opt_ler )
>>>>> +        return;
>>>>> +
>>>>> +    if ( !ler_msr && (ler_msr = calc_ler_msr()) )
>>>>> +        setup_force_cpu_cap(X86_FEATURE_XEN_LBR);
>>>> This does not hold up with the promise the description makes: If running
>>>> on an unrecognized model, calc_ler_msr() is going to be called more than
>>>> once. If it really was called just once, it could also become __init. With
>>>> the inverted sense of the feature flag (as suggested above) you could
>>>> check whether the flag bit is set or ler_msr is non-zero.
>>> Hmm - I suppose it doesn't quite match the description, but does it
>>> matter (if I tweak the description)?  It is debugging functionality, and
>>> I don't see any 64bit models missing from the list.
>> Non-Intel, non-AMD CPUs are clearly missing. We have Centaur (VIA)
>> support, and we're going to gain support for one more right after the
>> tree was branched for 4.11.
> 
> Ok, but all of this is behind !opt_ler which means it doesn't get
> executed in the general case.

Of course.

Jan



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

[PATCH v2 1/6] x86/vmx: Fix handing of MSR_DEBUGCTL on VMExit

[Andrew Cooper]

Currently, whenever the guest writes a nonzero value to MSR_DEBUGCTL, Xen
updates a host MSR load list entry with the current hardware value of
MSR_DEBUGCTL.

On VMExit, hardware automatically resets MSR_DEBUGCTL to 0.  Later, when the
guest writes to MSR_DEBUGCTL, the current value in hardware (0) is fed back
into guest load list.  As a practical result, `ler` debugging gets lost on any
PCPU which has ever scheduled an HVM vcpu, and the common case when `ler`
debugging isn't active, guest actions result in an unnecessary load list entry
repeating the MSR_DEBUGCTL reset.

Restoration of Xen's debugging setting needs to happen from the very first
vmexit.  Due to the automatic reset, Xen need take no action in the general
case, and only needs to load a value when debugging is active.

This could be fixed by using a host MSR load list entry set up during
construct_vmcs().  However, a more efficient option is to use an alternative
block in the VMExit path, keyed on whether hypervisor debugging has been
enabled.

In order to set this up, drop the per cpu ler_msr variable (as there is no
point having it per cpu when it will be the same everywhere), and use a single
read_mostly variable instead.  Split calc_ler_msr() out of percpu_traps_init()
for clarity.

Finally, clean up do_debug().  Reinstate LBR early to help catch cascade
errors, which allows for the removal of the out label.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Jun Nakajima <jun.nakajima@intel.com>
CC: Kevin Tian <kevin.tian@intel.com>
CC: Wei Liu <wei.liu2@citrix.com>
CC: Roger Pau Monné <roger.pau@citrix.com>

v2:
 * Reinstate LBR early in do_debug()
 * Rewrite the commit message.
---
 xen/arch/x86/hvm/vmx/entry.S      |  9 ++++++
 xen/arch/x86/hvm/vmx/vmx.c        |  3 +-
 xen/arch/x86/traps.c              | 64 ++++++++++++++++++---------------------
 xen/arch/x86/x86_64/traps.c       |  7 +++--
 xen/include/asm-x86/cpufeature.h  |  1 +
 xen/include/asm-x86/cpufeatures.h |  1 +
 xen/include/asm-x86/msr.h         |  2 +-
 7 files changed, 47 insertions(+), 40 deletions(-)

diff --git a/xen/arch/x86/hvm/vmx/entry.S b/xen/arch/x86/hvm/vmx/entry.S
index aa2f103..afd552f 100644
--- a/xen/arch/x86/hvm/vmx/entry.S
+++ b/xen/arch/x86/hvm/vmx/entry.S
@@ -41,6 +41,15 @@ ENTRY(vmx_asm_vmexit_handler)
         SPEC_CTRL_ENTRY_FROM_HVM    /* Req: b=curr %rsp=regs/cpuinfo, Clob: acd */
         /* WARNING! `ret`, `call *`, `jmp *` not safe before this point. */
 
+        /* Hardware clears MSR_DEBUGCTL on VMExit.  Reinstate it if debugging Xen. */
+        .macro restore_lbr
+            mov $IA32_DEBUGCTLMSR_LBR, %eax
+            mov $MSR_IA32_DEBUGCTLMSR, %ecx
+            xor %edx, %edx
+            wrmsr
+        .endm
+        ALTERNATIVE "", restore_lbr, X86_FEATURE_XEN_LBR
+
         mov  %rsp,%rdi
         call vmx_vmexit_handler
 
diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c
index 9707514..33d39f6 100644
--- a/xen/arch/x86/hvm/vmx/vmx.c
+++ b/xen/arch/x86/hvm/vmx/vmx.c
@@ -3120,8 +3120,7 @@ static int vmx_msr_write_intercept(unsigned int msr, uint64_t msr_content)
                     }
         }
 
-        if ( (rc < 0) ||
-             (msr_content && (vmx_add_host_load_msr(msr) < 0)) )
+        if ( rc < 0 )
             hvm_inject_hw_exception(TRAP_machine_check, X86_EVENT_NO_EC);
         else
             __vmwrite(GUEST_IA32_DEBUGCTL, msr_content);
diff --git a/xen/arch/x86/traps.c b/xen/arch/x86/traps.c
index 8a99174..2b9c276 100644
--- a/xen/arch/x86/traps.c
+++ b/xen/arch/x86/traps.c
@@ -96,8 +96,6 @@ string_param("nmi", opt_nmi);
 DEFINE_PER_CPU(uint64_t, efer);
 static DEFINE_PER_CPU(unsigned long, last_extable_addr);
 
-DEFINE_PER_CPU_READ_MOSTLY(u32, ler_msr);
-
 DEFINE_PER_CPU_READ_MOSTLY(struct desc_struct *, gdt_table);
 DEFINE_PER_CPU_READ_MOSTLY(struct desc_struct *, compat_gdt_table);
 
@@ -117,6 +115,9 @@ integer_param("debug_stack_lines", debug_stack_lines);
 static bool opt_ler;
 boolean_param("ler", opt_ler);
 
+/* LastExceptionFromIP on this hardware.  Zero if LER is not in use. */
+uint32_t __read_mostly ler_msr;
+
 #define stack_words_per_line 4
 #define ESP_BEFORE_EXCEPTION(regs) ((unsigned long *)regs->rsp)
 
@@ -1764,17 +1765,6 @@ void do_device_not_available(struct cpu_user_regs *regs)
     return;
 }
 
-static void ler_enable(void)
-{
-    u64 debugctl;
-
-    if ( !this_cpu(ler_msr) )
-        return;
-
-    rdmsrl(MSR_IA32_DEBUGCTLMSR, debugctl);
-    wrmsrl(MSR_IA32_DEBUGCTLMSR, debugctl | IA32_DEBUGCTLMSR_LBR);
-}
-
 void do_debug(struct cpu_user_regs *regs)
 {
     unsigned long dr6;
@@ -1807,6 +1797,10 @@ void do_debug(struct cpu_user_regs *regs)
      */
     write_debugreg(6, X86_DR6_DEFAULT);
 
+    /* #DB automatically disabled LBR.  Reinstate it if debugging Xen. */
+    if ( cpu_has_xen_lbr )
+        wrmsrl(MSR_IA32_DEBUGCTLMSR, IA32_DEBUGCTLMSR_LBR);
+
     if ( !guest_mode(regs) )
     {
         if ( regs->eflags & X86_EFLAGS_TF )
@@ -1817,7 +1811,7 @@ void do_debug(struct cpu_user_regs *regs)
             {
                 if ( regs->rip == (unsigned long)sysenter_eflags_saved )
                     regs->eflags &= ~X86_EFLAGS_TF;
-                goto out;
+                return;
             }
             if ( !debugger_trap_fatal(TRAP_debug, regs) )
             {
@@ -1863,20 +1857,14 @@ void do_debug(struct cpu_user_regs *regs)
                 regs->cs, _p(regs->rip), _p(regs->rip),
                 regs->ss, _p(regs->rsp), dr6);
 
-        goto out;
+        return;
     }
 
     /* Save debug status register where guest OS can peek at it */
     v->arch.debugreg[6] |= (dr6 & ~X86_DR6_DEFAULT);
     v->arch.debugreg[6] &= (dr6 | ~X86_DR6_DEFAULT);
 
-    ler_enable();
     pv_inject_hw_exception(TRAP_debug, X86_EVENT_NO_EC);
-    return;
-
- out:
-    ler_enable();
-    return;
 }
 
 static void __init noinline __set_intr_gate(unsigned int n,
@@ -1920,38 +1908,46 @@ void load_TR(void)
         : "=m" (old_gdt) : "rm" (TSS_ENTRY << 3), "m" (tss_gdt) : "memory" );
 }
 
-void percpu_traps_init(void)
+static unsigned int calc_ler_msr(void)
 {
-    subarch_percpu_traps_init();
-
-    if ( !opt_ler )
-        return;
-
     switch ( boot_cpu_data.x86_vendor )
     {
     case X86_VENDOR_INTEL:
         switch ( boot_cpu_data.x86 )
         {
         case 6:
-            this_cpu(ler_msr) = MSR_IA32_LASTINTFROMIP;
-            break;
+            return MSR_IA32_LASTINTFROMIP;
+
         case 15:
-            this_cpu(ler_msr) = MSR_P4_LER_FROM_LIP;
-            break;
+            return MSR_P4_LER_FROM_LIP;
         }
         break;
+
     case X86_VENDOR_AMD:
         switch ( boot_cpu_data.x86 )
         {
         case 6:
         case 0xf ... 0x17:
-            this_cpu(ler_msr) = MSR_IA32_LASTINTFROMIP;
-            break;
+            return MSR_IA32_LASTINTFROMIP;
         }
         break;
     }
 
-    ler_enable();
+    return 0;
+}
+
+void percpu_traps_init(void)
+{
+    subarch_percpu_traps_init();
+
+    if ( !opt_ler )
+        return;
+
+    if ( !ler_msr && (ler_msr = calc_ler_msr()) )
+        setup_force_cpu_cap(X86_FEATURE_XEN_LBR);
+
+    if ( cpu_has_xen_lbr )
+        wrmsrl(MSR_IA32_DEBUGCTLMSR, IA32_DEBUGCTLMSR_LBR);
 }
 
 void __init init_idt_traps(void)
diff --git a/xen/arch/x86/x86_64/traps.c b/xen/arch/x86/x86_64/traps.c
index f7f6928..b040185 100644
--- a/xen/arch/x86/x86_64/traps.c
+++ b/xen/arch/x86/x86_64/traps.c
@@ -144,11 +144,12 @@ void show_registers(const struct cpu_user_regs *regs)
     printk("CPU:    %d\n", smp_processor_id());
     _show_registers(&fault_regs, fault_crs, context, v);
 
-    if ( this_cpu(ler_msr) && !guest_mode(regs) )
+    if ( ler_msr && !guest_mode(regs) )
     {
         u64 from, to;
-        rdmsrl(this_cpu(ler_msr), from);
-        rdmsrl(this_cpu(ler_msr) + 1, to);
+
+        rdmsrl(ler_msr, from);
+        rdmsrl(ler_msr + 1, to);
         printk("ler: %016lx -> %016lx\n", from, to);
     }
 }
diff --git a/xen/include/asm-x86/cpufeature.h b/xen/include/asm-x86/cpufeature.h
index 2cf8f7e..b237da1 100644
--- a/xen/include/asm-x86/cpufeature.h
+++ b/xen/include/asm-x86/cpufeature.h
@@ -113,6 +113,7 @@
 #define cpu_has_aperfmperf      boot_cpu_has(X86_FEATURE_APERFMPERF)
 #define cpu_has_lfence_dispatch boot_cpu_has(X86_FEATURE_LFENCE_DISPATCH)
 #define cpu_has_no_xpti         boot_cpu_has(X86_FEATURE_NO_XPTI)
+#define cpu_has_xen_lbr         boot_cpu_has(X86_FEATURE_XEN_LBR)
 
 enum _cache_type {
     CACHE_TYPE_NULL = 0,
diff --git a/xen/include/asm-x86/cpufeatures.h b/xen/include/asm-x86/cpufeatures.h
index b90aa2d..8e5cc53 100644
--- a/xen/include/asm-x86/cpufeatures.h
+++ b/xen/include/asm-x86/cpufeatures.h
@@ -32,3 +32,4 @@ XEN_CPUFEATURE(SC_RSB_PV,       (FSCAPINTS+0)*32+18) /* RSB overwrite needed for
 XEN_CPUFEATURE(SC_RSB_HVM,      (FSCAPINTS+0)*32+19) /* RSB overwrite needed for HVM */
 XEN_CPUFEATURE(NO_XPTI,         (FSCAPINTS+0)*32+20) /* XPTI mitigation not in use */
 XEN_CPUFEATURE(SC_MSR_IDLE,     (FSCAPINTS+0)*32+21) /* (SC_MSR_PV || SC_MSR_HVM) && default_xen_spec_ctrl */
+XEN_CPUFEATURE(XEN_LBR,         (FSCAPINTS+0)*32+22) /* Xen uses MSR_DEBUGCTL.LBR */
diff --git a/xen/include/asm-x86/msr.h b/xen/include/asm-x86/msr.h
index f14f265..9f6d3b2 100644
--- a/xen/include/asm-x86/msr.h
+++ b/xen/include/asm-x86/msr.h
@@ -241,7 +241,7 @@ static inline void write_efer(uint64_t val)
     wrmsrl(MSR_EFER, val);
 }
 
-DECLARE_PER_CPU(u32, ler_msr);
+extern uint32_t ler_msr;
 
 DECLARE_PER_CPU(uint32_t, tsc_aux);
 
-- 
2.1.4


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

Re: [PATCH v2 1/6] x86/vmx: Fix handing of MSR_DEBUGCTL on VMExit

[Jan Beulich]

>>> On 30.05.18 at 19:34, <andrew.cooper3@citrix.com> wrote:
> @@ -117,6 +115,9 @@ integer_param("debug_stack_lines", debug_stack_lines);
>  static bool opt_ler;
>  boolean_param("ler", opt_ler);
>  
> +/* LastExceptionFromIP on this hardware.  Zero if LER is not in use. */
> +uint32_t __read_mostly ler_msr;

Hmm, this is still uint32_t rather than unsigned int, while you did change
calc_ler_msr()'s return type.

> +void percpu_traps_init(void)
> +{
> +    subarch_percpu_traps_init();
> +
> +    if ( !opt_ler )
> +        return;
> +
> +    if ( !ler_msr && (ler_msr = calc_ler_msr()) )
> +        setup_force_cpu_cap(X86_FEATURE_XEN_LBR);

I assume it was on purpose that you've adjusted the commit message
rather than the code here regarding the possibility of pointless multiple
invocation?

With preferably the type inconsistency addressed
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

Re: [PATCH v2 1/6] x86/vmx: Fix handing of MSR_DEBUGCTL on VMExit

[Tian, Kevin]

> From: Andrew Cooper [mailto:andrew.cooper3@citrix.com]
> Sent: Thursday, May 31, 2018 1:35 AM
> 
> Currently, whenever the guest writes a nonzero value to MSR_DEBUGCTL,
> Xen
> updates a host MSR load list entry with the current hardware value of
> MSR_DEBUGCTL.
> 
> On VMExit, hardware automatically resets MSR_DEBUGCTL to 0.  Later,
> when the
> guest writes to MSR_DEBUGCTL, the current value in hardware (0) is fed
> back
> into guest load list.  As a practical result, `ler` debugging gets lost on any
> PCPU which has ever scheduled an HVM vcpu, and the common case when
> `ler`
> debugging isn't active, guest actions result in an unnecessary load list entry
> repeating the MSR_DEBUGCTL reset.
> 
> Restoration of Xen's debugging setting needs to happen from the very first
> vmexit.  Due to the automatic reset, Xen need take no action in the general
> case, and only needs to load a value when debugging is active.
> 
> This could be fixed by using a host MSR load list entry set up during
> construct_vmcs().  However, a more efficient option is to use an alternative
> block in the VMExit path, keyed on whether hypervisor debugging has been
> enabled.
> 
> In order to set this up, drop the per cpu ler_msr variable (as there is no
> point having it per cpu when it will be the same everywhere), and use a
> single
> read_mostly variable instead.  Split calc_ler_msr() out of percpu_traps_init()
> for clarity.
> 
> Finally, clean up do_debug().  Reinstate LBR early to help catch cascade
> errors, which allows for the removal of the out label.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

nice cleanup. 

Reviewed-by: Kevin Tian <kevin.tian@intel.com>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 2/6] x86: Improvements to ler debugging

[Tian, Kevin]

> From: Andrew Cooper [mailto:andrew.cooper3@citrix.com]
> Sent: Monday, May 28, 2018 10:28 PM
> 
>  * Command line documentation for what the option does.
>  * Implement a canonicalise_addr() helper and replace the opencoded use
> in
>    sign_extend_msr()
>  * Canonicalise the ler pointers and print symbol information.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Reviewed-by: Kevin Tian <kevin.tian@intel.com>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 4/6] x86/vmx: Simplify PAT handling during vcpu construction

[Tian, Kevin]

> From: Andrew Cooper [mailto:andrew.cooper3@citrix.com]
> Sent: Monday, May 28, 2018 10:28 PM
> 
> The host PAT value is a compile time constant, and doesn't need to be read
> out
> of hardware.  Merge this if block into the previous block, which has an
> identical condition.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Acked-by: Kevin Tian <kevin.tian@intel.com>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 5/6] x86/vmx: Defer vmx_vmcs_exit() as long as possible in construct_vmcs()

[Tian, Kevin]

> From: Andrew Cooper [mailto:andrew.cooper3@citrix.com]
> Sent: Monday, May 28, 2018 10:28 PM
> 
> paging_update_paging_modes() and vmx_vlapic_msr_changed() both
> operate on the
> VMCS being constructed.  Avoid dropping and re-acquiring the reference
> multiple times.
> 

Acked-by: Kevin Tian <kevin.tian@intel.com>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 6/6] x86/vmx: Drop VMX signal for full real-mode

[Tian, Kevin]

> From: Andrew Cooper [mailto:andrew.cooper3@citrix.com]
> Sent: Monday, May 28, 2018 10:28 PM
> 
> The hvmloader code which used this signal was deleted 10 years ago (c/s
> 50b12df83 "x86 vmx: Remove vmxassist").  Furthermore, the value gets
> discarded
> anyway because the HVM domain builder unconditionally sets %rax to 0 in
> the
> same action it uses to set %rip to the appropriate entrypoint.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Acked-by: Kevin Tian <kevin.tian@intel.com>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 3/6] x86/pat: Simplify host PAT handling

[Roger Pau Monné]

On Mon, May 28, 2018 at 03:27:55PM +0100, Andrew Cooper wrote:
> With the removal of the 32bit hypervisor build, host_pat is a constant value.
> Drop the variable and the redundant cpu_has_pat predicate, and use a define
> instead.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>

> diff --git a/xen/include/asm-x86/processor.h b/xen/include/asm-x86/processor.h
> index 9924cdf..ac1577c 100644
> --- a/xen/include/asm-x86/processor.h
> +++ b/xen/include/asm-x86/processor.h
> @@ -97,6 +97,12 @@
>                            X86_EFLAGS_NT|X86_EFLAGS_DF|X86_EFLAGS_IF|    \
>                            X86_EFLAGS_TF)
>  
> +/*
> + * Host IA32_CR_PAT value to cover all memory types.  This is not the default
> + * MSR_PAT value, and is an ABI with PV guests.
> + */
> +#define XEN_MSR_PAT 0x050100070406ul

Not sure whether it would make sense to use MASK_INSR and define each
page attribute field in order to create this value.

Thanks, Roger.

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

Re: [PATCH 4/6] x86/vmx: Simplify PAT handling during vcpu construction

[Roger Pau Monné]

On Mon, May 28, 2018 at 03:27:56PM +0100, Andrew Cooper wrote:
> The host PAT value is a compile time constant, and doesn't need to be read out
> of hardware.  Merge this if block into the previous block, which has an
> identical condition.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>

Thanks.

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

Re: [PATCH 5/6] x86/vmx: Defer vmx_vmcs_exit() as long as possible in construct_vmcs()

[Roger Pau Monné]

On Mon, May 28, 2018 at 03:27:57PM +0100, Andrew Cooper wrote:
> paging_update_paging_modes() and vmx_vlapic_msr_changed() both operate on the
> VMCS being constructed.  Avoid dropping and re-acquiring the reference
> multiple times.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> ---
> CC: Jan Beulich <JBeulich@suse.com>
> CC: Jun Nakajima <jun.nakajima@intel.com>
> CC: Kevin Tian <kevin.tian@intel.com>
> CC: Wei Liu <wei.liu2@citrix.com>
> CC: Roger Pau Monné <roger.pau@citrix.com>
> ---
>  xen/arch/x86/hvm/vmx/vmcs.c | 10 ++++++----
>  1 file changed, 6 insertions(+), 4 deletions(-)
> 
> diff --git a/xen/arch/x86/hvm/vmx/vmcs.c b/xen/arch/x86/hvm/vmx/vmcs.c
> index be02be1..ce78f19 100644
> --- a/xen/arch/x86/hvm/vmx/vmcs.c
> +++ b/xen/arch/x86/hvm/vmx/vmcs.c
> @@ -996,6 +996,7 @@ static int construct_vmcs(struct vcpu *v)
>      struct domain *d = v->domain;
>      u32 vmexit_ctl = vmx_vmexit_control;
>      u32 vmentry_ctl = vmx_vmentry_control;
> +    int rc;
>  
>      vmx_vmcs_enter(v);
>  
> @@ -1083,8 +1084,8 @@ static int construct_vmcs(struct vcpu *v)
>  
>          if ( msr_bitmap == NULL )
>          {
> -            vmx_vmcs_exit(v);
> -            return -ENOMEM;
> +            rc = -ENOMEM;
> +            goto out;
>          }
>  
>          memset(msr_bitmap, ~0, PAGE_SIZE);
> @@ -1258,13 +1259,14 @@ static int construct_vmcs(struct vcpu *v)
>      if ( cpu_has_vmx_tsc_scaling )
>          __vmwrite(TSC_MULTIPLIER, d->arch.hvm_domain.tsc_scaling_ratio);
>  
> -    vmx_vmcs_exit(v);
> -
>      /* will update HOST & GUEST_CR3 as reqd */
>      paging_update_paging_modes(v);
>  
>      vmx_vlapic_msr_changed(v);
>  
> + out:
> +    vmx_vmcs_exit(v);
> +
>      return 0;

Shouldn't you return rc here? Or else you lose the error value.

Roger.

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

Re: [PATCH 6/6] x86/vmx: Drop VMX signal for full real-mode

[Roger Pau Monné]

On Mon, May 28, 2018 at 03:27:58PM +0100, Andrew Cooper wrote:
> The hvmloader code which used this signal was deleted 10 years ago (c/s
> 50b12df83 "x86 vmx: Remove vmxassist").  Furthermore, the value gets discarded
> anyway because the HVM domain builder unconditionally sets %rax to 0 in the
> same action it uses to set %rip to the appropriate entrypoint.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>

Thanks.

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

Re: [PATCH 5/6] x86/vmx: Defer vmx_vmcs_exit() as long as possible in construct_vmcs()

[Andrew Cooper]

On 06/06/18 10:45, Roger Pau Monné wrote:
> On Mon, May 28, 2018 at 03:27:57PM +0100, Andrew Cooper wrote:
>> paging_update_paging_modes() and vmx_vlapic_msr_changed() both operate on the
>> VMCS being constructed.  Avoid dropping and re-acquiring the reference
>> multiple times.
>>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
>> ---
>> CC: Jan Beulich <JBeulich@suse.com>
>> CC: Jun Nakajima <jun.nakajima@intel.com>
>> CC: Kevin Tian <kevin.tian@intel.com>
>> CC: Wei Liu <wei.liu2@citrix.com>
>> CC: Roger Pau Monné <roger.pau@citrix.com>
>> ---
>>  xen/arch/x86/hvm/vmx/vmcs.c | 10 ++++++----
>>  1 file changed, 6 insertions(+), 4 deletions(-)
>>
>> diff --git a/xen/arch/x86/hvm/vmx/vmcs.c b/xen/arch/x86/hvm/vmx/vmcs.c
>> index be02be1..ce78f19 100644
>> --- a/xen/arch/x86/hvm/vmx/vmcs.c
>> +++ b/xen/arch/x86/hvm/vmx/vmcs.c
>> @@ -996,6 +996,7 @@ static int construct_vmcs(struct vcpu *v)
>>      struct domain *d = v->domain;
>>      u32 vmexit_ctl = vmx_vmexit_control;
>>      u32 vmentry_ctl = vmx_vmentry_control;
>> +    int rc;
>>  
>>      vmx_vmcs_enter(v);
>>  
>> @@ -1083,8 +1084,8 @@ static int construct_vmcs(struct vcpu *v)
>>  
>>          if ( msr_bitmap == NULL )
>>          {
>> -            vmx_vmcs_exit(v);
>> -            return -ENOMEM;
>> +            rc = -ENOMEM;
>> +            goto out;
>>          }
>>  
>>          memset(msr_bitmap, ~0, PAGE_SIZE);
>> @@ -1258,13 +1259,14 @@ static int construct_vmcs(struct vcpu *v)
>>      if ( cpu_has_vmx_tsc_scaling )
>>          __vmwrite(TSC_MULTIPLIER, d->arch.hvm_domain.tsc_scaling_ratio);
>>  
>> -    vmx_vmcs_exit(v);
>> -
>>      /* will update HOST & GUEST_CR3 as reqd */
>>      paging_update_paging_modes(v);
>>  
>>      vmx_vlapic_msr_changed(v);
>>  
>> + out:
>> +    vmx_vmcs_exit(v);
>> +
>>      return 0;
> Shouldn't you return rc here? Or else you lose the error value.

Yeah - Coverity told me the same...  Fixed up.

~Andrew

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