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

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

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

>>> @@ -1920,38 +1910,46 @@ void load_TR(void)
>>>          : "=m" (old_gdt) : "rm" (TSS_ENTRY << 3), "m" (tss_gdt) : "memory" );
>>>  }
>>>  
>>> -void percpu_traps_init(void)
>>> +static uint32_t calc_ler_msr(void)
>> Here and elsewhere "unsigned int" would be more appropriate to use.
>> We don't require MSR indexes to be exactly 32 bits wide, but only at
>> least as wide.
> 
> MSR indices are architecturally 32 bits wide.

Correct. Hence for communicating such values between functions we need
a type at least as wide as 32 bits, not exactly as wide. There's a reason the
standard also defined uint_least32_t et al; it's just that I think unsigned int
is less ugly to read and fulfills the same purpose (with the prereq that we
assume to run only on architectures where unsigned int is at least 32 bits
wide, just like we make an even more strict assumption on unsigned long).

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

Jan



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