On Wed, Sep 02, 2020 at 09:56:48PM +0100, Andrew Cooper wrote:
> On 01/09/2020 11:54, Roger Pau Monne wrote:
> > 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é <[email protected]>
> > ---
> > 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(-)
> >
> > 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 e84107ac7b..cc2f111a90 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>
> > @@ -197,6 +198,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;
>
> The MSR is available if:
>
> "If any one enumeration
> condition for defined bit
> field position greater than
> bit 0 holds."
>
> which for us means cp->basic.vmx || cp->feat.lmce at the moment, with
> perhaps some smx/sgx in the future.
I don't think there's a lmce cpu bit (seems to be signaled in
IA32_MCG_CAP[27] = 1), maybe I should just use vmce_has_lmce?
if ( !cp->basic.vmx || !vmce_has_lmce(v) )
goto gp_fault;
Is it fine however to return a #GP if we don't provide any of those
features to guests, but the underlying CPU does support them?
That seems to be slightly different from how we currently handle reads
to FEATURE_CONTROL, since it will never fault on Intel (or compliant),
even if we just return with bit 0 set alone. The new approach seems
closer to what real hardware would do.
Thanks, Roger.
