| Message ID | 20170626104435.25508-3-sergey.dyasli@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
>>> On 26.06.17 at 12:44, <sergey.dyasli@citrix.com> wrote: > --- a/xen/arch/x86/hvm/vmx/vmcs.c > +++ b/xen/arch/x86/hvm/vmx/vmcs.c > @@ -144,6 +144,8 @@ static void __init vmx_display_features(void) > printk(" - none\n"); > } > > +struct vmx_msr_policy __read_mostly raw_vmx_msr_policy; Does this really need to be non-static? I don't see a use outside of this file in the patch here at least. > @@ -152,6 +154,74 @@ bool vmx_msr_available(struct vmx_msr_policy *p, uint32_t msr) > return p->available & (1u << (msr - MSR_IA32_VMX_BASIC)); > } > > +int calculate_raw_policy(bool bsp) > +{ > + struct vmx_msr_policy policy; > + struct vmx_msr_policy *p = &policy; > + int msr; unsigned int > + /* Raw policy is filled only on boot CPU */ > + if ( bsp ) > + p = &raw_vmx_msr_policy; > + else > + memset(&policy, 0, sizeof(policy)); > + > + p->available = 0x7ff; (1u << (MSR_IA32_VMX_VMCS_ENUM + 1 - MSR_IA32_VMX_BASIC)) - 1 > + for ( msr = MSR_IA32_VMX_BASIC; msr <= MSR_IA32_VMX_VMCS_ENUM; msr++ ) > + rdmsrl(msr, p->msr[msr - MSR_IA32_VMX_BASIC]); > + > + if ( p->basic.default1_zero ) > + { > + p->available |= 0x1e000; Same here and further down - please calculate the values from available constants. Maybe you want to have a helper macro or inline function. > + /* Check that secondary CPUs have exactly the same bits in VMX MSRs */ > + if ( !bsp && memcmp(p, &raw_vmx_msr_policy, sizeof(*p)) != 0 ) > + { > + for ( msr = MSR_IA32_VMX_BASIC; msr <= MSR_IA32_VMX_VMFUNC; msr++ ) > + { > + if ( p->msr[msr - MSR_IA32_VMX_BASIC] != > + raw_vmx_msr_policy.msr[msr - MSR_IA32_VMX_BASIC] ) > + { > + printk("VMX msr %#x: saw 0x%016"PRIx64" expected 0x%016"PRIx64 > + "\n", msr, p->msr[msr - MSR_IA32_VMX_BASIC], Please keep the newline on the same line as the rest of the format string. It being slightly longer then really wanted is okay for format strings. > @@ -611,6 +624,9 @@ int vmx_cpu_up(void) > > BUG_ON(!(read_cr4() & X86_CR4_VMXE)); > > + if ( (rc = calculate_raw_policy(false)) != 0 ) > + return rc; > + > /* > * Ensure the current processor operating mode meets > * the requred CRO fixed bits in VMX operation. Following here are reads of MSR_IA32_VMX_CR0_FIXED{0,1} which you should be able to drop now, instead using the raw policy you've just checked matches this CPU. Btw., is it intentional that the function is being invoked for the BSP a second time here (after start_vmx() did so already), with the flag now being passed with the wrong value? > --- a/xen/arch/x86/hvm/vmx/vmx.c > +++ b/xen/arch/x86/hvm/vmx/vmx.c > @@ -2432,6 +2432,8 @@ static void pi_notification_interrupt(struct cpu_user_regs *regs) > raise_softirq(VCPU_KICK_SOFTIRQ); > } > > +int calculate_raw_policy(bool bsp); Declarations need to go in a header included by both producer and consumer, so that someone changing only one of definition and declaration will be forced to also change the other side. Jan
On Tue, 2017-07-04 at 08:15 -0600, Jan Beulich wrote: > > > > On 26.06.17 at 12:44, <sergey.dyasli@citrix.com> wrote: > > > > @@ -611,6 +624,9 @@ int vmx_cpu_up(void) > > > > BUG_ON(!(read_cr4() & X86_CR4_VMXE)); > > > > + if ( (rc = calculate_raw_policy(false)) != 0 ) > > + return rc; > > + > > /* > > * Ensure the current processor operating mode meets > > * the requred CRO fixed bits in VMX operation. > > Btw., is it intentional that the function is being invoked for the BSP a > second time here (after start_vmx() did so already), with the flag > now being passed with the wrong value? Unfortunately, I couldn't find a better way of detecting if the code is running on the boot CPU. And I decided to use the existing practice of passing a flag. -- Thanks, Sergey
>>> On 06.07.17 at 11:29, <sergey.dyasli@citrix.com> wrote: > On Tue, 2017-07-04 at 08:15 -0600, Jan Beulich wrote: >> > > > On 26.06.17 at 12:44, <sergey.dyasli@citrix.com> wrote: >> > >> > @@ -611,6 +624,9 @@ int vmx_cpu_up(void) >> > >> > BUG_ON(!(read_cr4() & X86_CR4_VMXE)); >> > >> > + if ( (rc = calculate_raw_policy(false)) != 0 ) >> > + return rc; >> > + >> > /* >> > * Ensure the current processor operating mode meets >> > * the requred CRO fixed bits in VMX operation. >> >> Btw., is it intentional that the function is being invoked for the BSP a >> second time here (after start_vmx() did so already), with the flag >> now being passed with the wrong value? > > Unfortunately, I couldn't find a better way of detecting if the code is running > on the boot CPU. And I decided to use the existing practice of passing a flag. This passing of a flag is fine; what I'm uncomfortable with is that the second invocation on the BSP will say it's not on the BSP. While this looks to be benign, it would feel better if there wasn't such a second invocation at all. Jan
diff --git a/xen/arch/x86/hvm/vmx/vmcs.c b/xen/arch/x86/hvm/vmx/vmcs.c index e6ea197230..00fbc0ccb8 100644 --- a/xen/arch/x86/hvm/vmx/vmcs.c +++ b/xen/arch/x86/hvm/vmx/vmcs.c @@ -144,6 +144,8 @@ static void __init vmx_display_features(void) printk(" - none\n"); } +struct vmx_msr_policy __read_mostly raw_vmx_msr_policy; + bool vmx_msr_available(struct vmx_msr_policy *p, uint32_t msr) { if ( msr < MSR_IA32_VMX_BASIC || msr > MSR_IA32_VMX_VMFUNC ) @@ -152,6 +154,74 @@ bool vmx_msr_available(struct vmx_msr_policy *p, uint32_t msr) return p->available & (1u << (msr - MSR_IA32_VMX_BASIC)); } +int calculate_raw_policy(bool bsp) +{ + struct vmx_msr_policy policy; + struct vmx_msr_policy *p = &policy; + int msr; + + /* Raw policy is filled only on boot CPU */ + if ( bsp ) + p = &raw_vmx_msr_policy; + else + memset(&policy, 0, sizeof(policy)); + + p->available = 0x7ff; + for ( msr = MSR_IA32_VMX_BASIC; msr <= MSR_IA32_VMX_VMCS_ENUM; msr++ ) + rdmsrl(msr, p->msr[msr - MSR_IA32_VMX_BASIC]); + + if ( p->basic.default1_zero ) + { + p->available |= 0x1e000; + for ( msr = MSR_IA32_VMX_TRUE_PINBASED_CTLS; + msr <= MSR_IA32_VMX_TRUE_ENTRY_CTLS; msr++ ) + rdmsrl(msr, p->msr[msr - MSR_IA32_VMX_BASIC]); + } + + if ( p->procbased_ctls.allowed_1.activate_secondary_controls ) + { + p->available |= 0x800; + msr = MSR_IA32_VMX_PROCBASED_CTLS2; + rdmsrl(msr, p->msr[msr - MSR_IA32_VMX_BASIC]); + + if ( p->procbased_ctls2.allowed_1.enable_ept || + p->procbased_ctls2.allowed_1.enable_vpid ) + { + p->available |= 0x1000; + msr = MSR_IA32_VMX_EPT_VPID_CAP; + rdmsrl(msr, p->msr[msr - MSR_IA32_VMX_BASIC]); + } + + if ( p->procbased_ctls2.allowed_1.enable_vm_functions ) + { + p->available |= 0x20000; + msr = MSR_IA32_VMX_VMFUNC; + rdmsrl(msr, p->msr[msr - MSR_IA32_VMX_BASIC]); + } + } + + /* Check that secondary CPUs have exactly the same bits in VMX MSRs */ + if ( !bsp && memcmp(p, &raw_vmx_msr_policy, sizeof(*p)) != 0 ) + { + for ( msr = MSR_IA32_VMX_BASIC; msr <= MSR_IA32_VMX_VMFUNC; msr++ ) + { + if ( p->msr[msr - MSR_IA32_VMX_BASIC] != + raw_vmx_msr_policy.msr[msr - MSR_IA32_VMX_BASIC] ) + { + printk("VMX msr %#x: saw 0x%016"PRIx64" expected 0x%016"PRIx64 + "\n", msr, p->msr[msr - MSR_IA32_VMX_BASIC], + raw_vmx_msr_policy.msr[msr - MSR_IA32_VMX_BASIC]); + } + } + + printk("VMX: Capabilities fatally differ between CPU%d and boot CPU\n", + smp_processor_id()); + return -EINVAL; + } + + return 0; +} + static u32 adjust_vmx_controls( const char *name, u32 ctl_min, u32 ctl_opt, u32 msr, bool_t *mismatch) { @@ -173,13 +243,6 @@ static u32 adjust_vmx_controls( return ctl; } -static bool_t cap_check(const char *name, u32 expected, u32 saw) -{ - if ( saw != expected ) - printk("VMX %s: saw %#x expected %#x\n", name, saw, expected); - return saw != expected; -} - static int vmx_init_vmcs_config(void) { u32 vmx_basic_msr_low, vmx_basic_msr_high, min, opt; @@ -412,56 +475,6 @@ static int vmx_init_vmcs_config(void) return -EINVAL; } } - else - { - /* Globals are already initialised: re-check them. */ - mismatch |= cap_check( - "VMCS revision ID", - 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); - mismatch |= cap_check( - "CPU-Based Exec Control", - vmx_cpu_based_exec_control, _vmx_cpu_based_exec_control); - mismatch |= cap_check( - "Secondary Exec Control", - vmx_secondary_exec_control, _vmx_secondary_exec_control); - mismatch |= cap_check( - "VMExit Control", - vmx_vmexit_control, _vmx_vmexit_control); - mismatch |= cap_check( - "VMEntry Control", - vmx_vmentry_control, _vmx_vmentry_control); - mismatch |= cap_check( - "EPT and VPID Capability", - vmx_ept_vpid_cap, _vmx_ept_vpid_cap); - mismatch |= cap_check( - "VMFUNC Capability", - vmx_vmfunc, _vmx_vmfunc); - if ( cpu_has_vmx_ins_outs_instr_info != - !!(vmx_basic_msr_high & (VMX_BASIC_INS_OUT_INFO >> 32)) ) - { - printk("VMX INS/OUTS Instruction Info: saw %d expected %d\n", - !!(vmx_basic_msr_high & (VMX_BASIC_INS_OUT_INFO >> 32)), - cpu_has_vmx_ins_outs_instr_info); - mismatch = 1; - } - if ( (vmx_basic_msr_high & (VMX_BASIC_VMCS_SIZE_MASK >> 32)) != - ((vmx_basic_msr & VMX_BASIC_VMCS_SIZE_MASK) >> 32) ) - { - printk("VMX: CPU%d unexpected VMCS size %Lu\n", - smp_processor_id(), - vmx_basic_msr_high & (VMX_BASIC_VMCS_SIZE_MASK >> 32)); - mismatch = 1; - } - if ( mismatch ) - { - printk("VMX: Capabilities fatally differ between CPU%d and CPU0\n", - smp_processor_id()); - return -EINVAL; - } - } /* IA-32 SDM Vol 3B: 64-bit CPUs always have VMX_BASIC_MSR[48]==0. */ if ( vmx_basic_msr_high & (VMX_BASIC_32BIT_ADDRESSES >> 32) ) @@ -611,6 +624,9 @@ int vmx_cpu_up(void) BUG_ON(!(read_cr4() & X86_CR4_VMXE)); + if ( (rc = calculate_raw_policy(false)) != 0 ) + return rc; + /* * Ensure the current processor operating mode meets * the requred CRO fixed bits in VMX operation. diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c index c53b24955a..f344d6a5ea 100644 --- a/xen/arch/x86/hvm/vmx/vmx.c +++ b/xen/arch/x86/hvm/vmx/vmx.c @@ -2432,6 +2432,8 @@ static void pi_notification_interrupt(struct cpu_user_regs *regs) raise_softirq(VCPU_KICK_SOFTIRQ); } +int calculate_raw_policy(bool bsp); + static void __init lbr_tsx_fixup_check(void); static void __init bdw_erratum_bdf14_fixup_check(void); @@ -2439,6 +2441,8 @@ const struct hvm_function_table * __init start_vmx(void) { set_in_cr4(X86_CR4_VMXE); + calculate_raw_policy(true); + if ( vmx_cpu_up() ) { printk("VMX: failed to initialise.\n"); diff --git a/xen/include/asm-x86/hvm/vmx/vmcs.h b/xen/include/asm-x86/hvm/vmx/vmcs.h index fca1e62e4c..8b97f85c46 100644 --- a/xen/include/asm-x86/hvm/vmx/vmcs.h +++ b/xen/include/asm-x86/hvm/vmx/vmcs.h @@ -904,6 +904,8 @@ struct vmx_msr_policy }; }; +extern struct vmx_msr_policy raw_vmx_msr_policy; + bool vmx_msr_available(struct vmx_msr_policy *p, uint32_t msr); #endif /* ASM_X86_HVM_VMX_VMCS_H__ */
Add calculate_raw_policy() which fills raw_vmx_msr_policy (the actual contents of H/W VMX MSRs) on the boot CPU. On secondary CPUs, this function checks that contents of VMX MSRs match the boot CPU's contents. Remove lesser version of same-contents-check from vmx_init_vmcs_config(). Signed-off-by: Sergey Dyasli <sergey.dyasli@citrix.com> --- xen/arch/x86/hvm/vmx/vmcs.c | 130 +++++++++++++++++++++---------------- xen/arch/x86/hvm/vmx/vmx.c | 4 ++ xen/include/asm-x86/hvm/vmx/vmcs.h | 2 + 3 files changed, 79 insertions(+), 57 deletions(-)