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

[Paul Durrant <Paul.Durrant@xxxxxxxxxx>]

> -----Original Message-----
> From: Xen-devel [mailto:xen-devel-bounces@xxxxxxxxxxxxxxxxxxxx] On Behalf Of 
> Jan Beulich
> Sent: 13 March 2019 15:55
> To: Andrew Cooper <Andrew.Cooper3@xxxxxxxxxx>; Paul Durrant 
> <Paul.Durrant@xxxxxxxxxx>
> Cc: xen-devel <xen-devel@xxxxxxxxxxxxxxxxxxxx>; Kevin Tian 
> <kevin.tian@xxxxxxxxx>; Wei Liu
> <wei.liu2@xxxxxxxxxx>; Jun Nakajima <jun.nakajima@xxxxxxxxx>; Roger Pau Monne 
> <roger.pau@xxxxxxxxxx>
> Subject: Re: [Xen-devel] [PATCH v2 1/4] x86: stop handling MSR_IA32_BNDCFGS 
> save/restore in
> implementation code
> 
> >>> On 11.03.19 at 19:09, <paul.durrant@xxxxxxxxxx> wrote:
> > v2:
> >  - Addressed comments from Jan by largely removing hunks
> 
> I think you've removed more than I was expecting, but I'm fine
> this way.
> 
> > @@ -158,6 +158,13 @@ int guest_rdmsr(const struct vcpu *v, uint32_t msr, 
> > uint64_t *val)
> >          ret = guest_rdmsr_x2apic(v, msr, val);
> >          break;
> >
> > +    case MSR_IA32_BNDCFGS:
> > +        if ( !is_hvm_domain(d) || !cp->feat.mpx ||
> > +             !hvm_get_guest_bndcfgs(v, val) )
> > +            goto gp_fault;
> > +
> > +        break;
> > +
> >      case 0x40000000 ... 0x400001ff:
> >          if ( is_viridian_domain(d) )
> >          {
> > @@ -319,6 +326,13 @@ int guest_wrmsr(struct vcpu *v, uint32_t msr, uint64_t 
> > val)
> >          ret = guest_wrmsr_x2apic(v, msr, val);
> >          break;
> >
> > +    case MSR_IA32_BNDCFGS:
> > +        if ( !is_hvm_domain(d) || !cp->feat.mpx ||
> > +             !hvm_set_guest_bndcfgs(v, val) )
> > +            goto gp_fault;
> 
> In both cases the is_hvm_*() check looks to be redundant, as
> for PV guests cp->feat.mpx can't be set. Personally I'd prefer
> this to be an ASSERT() instead, but I'd listen to Andrew (as
> the main author of this code) saying otherwise.

I'm ok with an ASSERT but I'll check with Andrew as you suggest.

> 
> > --- a/xen/include/asm-x86/msr.h
> > +++ b/xen/include/asm-x86/msr.h
> > @@ -328,7 +328,7 @@ int init_vcpu_msr_policy(struct vcpu *v);
> >   * These functions are also used by the migration logic, so need to cope 
> > with
> >   * being used outside of v's context.
> >   */
> > -int guest_rdmsr(const struct vcpu *v, uint32_t msr, uint64_t *val);
> > +int guest_rdmsr(struct vcpu *v, uint32_t msr, uint64_t *val);
> 
> I find this pretty undesirable, and I'd like to at least put out
> for discussion a means how to avoid it: Any entity being
> passed a const struct vcpu *cv can get hold of a non-const
> one by doing
> 
>     struct vcpu *v = cv->domain->vcpu[cv->vcpu_id];
> 

Looks kind of odd, but of course it will certainly work.

> Of course this shouldn't be used arbitrarily, but to hide an
> implementation detail like that of vmx_vmcs_enter() I think
> this could be justified. Thoughts?
> 

I guess the question is at what level to do this? Probably in the hvm code 
rather than in the vmx code.

  Paul 

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