Re: [Xen-devel] [PATCH] x86/msr: Fix fallout from mostly c/s 832c180

[Paul Durrant <Paul.Durrant@xxxxxxxxxx>]

> -----Original Message-----
> From: Paul Durrant
> Sent: 10 April 2019 09:28
> To: Andrew Cooper <Andrew.Cooper3@xxxxxxxxxx>; Xen-devel 
> <xen-devel@xxxxxxxxxxxxx>
> Cc: Andrew Cooper <Andrew.Cooper3@xxxxxxxxxx>; Jan Beulich 
> <JBeulich@xxxxxxxx>; Wei Liu
> <wei.liu2@xxxxxxxxxx>; Roger Pau Monne <roger.pau@xxxxxxxxxx>; Jun Nakajima 
> <jun.nakajima@xxxxxxxxx>;
> Kevin Tian <kevin.tian@xxxxxxxxx>
> Subject: RE: [PATCH] x86/msr: Fix fallout from mostly c/s 832c180
> 
> > -----Original Message-----
> > From: Andrew Cooper [mailto:andrew.cooper3@xxxxxxxxxx]
> > Sent: 09 April 2019 18:54
> > To: Xen-devel <xen-devel@xxxxxxxxxxxxx>
> > Cc: Andrew Cooper <Andrew.Cooper3@xxxxxxxxxx>; Jan Beulich 
> > <JBeulich@xxxxxxxx>; Wei Liu
> > <wei.liu2@xxxxxxxxxx>; Roger Pau Monne <roger.pau@xxxxxxxxxx>; Paul Durrant
> <Paul.Durrant@xxxxxxxxxx>;
> > Jun Nakajima <jun.nakajima@xxxxxxxxx>; Kevin Tian <kevin.tian@xxxxxxxxx>
> > Subject: [PATCH] x86/msr: Fix fallout from mostly c/s 832c180
> >
> > The series 832c1803^..f61685a6 was committed without adequate review.
> >
> 
> v1 of the series was posted on 7th Jan and v4 on 14th March. It was committed 
> yesterday. I'd say that
> there was certainly adequate time for review.
> 
> >  * Fix the shim build by providing a !CONFIG_HVM declaration for
> >    hvm_get_guest_bndcfgs()
> >  * Revert the bogus de-const'ing of the vcpu pointer in
> >    vmx_get_guest_bndcfgs().  vmx_vmcs_enter() really does mutate the vcpu, 
> > and
> >    may cause it to undergo a full de/reschedule, which is in violation of 
> > the
> >    ABI described by hvm_get_guest_bndcfgs().  guest_rdmsr() was always going
> >    to need to lose its const parameter, and this was the correct time for it
> >    to happen.
> 
> In the case of vmx_get_guest_bndcfgs() is there actually possibility of a 
> re-schedule? It's either
> going to be in current context (in which case IIUC vmx_vmcs_enter() is going 
> to be largely a no-op) or
> the vcpu in question should have already been paused. I'm no

^^^ that got truncated thanks to exchange going bye bye... I was just about to 
say I was no particular fan of the de-consting trick but the mail thread was:

-----
>> From: Xen-devel [mailto:xen-devel-bounces@xxxxxxxxxxxxxxxxxxxx] On Behalf Of 
>> Jan Beulich
>> Sent: 13 March 2019 15:55
>> 
>> >>> On 11.03.19 at 19:09, <paul.durrant@xxxxxxxxxx> wrote:
>> > --- 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.

I think it should be in VMX code, as that's where the
vmx_vmcs_enter() oddity lives.
-----

So you can see Jan asked for other opinions at the time. None were forthcoming. 
I therefore re-coded as he requested.

> 
> >  * Remove the introduced ASSERT(is_hvm_domain(d)) and check the predicate
> >    directly.  While we expect it to be true, the result is potential type
> >    confusion in release builds based on several subtle aspects of the CPUID
> >    feature derivation logic with no other safety checks.  This also fixes 
> > the
> >    a linker error in the release build of the shim, again for !CONFIG_HVM
> >    reasons.
> 
> Again, digging back in mail...
> 
> -----
> [From Jan]
> > +    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.
> -----
> 
> ...and I do recall asking for your opinion at the time. I guess you changed 
> your mind.
> 
> >  * The MSRs in vcpu_msrs are in numeric order.  Re-position XSS to match.
> >
> 
> That was not at all obvious. If this is the case then there should be comment 
> above the declaration of
> struct vcpu_msrs.
> 
> > Signed-off-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
> 
> However, the changes look ok to me and brings things closer to my earlier 
> code anyway so, with the
> comment requested above added...
> 
> Reviewed-by: Paul Durrant <paul.durrant@xxxxxxxxxx>

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