Re: [Xen-devel] [PATCH 1/6] x86: stop handling MSR_IA32_BNDCFGS save/restore in implementation code

[Paul Durrant <Paul.Durrant@xxxxxxxxxx>]

> -----Original Message-----
> From: Jan Beulich [mailto:JBeulich@xxxxxxxx]
> Sent: 08 March 2019 16:40
> To: Paul Durrant <Paul.Durrant@xxxxxxxxxx>
> Cc: Andrew Cooper <Andrew.Cooper3@xxxxxxxxxx>; Roger Pau Monne 
> <roger.pau@xxxxxxxxxx>; Wei Liu
> <wei.liu2@xxxxxxxxxx>; Jun Nakajima <jun.nakajima@xxxxxxxxx>; Kevin Tian 
> <kevin.tian@xxxxxxxxx>; xen-
> devel <xen-devel@xxxxxxxxxxxxxxxxxxxx>
> Subject: Re: [PATCH 1/6] x86: stop handling MSR_IA32_BNDCFGS save/restore in 
> implementation code
> 
> >>> On 07.01.19 at 13:02, <paul.durrant@xxxxxxxxxx> wrote:
> > --- a/xen/arch/x86/hvm/hvm.c
> > +++ b/xen/arch/x86/hvm/hvm.c
> > @@ -308,11 +308,16 @@ int hvm_set_guest_pat(struct vcpu *v, u64 guest_pat)
> >      return 1;
> >  }
> >
> > -bool hvm_set_guest_bndcfgs(struct vcpu *v, u64 val)
> > +bool hvm_set_guest_bndcfgs(struct vcpu *v, uint64_t val)
> >  {
> > -    if ( !hvm_funcs.set_guest_bndcfgs ||
> > -         !is_canonical_address(val) ||
> > -         (val & IA32_BNDCFGS_RESERVED) )
> > +    const struct cpuid_policy *cp = v->domain->arch.cpuid;
> > +
> > +    if ( !cp->feat.mpx )
> 
> Is the local variable really worth it?

Maybe not.

> 
> > +        return false;
> > +
> > +    ASSERT(hvm_funcs.set_guest_bndcfgs);
> 
> I think it would be better (more safe for release builds) if this was
> left as part of the if(). Otherwise, ...
> 
> > @@ -342,7 +347,22 @@ bool hvm_set_guest_bndcfgs(struct vcpu *v, u64 val)
> >              /* nothing, best effort only */;
> >      }
> >
> > -    return hvm_funcs.set_guest_bndcfgs(v, val);
> > +    hvm_funcs.set_guest_bndcfgs(v, val);
> 
> ... you risk calling NULL here.

Ok, I'll return false instead.

> 
> > @@ -3472,12 +3494,6 @@ int hvm_msr_read_intercept(unsigned int msr, 
> > uint64_t *msr_content)
> >          *msr_content = v->arch.hvm.msr_xss;
> >          break;
> >
> > -    case MSR_IA32_BNDCFGS:
> > -        if ( !d->arch.cpuid->feat.mpx ||
> > -             !hvm_get_guest_bndcfgs(v, msr_content) )
> > -            goto gp_fault;
> > -        break;
> 
> Note how this already checks the CPUID policy, somewhat contrary to
> what you say in the commit message. I think this wants to remain this
> way in guest_{rd,wr}msr() - see all the other CPUID policy checks there.
> That way hvm_get_guest_bndcfgs() can be made consistent with the
> hook pointer type again, and it can remain an inline function as before.

I thought it was neater to put the check inside the helper but I can pull it 
out into the switch statement if that's preferred.

  Paul

> 
> Jan
> 


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel