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

[Andrew Cooper]

On 17/04/18 13:45, Jan Beulich wrote:
 On 17.04.18 at 14:30,  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 
>>>
>>> --- 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 

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

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

[Juergen Gross]

On 17/04/18 13: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 

Release-acked-by: Juergen Gross 


Juergen

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

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

[Jan Beulich]

>>> On 17.04.18 at 14:30,  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 
>>
>> --- 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@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

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

[Andrew Cooper]

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

~Andrew

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

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

[Jan Beulich]

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 

--- 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);
 break;
 
 case MSR_INTEL_MISC_FEATURES_ENABLES:



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