[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Xen-devel] [PATCH] x86/msr: further correct the emulation behaviour of MSR_PRED_CMD

On 17/04/18 13:45, Jan Beulich wrote:
>>>> On 17.04.18 at 14:30, <andrew.cooper3@xxxxxxxxxx> wrote:
>> On 17/04/18 12:41, Jan Beulich wrote:
>>> Following commit a6aa678fa3 ("x86/msr: Correct the emulation behaviour
>>> of MSR_PRED_CMD") we may end up writing the low bit with the wrong
>>> value. While it's unlikely for a guest to want to write zero there, we
>>> should still permit (this without incurring the overhead of an actual
>>> barrier). Correcting this right away will also help whenever further
>>> bits in the MSR might become defined.
>>> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
>>> --- a/xen/arch/x86/msr.c
>>> +++ b/xen/arch/x86/msr.c
>>> @@ -247,7 +247,7 @@ int guest_wrmsr(struct vcpu *v, uint32_t
>>>              goto gp_fault; /* Rsvd bit set? */
>>>          if ( v == curr )
>>> -            wrmsrl(MSR_PRED_CMD, PRED_CMD_IBPB);
>>> +            wrmsrl(MSR_PRED_CMD, val);
>> I was on the fence about making this change, because if the reserved bit
>> testing happens to be wrong, we might suffer a fatal #GP here.
>> Then again, the same could be said of the the CPUID check and explicit
>> use of PRED_CMD_IBPB.
>> I also wondered if we would be better using wrmsr_safe() to cope better
>> in release situations, where at least bad logic here would result in
>> host crash.
> Any of this likely would equally be an issue for some other MSRs,
> and I think that's orthogonal to the change (with the given description)
> here: It is clearly wrong to write bit 0 with 1 when the original guest
> value has the bit clear. If anything I could agree to writing
> val & PRED_CMD_IBPB, but that's then obviously redundant with the
> check immediately ahead of the write.

The only time we will see 0 here is in my XTF test cases, but I accept
your point.

Acked-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>

Xen-devel mailing list



Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.