Re: [Xen-devel] [PATCH 1/6] x86/vmx: Fix handing of MSR_DEBUGCTL on VMExit

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 30/05/18 08:32, Jan Beulich wrote:
>>>> On 29.05.18 at 20:08, <andrew.cooper3@xxxxxxxxxx> wrote:
>> On 29/05/18 11:33, Jan Beulich wrote:
>>>>>> On 28.05.18 at 16:27, <andrew.cooper3@xxxxxxxxxx> wrote:
>>>> Currently, whenever the guest writes a nonzero value to MSR_DEBUGCTL, Xen
>>>> updates a host MSR load list entry with the current hardware value of
>>>> MSR_DEBUGCTL.  This is wrong.
>>> "This is wrong" goes too far for my taste: It is not very efficient to do 
>>> it that
>>> way, but it's still correct. Unless, of course, the zeroing of the register
>>> happens after the processing of the MSR load list (which I doubt it does).
>> It is functionally broken.  Restoration of Xen's debugging setting must
>> happen from the first vmexit, not the first vmexit after the guest plays
>> with MSR_DEBUGCTL.
>>
>> With the current behaviour, Xen looses its MSR_DEBUGCTL setting on any
>> pcpu where an HVM guest has been scheduled, and then feeds the current
>> value (0) into the host load list, even when it was attempting to set a
>> non-zero value.
> Oh, indeed, you're right.

I've rewritten this bit of the commit message.  How about:

Currently, whenever the guest writes a nonzero value to MSR_DEBUGCTL, Xen
updates a host MSR load list entry with the current hardware value of
MSR_DEBUGCTL.

On VMExit, hardware automatically resets MSR_DEBUGCTL to 0.  Later, when the
guest writes to MSR_DEBUGCTL, the current value in hardware (0) is fed back
into guest load list.  As a practical result, `ler` debugging gets lost on any
PCPU which has ever scheduled an HVM vcpu, and the common case when `ler`
debugging isn't active, guest actions result in an unnecessary load list entry
repeating the MSR_DEBUGCTL reset.

Restoration of Xen's debugging setting needs to happen from the very first
vmexit.  Due to the automatic reset, Xen need take no action in the general
case, and only needs to load a value when debugging is active.

>>>> +void percpu_traps_init(void)
>>>> +{
>>>> +    subarch_percpu_traps_init();
>>>> +
>>>> +    if ( !opt_ler )
>>>> +        return;
>>>> +
>>>> +    if ( !ler_msr && (ler_msr = calc_ler_msr()) )
>>>> +        setup_force_cpu_cap(X86_FEATURE_XEN_LBR);
>>> This does not hold up with the promise the description makes: If running
>>> on an unrecognized model, calc_ler_msr() is going to be called more than
>>> once. If it really was called just once, it could also become __init. With
>>> the inverted sense of the feature flag (as suggested above) you could
>>> check whether the flag bit is set or ler_msr is non-zero.
>> Hmm - I suppose it doesn't quite match the description, but does it
>> matter (if I tweak the description)?  It is debugging functionality, and
>> I don't see any 64bit models missing from the list.
> Non-Intel, non-AMD CPUs are clearly missing. We have Centaur (VIA)
> support, and we're going to gain support for one more right after the
> tree was branched for 4.11.

Ok, but all of this is behind !opt_ler which means it doesn't get
executed in the general case.

~Andrew

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