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

["Jan Beulich" <JBeulich@xxxxxxxx>]

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

Jan



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