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

["Jan Beulich" <JBeulich@suse.com>]

>>> On 30.05.18 at 12:28, <andrew.cooper3@citrix.com> wrote:
> On 30/05/18 08:32, Jan Beulich wrote:
>>>>> On 29.05.18 at 20:08, <andrew.cooper3@citrix.com> wrote:
>>> On 29/05/18 11:33, Jan Beulich wrote:
>>>>>>> On 28.05.18 at 16:27, <andrew.cooper3@citrix.com> 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.

Thanks, this makes things more explicit imo.

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

Of course.

Jan



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