| Message ID | 20200820150835.27440-6-roger.pau@citrix.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | x86: switch default MSR behavior | expand |
On 20.08.2020 17:08, Roger Pau Monne wrote: > @@ -181,6 +182,18 @@ int guest_rdmsr(struct vcpu *v, uint32_t msr, uint64_t *val) > /* Not offered to guests. */ > goto gp_fault; > > + case MSR_IA32_FEATURE_CONTROL: > + if ( !(cp->x86_vendor & X86_VENDOR_INTEL) ) > + goto gp_fault; Can we really do it this way round, rather than raising #GP when we know the MSR isn't there (AMD / Hygon)? I realized code e.g. ... > + *val = IA32_FEATURE_CONTROL_LOCK; > + if ( vmce_has_lmce(v) ) > + *val |= IA32_FEATURE_CONTROL_LMCE_ON; > + if ( nestedhvm_enabled(d) ) > + *val |= IA32_FEATURE_CONTROL_ENABLE_VMXON_OUTSIDE_SMX; > + break; > + > + > case MSR_IA32_PLATFORM_ID: > if ( !(cp->x86_vendor & X86_VENDOR_INTEL) || > !(boot_cpu_data.x86_vendor & X86_VENDOR_INTEL) ) ... in context right here does it the same way, but I still wonder whether we wouldn't better switch existing instances, too. Jan
On Thu, Aug 27, 2020 at 05:53:16PM +0200, Jan Beulich wrote: > On 20.08.2020 17:08, Roger Pau Monne wrote: > > @@ -181,6 +182,18 @@ int guest_rdmsr(struct vcpu *v, uint32_t msr, uint64_t *val) > > /* Not offered to guests. */ > > goto gp_fault; > > > > + case MSR_IA32_FEATURE_CONTROL: > > + if ( !(cp->x86_vendor & X86_VENDOR_INTEL) ) > > + goto gp_fault; > > Can we really do it this way round, rather than raising #GP when > we know the MSR isn't there (AMD / Hygon)? I realized code e.g. > ... > > > + *val = IA32_FEATURE_CONTROL_LOCK; > > + if ( vmce_has_lmce(v) ) > > + *val |= IA32_FEATURE_CONTROL_LMCE_ON; > > + if ( nestedhvm_enabled(d) ) > > + *val |= IA32_FEATURE_CONTROL_ENABLE_VMXON_OUTSIDE_SMX; > > + break; > > + > > + > > case MSR_IA32_PLATFORM_ID: > > if ( !(cp->x86_vendor & X86_VENDOR_INTEL) || > > !(boot_cpu_data.x86_vendor & X86_VENDOR_INTEL) ) > > ... in context right here does it the same way, but I still > wonder whether we wouldn't better switch existing instances, too. Hm, no idea really. Right now it seems better to check for != Intel rather than AMD | Hygon | Centaur | Shanghai, as that's a MSR specific to Intel. Do those MSRs exist in Centaur / Shanghai? Thanks, Roger.
On 31.08.2020 17:12, Roger Pau Monné wrote: > On Thu, Aug 27, 2020 at 05:53:16PM +0200, Jan Beulich wrote: >> On 20.08.2020 17:08, Roger Pau Monne wrote: >>> @@ -181,6 +182,18 @@ int guest_rdmsr(struct vcpu *v, uint32_t msr, uint64_t *val) >>> /* Not offered to guests. */ >>> goto gp_fault; >>> >>> + case MSR_IA32_FEATURE_CONTROL: >>> + if ( !(cp->x86_vendor & X86_VENDOR_INTEL) ) >>> + goto gp_fault; >> >> Can we really do it this way round, rather than raising #GP when >> we know the MSR isn't there (AMD / Hygon)? I realized code e.g. >> ... >> >>> + *val = IA32_FEATURE_CONTROL_LOCK; >>> + if ( vmce_has_lmce(v) ) >>> + *val |= IA32_FEATURE_CONTROL_LMCE_ON; >>> + if ( nestedhvm_enabled(d) ) >>> + *val |= IA32_FEATURE_CONTROL_ENABLE_VMXON_OUTSIDE_SMX; >>> + break; >>> + >>> + >>> case MSR_IA32_PLATFORM_ID: >>> if ( !(cp->x86_vendor & X86_VENDOR_INTEL) || >>> !(boot_cpu_data.x86_vendor & X86_VENDOR_INTEL) ) >> >> ... in context right here does it the same way, but I still >> wonder whether we wouldn't better switch existing instances, too. > > Hm, no idea really. Right now it seems better to check for != Intel > rather than AMD | Hygon | Centaur | Shanghai, as that's a MSR specific > to Intel. > > Do those MSRs exist in Centaur / Shanghai? I can't tell for sure, but I suppose they do, as they're (in a way) cloning Intel's implementation. My thinking here is that by not exposing an MSR when we should we potentially cause guests to crash. Whereas by exposing an MSR that ought not be there fallout would result only if the vendor actually has something else at that index. And I'd hope vendors apply some care to avoid doing so. Jan
diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c index 4717e50d4a..f6657af923 100644 --- a/xen/arch/x86/hvm/vmx/vmx.c +++ b/xen/arch/x86/hvm/vmx/vmx.c @@ -2980,13 +2980,7 @@ static int vmx_msr_read_intercept(unsigned int msr, uint64_t *msr_content) case MSR_IA32_DEBUGCTLMSR: __vmread(GUEST_IA32_DEBUGCTL, msr_content); break; - case MSR_IA32_FEATURE_CONTROL: - *msr_content = IA32_FEATURE_CONTROL_LOCK; - if ( vmce_has_lmce(curr) ) - *msr_content |= IA32_FEATURE_CONTROL_LMCE_ON; - if ( nestedhvm_enabled(curr->domain) ) - *msr_content |= IA32_FEATURE_CONTROL_ENABLE_VMXON_OUTSIDE_SMX; - break; + case MSR_IA32_VMX_BASIC...MSR_IA32_VMX_VMFUNC: if ( !nvmx_msr_read_intercept(msr, msr_content) ) goto gp_fault; diff --git a/xen/arch/x86/msr.c b/xen/arch/x86/msr.c index a890cb9976..bb0dd5ff0a 100644 --- a/xen/arch/x86/msr.c +++ b/xen/arch/x86/msr.c @@ -25,6 +25,7 @@ #include <xen/sched.h> #include <asm/debugreg.h> +#include <asm/hvm/nestedhvm.h> #include <asm/hvm/viridian.h> #include <asm/msr.h> #include <asm/setup.h> @@ -181,6 +182,18 @@ int guest_rdmsr(struct vcpu *v, uint32_t msr, uint64_t *val) /* Not offered to guests. */ goto gp_fault; + case MSR_IA32_FEATURE_CONTROL: + if ( !(cp->x86_vendor & X86_VENDOR_INTEL) ) + goto gp_fault; + + *val = IA32_FEATURE_CONTROL_LOCK; + if ( vmce_has_lmce(v) ) + *val |= IA32_FEATURE_CONTROL_LMCE_ON; + if ( nestedhvm_enabled(d) ) + *val |= IA32_FEATURE_CONTROL_ENABLE_VMXON_OUTSIDE_SMX; + break; + + case MSR_IA32_PLATFORM_ID: if ( !(cp->x86_vendor & X86_VENDOR_INTEL) || !(boot_cpu_data.x86_vendor & X86_VENDOR_INTEL) )
Linux PV guests will attempt to read the FEATURE_CONTROL MSR, so move the handling done in VMX code into guest_rdmsr as it can be shared between PV and HVM guests that way. Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> --- Changes from v1: - Move the VMX implementation into guest_rdmsr. --- xen/arch/x86/hvm/vmx/vmx.c | 8 +------- xen/arch/x86/msr.c | 13 +++++++++++++ 2 files changed, 14 insertions(+), 7 deletions(-)