[PATCH v2 00/15] x86/HVM: misc tidying

[PATCH v2 00/15] x86/HVM: misc tidying

[Jan Beulich]

01: VMX: drop vmx_virt_exception and make vmx_vmfunc static
02: x86/HVM: hide SVM/VMX when their enabling is prohibited by firmware
03: VMX: don't run with CR4.VMXE set when VMX could not be enabled
04: x86/HVM: drop tsc_scaling.setup() hook
05: x86: amend cpu_has_xen_{ibt,shstk}
06: x86/HVM: improve CET-IBT pruning of ENDBR
07: VMX: drop vmcs_revision_id
08: VMX: convert vmx_basic_msr
09: VMX: convert vmx_pin_based_exec_control
10: VMX: convert vmx_cpu_based_exec_control
11: VMX: convert vmx_secondary_exec_control
12: VMX: convert vmx_vmexit_control
13: VMX: convert vmx_vmentry_control
14: VMX: convert vmx_ept_vpid_cap
15: VMX: convert vmx_vmfunc

Jan

[PATCH v2 01/15] VMX: drop vmx_virt_exception and make vmx_vmfunc static

[Jan Beulich]

The variable was introduced by 69b830e5ffb4 ("VMX: VMFUNC and #VE
definitions and detection") without any use and - violating Misra C:2012
rule 8.4 - without a declaration. Since no use has appeared, drop it.

For vmx_vmfunc the situation is similar, but not identical: It at least
has one use. Convert it to be static (and make style adjustments while
there).

Signed-off-by: Jan Beulich <jbeulich@suse.com>
Acked-by: Roger Pau Monné <roger.pau@citrix.com>
---
In how far the sole vmx_vmfunc use is actually meaningful (on its own)
I'm not really sure.

--- a/xen/arch/x86/hvm/vmx/vmcs.c
+++ b/xen/arch/x86/hvm/vmx/vmcs.c
@@ -167,8 +167,7 @@ u32 vmx_secondary_exec_control __read_mo
 u32 vmx_vmexit_control __read_mostly;
 u32 vmx_vmentry_control __read_mostly;
 u64 vmx_ept_vpid_cap __read_mostly;
-u64 vmx_vmfunc __read_mostly;
-bool vmx_virt_exception __read_mostly;
+static uint64_t __read_mostly vmx_vmfunc;
 
 static DEFINE_PER_CPU_READ_MOSTLY(paddr_t, vmxon_region);
 static DEFINE_PER_CPU(paddr_t, current_vmcs);
@@ -475,8 +474,7 @@ static int vmx_init_vmcs_config(bool bsp
         vmx_basic_msr              = ((u64)vmx_basic_msr_high << 32) |
                                      vmx_basic_msr_low;
         vmx_vmfunc                 = _vmx_vmfunc;
-        vmx_virt_exception         = !!(_vmx_secondary_exec_control &
-                                       SECONDARY_EXEC_ENABLE_VIRT_EXCEPTIONS);
+
         vmx_display_features();
 
         /* IA-32 SDM Vol 3B: VMCS size is never greater than 4kB. */

[PATCH v2 02/15] x86/HVM: hide SVM/VMX when their enabling is prohibited by firmware

[Jan Beulich]

... or we fail to enable the functionality on the BSP for other reasons.
The only place where hardware announcing the feature is recorded is the
raw CPU policy/featureset.

Inspired by https://lore.kernel.org/all/20230921114940.957141-1-pbonzini@redhat.com/.

Signed-off-by: Jan Beulich <jbeulich@suse.com>
Acked-by: Roger Pau Monné <roger.pau@citrix.com>

--- a/xen/arch/x86/hvm/svm/svm.c
+++ b/xen/arch/x86/hvm/svm/svm.c
@@ -2543,6 +2543,7 @@ const struct hvm_function_table * __init
 
     if ( _svm_cpu_up(true) )
     {
+        setup_clear_cpu_cap(X86_FEATURE_SVM);
         printk("SVM: failed to initialise.\n");
         return NULL;
     }
--- a/xen/arch/x86/hvm/vmx/vmcs.c
+++ b/xen/arch/x86/hvm/vmx/vmcs.c
@@ -2162,6 +2162,23 @@ int __init vmx_vmcs_init(void)
 
     if ( !ret )
         register_keyhandler('v', vmcs_dump, "dump VT-x VMCSs", 1);
+    else
+    {
+        setup_clear_cpu_cap(X86_FEATURE_VMX);
+
+        /*
+         * _vmx_vcpu_up() may have made it past feature identification.
+         * Make sure all dependent features are off as well.
+         */
+        vmx_basic_msr              = 0;
+        vmx_pin_based_exec_control = 0;
+        vmx_cpu_based_exec_control = 0;
+        vmx_secondary_exec_control = 0;
+        vmx_vmexit_control         = 0;
+        vmx_vmentry_control        = 0;
+        vmx_ept_vpid_cap           = 0;
+        vmx_vmfunc                 = 0;
+    }
 
     return ret;
 }

[PATCH v2 03/15] VMX: don't run with CR4.VMXE set when VMX could not be enabled

[Jan Beulich]

While generally benign, doing so is still at best misleading.

Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
Using set_in_cr4() seems favorable over updating mmu_cr4_features
despite the resulting redundant CR4 update. But I certainly could be
talked into going the alternative route.
---
v2: Actually clear CR4.VMXE for the BSP on the error path.

--- a/xen/arch/x86/hvm/vmx/vmx.c
+++ b/xen/arch/x86/hvm/vmx/vmx.c
@@ -2959,14 +2959,18 @@ static bool __init has_if_pschange_mc(vo
 
 const struct hvm_function_table * __init start_vmx(void)
 {
-    set_in_cr4(X86_CR4_VMXE);
+    write_cr4(read_cr4() | X86_CR4_VMXE);
 
     if ( vmx_vmcs_init() )
     {
+        write_cr4(read_cr4() & ~X86_CR4_VMXE);
         printk("VMX: failed to initialise.\n");
         return NULL;
     }
 
+    /* Arrange for APs to have CR4.VMXE set early on. */
+    set_in_cr4(X86_CR4_VMXE);
+
     vmx_function_table.singlestep_supported = cpu_has_monitor_trap_flag;
 
     if ( cpu_has_vmx_dt_exiting )

[PATCH v2 04/15] x86/HVM: drop tsc_scaling.setup() hook

[Jan Beulich]

This was used by VMX only, and the intended VMCS write can as well
happen from vmx_set_tsc_offset(), invoked (directly or indirectly)
almost immediately after the present call sites of the hook.
vmx_set_tsc_offset() isn't invoked frequently elsewhere, so the extra
VMCS write shouldn't raise performance concerns.

Signed-off-by: Jan Beulich <jbeulich@suse.com>
Acked-by: Roger Pau Monné <roger.pau@citrix.com>

--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -1086,9 +1086,6 @@ static int cf_check hvm_load_cpu_ctxt(st
     v->arch.hvm.guest_cr[2] = ctxt.cr2;
     hvm_update_guest_cr(v, 2);
 
-    if ( hvm_funcs.tsc_scaling.setup )
-        alternative_vcall(hvm_funcs.tsc_scaling.setup, v);
-
     v->arch.msrs->tsc_aux = ctxt.msr_tsc_aux;
 
     hvm_set_guest_tsc_fixed(v, ctxt.tsc, d->arch.hvm.sync_tsc);
@@ -4033,9 +4030,6 @@ void hvm_vcpu_reset_state(struct vcpu *v
     hvm_set_segment_register(v, x86_seg_gdtr, &reg);
     hvm_set_segment_register(v, x86_seg_idtr, &reg);
 
-    if ( hvm_funcs.tsc_scaling.setup )
-        alternative_vcall(hvm_funcs.tsc_scaling.setup, v);
-
     /* Sync AP's TSC with BSP's. */
     v->arch.hvm.cache_tsc_offset =
         v->domain->vcpu[0]->arch.hvm.cache_tsc_offset;
--- a/xen/arch/x86/hvm/vmx/vmx.c
+++ b/xen/arch/x86/hvm/vmx/vmx.c
@@ -1454,20 +1454,13 @@ static void cf_check vmx_handle_cd(struc
     }
 }
 
-static void cf_check vmx_setup_tsc_scaling(struct vcpu *v)
-{
-    if ( v->domain->arch.vtsc )
-        return;
-
-    vmx_vmcs_enter(v);
-    __vmwrite(TSC_MULTIPLIER, hvm_tsc_scaling_ratio(v->domain));
-    vmx_vmcs_exit(v);
-}
-
 static void cf_check vmx_set_tsc_offset(struct vcpu *v, u64 offset, u64 at_tsc)
 {
     vmx_vmcs_enter(v);
 
+    if ( !v->domain->arch.vtsc && cpu_has_vmx_tsc_scaling )
+        __vmwrite(TSC_MULTIPLIER, hvm_tsc_scaling_ratio(v->domain));
+
     if ( nestedhvm_vcpu_in_guestmode(v) )
         offset += nvmx_get_tsc_offset(v);
 
@@ -3031,10 +3024,7 @@ const struct hvm_function_table * __init
     }
 
     if ( cpu_has_vmx_tsc_scaling )
-    {
         vmx_function_table.tsc_scaling.ratio_frac_bits = 48;
-        vmx_function_table.tsc_scaling.setup = vmx_setup_tsc_scaling;
-    }
 
     model_specific_lbr = get_model_specific_lbr();
     lbr_tsx_fixup_check();
--- a/xen/arch/x86/include/asm/hvm/hvm.h
+++ b/xen/arch/x86/include/asm/hvm/hvm.h
@@ -240,9 +240,6 @@ struct hvm_function_table {
         uint8_t  ratio_frac_bits;
         /* maximum-allowed TSC scaling ratio */
         uint64_t max_ratio;
-
-        /* Architecture function to setup TSC scaling ratio */
-        void (*setup)(struct vcpu *v);
     } tsc_scaling;
 };
 

[PATCH v2 05/15] x86: amend cpu_has_xen_[ibt,shstk}

[Jan Beulich]

... to evaluate to false at compile-time when the respective Kconfig
control is off, thus allowing the compiler to eliminate then-dead code.

Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
v2: New.

--- a/xen/arch/x86/include/asm/cpufeature.h
+++ b/xen/arch/x86/include/asm/cpufeature.h
@@ -215,8 +215,10 @@ static inline bool boot_cpu_has(unsigned
 #define cpu_has_lfence_dispatch boot_cpu_has(X86_FEATURE_LFENCE_DISPATCH)
 #define cpu_has_nscb            boot_cpu_has(X86_FEATURE_NSCB)
 #define cpu_has_xen_lbr         boot_cpu_has(X86_FEATURE_XEN_LBR)
-#define cpu_has_xen_shstk       boot_cpu_has(X86_FEATURE_XEN_SHSTK)
-#define cpu_has_xen_ibt         boot_cpu_has(X86_FEATURE_XEN_IBT)
+#define cpu_has_xen_shstk       (IS_ENABLED(CONFIG_XEN_SHSTK) && \
+                                 boot_cpu_has(X86_FEATURE_XEN_SHSTK))
+#define cpu_has_xen_ibt         (IS_ENABLED(CONFIG_XEN_IBT) && \
+                                 boot_cpu_has(X86_FEATURE_XEN_IBT))
 
 #define cpu_has_msr_tsc_aux     (cpu_has_rdtscp || cpu_has_rdpid)
 

[PATCH v2 06/15] x86/HVM: improve CET-IBT pruning of ENDBR

[Jan Beulich]

__init{const,data}_cf_clobber can have an effect only for pointers
actually populated in the respective tables. While not the case for SVM
right now, VMX installs a number of pointers only under certain
conditions. Hence the respective functions would have their ENDBR purged
only when those conditions are met. Invoke "pruning" functions after
having copied the respective tables, for them to install any "missing"
pointers.

Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
This is largely cosmetic for present hardware, which when supporting
CET-IBT likely also supports all of the advanced VMX features for which
hook pointers are installed conditionally. The only case this would make
a difference there is when use of respective features was suppressed via
command line option (where available). For future hooks it may end up
relevant even by default, and it also would be if AMD started supporting
CET-IBT; right now it matters only for .pi_update_irte, as iommu_intpost
continues to default to off.

Originally I had meant to put the SVM and VMX functions in presmp-
initcalls, but hvm/{svm,vmx}/built_in.o are linked into hvm/built_in.o
before hvm/hvm.o. And I don't think I want to fiddle with link order
here.
---
v2: Use cpu_has_xen_ibt in prune_{svm,vmx}().

--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -161,10 +161,15 @@ static int __init cf_check hvm_enable(vo
     else if ( cpu_has_svm )
         fns = start_svm();
 
+    if ( fns )
+        hvm_funcs = *fns;
+
+    prune_vmx();
+    prune_svm();
+
     if ( fns == NULL )
         return 0;
 
-    hvm_funcs = *fns;
     hvm_enabled = 1;
 
     printk("HVM: %s enabled\n", fns->name);
--- a/xen/arch/x86/hvm/svm/svm.c
+++ b/xen/arch/x86/hvm/svm/svm.c
@@ -2587,6 +2587,19 @@ const struct hvm_function_table * __init
     return &svm_function_table;
 }
 
+void __init prune_svm(void)
+{
+    /*
+     * Now that svm_function_table was copied, populate all function pointers
+     * which may have been left at NULL, for __initdata_cf_clobber to have as
+     * much of an effect as possible.
+     */
+    if ( !cpu_has_xen_ibt )
+        return;
+
+    /* Nothing at present. */
+}
+
 void asmlinkage svm_vmexit_handler(void)
 {
     struct cpu_user_regs *regs = guest_cpu_user_regs();
--- a/xen/arch/x86/hvm/vmx/vmx.c
+++ b/xen/arch/x86/hvm/vmx/vmx.c
@@ -3033,6 +3033,30 @@ const struct hvm_function_table * __init
     return &vmx_function_table;
 }
 
+void __init prune_vmx(void)
+{
+    /*
+     * Now that vmx_function_table was copied, populate all function pointers
+     * which may have been left at NULL, for __initdata_cf_clobber to have as
+     * much of an effect as possible.
+     */
+    if ( !cpu_has_xen_ibt )
+        return;
+
+    vmx_function_table.set_descriptor_access_exiting =
+        vmx_set_descriptor_access_exiting;
+
+    vmx_function_table.update_eoi_exit_bitmap = vmx_update_eoi_exit_bitmap;
+    vmx_function_table.process_isr            = vmx_process_isr;
+    vmx_function_table.handle_eoi             = vmx_handle_eoi;
+
+    vmx_function_table.pi_update_irte = vmx_pi_update_irte;
+
+    vmx_function_table.deliver_posted_intr = vmx_deliver_posted_intr;
+    vmx_function_table.sync_pir_to_irr     = vmx_sync_pir_to_irr;
+    vmx_function_table.test_pir            = vmx_test_pir;
+}
+
 /*
  * Not all cases receive valid value in the VM-exit instruction length field.
  * Callers must know what they're doing!
--- a/xen/arch/x86/include/asm/hvm/hvm.h
+++ b/xen/arch/x86/include/asm/hvm/hvm.h
@@ -250,6 +250,9 @@ extern s8 hvm_port80_allowed;
 extern const struct hvm_function_table *start_svm(void);
 extern const struct hvm_function_table *start_vmx(void);
 
+void prune_svm(void);
+void prune_vmx(void);
+
 int hvm_domain_initialise(struct domain *d,
                           const struct xen_domctl_createdomain *config);
 void hvm_domain_relinquish_resources(struct domain *d);

[PATCH v2 07/15] VMX: drop vmcs_revision_id

[Jan Beulich]

It's effectively redundant with vmx_basic_msr. For the #define
replacement to work, struct vmcs_struct's respective field name also
needs to change: Drop the not really meaningful "vmcs_" prefix from it.

Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
v2: New.

--- a/xen/arch/x86/hvm/vmx/vmcs.c
+++ b/xen/arch/x86/hvm/vmx/vmcs.c
@@ -174,7 +174,7 @@ static DEFINE_PER_CPU(paddr_t, current_v
 static DEFINE_PER_CPU(struct list_head, active_vmcs_list);
 DEFINE_PER_CPU(bool, vmxon);
 
-static u32 vmcs_revision_id __read_mostly;
+#define vmcs_revision_id (vmx_basic_msr & VMX_BASIC_REVISION_MASK)
 u64 __read_mostly vmx_basic_msr;
 
 static void __init vmx_display_features(void)
@@ -464,7 +464,6 @@ static int vmx_init_vmcs_config(bool bsp
     if ( !vmx_pin_based_exec_control )
     {
         /* First time through. */
-        vmcs_revision_id           = vmx_basic_msr_low & VMX_BASIC_REVISION_MASK;
         vmx_pin_based_exec_control = _vmx_pin_based_exec_control;
         vmx_cpu_based_exec_control = _vmx_cpu_based_exec_control;
         vmx_secondary_exec_control = _vmx_secondary_exec_control;
@@ -572,7 +571,7 @@ static paddr_t vmx_alloc_vmcs(void)
 
     vmcs = __map_domain_page(pg);
     clear_page(vmcs);
-    vmcs->vmcs_revision_id = vmcs_revision_id;
+    vmcs->revision_id = vmcs_revision_id;
     unmap_domain_page(vmcs);
 
     return page_to_maddr(pg);
--- a/xen/arch/x86/hvm/vmx/vvmx.c
+++ b/xen/arch/x86/hvm/vmx/vvmx.c
@@ -1163,7 +1163,7 @@ static void nvmx_set_vmcs_pointer(struct
     paddr_t vvmcs_maddr = v->arch.hvm.vmx.vmcs_shadow_maddr;
 
     __vmpclear(vvmcs_maddr);
-    vvmcs->vmcs_revision_id |= VMCS_RID_TYPE_MASK;
+    vvmcs->revision_id |= VMCS_RID_TYPE_MASK;
     v->arch.hvm.vmx.secondary_exec_control |=
         SECONDARY_EXEC_ENABLE_VMCS_SHADOWING;
     __vmwrite(SECONDARY_VM_EXEC_CONTROL,
@@ -1178,7 +1178,7 @@ static void nvmx_clear_vmcs_pointer(stru
     paddr_t vvmcs_maddr = v->arch.hvm.vmx.vmcs_shadow_maddr;
 
     __vmpclear(vvmcs_maddr);
-    vvmcs->vmcs_revision_id &= ~VMCS_RID_TYPE_MASK;
+    vvmcs->revision_id &= ~VMCS_RID_TYPE_MASK;
     v->arch.hvm.vmx.secondary_exec_control &=
         ~SECONDARY_EXEC_ENABLE_VMCS_SHADOWING;
     __vmwrite(SECONDARY_VM_EXEC_CONTROL,
@@ -1794,10 +1794,10 @@ static int nvmx_handle_vmptrld(struct cp
             {
                 struct vmcs_struct *vvmcs = vvmcx;
 
-                if ( ((vvmcs->vmcs_revision_id ^ vmx_basic_msr) &
-                                         VMX_BASIC_REVISION_MASK) ||
+                if ( ((vvmcs->revision_id ^ vmx_basic_msr) &
+                      VMX_BASIC_REVISION_MASK) ||
                      (!cpu_has_vmx_vmcs_shadowing &&
-                      (vvmcs->vmcs_revision_id & ~VMX_BASIC_REVISION_MASK)) )
+                      (vvmcs->revision_id & ~VMX_BASIC_REVISION_MASK)) )
                 {
                     hvm_unmap_guest_frame(vvmcx, 1);
                     vmfail(regs, VMX_INSN_VMPTRLD_INCORRECT_VMCS_ID);
@@ -2209,7 +2209,7 @@ int nvmx_msr_read_intercept(unsigned int
             map_domain_page(_mfn(PFN_DOWN(v->arch.hvm.vmx.vmcs_pa)));
 
         data = (host_data & (~0ul << 32)) |
-               (vmcs->vmcs_revision_id & 0x7fffffff);
+               (vmcs->revision_id & 0x7fffffff);
         unmap_domain_page(vmcs);
 
         if ( !cpu_has_vmx_vmcs_shadowing )
--- a/xen/arch/x86/include/asm/hvm/vmx/vmcs.h
+++ b/xen/arch/x86/include/asm/hvm/vmx/vmcs.h
@@ -17,7 +17,7 @@ int cf_check vmx_cpu_up(void);
 void cf_check vmx_cpu_down(void);
 
 struct vmcs_struct {
-    u32 vmcs_revision_id;
+    uint32_t revision_id;
     unsigned char data [0]; /* vmcs size is read from MSR */
 };
 

[PATCH v2 08/15] VMX: convert vmx_basic_msr

[Jan Beulich]

... to a struct field, which is then going to be accompanied by other
capability/control data presently living in individual variables. As
this structure isn't supposed to be altered post-boot, put it in
.data.ro_after_init right away.

Suggested-by: Roger Pau Monné <roger.pau@citrix.com>
Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
v2: New.

--- a/xen/arch/x86/hvm/vmx/vmcs.c
+++ b/xen/arch/x86/hvm/vmx/vmcs.c
@@ -161,6 +161,7 @@ static int cf_check parse_ept_param_runt
 #endif
 
 /* Dynamic (run-time adjusted) execution control flags. */
+struct vmx_caps __ro_after_init vmx_caps;
 u32 vmx_pin_based_exec_control __read_mostly;
 u32 vmx_cpu_based_exec_control __read_mostly;
 u32 vmx_secondary_exec_control __read_mostly;
@@ -174,8 +175,7 @@ static DEFINE_PER_CPU(paddr_t, current_v
 static DEFINE_PER_CPU(struct list_head, active_vmcs_list);
 DEFINE_PER_CPU(bool, vmxon);
 
-#define vmcs_revision_id (vmx_basic_msr & VMX_BASIC_REVISION_MASK)
-u64 __read_mostly vmx_basic_msr;
+#define vmcs_revision_id (vmx_caps.basic_msr & VMX_BASIC_REVISION_MASK)
 
 static void __init vmx_display_features(void)
 {
@@ -470,8 +470,8 @@ static int vmx_init_vmcs_config(bool bsp
         vmx_ept_vpid_cap           = _vmx_ept_vpid_cap;
         vmx_vmexit_control         = _vmx_vmexit_control;
         vmx_vmentry_control        = _vmx_vmentry_control;
-        vmx_basic_msr              = ((u64)vmx_basic_msr_high << 32) |
-                                     vmx_basic_msr_low;
+        vmx_caps.basic_msr = ((uint64_t)vmx_basic_msr_high << 32) |
+                             vmx_basic_msr_low;
         vmx_vmfunc                 = _vmx_vmfunc;
 
         vmx_display_features();
@@ -522,7 +522,7 @@ static int vmx_init_vmcs_config(bool bsp
             mismatch = 1;
         }
         if ( (vmx_basic_msr_high & (VMX_BASIC_VMCS_SIZE_MASK >> 32)) !=
-             ((vmx_basic_msr & VMX_BASIC_VMCS_SIZE_MASK) >> 32) )
+             ((vmx_caps.basic_msr & VMX_BASIC_VMCS_SIZE_MASK) >> 32) )
         {
             printk("VMX: CPU%d unexpected VMCS size %Lu\n",
                    smp_processor_id(),
@@ -2169,7 +2169,7 @@ int __init vmx_vmcs_init(void)
          * _vmx_vcpu_up() may have made it past feature identification.
          * Make sure all dependent features are off as well.
          */
-        vmx_basic_msr              = 0;
+        memset(&vmx_caps, 0, sizeof(vmx_caps));
         vmx_pin_based_exec_control = 0;
         vmx_cpu_based_exec_control = 0;
         vmx_secondary_exec_control = 0;
--- a/xen/arch/x86/include/asm/hvm/vmx/vmcs.h
+++ b/xen/arch/x86/include/asm/hvm/vmx/vmcs.h
@@ -283,6 +283,12 @@ extern u64 vmx_ept_vpid_cap;
 
 #define VMX_TSC_MULTIPLIER_MAX                  0xffffffffffffffffULL
 
+/* Capabilities and dynamic (run-time adjusted) execution control flags. */
+struct vmx_caps {
+    uint64_t basic_msr;
+};
+extern struct vmx_caps vmx_caps;
+
 #define cpu_has_wbinvd_exiting \
     (vmx_secondary_exec_control & SECONDARY_EXEC_WBINVD_EXITING)
 #define cpu_has_vmx_virtualize_apic_accesses \
@@ -366,9 +372,8 @@ extern u64 vmx_ept_vpid_cap;
  */
 #define VMX_BASIC_DEFAULT1_ZERO		(1ULL << 55)
 
-extern u64 vmx_basic_msr;
 #define cpu_has_vmx_ins_outs_instr_info \
-    (!!(vmx_basic_msr & VMX_BASIC_INS_OUT_INFO))
+    (!!(vmx_caps.basic_msr & VMX_BASIC_INS_OUT_INFO))
 
 /* Guest interrupt status */
 #define VMX_GUEST_INTR_STATUS_SUBFIELD_BITMASK  0x0FF
--- a/xen/arch/x86/hvm/vmx/vvmx.c
+++ b/xen/arch/x86/hvm/vmx/vvmx.c
@@ -1556,7 +1556,7 @@ static int nvmx_handle_vmxon(struct cpu_
     rc = hvm_copy_from_guest_phys(&nvmcs_revid, gpa, sizeof(nvmcs_revid));
     if ( rc != HVMTRANS_okay ||
          (nvmcs_revid & ~VMX_BASIC_REVISION_MASK) ||
-         ((nvmcs_revid ^ vmx_basic_msr) & VMX_BASIC_REVISION_MASK) )
+         ((nvmcs_revid ^ vmx_caps.basic_msr) & VMX_BASIC_REVISION_MASK) )
     {
         vmfail_invalid(regs);
         return X86EMUL_OKAY;
@@ -1794,7 +1794,7 @@ static int nvmx_handle_vmptrld(struct cp
             {
                 struct vmcs_struct *vvmcs = vvmcx;
 
-                if ( ((vvmcs->revision_id ^ vmx_basic_msr) &
+                if ( ((vvmcs->revision_id ^ vmx_caps.basic_msr) &
                       VMX_BASIC_REVISION_MASK) ||
                      (!cpu_has_vmx_vmcs_shadowing &&
                       (vvmcs->revision_id & ~VMX_BASIC_REVISION_MASK)) )
@@ -2187,7 +2187,7 @@ int nvmx_msr_read_intercept(unsigned int
     case MSR_IA32_VMX_TRUE_PROCBASED_CTLS:
     case MSR_IA32_VMX_TRUE_EXIT_CTLS:
     case MSR_IA32_VMX_TRUE_ENTRY_CTLS:
-        if ( !(vmx_basic_msr & VMX_BASIC_DEFAULT1_ZERO) )
+        if ( !(vmx_caps.basic_msr & VMX_BASIC_DEFAULT1_ZERO) )
             return 0;
         break;
 

[PATCH v2 09/15] VMX: convert vmx_pin_based_exec_control

[Jan Beulich]

... to a field in the capability/controls struct. Use an instance of
that struct also in vmx_init_vmcs_config().

Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
v2: New.

--- a/xen/arch/x86/hvm/vmx/vmcs.c
+++ b/xen/arch/x86/hvm/vmx/vmcs.c
@@ -162,7 +162,6 @@ static int cf_check parse_ept_param_runt
 
 /* Dynamic (run-time adjusted) execution control flags. */
 struct vmx_caps __ro_after_init vmx_caps;
-u32 vmx_pin_based_exec_control __read_mostly;
 u32 vmx_cpu_based_exec_control __read_mostly;
 u32 vmx_secondary_exec_control __read_mostly;
 u32 vmx_vmexit_control __read_mostly;
@@ -238,7 +237,7 @@ static bool cap_check(const char *name,
 static int vmx_init_vmcs_config(bool bsp)
 {
     u32 vmx_basic_msr_low, vmx_basic_msr_high, min, opt;
-    u32 _vmx_pin_based_exec_control;
+    struct vmx_caps caps;
     u32 _vmx_cpu_based_exec_control;
     u32 _vmx_secondary_exec_control = 0;
     u64 _vmx_ept_vpid_cap = 0;
@@ -254,7 +253,7 @@ static int vmx_init_vmcs_config(bool bsp
            PIN_BASED_NMI_EXITING);
     opt = (PIN_BASED_VIRTUAL_NMIS |
            PIN_BASED_POSTED_INTERRUPT);
-    _vmx_pin_based_exec_control = adjust_vmx_controls(
+    caps.pin_based_exec_control = adjust_vmx_controls(
         "Pin-Based Exec Control", min, opt,
         MSR_IA32_VMX_PINBASED_CTLS, &mismatch);
 
@@ -406,7 +405,7 @@ static int vmx_init_vmcs_config(bool bsp
     if ( (_vmx_secondary_exec_control & SECONDARY_EXEC_PAUSE_LOOP_EXITING) &&
           ple_gap == 0 )
     {
-        if ( !vmx_pin_based_exec_control )
+        if ( !vmx_caps.pin_based_exec_control )
             printk(XENLOG_INFO "Disable Pause-Loop Exiting.\n");
         _vmx_secondary_exec_control &= ~ SECONDARY_EXEC_PAUSE_LOOP_EXITING;
     }
@@ -424,10 +423,10 @@ static int vmx_init_vmcs_config(bool bsp
      * is a minimal requirement, only check the former, which is optional.
      */
     if ( !(_vmx_secondary_exec_control & SECONDARY_EXEC_VIRTUAL_INTR_DELIVERY) )
-        _vmx_pin_based_exec_control &= ~PIN_BASED_POSTED_INTERRUPT;
+        caps.pin_based_exec_control &= ~PIN_BASED_POSTED_INTERRUPT;
 
     if ( iommu_intpost &&
-         !(_vmx_pin_based_exec_control & PIN_BASED_POSTED_INTERRUPT) )
+         !(caps.pin_based_exec_control & PIN_BASED_POSTED_INTERRUPT) )
     {
         printk("Intel VT-d Posted Interrupt is disabled for CPU-side Posted "
                "Interrupt is not enabled\n");
@@ -461,10 +460,10 @@ static int vmx_init_vmcs_config(bool bsp
     if ( mismatch )
         return -EINVAL;
 
-    if ( !vmx_pin_based_exec_control )
+    if ( !vmx_caps.pin_based_exec_control )
     {
         /* First time through. */
-        vmx_pin_based_exec_control = _vmx_pin_based_exec_control;
+        vmx_caps = caps;
         vmx_cpu_based_exec_control = _vmx_cpu_based_exec_control;
         vmx_secondary_exec_control = _vmx_secondary_exec_control;
         vmx_ept_vpid_cap           = _vmx_ept_vpid_cap;
@@ -494,7 +493,7 @@ static int vmx_init_vmcs_config(bool bsp
             vmcs_revision_id, vmx_basic_msr_low & VMX_BASIC_REVISION_MASK);
         mismatch |= cap_check(
             "Pin-Based Exec Control",
-            vmx_pin_based_exec_control, _vmx_pin_based_exec_control);
+            vmx_caps.pin_based_exec_control, caps.pin_based_exec_control);
         mismatch |= cap_check(
             "CPU-Based Exec Control",
             vmx_cpu_based_exec_control, _vmx_cpu_based_exec_control);
@@ -1072,7 +1071,7 @@ static int construct_vmcs(struct vcpu *v
     vmx_vmcs_enter(v);
 
     /* VMCS controls. */
-    __vmwrite(PIN_BASED_VM_EXEC_CONTROL, vmx_pin_based_exec_control);
+    __vmwrite(PIN_BASED_VM_EXEC_CONTROL, vmx_caps.pin_based_exec_control);
 
     v->arch.hvm.vmx.exec_control = vmx_cpu_based_exec_control;
     if ( d->arch.vtsc && !cpu_has_vmx_tsc_scaling )
@@ -2091,7 +2090,7 @@ void vmcs_dump_vcpu(struct vcpu *v)
     printk("TSC Offset = 0x%016lx  TSC Multiplier = 0x%016lx\n",
            vmr(TSC_OFFSET), vmr(TSC_MULTIPLIER));
     if ( (v->arch.hvm.vmx.exec_control & CPU_BASED_TPR_SHADOW) ||
-         (vmx_pin_based_exec_control & PIN_BASED_POSTED_INTERRUPT) )
+         (vmx_caps.pin_based_exec_control & PIN_BASED_POSTED_INTERRUPT) )
         printk("TPR Threshold = 0x%02x  PostedIntrVec = 0x%02x\n",
                vmr32(TPR_THRESHOLD), vmr16(POSTED_INTR_NOTIFICATION_VECTOR));
     if ( (v->arch.hvm.vmx.secondary_exec_control &
@@ -2170,7 +2169,6 @@ int __init vmx_vmcs_init(void)
          * Make sure all dependent features are off as well.
          */
         memset(&vmx_caps, 0, sizeof(vmx_caps));
-        vmx_pin_based_exec_control = 0;
         vmx_cpu_based_exec_control = 0;
         vmx_secondary_exec_control = 0;
         vmx_vmexit_control         = 0;
--- a/xen/arch/x86/hvm/vmx/vvmx.c
+++ b/xen/arch/x86/hvm/vmx/vvmx.c
@@ -1054,7 +1054,7 @@ static void load_shadow_control(struct v
      * and EXCEPTION
      * Enforce the removed features
      */
-    nvmx_update_pin_control(v, vmx_pin_based_exec_control);
+    nvmx_update_pin_control(v, vmx_caps.pin_based_exec_control);
     vmx_update_cpu_exec_control(v);
     vmx_update_secondary_exec_control(v);
     nvmx_update_exit_control(v, vmx_vmexit_control);
--- a/xen/arch/x86/include/asm/hvm/vmx/vmcs.h
+++ b/xen/arch/x86/include/asm/hvm/vmx/vmcs.h
@@ -215,7 +215,6 @@ extern u32 vmx_cpu_based_exec_control;
 #define PIN_BASED_VIRTUAL_NMIS          0x00000020
 #define PIN_BASED_PREEMPT_TIMER         0x00000040
 #define PIN_BASED_POSTED_INTERRUPT      0x00000080
-extern u32 vmx_pin_based_exec_control;
 
 #define VM_EXIT_SAVE_DEBUG_CNTRLS       0x00000004
 #define VM_EXIT_IA32E_MODE              0x00000200
@@ -286,6 +285,7 @@ extern u64 vmx_ept_vpid_cap;
 /* Capabilities and dynamic (run-time adjusted) execution control flags. */
 struct vmx_caps {
     uint64_t basic_msr;
+    uint32_t pin_based_exec_control;
 };
 extern struct vmx_caps vmx_caps;
 
@@ -296,7 +296,7 @@ extern struct vmx_caps vmx_caps;
 #define cpu_has_vmx_tpr_shadow \
     (vmx_cpu_based_exec_control & CPU_BASED_TPR_SHADOW)
 #define cpu_has_vmx_vnmi \
-    (vmx_pin_based_exec_control & PIN_BASED_VIRTUAL_NMIS)
+    (vmx_caps.pin_based_exec_control & PIN_BASED_VIRTUAL_NMIS)
 #define cpu_has_vmx_msr_bitmap \
     (vmx_cpu_based_exec_control & CPU_BASED_ACTIVATE_MSR_BITMAP)
 #define cpu_has_vmx_secondary_exec_control \
@@ -331,7 +331,7 @@ extern struct vmx_caps vmx_caps;
 #define cpu_has_vmx_virtualize_x2apic_mode \
     (vmx_secondary_exec_control & SECONDARY_EXEC_VIRTUALIZE_X2APIC_MODE)
 #define cpu_has_vmx_posted_intr_processing \
-    (vmx_pin_based_exec_control & PIN_BASED_POSTED_INTERRUPT)
+    (vmx_caps.pin_based_exec_control & PIN_BASED_POSTED_INTERRUPT)
 #define cpu_has_vmx_vmcs_shadowing \
     (vmx_secondary_exec_control & SECONDARY_EXEC_ENABLE_VMCS_SHADOWING)
 #define cpu_has_vmx_vmfunc \

[PATCH v2 10/15] VMX: convert vmx_cpu_based_exec_control

[Jan Beulich]

... to a field in the capability/controls struct.

Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
v2: New.

--- a/xen/arch/x86/hvm/vmx/vmcs.c
+++ b/xen/arch/x86/hvm/vmx/vmcs.c
@@ -162,7 +162,6 @@ static int cf_check parse_ept_param_runt
 
 /* Dynamic (run-time adjusted) execution control flags. */
 struct vmx_caps __ro_after_init vmx_caps;
-u32 vmx_cpu_based_exec_control __read_mostly;
 u32 vmx_secondary_exec_control __read_mostly;
 u32 vmx_vmexit_control __read_mostly;
 u32 vmx_vmentry_control __read_mostly;
@@ -238,7 +237,6 @@ static int vmx_init_vmcs_config(bool bsp
 {
     u32 vmx_basic_msr_low, vmx_basic_msr_high, min, opt;
     struct vmx_caps caps;
-    u32 _vmx_cpu_based_exec_control;
     u32 _vmx_secondary_exec_control = 0;
     u64 _vmx_ept_vpid_cap = 0;
     u64 _vmx_misc_cap = 0;
@@ -274,12 +272,12 @@ static int vmx_init_vmcs_config(bool bsp
            CPU_BASED_TPR_SHADOW |
            CPU_BASED_MONITOR_TRAP_FLAG |
            CPU_BASED_ACTIVATE_SECONDARY_CONTROLS);
-    _vmx_cpu_based_exec_control = adjust_vmx_controls(
+    caps.cpu_based_exec_control = adjust_vmx_controls(
         "CPU-Based Exec Control", min, opt,
         MSR_IA32_VMX_PROCBASED_CTLS, &mismatch);
-    _vmx_cpu_based_exec_control &= ~CPU_BASED_RDTSC_EXITING;
-    if ( _vmx_cpu_based_exec_control & CPU_BASED_TPR_SHADOW )
-        _vmx_cpu_based_exec_control &=
+    caps.cpu_based_exec_control &= ~CPU_BASED_RDTSC_EXITING;
+    if ( caps.cpu_based_exec_control & CPU_BASED_TPR_SHADOW )
+        caps.cpu_based_exec_control &=
             ~(CPU_BASED_CR8_LOAD_EXITING | CPU_BASED_CR8_STORE_EXITING);
 
     rdmsrl(MSR_IA32_VMX_MISC, _vmx_misc_cap);
@@ -296,7 +294,7 @@ static int vmx_init_vmcs_config(bool bsp
         return -EINVAL;
     }
 
-    if ( _vmx_cpu_based_exec_control & CPU_BASED_ACTIVATE_SECONDARY_CONTROLS )
+    if ( caps.cpu_based_exec_control & CPU_BASED_ACTIVATE_SECONDARY_CONTROLS )
     {
         min = 0;
         opt = (SECONDARY_EXEC_VIRTUALIZE_APIC_ACCESSES |
@@ -326,7 +324,7 @@ static int vmx_init_vmcs_config(bool bsp
          * "APIC Register Virtualization" and "Virtual Interrupt Delivery"
          * can be set only when "use TPR shadow" is set
          */
-        if ( (_vmx_cpu_based_exec_control & CPU_BASED_TPR_SHADOW) &&
+        if ( (caps.cpu_based_exec_control & CPU_BASED_TPR_SHADOW) &&
              opt_apicv_enabled )
             opt |= SECONDARY_EXEC_APIC_REGISTER_VIRT |
                    SECONDARY_EXEC_VIRTUAL_INTR_DELIVERY |
@@ -464,7 +462,6 @@ static int vmx_init_vmcs_config(bool bsp
     {
         /* First time through. */
         vmx_caps = caps;
-        vmx_cpu_based_exec_control = _vmx_cpu_based_exec_control;
         vmx_secondary_exec_control = _vmx_secondary_exec_control;
         vmx_ept_vpid_cap           = _vmx_ept_vpid_cap;
         vmx_vmexit_control         = _vmx_vmexit_control;
@@ -496,7 +493,7 @@ static int vmx_init_vmcs_config(bool bsp
             vmx_caps.pin_based_exec_control, caps.pin_based_exec_control);
         mismatch |= cap_check(
             "CPU-Based Exec Control",
-            vmx_cpu_based_exec_control, _vmx_cpu_based_exec_control);
+            vmx_caps.cpu_based_exec_control, caps.cpu_based_exec_control);
         mismatch |= cap_check(
             "Secondary Exec Control",
             vmx_secondary_exec_control, _vmx_secondary_exec_control);
@@ -1073,7 +1070,7 @@ static int construct_vmcs(struct vcpu *v
     /* VMCS controls. */
     __vmwrite(PIN_BASED_VM_EXEC_CONTROL, vmx_caps.pin_based_exec_control);
 
-    v->arch.hvm.vmx.exec_control = vmx_cpu_based_exec_control;
+    v->arch.hvm.vmx.exec_control = vmx_caps.cpu_based_exec_control;
     if ( d->arch.vtsc && !cpu_has_vmx_tsc_scaling )
         v->arch.hvm.vmx.exec_control |= CPU_BASED_RDTSC_EXITING;
 
@@ -2169,7 +2166,6 @@ int __init vmx_vmcs_init(void)
          * Make sure all dependent features are off as well.
          */
         memset(&vmx_caps, 0, sizeof(vmx_caps));
-        vmx_cpu_based_exec_control = 0;
         vmx_secondary_exec_control = 0;
         vmx_vmexit_control         = 0;
         vmx_vmentry_control        = 0;
--- a/xen/arch/x86/include/asm/hvm/vmx/vmcs.h
+++ b/xen/arch/x86/include/asm/hvm/vmx/vmcs.h
@@ -208,7 +208,6 @@ void vmx_vmcs_reload(struct vcpu *v);
 #define CPU_BASED_MONITOR_EXITING             0x20000000U
 #define CPU_BASED_PAUSE_EXITING               0x40000000U
 #define CPU_BASED_ACTIVATE_SECONDARY_CONTROLS 0x80000000U
-extern u32 vmx_cpu_based_exec_control;
 
 #define PIN_BASED_EXT_INTR_MASK         0x00000001
 #define PIN_BASED_NMI_EXITING           0x00000008
@@ -286,6 +285,7 @@ extern u64 vmx_ept_vpid_cap;
 struct vmx_caps {
     uint64_t basic_msr;
     uint32_t pin_based_exec_control;
+    uint32_t cpu_based_exec_control;
 };
 extern struct vmx_caps vmx_caps;
 
@@ -294,13 +294,13 @@ extern struct vmx_caps vmx_caps;
 #define cpu_has_vmx_virtualize_apic_accesses \
     (vmx_secondary_exec_control & SECONDARY_EXEC_VIRTUALIZE_APIC_ACCESSES)
 #define cpu_has_vmx_tpr_shadow \
-    (vmx_cpu_based_exec_control & CPU_BASED_TPR_SHADOW)
+    (vmx_caps.cpu_based_exec_control & CPU_BASED_TPR_SHADOW)
 #define cpu_has_vmx_vnmi \
     (vmx_caps.pin_based_exec_control & PIN_BASED_VIRTUAL_NMIS)
 #define cpu_has_vmx_msr_bitmap \
-    (vmx_cpu_based_exec_control & CPU_BASED_ACTIVATE_MSR_BITMAP)
+    (vmx_caps.cpu_based_exec_control & CPU_BASED_ACTIVATE_MSR_BITMAP)
 #define cpu_has_vmx_secondary_exec_control \
-    (vmx_cpu_based_exec_control & CPU_BASED_ACTIVATE_SECONDARY_CONTROLS)
+    (vmx_caps.cpu_based_exec_control & CPU_BASED_ACTIVATE_SECONDARY_CONTROLS)
 #define cpu_has_vmx_ept \
     (vmx_secondary_exec_control & SECONDARY_EXEC_ENABLE_EPT)
 #define cpu_has_vmx_dt_exiting \
@@ -310,7 +310,7 @@ extern struct vmx_caps vmx_caps;
 #define cpu_has_vmx_vpid \
     (vmx_secondary_exec_control & SECONDARY_EXEC_ENABLE_VPID)
 #define cpu_has_monitor_trap_flag \
-    (vmx_cpu_based_exec_control & CPU_BASED_MONITOR_TRAP_FLAG)
+    (vmx_caps.cpu_based_exec_control & CPU_BASED_MONITOR_TRAP_FLAG)
 #define cpu_has_vmx_pat \
     (vmx_vmentry_control & VM_ENTRY_LOAD_GUEST_PAT)
 #define cpu_has_vmx_efer \

[PATCH v2 11/15] VMX: convert vmx_secondary_exec_control

[Jan Beulich]

... to a field in the capability/controls struct.

Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
v2: New.

--- a/xen/arch/x86/hvm/vmx/vmcs.c
+++ b/xen/arch/x86/hvm/vmx/vmcs.c
@@ -162,7 +162,6 @@ static int cf_check parse_ept_param_runt
 
 /* Dynamic (run-time adjusted) execution control flags. */
 struct vmx_caps __ro_after_init vmx_caps;
-u32 vmx_secondary_exec_control __read_mostly;
 u32 vmx_vmexit_control __read_mostly;
 u32 vmx_vmentry_control __read_mostly;
 u64 vmx_ept_vpid_cap __read_mostly;
@@ -237,7 +236,6 @@ static int vmx_init_vmcs_config(bool bsp
 {
     u32 vmx_basic_msr_low, vmx_basic_msr_high, min, opt;
     struct vmx_caps caps;
-    u32 _vmx_secondary_exec_control = 0;
     u64 _vmx_ept_vpid_cap = 0;
     u64 _vmx_misc_cap = 0;
     u32 _vmx_vmexit_control;
@@ -330,13 +328,13 @@ static int vmx_init_vmcs_config(bool bsp
                    SECONDARY_EXEC_VIRTUAL_INTR_DELIVERY |
                    SECONDARY_EXEC_VIRTUALIZE_X2APIC_MODE;
 
-        _vmx_secondary_exec_control = adjust_vmx_controls(
+        caps.secondary_exec_control = adjust_vmx_controls(
             "Secondary Exec Control", min, opt,
             MSR_IA32_VMX_PROCBASED_CTLS2, &mismatch);
     }
 
     /* The IA32_VMX_EPT_VPID_CAP MSR exists only when EPT or VPID available */
-    if ( _vmx_secondary_exec_control & (SECONDARY_EXEC_ENABLE_EPT |
+    if ( caps.secondary_exec_control & (SECONDARY_EXEC_ENABLE_EPT |
                                         SECONDARY_EXEC_ENABLE_VPID) )
     {
         rdmsrl(MSR_IA32_VMX_EPT_VPID_CAP, _vmx_ept_vpid_cap);
@@ -358,7 +356,7 @@ static int vmx_init_vmcs_config(bool bsp
         if ( !(_vmx_ept_vpid_cap & VMX_EPT_MEMORY_TYPE_WB) ||
              !(_vmx_ept_vpid_cap & VMX_EPT_WALK_LENGTH_4_SUPPORTED) ||
              !(_vmx_ept_vpid_cap & VMX_EPT_INVEPT_ALL_CONTEXT) )
-            _vmx_secondary_exec_control &= ~SECONDARY_EXEC_ENABLE_EPT;
+            caps.secondary_exec_control &= ~SECONDARY_EXEC_ENABLE_EPT;
 
         /*
          * the CPU must support INVVPID all context invalidation, because we
@@ -367,14 +365,14 @@ static int vmx_init_vmcs_config(bool bsp
          * Or we just don't use VPID.
          */
         if ( !(_vmx_ept_vpid_cap & VMX_VPID_INVVPID_ALL_CONTEXT) )
-            _vmx_secondary_exec_control &= ~SECONDARY_EXEC_ENABLE_VPID;
+            caps.secondary_exec_control &= ~SECONDARY_EXEC_ENABLE_VPID;
 
         /* EPT A/D bits is required for PML */
         if ( !(_vmx_ept_vpid_cap & VMX_EPT_AD_BIT) )
-            _vmx_secondary_exec_control &= ~SECONDARY_EXEC_ENABLE_PML;
+            caps.secondary_exec_control &= ~SECONDARY_EXEC_ENABLE_PML;
     }
 
-    if ( _vmx_secondary_exec_control & SECONDARY_EXEC_ENABLE_EPT )
+    if ( caps.secondary_exec_control & SECONDARY_EXEC_ENABLE_EPT )
     {
         /*
          * To use EPT we expect to be able to clear certain intercepts.
@@ -387,25 +385,25 @@ static int vmx_init_vmcs_config(bool bsp
         if ( must_be_one & (CPU_BASED_INVLPG_EXITING |
                             CPU_BASED_CR3_LOAD_EXITING |
                             CPU_BASED_CR3_STORE_EXITING) )
-            _vmx_secondary_exec_control &=
+            caps.secondary_exec_control &=
                 ~(SECONDARY_EXEC_ENABLE_EPT |
                   SECONDARY_EXEC_UNRESTRICTED_GUEST);
     }
 
     /* PML cannot be supported if EPT is not used */
-    if ( !(_vmx_secondary_exec_control & SECONDARY_EXEC_ENABLE_EPT) )
-        _vmx_secondary_exec_control &= ~SECONDARY_EXEC_ENABLE_PML;
+    if ( !(caps.secondary_exec_control & SECONDARY_EXEC_ENABLE_EPT) )
+        caps.secondary_exec_control &= ~SECONDARY_EXEC_ENABLE_PML;
 
     /* Turn off opt_ept_pml if PML feature is not present. */
-    if ( !(_vmx_secondary_exec_control & SECONDARY_EXEC_ENABLE_PML) )
+    if ( !(caps.secondary_exec_control & SECONDARY_EXEC_ENABLE_PML) )
         opt_ept_pml = false;
 
-    if ( (_vmx_secondary_exec_control & SECONDARY_EXEC_PAUSE_LOOP_EXITING) &&
+    if ( (caps.secondary_exec_control & SECONDARY_EXEC_PAUSE_LOOP_EXITING) &&
           ple_gap == 0 )
     {
         if ( !vmx_caps.pin_based_exec_control )
             printk(XENLOG_INFO "Disable Pause-Loop Exiting.\n");
-        _vmx_secondary_exec_control &= ~ SECONDARY_EXEC_PAUSE_LOOP_EXITING;
+        caps.secondary_exec_control &= ~ SECONDARY_EXEC_PAUSE_LOOP_EXITING;
     }
 
     min = VM_EXIT_ACK_INTR_ON_EXIT;
@@ -420,7 +418,7 @@ static int vmx_init_vmcs_config(bool bsp
      * delivery" and "acknowledge interrupt on exit" is set. For the latter
      * is a minimal requirement, only check the former, which is optional.
      */
-    if ( !(_vmx_secondary_exec_control & SECONDARY_EXEC_VIRTUAL_INTR_DELIVERY) )
+    if ( !(caps.secondary_exec_control & SECONDARY_EXEC_VIRTUAL_INTR_DELIVERY) )
         caps.pin_based_exec_control &= ~PIN_BASED_POSTED_INTERRUPT;
 
     if ( iommu_intpost &&
@@ -432,7 +430,7 @@ static int vmx_init_vmcs_config(bool bsp
     }
 
     /* The IA32_VMX_VMFUNC MSR exists only when VMFUNC is available */
-    if ( _vmx_secondary_exec_control & SECONDARY_EXEC_ENABLE_VM_FUNCTIONS )
+    if ( caps.secondary_exec_control & SECONDARY_EXEC_ENABLE_VM_FUNCTIONS )
     {
         rdmsrl(MSR_IA32_VMX_VMFUNC, _vmx_vmfunc);
 
@@ -442,12 +440,12 @@ static int vmx_init_vmcs_config(bool bsp
          * Or we just don't use VMFUNC.
          */
         if ( !(_vmx_vmfunc & VMX_VMFUNC_EPTP_SWITCHING) )
-            _vmx_secondary_exec_control &= ~SECONDARY_EXEC_ENABLE_VM_FUNCTIONS;
+            caps.secondary_exec_control &= ~SECONDARY_EXEC_ENABLE_VM_FUNCTIONS;
     }
 
     /* Virtualization exceptions are only enabled if VMFUNC is enabled */
-    if ( !(_vmx_secondary_exec_control & SECONDARY_EXEC_ENABLE_VM_FUNCTIONS) )
-        _vmx_secondary_exec_control &= ~SECONDARY_EXEC_ENABLE_VIRT_EXCEPTIONS;
+    if ( !(caps.secondary_exec_control & SECONDARY_EXEC_ENABLE_VM_FUNCTIONS) )
+        caps.secondary_exec_control &= ~SECONDARY_EXEC_ENABLE_VIRT_EXCEPTIONS;
 
     min = 0;
     opt = (VM_ENTRY_LOAD_GUEST_PAT | VM_ENTRY_LOAD_GUEST_EFER |
@@ -462,7 +460,6 @@ static int vmx_init_vmcs_config(bool bsp
     {
         /* First time through. */
         vmx_caps = caps;
-        vmx_secondary_exec_control = _vmx_secondary_exec_control;
         vmx_ept_vpid_cap           = _vmx_ept_vpid_cap;
         vmx_vmexit_control         = _vmx_vmexit_control;
         vmx_vmentry_control        = _vmx_vmentry_control;
@@ -496,7 +493,7 @@ static int vmx_init_vmcs_config(bool bsp
             vmx_caps.cpu_based_exec_control, caps.cpu_based_exec_control);
         mismatch |= cap_check(
             "Secondary Exec Control",
-            vmx_secondary_exec_control, _vmx_secondary_exec_control);
+            vmx_caps.secondary_exec_control, caps.secondary_exec_control);
         mismatch |= cap_check(
             "VMExit Control",
             vmx_vmexit_control, _vmx_vmexit_control);
@@ -1074,7 +1071,7 @@ static int construct_vmcs(struct vcpu *v
     if ( d->arch.vtsc && !cpu_has_vmx_tsc_scaling )
         v->arch.hvm.vmx.exec_control |= CPU_BASED_RDTSC_EXITING;
 
-    v->arch.hvm.vmx.secondary_exec_control = vmx_secondary_exec_control;
+    v->arch.hvm.vmx.secondary_exec_control = vmx_caps.secondary_exec_control;
 
     /*
      * Disable features which we don't want active by default:
@@ -2166,7 +2163,6 @@ int __init vmx_vmcs_init(void)
          * Make sure all dependent features are off as well.
          */
         memset(&vmx_caps, 0, sizeof(vmx_caps));
-        vmx_secondary_exec_control = 0;
         vmx_vmexit_control         = 0;
         vmx_vmentry_control        = 0;
         vmx_ept_vpid_cap           = 0;
--- a/xen/arch/x86/include/asm/hvm/vmx/vmcs.h
+++ b/xen/arch/x86/include/asm/hvm/vmx/vmcs.h
@@ -256,7 +256,6 @@ extern u32 vmx_vmentry_control;
 #define SECONDARY_EXEC_TSC_SCALING              0x02000000U
 #define SECONDARY_EXEC_BUS_LOCK_DETECTION       0x40000000U
 #define SECONDARY_EXEC_NOTIFY_VM_EXITING        0x80000000U
-extern u32 vmx_secondary_exec_control;
 
 #define VMX_EPT_EXEC_ONLY_SUPPORTED                         0x00000001
 #define VMX_EPT_WALK_LENGTH_4_SUPPORTED                     0x00000040
@@ -286,13 +285,14 @@ struct vmx_caps {
     uint64_t basic_msr;
     uint32_t pin_based_exec_control;
     uint32_t cpu_based_exec_control;
+    uint32_t secondary_exec_control;
 };
 extern struct vmx_caps vmx_caps;
 
 #define cpu_has_wbinvd_exiting \
-    (vmx_secondary_exec_control & SECONDARY_EXEC_WBINVD_EXITING)
+    (vmx_caps.secondary_exec_control & SECONDARY_EXEC_WBINVD_EXITING)
 #define cpu_has_vmx_virtualize_apic_accesses \
-    (vmx_secondary_exec_control & SECONDARY_EXEC_VIRTUALIZE_APIC_ACCESSES)
+    (vmx_caps.secondary_exec_control & SECONDARY_EXEC_VIRTUALIZE_APIC_ACCESSES)
 #define cpu_has_vmx_tpr_shadow \
     (vmx_caps.cpu_based_exec_control & CPU_BASED_TPR_SHADOW)
 #define cpu_has_vmx_vnmi \
@@ -302,13 +302,13 @@ extern struct vmx_caps vmx_caps;
 #define cpu_has_vmx_secondary_exec_control \
     (vmx_caps.cpu_based_exec_control & CPU_BASED_ACTIVATE_SECONDARY_CONTROLS)
 #define cpu_has_vmx_ept \
-    (vmx_secondary_exec_control & SECONDARY_EXEC_ENABLE_EPT)
+    (vmx_caps.secondary_exec_control & SECONDARY_EXEC_ENABLE_EPT)
 #define cpu_has_vmx_dt_exiting \
-    (vmx_secondary_exec_control & SECONDARY_EXEC_DESCRIPTOR_TABLE_EXITING)
+    (vmx_caps.secondary_exec_control & SECONDARY_EXEC_DESCRIPTOR_TABLE_EXITING)
 #define cpu_has_vmx_rdtscp \
-    (vmx_secondary_exec_control & SECONDARY_EXEC_ENABLE_RDTSCP)
+    (vmx_caps.secondary_exec_control & SECONDARY_EXEC_ENABLE_RDTSCP)
 #define cpu_has_vmx_vpid \
-    (vmx_secondary_exec_control & SECONDARY_EXEC_ENABLE_VPID)
+    (vmx_caps.secondary_exec_control & SECONDARY_EXEC_ENABLE_VPID)
 #define cpu_has_monitor_trap_flag \
     (vmx_caps.cpu_based_exec_control & CPU_BASED_MONITOR_TRAP_FLAG)
 #define cpu_has_vmx_pat \
@@ -316,41 +316,41 @@ extern struct vmx_caps vmx_caps;
 #define cpu_has_vmx_efer \
     (vmx_vmentry_control & VM_ENTRY_LOAD_GUEST_EFER)
 #define cpu_has_vmx_unrestricted_guest \
-    (vmx_secondary_exec_control & SECONDARY_EXEC_UNRESTRICTED_GUEST)
+    (vmx_caps.secondary_exec_control & SECONDARY_EXEC_UNRESTRICTED_GUEST)
 #define vmx_unrestricted_guest(v)               \
     ((v)->arch.hvm.vmx.secondary_exec_control & \
      SECONDARY_EXEC_UNRESTRICTED_GUEST)
 #define cpu_has_vmx_ple \
-    (vmx_secondary_exec_control & SECONDARY_EXEC_PAUSE_LOOP_EXITING)
+    (vmx_caps.secondary_exec_control & SECONDARY_EXEC_PAUSE_LOOP_EXITING)
 #define cpu_has_vmx_invpcid \
-    (vmx_secondary_exec_control & SECONDARY_EXEC_ENABLE_INVPCID)
+    (vmx_caps.secondary_exec_control & SECONDARY_EXEC_ENABLE_INVPCID)
 #define cpu_has_vmx_apic_reg_virt \
-    (vmx_secondary_exec_control & SECONDARY_EXEC_APIC_REGISTER_VIRT)
+    (vmx_caps.secondary_exec_control & SECONDARY_EXEC_APIC_REGISTER_VIRT)
 #define cpu_has_vmx_virtual_intr_delivery \
-    (vmx_secondary_exec_control & SECONDARY_EXEC_VIRTUAL_INTR_DELIVERY)
+    (vmx_caps.secondary_exec_control & SECONDARY_EXEC_VIRTUAL_INTR_DELIVERY)
 #define cpu_has_vmx_virtualize_x2apic_mode \
-    (vmx_secondary_exec_control & SECONDARY_EXEC_VIRTUALIZE_X2APIC_MODE)
+    (vmx_caps.secondary_exec_control & SECONDARY_EXEC_VIRTUALIZE_X2APIC_MODE)
 #define cpu_has_vmx_posted_intr_processing \
     (vmx_caps.pin_based_exec_control & PIN_BASED_POSTED_INTERRUPT)
 #define cpu_has_vmx_vmcs_shadowing \
-    (vmx_secondary_exec_control & SECONDARY_EXEC_ENABLE_VMCS_SHADOWING)
+    (vmx_caps.secondary_exec_control & SECONDARY_EXEC_ENABLE_VMCS_SHADOWING)
 #define cpu_has_vmx_vmfunc \
-    (vmx_secondary_exec_control & SECONDARY_EXEC_ENABLE_VM_FUNCTIONS)
+    (vmx_caps.secondary_exec_control & SECONDARY_EXEC_ENABLE_VM_FUNCTIONS)
 #define cpu_has_vmx_virt_exceptions \
-    (vmx_secondary_exec_control & SECONDARY_EXEC_ENABLE_VIRT_EXCEPTIONS)
+    (vmx_caps.secondary_exec_control & SECONDARY_EXEC_ENABLE_VIRT_EXCEPTIONS)
 #define cpu_has_vmx_pml \
-    (vmx_secondary_exec_control & SECONDARY_EXEC_ENABLE_PML)
+    (vmx_caps.secondary_exec_control & SECONDARY_EXEC_ENABLE_PML)
 #define cpu_has_vmx_mpx \
     ((vmx_vmexit_control & VM_EXIT_CLEAR_BNDCFGS) && \
      (vmx_vmentry_control & VM_ENTRY_LOAD_BNDCFGS))
 #define cpu_has_vmx_xsaves \
-    (vmx_secondary_exec_control & SECONDARY_EXEC_XSAVES)
+    (vmx_caps.secondary_exec_control & SECONDARY_EXEC_XSAVES)
 #define cpu_has_vmx_tsc_scaling \
-    (vmx_secondary_exec_control & SECONDARY_EXEC_TSC_SCALING)
+    (vmx_caps.secondary_exec_control & SECONDARY_EXEC_TSC_SCALING)
 #define cpu_has_vmx_bus_lock_detection \
-    (vmx_secondary_exec_control & SECONDARY_EXEC_BUS_LOCK_DETECTION)
+    (vmx_caps.secondary_exec_control & SECONDARY_EXEC_BUS_LOCK_DETECTION)
 #define cpu_has_vmx_notify_vm_exiting \
-    (vmx_secondary_exec_control & SECONDARY_EXEC_NOTIFY_VM_EXITING)
+    (vmx_caps.secondary_exec_control & SECONDARY_EXEC_NOTIFY_VM_EXITING)
 
 #define VMCS_RID_TYPE_MASK              0x80000000U
 

[PATCH v2 12/15] VMX: convert vmx_vmexit_control

[Jan Beulich]

... to a field in the capability/controls struct.

Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
v2: New.

--- a/xen/arch/x86/hvm/vmx/vmcs.c
+++ b/xen/arch/x86/hvm/vmx/vmcs.c
@@ -162,7 +162,6 @@ static int cf_check parse_ept_param_runt
 
 /* Dynamic (run-time adjusted) execution control flags. */
 struct vmx_caps __ro_after_init vmx_caps;
-u32 vmx_vmexit_control __read_mostly;
 u32 vmx_vmentry_control __read_mostly;
 u64 vmx_ept_vpid_cap __read_mostly;
 static uint64_t __read_mostly vmx_vmfunc;
@@ -238,7 +237,6 @@ static int vmx_init_vmcs_config(bool bsp
     struct vmx_caps caps;
     u64 _vmx_ept_vpid_cap = 0;
     u64 _vmx_misc_cap = 0;
-    u32 _vmx_vmexit_control;
     u32 _vmx_vmentry_control;
     u64 _vmx_vmfunc = 0;
     bool mismatch = false;
@@ -410,7 +408,7 @@ static int vmx_init_vmcs_config(bool bsp
     opt = (VM_EXIT_SAVE_GUEST_PAT | VM_EXIT_LOAD_HOST_PAT |
            VM_EXIT_LOAD_HOST_EFER | VM_EXIT_CLEAR_BNDCFGS);
     min |= VM_EXIT_IA32E_MODE;
-    _vmx_vmexit_control = adjust_vmx_controls(
+    caps.vmexit_control = adjust_vmx_controls(
         "VMExit Control", min, opt, MSR_IA32_VMX_EXIT_CTLS, &mismatch);
 
     /*
@@ -461,7 +459,6 @@ static int vmx_init_vmcs_config(bool bsp
         /* First time through. */
         vmx_caps = caps;
         vmx_ept_vpid_cap           = _vmx_ept_vpid_cap;
-        vmx_vmexit_control         = _vmx_vmexit_control;
         vmx_vmentry_control        = _vmx_vmentry_control;
         vmx_caps.basic_msr = ((uint64_t)vmx_basic_msr_high << 32) |
                              vmx_basic_msr_low;
@@ -496,7 +493,7 @@ static int vmx_init_vmcs_config(bool bsp
             vmx_caps.secondary_exec_control, caps.secondary_exec_control);
         mismatch |= cap_check(
             "VMExit Control",
-            vmx_vmexit_control, _vmx_vmexit_control);
+            vmx_caps.vmexit_control, caps.vmexit_control);
         mismatch |= cap_check(
             "VMEntry Control",
             vmx_vmentry_control, _vmx_vmentry_control);
@@ -1058,7 +1055,7 @@ void nocall vmx_asm_vmexit_handler(void)
 static int construct_vmcs(struct vcpu *v)
 {
     struct domain *d = v->domain;
-    u32 vmexit_ctl = vmx_vmexit_control;
+    uint32_t vmexit_ctl = vmx_caps.vmexit_control;
     u32 vmentry_ctl = vmx_vmentry_control;
     int rc = 0;
 
@@ -2163,7 +2160,6 @@ int __init vmx_vmcs_init(void)
          * Make sure all dependent features are off as well.
          */
         memset(&vmx_caps, 0, sizeof(vmx_caps));
-        vmx_vmexit_control         = 0;
         vmx_vmentry_control        = 0;
         vmx_ept_vpid_cap           = 0;
         vmx_vmfunc                 = 0;
--- a/xen/arch/x86/hvm/vmx/vvmx.c
+++ b/xen/arch/x86/hvm/vmx/vvmx.c
@@ -1057,7 +1057,7 @@ static void load_shadow_control(struct v
     nvmx_update_pin_control(v, vmx_caps.pin_based_exec_control);
     vmx_update_cpu_exec_control(v);
     vmx_update_secondary_exec_control(v);
-    nvmx_update_exit_control(v, vmx_vmexit_control);
+    nvmx_update_exit_control(v, vmx_caps.vmexit_control);
     nvmx_update_entry_control(v);
     vmx_update_exception_bitmap(v);
     nvmx_update_apic_access_address(v);
--- a/xen/arch/x86/include/asm/hvm/vmx/vmcs.h
+++ b/xen/arch/x86/include/asm/hvm/vmx/vmcs.h
@@ -225,7 +225,6 @@ void vmx_vmcs_reload(struct vcpu *v);
 #define VM_EXIT_LOAD_HOST_EFER          0x00200000
 #define VM_EXIT_SAVE_PREEMPT_TIMER      0x00400000
 #define VM_EXIT_CLEAR_BNDCFGS           0x00800000
-extern u32 vmx_vmexit_control;
 
 #define VM_ENTRY_IA32E_MODE             0x00000200
 #define VM_ENTRY_SMM                    0x00000400
@@ -286,6 +285,7 @@ struct vmx_caps {
     uint32_t pin_based_exec_control;
     uint32_t cpu_based_exec_control;
     uint32_t secondary_exec_control;
+    uint32_t vmexit_control;
 };
 extern struct vmx_caps vmx_caps;
 
@@ -341,7 +341,7 @@ extern struct vmx_caps vmx_caps;
 #define cpu_has_vmx_pml \
     (vmx_caps.secondary_exec_control & SECONDARY_EXEC_ENABLE_PML)
 #define cpu_has_vmx_mpx \
-    ((vmx_vmexit_control & VM_EXIT_CLEAR_BNDCFGS) && \
+    ((vmx_caps.vmexit_control & VM_EXIT_CLEAR_BNDCFGS) && \
      (vmx_vmentry_control & VM_ENTRY_LOAD_BNDCFGS))
 #define cpu_has_vmx_xsaves \
     (vmx_caps.secondary_exec_control & SECONDARY_EXEC_XSAVES)

[PATCH v2 13/15] VMX: convert vmx_vmentry_control

[Jan Beulich]

... to a field in the capability/controls struct.

Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
v2: New.

--- a/xen/arch/x86/hvm/vmx/vmcs.c
+++ b/xen/arch/x86/hvm/vmx/vmcs.c
@@ -162,7 +162,6 @@ static int cf_check parse_ept_param_runt
 
 /* Dynamic (run-time adjusted) execution control flags. */
 struct vmx_caps __ro_after_init vmx_caps;
-u32 vmx_vmentry_control __read_mostly;
 u64 vmx_ept_vpid_cap __read_mostly;
 static uint64_t __read_mostly vmx_vmfunc;
 
@@ -237,7 +236,6 @@ static int vmx_init_vmcs_config(bool bsp
     struct vmx_caps caps;
     u64 _vmx_ept_vpid_cap = 0;
     u64 _vmx_misc_cap = 0;
-    u32 _vmx_vmentry_control;
     u64 _vmx_vmfunc = 0;
     bool mismatch = false;
 
@@ -448,7 +446,7 @@ static int vmx_init_vmcs_config(bool bsp
     min = 0;
     opt = (VM_ENTRY_LOAD_GUEST_PAT | VM_ENTRY_LOAD_GUEST_EFER |
            VM_ENTRY_LOAD_BNDCFGS);
-    _vmx_vmentry_control = adjust_vmx_controls(
+    caps.vmentry_control = adjust_vmx_controls(
         "VMEntry Control", min, opt, MSR_IA32_VMX_ENTRY_CTLS, &mismatch);
 
     if ( mismatch )
@@ -459,7 +457,6 @@ static int vmx_init_vmcs_config(bool bsp
         /* First time through. */
         vmx_caps = caps;
         vmx_ept_vpid_cap           = _vmx_ept_vpid_cap;
-        vmx_vmentry_control        = _vmx_vmentry_control;
         vmx_caps.basic_msr = ((uint64_t)vmx_basic_msr_high << 32) |
                              vmx_basic_msr_low;
         vmx_vmfunc                 = _vmx_vmfunc;
@@ -496,7 +493,7 @@ static int vmx_init_vmcs_config(bool bsp
             vmx_caps.vmexit_control, caps.vmexit_control);
         mismatch |= cap_check(
             "VMEntry Control",
-            vmx_vmentry_control, _vmx_vmentry_control);
+            vmx_caps.vmentry_control, caps.vmentry_control);
         mismatch |= cap_check(
             "EPT and VPID Capability",
             vmx_ept_vpid_cap, _vmx_ept_vpid_cap);
@@ -1056,7 +1053,7 @@ static int construct_vmcs(struct vcpu *v
 {
     struct domain *d = v->domain;
     uint32_t vmexit_ctl = vmx_caps.vmexit_control;
-    u32 vmentry_ctl = vmx_vmentry_control;
+    u32 vmentry_ctl = vmx_caps.vmentry_control;
     int rc = 0;
 
     vmx_vmcs_enter(v);
@@ -2160,7 +2157,6 @@ int __init vmx_vmcs_init(void)
          * Make sure all dependent features are off as well.
          */
         memset(&vmx_caps, 0, sizeof(vmx_caps));
-        vmx_vmentry_control        = 0;
         vmx_ept_vpid_cap           = 0;
         vmx_vmfunc                 = 0;
     }
--- a/xen/arch/x86/include/asm/hvm/vmx/vmcs.h
+++ b/xen/arch/x86/include/asm/hvm/vmx/vmcs.h
@@ -233,7 +233,6 @@ void vmx_vmcs_reload(struct vcpu *v);
 #define VM_ENTRY_LOAD_GUEST_PAT         0x00004000
 #define VM_ENTRY_LOAD_GUEST_EFER        0x00008000
 #define VM_ENTRY_LOAD_BNDCFGS           0x00010000
-extern u32 vmx_vmentry_control;
 
 #define SECONDARY_EXEC_VIRTUALIZE_APIC_ACCESSES 0x00000001U
 #define SECONDARY_EXEC_ENABLE_EPT               0x00000002U
@@ -286,6 +285,7 @@ struct vmx_caps {
     uint32_t cpu_based_exec_control;
     uint32_t secondary_exec_control;
     uint32_t vmexit_control;
+    uint32_t vmentry_control;
 };
 extern struct vmx_caps vmx_caps;
 
@@ -312,9 +312,9 @@ extern struct vmx_caps vmx_caps;
 #define cpu_has_monitor_trap_flag \
     (vmx_caps.cpu_based_exec_control & CPU_BASED_MONITOR_TRAP_FLAG)
 #define cpu_has_vmx_pat \
-    (vmx_vmentry_control & VM_ENTRY_LOAD_GUEST_PAT)
+    (vmx_caps.vmentry_control & VM_ENTRY_LOAD_GUEST_PAT)
 #define cpu_has_vmx_efer \
-    (vmx_vmentry_control & VM_ENTRY_LOAD_GUEST_EFER)
+    (vmx_caps.vmentry_control & VM_ENTRY_LOAD_GUEST_EFER)
 #define cpu_has_vmx_unrestricted_guest \
     (vmx_caps.secondary_exec_control & SECONDARY_EXEC_UNRESTRICTED_GUEST)
 #define vmx_unrestricted_guest(v)               \
@@ -342,7 +342,7 @@ extern struct vmx_caps vmx_caps;
     (vmx_caps.secondary_exec_control & SECONDARY_EXEC_ENABLE_PML)
 #define cpu_has_vmx_mpx \
     ((vmx_caps.vmexit_control & VM_EXIT_CLEAR_BNDCFGS) && \
-     (vmx_vmentry_control & VM_ENTRY_LOAD_BNDCFGS))
+     (vmx_caps.vmentry_control & VM_ENTRY_LOAD_BNDCFGS))
 #define cpu_has_vmx_xsaves \
     (vmx_caps.secondary_exec_control & SECONDARY_EXEC_XSAVES)
 #define cpu_has_vmx_tsc_scaling \

[PATCH v2 14/15] VMX: convert vmx_ept_vpid_cap

[Jan Beulich]

... to fields in the capability/controls struct: Take the opportunity
and split the two halves into separate EPT and VPID fields.

Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
v2: New.

--- a/xen/arch/x86/hvm/vmx/vmcs.c
+++ b/xen/arch/x86/hvm/vmx/vmcs.c
@@ -162,7 +162,6 @@ static int cf_check parse_ept_param_runt
 
 /* Dynamic (run-time adjusted) execution control flags. */
 struct vmx_caps __ro_after_init vmx_caps;
-u64 vmx_ept_vpid_cap __read_mostly;
 static uint64_t __read_mostly vmx_vmfunc;
 
 static DEFINE_PER_CPU_READ_MOSTLY(paddr_t, vmxon_region);
@@ -234,7 +233,6 @@ static int vmx_init_vmcs_config(bool bsp
 {
     u32 vmx_basic_msr_low, vmx_basic_msr_high, min, opt;
     struct vmx_caps caps;
-    u64 _vmx_ept_vpid_cap = 0;
     u64 _vmx_misc_cap = 0;
     u64 _vmx_vmfunc = 0;
     bool mismatch = false;
@@ -333,10 +331,10 @@ static int vmx_init_vmcs_config(bool bsp
     if ( caps.secondary_exec_control & (SECONDARY_EXEC_ENABLE_EPT |
                                         SECONDARY_EXEC_ENABLE_VPID) )
     {
-        rdmsrl(MSR_IA32_VMX_EPT_VPID_CAP, _vmx_ept_vpid_cap);
+        rdmsr(MSR_IA32_VMX_EPT_VPID_CAP, caps.ept, caps.vpid);
 
         if ( !opt_ept_ad )
-            _vmx_ept_vpid_cap &= ~VMX_EPT_AD_BIT;
+            caps.ept &= ~VMX_EPT_AD_BIT;
 
         /*
          * Additional sanity checking before using EPT:
@@ -349,9 +347,9 @@ static int vmx_init_vmcs_config(bool bsp
          *
          * Or we just don't use EPT.
          */
-        if ( !(_vmx_ept_vpid_cap & VMX_EPT_MEMORY_TYPE_WB) ||
-             !(_vmx_ept_vpid_cap & VMX_EPT_WALK_LENGTH_4_SUPPORTED) ||
-             !(_vmx_ept_vpid_cap & VMX_EPT_INVEPT_ALL_CONTEXT) )
+        if ( !(caps.ept & VMX_EPT_MEMORY_TYPE_WB) ||
+             !(caps.ept & VMX_EPT_WALK_LENGTH_4_SUPPORTED) ||
+             !(caps.ept & VMX_EPT_INVEPT_ALL_CONTEXT) )
             caps.secondary_exec_control &= ~SECONDARY_EXEC_ENABLE_EPT;
 
         /*
@@ -360,11 +358,11 @@ static int vmx_init_vmcs_config(bool bsp
          *
          * Or we just don't use VPID.
          */
-        if ( !(_vmx_ept_vpid_cap & VMX_VPID_INVVPID_ALL_CONTEXT) )
+        if ( !(caps.vpid & VMX_VPID_INVVPID_ALL_CONTEXT) )
             caps.secondary_exec_control &= ~SECONDARY_EXEC_ENABLE_VPID;
 
         /* EPT A/D bits is required for PML */
-        if ( !(_vmx_ept_vpid_cap & VMX_EPT_AD_BIT) )
+        if ( !(caps.ept & VMX_EPT_AD_BIT) )
             caps.secondary_exec_control &= ~SECONDARY_EXEC_ENABLE_PML;
     }
 
@@ -456,7 +454,6 @@ static int vmx_init_vmcs_config(bool bsp
     {
         /* First time through. */
         vmx_caps = caps;
-        vmx_ept_vpid_cap           = _vmx_ept_vpid_cap;
         vmx_caps.basic_msr = ((uint64_t)vmx_basic_msr_high << 32) |
                              vmx_basic_msr_low;
         vmx_vmfunc                 = _vmx_vmfunc;
@@ -494,9 +491,8 @@ static int vmx_init_vmcs_config(bool bsp
         mismatch |= cap_check(
             "VMEntry Control",
             vmx_caps.vmentry_control, caps.vmentry_control);
-        mismatch |= cap_check(
-            "EPT and VPID Capability",
-            vmx_ept_vpid_cap, _vmx_ept_vpid_cap);
+        mismatch |= cap_check("EPT Capability", vmx_caps.ept, caps.ept);
+        mismatch |= cap_check("VPID Capability", vmx_caps.vpid, caps.vpid);
         mismatch |= cap_check(
             "VMFUNC Capability",
             vmx_vmfunc, _vmx_vmfunc);
@@ -2157,7 +2153,6 @@ int __init vmx_vmcs_init(void)
          * Make sure all dependent features are off as well.
          */
         memset(&vmx_caps, 0, sizeof(vmx_caps));
-        vmx_ept_vpid_cap           = 0;
         vmx_vmfunc                 = 0;
     }
 
--- a/xen/arch/x86/include/asm/hvm/vmx/vmcs.h
+++ b/xen/arch/x86/include/asm/hvm/vmx/vmcs.h
@@ -265,12 +265,11 @@ void vmx_vmcs_reload(struct vcpu *v);
 #define VMX_EPT_AD_BIT                                      0x00200000
 #define VMX_EPT_INVEPT_SINGLE_CONTEXT                       0x02000000
 #define VMX_EPT_INVEPT_ALL_CONTEXT                          0x04000000
-#define VMX_VPID_INVVPID_INSTRUCTION                     0x00100000000ULL
-#define VMX_VPID_INVVPID_INDIVIDUAL_ADDR                 0x10000000000ULL
-#define VMX_VPID_INVVPID_SINGLE_CONTEXT                  0x20000000000ULL
-#define VMX_VPID_INVVPID_ALL_CONTEXT                     0x40000000000ULL
-#define VMX_VPID_INVVPID_SINGLE_CONTEXT_RETAINING_GLOBAL 0x80000000000ULL
-extern u64 vmx_ept_vpid_cap;
+#define VMX_VPID_INVVPID_INSTRUCTION                        0x00000001
+#define VMX_VPID_INVVPID_INDIVIDUAL_ADDR                    0x00000100
+#define VMX_VPID_INVVPID_SINGLE_CONTEXT                     0x00000200
+#define VMX_VPID_INVVPID_ALL_CONTEXT                        0x00000400
+#define VMX_VPID_INVVPID_SINGLE_CONTEXT_RETAINING_GLOBAL    0x00000800
 
 #define VMX_MISC_PROC_TRACE                     0x00004000
 #define VMX_MISC_CR3_TARGET                     0x01ff0000
@@ -286,6 +285,8 @@ struct vmx_caps {
     uint32_t secondary_exec_control;
     uint32_t vmexit_control;
     uint32_t vmentry_control;
+    uint32_t ept;
+    uint32_t vpid;
 };
 extern struct vmx_caps vmx_caps;
 
--- a/xen/arch/x86/include/asm/hvm/vmx/vmx.h
+++ b/xen/arch/x86/include/asm/hvm/vmx/vmx.h
@@ -278,17 +278,17 @@ typedef union cr_access_qual {
 extern uint8_t posted_intr_vector;
 
 #define cpu_has_vmx_ept_exec_only_supported        \
-    (vmx_ept_vpid_cap & VMX_EPT_EXEC_ONLY_SUPPORTED)
+    (vmx_caps.ept & VMX_EPT_EXEC_ONLY_SUPPORTED)
 
 #define cpu_has_vmx_ept_wl4_supported           \
-    (vmx_ept_vpid_cap & VMX_EPT_WALK_LENGTH_4_SUPPORTED)
-#define cpu_has_vmx_ept_mt_uc (vmx_ept_vpid_cap & VMX_EPT_MEMORY_TYPE_UC)
-#define cpu_has_vmx_ept_mt_wb (vmx_ept_vpid_cap & VMX_EPT_MEMORY_TYPE_WB)
-#define cpu_has_vmx_ept_2mb   (vmx_ept_vpid_cap & VMX_EPT_SUPERPAGE_2MB)
-#define cpu_has_vmx_ept_1gb   (vmx_ept_vpid_cap & VMX_EPT_SUPERPAGE_1GB)
-#define cpu_has_vmx_ept_ad    (vmx_ept_vpid_cap & VMX_EPT_AD_BIT)
+    (vmx_caps.ept & VMX_EPT_WALK_LENGTH_4_SUPPORTED)
+#define cpu_has_vmx_ept_mt_uc (vmx_caps.ept & VMX_EPT_MEMORY_TYPE_UC)
+#define cpu_has_vmx_ept_mt_wb (vmx_caps.ept & VMX_EPT_MEMORY_TYPE_WB)
+#define cpu_has_vmx_ept_2mb   (vmx_caps.ept & VMX_EPT_SUPERPAGE_2MB)
+#define cpu_has_vmx_ept_1gb   (vmx_caps.ept & VMX_EPT_SUPERPAGE_1GB)
+#define cpu_has_vmx_ept_ad    (vmx_caps.ept & VMX_EPT_AD_BIT)
 #define cpu_has_vmx_ept_invept_single_context   \
-    (vmx_ept_vpid_cap & VMX_EPT_INVEPT_SINGLE_CONTEXT)
+    (vmx_caps.ept & VMX_EPT_INVEPT_SINGLE_CONTEXT)
 
 #define EPT_2MB_SHIFT     16
 #define EPT_1GB_SHIFT     17
@@ -299,11 +299,11 @@ extern uint8_t posted_intr_vector;
 #define INVEPT_ALL_CONTEXT      2
 
 #define cpu_has_vmx_vpid_invvpid_individual_addr                    \
-    (vmx_ept_vpid_cap & VMX_VPID_INVVPID_INDIVIDUAL_ADDR)
+    (vmx_caps.vpid & VMX_VPID_INVVPID_INDIVIDUAL_ADDR)
 #define cpu_has_vmx_vpid_invvpid_single_context                     \
-    (vmx_ept_vpid_cap & VMX_VPID_INVVPID_SINGLE_CONTEXT)
+    (vmx_caps.vpid & VMX_VPID_INVVPID_SINGLE_CONTEXT)
 #define cpu_has_vmx_vpid_invvpid_single_context_retaining_global    \
-    (vmx_ept_vpid_cap & VMX_VPID_INVVPID_SINGLE_CONTEXT_RETAINING_GLOBAL)
+    (vmx_caps.vpid & VMX_VPID_INVVPID_SINGLE_CONTEXT_RETAINING_GLOBAL)
 
 #define INVVPID_INDIVIDUAL_ADDR                 0
 #define INVVPID_SINGLE_CONTEXT                  1

[PATCH v2 14/15] VMX: convert vmx_vmfunc

[Jan Beulich]

... to a field in the capability/controls struct.

Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
v2: New.

--- a/xen/arch/x86/hvm/vmx/vmcs.c
+++ b/xen/arch/x86/hvm/vmx/vmcs.c
@@ -162,7 +162,6 @@ static int cf_check parse_ept_param_runt
 
 /* Dynamic (run-time adjusted) execution control flags. */
 struct vmx_caps __ro_after_init vmx_caps;
-static uint64_t __read_mostly vmx_vmfunc;
 
 static DEFINE_PER_CPU_READ_MOSTLY(paddr_t, vmxon_region);
 static DEFINE_PER_CPU(paddr_t, current_vmcs);
@@ -234,7 +233,6 @@ static int vmx_init_vmcs_config(bool bsp
     u32 vmx_basic_msr_low, vmx_basic_msr_high, min, opt;
     struct vmx_caps caps;
     u64 _vmx_misc_cap = 0;
-    u64 _vmx_vmfunc = 0;
     bool mismatch = false;
 
     rdmsr(MSR_IA32_VMX_BASIC, vmx_basic_msr_low, vmx_basic_msr_high);
@@ -426,14 +424,14 @@ static int vmx_init_vmcs_config(bool bsp
     /* The IA32_VMX_VMFUNC MSR exists only when VMFUNC is available */
     if ( caps.secondary_exec_control & SECONDARY_EXEC_ENABLE_VM_FUNCTIONS )
     {
-        rdmsrl(MSR_IA32_VMX_VMFUNC, _vmx_vmfunc);
+        rdmsrl(MSR_IA32_VMX_VMFUNC, caps.vmfunc);
 
         /*
          * VMFUNC leaf 0 (EPTP switching) must be supported.
          *
          * Or we just don't use VMFUNC.
          */
-        if ( !(_vmx_vmfunc & VMX_VMFUNC_EPTP_SWITCHING) )
+        if ( !(caps.vmfunc & VMX_VMFUNC_EPTP_SWITCHING) )
             caps.secondary_exec_control &= ~SECONDARY_EXEC_ENABLE_VM_FUNCTIONS;
     }
 
@@ -456,7 +454,6 @@ static int vmx_init_vmcs_config(bool bsp
         vmx_caps = caps;
         vmx_caps.basic_msr = ((uint64_t)vmx_basic_msr_high << 32) |
                              vmx_basic_msr_low;
-        vmx_vmfunc                 = _vmx_vmfunc;
 
         vmx_display_features();
 
@@ -495,7 +492,7 @@ static int vmx_init_vmcs_config(bool bsp
         mismatch |= cap_check("VPID Capability", vmx_caps.vpid, caps.vpid);
         mismatch |= cap_check(
             "VMFUNC Capability",
-            vmx_vmfunc, _vmx_vmfunc);
+            vmx_caps.vmfunc, caps.vmfunc);
         if ( cpu_has_vmx_ins_outs_instr_info !=
              !!(vmx_basic_msr_high & (VMX_BASIC_INS_OUT_INFO >> 32)) )
         {
@@ -2153,7 +2150,6 @@ int __init vmx_vmcs_init(void)
          * Make sure all dependent features are off as well.
          */
         memset(&vmx_caps, 0, sizeof(vmx_caps));
-        vmx_vmfunc                 = 0;
     }
 
     return ret;
--- a/xen/arch/x86/include/asm/hvm/vmx/vmcs.h
+++ b/xen/arch/x86/include/asm/hvm/vmx/vmcs.h
@@ -287,6 +287,7 @@ struct vmx_caps {
     uint32_t vmentry_control;
     uint32_t ept;
     uint32_t vpid;
+    uint64_t vmfunc;
 };
 extern struct vmx_caps vmx_caps;
 

Re: [PATCH v2 15/15] VMX: convert vmx_vmfunc

[Jan Beulich]

On 24.11.2023 09:43, Jan Beulich wrote:
> ... to a field in the capability/controls struct.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> ---
> v2: New.

I'm sorry, this really is 15/15 ($subject also adjusted).

Jan

> --- a/xen/arch/x86/hvm/vmx/vmcs.c
> +++ b/xen/arch/x86/hvm/vmx/vmcs.c
> @@ -162,7 +162,6 @@ static int cf_check parse_ept_param_runt
>  
>  /* Dynamic (run-time adjusted) execution control flags. */
>  struct vmx_caps __ro_after_init vmx_caps;
> -static uint64_t __read_mostly vmx_vmfunc;
>  
>  static DEFINE_PER_CPU_READ_MOSTLY(paddr_t, vmxon_region);
>  static DEFINE_PER_CPU(paddr_t, current_vmcs);
> @@ -234,7 +233,6 @@ static int vmx_init_vmcs_config(bool bsp
>      u32 vmx_basic_msr_low, vmx_basic_msr_high, min, opt;
>      struct vmx_caps caps;
>      u64 _vmx_misc_cap = 0;
> -    u64 _vmx_vmfunc = 0;
>      bool mismatch = false;
>  
>      rdmsr(MSR_IA32_VMX_BASIC, vmx_basic_msr_low, vmx_basic_msr_high);
> @@ -426,14 +424,14 @@ static int vmx_init_vmcs_config(bool bsp
>      /* The IA32_VMX_VMFUNC MSR exists only when VMFUNC is available */
>      if ( caps.secondary_exec_control & SECONDARY_EXEC_ENABLE_VM_FUNCTIONS )
>      {
> -        rdmsrl(MSR_IA32_VMX_VMFUNC, _vmx_vmfunc);
> +        rdmsrl(MSR_IA32_VMX_VMFUNC, caps.vmfunc);
>  
>          /*
>           * VMFUNC leaf 0 (EPTP switching) must be supported.
>           *
>           * Or we just don't use VMFUNC.
>           */
> -        if ( !(_vmx_vmfunc & VMX_VMFUNC_EPTP_SWITCHING) )
> +        if ( !(caps.vmfunc & VMX_VMFUNC_EPTP_SWITCHING) )
>              caps.secondary_exec_control &= ~SECONDARY_EXEC_ENABLE_VM_FUNCTIONS;
>      }
>  
> @@ -456,7 +454,6 @@ static int vmx_init_vmcs_config(bool bsp
>          vmx_caps = caps;
>          vmx_caps.basic_msr = ((uint64_t)vmx_basic_msr_high << 32) |
>                               vmx_basic_msr_low;
> -        vmx_vmfunc                 = _vmx_vmfunc;
>  
>          vmx_display_features();
>  
> @@ -495,7 +492,7 @@ static int vmx_init_vmcs_config(bool bsp
>          mismatch |= cap_check("VPID Capability", vmx_caps.vpid, caps.vpid);
>          mismatch |= cap_check(
>              "VMFUNC Capability",
> -            vmx_vmfunc, _vmx_vmfunc);
> +            vmx_caps.vmfunc, caps.vmfunc);
>          if ( cpu_has_vmx_ins_outs_instr_info !=
>               !!(vmx_basic_msr_high & (VMX_BASIC_INS_OUT_INFO >> 32)) )
>          {
> @@ -2153,7 +2150,6 @@ int __init vmx_vmcs_init(void)
>           * Make sure all dependent features are off as well.
>           */
>          memset(&vmx_caps, 0, sizeof(vmx_caps));
> -        vmx_vmfunc                 = 0;
>      }
>  
>      return ret;
> --- a/xen/arch/x86/include/asm/hvm/vmx/vmcs.h
> +++ b/xen/arch/x86/include/asm/hvm/vmx/vmcs.h
> @@ -287,6 +287,7 @@ struct vmx_caps {
>      uint32_t vmentry_control;
>      uint32_t ept;
>      uint32_t vpid;
> +    uint64_t vmfunc;
>  };
>  extern struct vmx_caps vmx_caps;
>  
> 
> 

Re: [PATCH v2 05/15] x86: amend cpu_has_xen_[ibt,shstk}

[Andrew Cooper]

In the subject, [ -> {

?

On 24/11/2023 8:38 am, Jan Beulich wrote:
> ... to evaluate to false at compile-time when the respective Kconfig
> control is off, thus allowing the compiler to eliminate then-dead code.
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Acked-by: Andrew Cooper <andrew.cooper3@citrix.com>

I've got part of a series trying to do some cleanup to this effect, but
I'll rebase it around this.

Re: [PATCH v2 06/15] x86/HVM: improve CET-IBT pruning of ENDBR

[Andrew Cooper]

On 24/11/2023 8:39 am, Jan Beulich wrote:
> __init{const,data}_cf_clobber can have an effect only for pointers
> actually populated in the respective tables. While not the case for SVM
> right now, VMX installs a number of pointers only under certain
> conditions. Hence the respective functions would have their ENDBR purged
> only when those conditions are met. Invoke "pruning" functions after
> having copied the respective tables, for them to install any "missing"
> pointers.
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

In theory Acked-by: Andrew Cooper <andrew.cooper3@citrix.com>, but see
later.

I have to admit that I'd overlooked this point when putting together
__init{}_cf_clobber originally.  Then again, I did have more urgent
things on my mind at the time.

> ---
> This is largely cosmetic for present hardware, which when supporting
> CET-IBT likely also supports all of the advanced VMX features for which
> hook pointers are installed conditionally. The only case this would make
> a difference there is when use of respective features was suppressed via
> command line option (where available). For future hooks it may end up
> relevant even by default, and it also would be if AMD started supporting
> CET-IBT; right now it matters only for .pi_update_irte, as iommu_intpost
> continues to default to off.
>
> Originally I had meant to put the SVM and VMX functions in presmp-
> initcalls, but hvm/{svm,vmx}/built_in.o are linked into hvm/built_in.o
> before hvm/hvm.o. And I don't think I want to fiddle with link order
> here.

An alternative is the form I used for microcode, where start_{vmx,svm}()
fills in fns, and doesn't have to fill in all hooks.

That will be more amenable to Kconfig-ing generally, and will probably
be less fragile to getting forgotten.

> ---
> v2: Use cpu_has_xen_ibt in prune_{svm,vmx}().
>
> --- a/xen/arch/x86/hvm/hvm.c
> +++ b/xen/arch/x86/hvm/hvm.c
> @@ -161,10 +161,15 @@ static int __init cf_check hvm_enable(vo
>      else if ( cpu_has_svm )
>          fns = start_svm();
>  
> +    if ( fns )
> +        hvm_funcs = *fns;
> +
> +    prune_vmx();
> +    prune_svm();
> +
>      if ( fns == NULL )
>          return 0;
>  
> -    hvm_funcs = *fns;
>      hvm_enabled = 1;
>  
>      printk("HVM: %s enabled\n", fns->name);
> --- a/xen/arch/x86/hvm/svm/svm.c
> +++ b/xen/arch/x86/hvm/svm/svm.c
> @@ -2587,6 +2587,19 @@ const struct hvm_function_table * __init
>      return &svm_function_table;
>  }
>  
> +void __init prune_svm(void)
> +{
> +    /*
> +     * Now that svm_function_table was copied, populate all function pointers
> +     * which may have been left at NULL, for __initdata_cf_clobber to have as
> +     * much of an effect as possible.
> +     */
> +    if ( !cpu_has_xen_ibt )
> +        return;
> +
> +    /* Nothing at present. */
> +}
> +
>  void asmlinkage svm_vmexit_handler(void)
>  {
>      struct cpu_user_regs *regs = guest_cpu_user_regs();
> --- a/xen/arch/x86/hvm/vmx/vmx.c
> +++ b/xen/arch/x86/hvm/vmx/vmx.c
> @@ -3033,6 +3033,30 @@ const struct hvm_function_table * __init
>      return &vmx_function_table;
>  }
>  
> +void __init prune_vmx(void)
> +{
> +    /*
> +     * Now that vmx_function_table was copied, populate all function pointers
> +     * which may have been left at NULL, for __initdata_cf_clobber to have as
> +     * much of an effect as possible.
> +     */
> +    if ( !cpu_has_xen_ibt )
> +        return;
> +
> +    vmx_function_table.set_descriptor_access_exiting =
> +        vmx_set_descriptor_access_exiting;
> +
> +    vmx_function_table.update_eoi_exit_bitmap = vmx_update_eoi_exit_bitmap;
> +    vmx_function_table.process_isr            = vmx_process_isr;
> +    vmx_function_table.handle_eoi             = vmx_handle_eoi;
> +
> +    vmx_function_table.pi_update_irte = vmx_pi_update_irte;
> +
> +    vmx_function_table.deliver_posted_intr = vmx_deliver_posted_intr;
> +    vmx_function_table.sync_pir_to_irr     = vmx_sync_pir_to_irr;
> +    vmx_function_table.test_pir            = vmx_test_pir;

That said...

This (the hooks being conditional in the first place) is bogus to begin
with.  Posted interrupts (or not) are a per-VM property even if we don't
wire this up properly yet.  It will be forced to be done properly in
order to support nested virt, as L0 Xen *must* comply with the settings
chosen by the L1 hypervisor.

So the choice to use the hooks will have to come from per-vCPU state,
and not from the conditional-ness of them.

Any chance I can talk you into instead making the hooks unconditional? 
If not, someone (George was volunteering) is going to have to undo this
in fairly short order.

~Andrew

Re: [PATCH v2 08/15] VMX: convert vmx_basic_msr

[Andrew Cooper]

On 24/11/2023 8:41 am, Jan Beulich wrote:
> ... to a struct field, which is then going to be accompanied by other
> capability/control data presently living in individual variables. As
> this structure isn't supposed to be altered post-boot, put it in
> .data.ro_after_init right away.
>
> Suggested-by: Roger Pau Monné <roger.pau@citrix.com>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

For (usable) nested virt, we're going to need the VMX MSRs, in their
architectural form, in struct cpu_policy.  And just like CPUID features,
I want it to end up with nice bitfields to use.

Looking through the rest of this series, vmx_caps ends up almost in
architectural form.

Could I talk you into having a "struct vmx_msrs" (or similar - 'caps'
doesn't feel quite right here) in the policy object, and also
instantiating one instance of it for this purpose here?

AFAICT, it would only be a minor deviation to the latter half of this
series, but it would be an excellent start to fixing nested virt - and
getting this data in the policy really is the first task in getting the
ball rolling on nested virt.

I don't mind about serialising/de-serialsing it - that still has a bit
of userspace complexity to work out, and depends on some of the cleanup
still needing a repost.

If you don't want to take the added space in cpu_policy yet, how about
having the declaration there and just forgo instantiating the subobject
in the short term?

~Andrew

Re: [PATCH v2 05/15] x86: amend cpu_has_xen_[ibt,shstk}

[Jan Beulich]

On 24.11.2023 14:52, Andrew Cooper wrote:
> In the subject, [ -> {
> 
> ?

Right, noticed (sadly) just after sending, and already corrected locally.

> On 24/11/2023 8:38 am, Jan Beulich wrote:
>> ... to evaluate to false at compile-time when the respective Kconfig
>> control is off, thus allowing the compiler to eliminate then-dead code.
>>
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> 
> Acked-by: Andrew Cooper <andrew.cooper3@citrix.com>

Thanks.

Jan

Re: [PATCH v2 06/15] x86/HVM: improve CET-IBT pruning of ENDBR

[Jan Beulich]

On 24.11.2023 23:28, Andrew Cooper wrote:
> On 24/11/2023 8:39 am, Jan Beulich wrote:
>> __init{const,data}_cf_clobber can have an effect only for pointers
>> actually populated in the respective tables. While not the case for SVM
>> right now, VMX installs a number of pointers only under certain
>> conditions. Hence the respective functions would have their ENDBR purged
>> only when those conditions are met. Invoke "pruning" functions after
>> having copied the respective tables, for them to install any "missing"
>> pointers.
>>
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> 
> In theory Acked-by: Andrew Cooper <andrew.cooper3@citrix.com>, but see
> later.

Thanks. See below as well.

>> ---
>> This is largely cosmetic for present hardware, which when supporting
>> CET-IBT likely also supports all of the advanced VMX features for which
>> hook pointers are installed conditionally. The only case this would make
>> a difference there is when use of respective features was suppressed via
>> command line option (where available). For future hooks it may end up
>> relevant even by default, and it also would be if AMD started supporting
>> CET-IBT; right now it matters only for .pi_update_irte, as iommu_intpost
>> continues to default to off.
>>
>> Originally I had meant to put the SVM and VMX functions in presmp-
>> initcalls, but hvm/{svm,vmx}/built_in.o are linked into hvm/built_in.o
>> before hvm/hvm.o. And I don't think I want to fiddle with link order
>> here.
> 
> An alternative is the form I used for microcode, where start_{vmx,svm}()
> fills in fns, and doesn't have to fill in all hooks.
> 
> That will be more amenable to Kconfig-ing generally, and will probably
> be less fragile to getting forgotten.

You mean specifically "x86/ucode: Move vendor specifics back out of
early_microcode_init()", which looks to not have gone in yet? That's going
the opposite route (NULLing out hooks after copying), yet this looks to
still go against what you're asking for below. In how far this would reduce
the chance of being forgotten is not entirely clear to me right away, but I
think I see your point.

One (further) benefit of copying before editing would of course be that the
static struct instances then can be const.

>> --- a/xen/arch/x86/hvm/vmx/vmx.c
>> +++ b/xen/arch/x86/hvm/vmx/vmx.c
>> @@ -3033,6 +3033,30 @@ const struct hvm_function_table * __init
>>      return &vmx_function_table;
>>  }
>>  
>> +void __init prune_vmx(void)
>> +{
>> +    /*
>> +     * Now that vmx_function_table was copied, populate all function pointers
>> +     * which may have been left at NULL, for __initdata_cf_clobber to have as
>> +     * much of an effect as possible.
>> +     */
>> +    if ( !cpu_has_xen_ibt )
>> +        return;
>> +
>> +    vmx_function_table.set_descriptor_access_exiting =
>> +        vmx_set_descriptor_access_exiting;
>> +
>> +    vmx_function_table.update_eoi_exit_bitmap = vmx_update_eoi_exit_bitmap;
>> +    vmx_function_table.process_isr            = vmx_process_isr;
>> +    vmx_function_table.handle_eoi             = vmx_handle_eoi;
>> +
>> +    vmx_function_table.pi_update_irte = vmx_pi_update_irte;
>> +
>> +    vmx_function_table.deliver_posted_intr = vmx_deliver_posted_intr;
>> +    vmx_function_table.sync_pir_to_irr     = vmx_sync_pir_to_irr;
>> +    vmx_function_table.test_pir            = vmx_test_pir;
> 
> That said...
> 
> This (the hooks being conditional in the first place) is bogus to begin
> with.  Posted interrupts (or not) are a per-VM property even if we don't
> wire this up properly yet.  It will be forced to be done properly in
> order to support nested virt, as L0 Xen *must* comply with the settings
> chosen by the L1 hypervisor.
> 
> So the choice to use the hooks will have to come from per-vCPU state,
> and not from the conditional-ness of them.
> 
> Any chance I can talk you into instead making the hooks unconditional? 
> If not, someone (George was volunteering) is going to have to undo this
> in fairly short order.

As you can see from "x86/HVM: drop tsc_scaling.setup() hook" I actually
did consider removing the conditional parts, where sufficiently
straightforward. I'll take another close look for the other hooks, but
I'm not going to promise anything towards actually taking the alternative
route.

Jan

Re: [PATCH v2 08/15] VMX: convert vmx_basic_msr

[Jan Beulich]

On 24.11.2023 23:41, Andrew Cooper wrote:
> On 24/11/2023 8:41 am, Jan Beulich wrote:
>> ... to a struct field, which is then going to be accompanied by other
>> capability/control data presently living in individual variables. As
>> this structure isn't supposed to be altered post-boot, put it in
>> .data.ro_after_init right away.
>>
>> Suggested-by: Roger Pau Monné <roger.pau@citrix.com>
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> 
> For (usable) nested virt, we're going to need the VMX MSRs, in their
> architectural form, in struct cpu_policy.  And just like CPUID features,
> I want it to end up with nice bitfields to use.
> 
> Looking through the rest of this series, vmx_caps ends up almost in
> architectural form.
> 
> Could I talk you into having a "struct vmx_msrs" (or similar - 'caps'
> doesn't feel quite right here) in the policy object, and also
> instantiating one instance of it for this purpose here?

I was actually wondering while doing the conversion. The main reason I
didn't go that route right away was that I wasn't really certain whether
what I'd put there would the really be the (largely) final shape it
wants to take there. (One thing you've likely noticed I didn't convert
is _vmx_misc_cap, which right now only exists as a local variable in
vmx_init_vmcs_config().)

> AFAICT, it would only be a minor deviation to the latter half of this
> series, but it would be an excellent start to fixing nested virt - and
> getting this data in the policy really is the first task in getting the
> ball rolling on nested virt.

How much of a further change it would end up being (or where that change
would occur) depends on another aspect: When put in cpu-policy.h (and I
take it you mean the lib/ instance, not the asm/ one), it would seem
natural and perhaps even necessary to properly introduce bitfields for
each of the MSRs right away. That'll lead to a "raw" field as well. In
VMX code (mostly its cpu_has_* #define-s), I'd then either need to use
.raw (perhaps a little ugly here and there) or go with using the
individual bitfields right away (likely eliminating the need for many of
the constant #define-s), which increases the risk of inadvertent mistakes
(and their overlooking during review).

> I don't mind about serialising/de-serialsing it - that still has a bit
> of userspace complexity to work out, and depends on some of the cleanup
> still needing a repost.
> 
> If you don't want to take the added space in cpu_policy yet, how about
> having the declaration there and just forgo instantiating the subobject
> in the short term?

There's quite a bit of effectively dead space in the struct already; I
think I wouldn't mind instantiating the struct there right away. So long
as you're convinced it's going to be used there in not too distant a
future.

But: If I go as far, why would I introduce a global instance of the new
struct? Wouldn't it then make more sense to use host_cpu_policy right
away? I probably would keep populating it in vmx_init_vmcs_config() to
limit churn for now, but consumers of the flags could then right away
use the host policy.

Jan

Re: [PATCH v2 08/15] VMX: convert vmx_basic_msr

[Andrew Cooper]

On 27/11/2023 12:44 pm, Jan Beulich wrote:
> On 24.11.2023 23:41, Andrew Cooper wrote:
>> On 24/11/2023 8:41 am, Jan Beulich wrote:
>>> ... to a struct field, which is then going to be accompanied by other
>>> capability/control data presently living in individual variables. As
>>> this structure isn't supposed to be altered post-boot, put it in
>>> .data.ro_after_init right away.
>>>
>>> Suggested-by: Roger Pau Monné <roger.pau@citrix.com>
>>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>> For (usable) nested virt, we're going to need the VMX MSRs, in their
>> architectural form, in struct cpu_policy.  And just like CPUID features,
>> I want it to end up with nice bitfields to use.
>>
>> Looking through the rest of this series, vmx_caps ends up almost in
>> architectural form.
>>
>> Could I talk you into having a "struct vmx_msrs" (or similar - 'caps'
>> doesn't feel quite right here) in the policy object, and also
>> instantiating one instance of it for this purpose here?
> I was actually wondering while doing the conversion. The main reason I
> didn't go that route right away was that I wasn't really certain whether
> what I'd put there would the really be the (largely) final shape it
> wants to take there. (One thing you've likely noticed I didn't convert
> is _vmx_misc_cap, which right now only exists as a local variable in
> vmx_init_vmcs_config().)
>
>> AFAICT, it would only be a minor deviation to the latter half of this
>> series, but it would be an excellent start to fixing nested virt - and
>> getting this data in the policy really is the first task in getting the
>> ball rolling on nested virt.
> How much of a further change it would end up being (or where that change
> would occur) depends on another aspect: When put in cpu-policy.h (and I
> take it you mean the lib/ instance, not the asm/ one), it would seem
> natural and perhaps even necessary to properly introduce bitfields for
> each of the MSRs right away. That'll lead to a "raw" field as well. In
> VMX code (mostly its cpu_has_* #define-s), I'd then either need to use
> .raw (perhaps a little ugly here and there) or go with using the
> individual bitfields right away (likely eliminating the need for many of
> the constant #define-s), which increases the risk of inadvertent mistakes
> (and their overlooking during review).
>
>> I don't mind about serialising/de-serialsing it - that still has a bit
>> of userspace complexity to work out, and depends on some of the cleanup
>> still needing a repost.
>>
>> If you don't want to take the added space in cpu_policy yet, how about
>> having the declaration there and just forgo instantiating the subobject
>> in the short term?
> There's quite a bit of effectively dead space in the struct already; I
> think I wouldn't mind instantiating the struct there right away. So long
> as you're convinced it's going to be used there in not too distant a
> future.
>
> But: If I go as far, why would I introduce a global instance of the new
> struct? Wouldn't it then make more sense to use host_cpu_policy right
> away? I probably would keep populating it in vmx_init_vmcs_config() to
> limit churn for now, but consumers of the flags could then right away
> use the host policy.

George has stated an intent to pick nested virt up imminently.  I'll
have to defer to him on when this will actually start.

But, sorting out this data in the policies is the next step, whenever
that occurs.


If you fancy going all the way to use the raw/host policy then great,
but I expect that would be a large amount of extra work, hence the
suggestion to just use the "inner" struct in the short term.

Conversion to bitfields would want to be separate patch anyway, at which
point an A/B compile can confirm whether there was no resulting change.

I'm happy if you want to do all of this, but it's a lot of work, and
simply having the data in plain architectural uint64_t in the host
policy is something that I thought would be a very minor change to your
current series, but with a useful step towards nested virt.

One open question, before we get too far into this, is still whether to
express half of these as MSR-features like ARCH_CAPS.  Linux does, and
there is a very complex set of dependencies between certain properties,
although I have a sneaking suspicion that the dependency logic will
needed at runtime as the L1 hypervisor changes the various controls.

~Andrew

Re: [PATCH v2 08/15] VMX: convert vmx_basic_msr

[Jan Beulich]

On 18.12.2023 18:29, Andrew Cooper wrote:
> On 27/11/2023 12:44 pm, Jan Beulich wrote:
>> On 24.11.2023 23:41, Andrew Cooper wrote:
>>> On 24/11/2023 8:41 am, Jan Beulich wrote:
>>>> ... to a struct field, which is then going to be accompanied by other
>>>> capability/control data presently living in individual variables. As
>>>> this structure isn't supposed to be altered post-boot, put it in
>>>> .data.ro_after_init right away.
>>>>
>>>> Suggested-by: Roger Pau Monné <roger.pau@citrix.com>
>>>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>>> For (usable) nested virt, we're going to need the VMX MSRs, in their
>>> architectural form, in struct cpu_policy.  And just like CPUID features,
>>> I want it to end up with nice bitfields to use.
>>>
>>> Looking through the rest of this series, vmx_caps ends up almost in
>>> architectural form.
>>>
>>> Could I talk you into having a "struct vmx_msrs" (or similar - 'caps'
>>> doesn't feel quite right here) in the policy object, and also
>>> instantiating one instance of it for this purpose here?
>> I was actually wondering while doing the conversion. The main reason I
>> didn't go that route right away was that I wasn't really certain whether
>> what I'd put there would the really be the (largely) final shape it
>> wants to take there. (One thing you've likely noticed I didn't convert
>> is _vmx_misc_cap, which right now only exists as a local variable in
>> vmx_init_vmcs_config().)
>>
>>> AFAICT, it would only be a minor deviation to the latter half of this
>>> series, but it would be an excellent start to fixing nested virt - and
>>> getting this data in the policy really is the first task in getting the
>>> ball rolling on nested virt.
>> How much of a further change it would end up being (or where that change
>> would occur) depends on another aspect: When put in cpu-policy.h (and I
>> take it you mean the lib/ instance, not the asm/ one), it would seem
>> natural and perhaps even necessary to properly introduce bitfields for
>> each of the MSRs right away. That'll lead to a "raw" field as well. In
>> VMX code (mostly its cpu_has_* #define-s), I'd then either need to use
>> .raw (perhaps a little ugly here and there) or go with using the
>> individual bitfields right away (likely eliminating the need for many of
>> the constant #define-s), which increases the risk of inadvertent mistakes
>> (and their overlooking during review).
>>
>>> I don't mind about serialising/de-serialsing it - that still has a bit
>>> of userspace complexity to work out, and depends on some of the cleanup
>>> still needing a repost.
>>>
>>> If you don't want to take the added space in cpu_policy yet, how about
>>> having the declaration there and just forgo instantiating the subobject
>>> in the short term?
>> There's quite a bit of effectively dead space in the struct already; I
>> think I wouldn't mind instantiating the struct there right away. So long
>> as you're convinced it's going to be used there in not too distant a
>> future.
>>
>> But: If I go as far, why would I introduce a global instance of the new
>> struct? Wouldn't it then make more sense to use host_cpu_policy right
>> away? I probably would keep populating it in vmx_init_vmcs_config() to
>> limit churn for now, but consumers of the flags could then right away
>> use the host policy.
> 
> George has stated an intent to pick nested virt up imminently.  I'll
> have to defer to him on when this will actually start.
> 
> But, sorting out this data in the policies is the next step, whenever
> that occurs.
> 
> 
> If you fancy going all the way to use the raw/host policy then great,
> but I expect that would be a large amount of extra work, hence the
> suggestion to just use the "inner" struct in the short term.

Even the inner struct plan falls apart pretty quickly (or grows what
needs doing by too much for my taste, in the context right here):
While for basic_msr this works, and it would apparently also work
for vmfunc and tertiary exec control (the latter is itself only part
of a yet to be reviewed / approved patch), it doesn't for all the
others with split 0-setting and 1-setting halves. This is because
what VMX code wants are the calculated values to put in the VMCS,
whereas imo in the policy we'd want to store both halves (and what
exactly wants to be in the host policy there isn't really clear to
me). As a result I can't create a single uniform structure properly
serving both purposes. Nor could I have VMX code use the host
policy for most of its capability checks.

Thought / ideas?

Jan

> Conversion to bitfields would want to be separate patch anyway, at which
> point an A/B compile can confirm whether there was no resulting change.
> 
> I'm happy if you want to do all of this, but it's a lot of work, and
> simply having the data in plain architectural uint64_t in the host
> policy is something that I thought would be a very minor change to your
> current series, but with a useful step towards nested virt.
> 
> One open question, before we get too far into this, is still whether to
> express half of these as MSR-features like ARCH_CAPS.  Linux does, and
> there is a very complex set of dependencies between certain properties,
> although I have a sneaking suspicion that the dependency logic will
> needed at runtime as the L1 hypervisor changes the various controls.
> 
> ~Andrew

Re: [PATCH v2 08/15] VMX: convert vmx_basic_msr

[Andrew Cooper]

On 09/01/2024 2:39 pm, Jan Beulich wrote:
> On 18.12.2023 18:29, Andrew Cooper wrote:
>> On 27/11/2023 12:44 pm, Jan Beulich wrote:
>>> On 24.11.2023 23:41, Andrew Cooper wrote:
>>>> On 24/11/2023 8:41 am, Jan Beulich wrote:
>>>>> ... to a struct field, which is then going to be accompanied by other
>>>>> capability/control data presently living in individual variables. As
>>>>> this structure isn't supposed to be altered post-boot, put it in
>>>>> .data.ro_after_init right away.
>>>>>
>>>>> Suggested-by: Roger Pau Monné <roger.pau@citrix.com>
>>>>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>>>> For (usable) nested virt, we're going to need the VMX MSRs, in their
>>>> architectural form, in struct cpu_policy.  And just like CPUID features,
>>>> I want it to end up with nice bitfields to use.
>>>>
>>>> Looking through the rest of this series, vmx_caps ends up almost in
>>>> architectural form.
>>>>
>>>> Could I talk you into having a "struct vmx_msrs" (or similar - 'caps'
>>>> doesn't feel quite right here) in the policy object, and also
>>>> instantiating one instance of it for this purpose here?
>>> I was actually wondering while doing the conversion. The main reason I
>>> didn't go that route right away was that I wasn't really certain whether
>>> what I'd put there would the really be the (largely) final shape it
>>> wants to take there. (One thing you've likely noticed I didn't convert
>>> is _vmx_misc_cap, which right now only exists as a local variable in
>>> vmx_init_vmcs_config().)
>>>
>>>> AFAICT, it would only be a minor deviation to the latter half of this
>>>> series, but it would be an excellent start to fixing nested virt - and
>>>> getting this data in the policy really is the first task in getting the
>>>> ball rolling on nested virt.
>>> How much of a further change it would end up being (or where that change
>>> would occur) depends on another aspect: When put in cpu-policy.h (and I
>>> take it you mean the lib/ instance, not the asm/ one), it would seem
>>> natural and perhaps even necessary to properly introduce bitfields for
>>> each of the MSRs right away. That'll lead to a "raw" field as well. In
>>> VMX code (mostly its cpu_has_* #define-s), I'd then either need to use
>>> .raw (perhaps a little ugly here and there) or go with using the
>>> individual bitfields right away (likely eliminating the need for many of
>>> the constant #define-s), which increases the risk of inadvertent mistakes
>>> (and their overlooking during review).
>>>
>>>> I don't mind about serialising/de-serialsing it - that still has a bit
>>>> of userspace complexity to work out, and depends on some of the cleanup
>>>> still needing a repost.
>>>>
>>>> If you don't want to take the added space in cpu_policy yet, how about
>>>> having the declaration there and just forgo instantiating the subobject
>>>> in the short term?
>>> There's quite a bit of effectively dead space in the struct already; I
>>> think I wouldn't mind instantiating the struct there right away. So long
>>> as you're convinced it's going to be used there in not too distant a
>>> future.
>>>
>>> But: If I go as far, why would I introduce a global instance of the new
>>> struct? Wouldn't it then make more sense to use host_cpu_policy right
>>> away? I probably would keep populating it in vmx_init_vmcs_config() to
>>> limit churn for now, but consumers of the flags could then right away
>>> use the host policy.
>> George has stated an intent to pick nested virt up imminently.  I'll
>> have to defer to him on when this will actually start.
>>
>> But, sorting out this data in the policies is the next step, whenever
>> that occurs.
>>
>>
>> If you fancy going all the way to use the raw/host policy then great,
>> but I expect that would be a large amount of extra work, hence the
>> suggestion to just use the "inner" struct in the short term.
> Even the inner struct plan falls apart pretty quickly (or grows what
> needs doing by too much for my taste, in the context right here):
> While for basic_msr this works, and it would apparently also work
> for vmfunc and tertiary exec control (the latter is itself only part
> of a yet to be reviewed / approved patch), it doesn't for all the
> others with split 0-setting and 1-setting halves. This is because
> what VMX code wants are the calculated values to put in the VMCS,
> whereas imo in the policy we'd want to store both halves (and what
> exactly wants to be in the host policy there isn't really clear to
> me). As a result I can't create a single uniform structure properly
> serving both purposes. Nor could I have VMX code use the host
> policy for most of its capability checks.
>
> Thought / ideas?

If it's not actually trivial, then don't worry.

The policy does need to hold the architectural representation.  The
in-use settings need storing per-vCPU because they do (or need to me
made to) vary based on the configuration of the VM, and because they're
needed on every virtual vmentry when re-calculating VMCS02.

~Andrew

Re: [PATCH v2 08/15] VMX: convert vmx_basic_msr

[Jan Beulich]

On 09.01.2024 15:54, Andrew Cooper wrote:
> On 09/01/2024 2:39 pm, Jan Beulich wrote:
>> Even the inner struct plan falls apart pretty quickly (or grows what
>> needs doing by too much for my taste, in the context right here):
>> While for basic_msr this works, and it would apparently also work
>> for vmfunc and tertiary exec control (the latter is itself only part
>> of a yet to be reviewed / approved patch), it doesn't for all the
>> others with split 0-setting and 1-setting halves. This is because
>> what VMX code wants are the calculated values to put in the VMCS,
>> whereas imo in the policy we'd want to store both halves (and what
>> exactly wants to be in the host policy there isn't really clear to
>> me). As a result I can't create a single uniform structure properly
>> serving both purposes. Nor could I have VMX code use the host
>> policy for most of its capability checks.
>>
>> Thought / ideas?
> 
> If it's not actually trivial, then don't worry.
> 
> The policy does need to hold the architectural representation.  The
> in-use settings need storing per-vCPU because they do (or need to me
> made to) vary based on the configuration of the VM, and because they're
> needed on every virtual vmentry when re-calculating VMCS02.

Would it help if I did a hybrid approach, i.e. move to raw/host policies
only what can be easily moved, with the rest kept as is (perhaps with
vmx_caps renamed to vmx_ctls then)?

Jan