Re: [PATCH v3 5/8] x86/pv: allow reading FEATURE_CONTROL MSR

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

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é <roger.pau@xxxxxxxxxx>
> > ---
> > 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.