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

[Paul Durrant <Paul.Durrant@xxxxxxxxxx>]

> -----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

>  * 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