[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH 1/6] x86/vmx: Fix handing of MSR_DEBUGCTL on VMExit
>>> On 30.05.18 at 12:28, <andrew.cooper3@xxxxxxxxxx> wrote: > 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. 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@xxxxxxxxxxxxxxxxxxxx https://lists.xenproject.org/mailman/listinfo/xen-devel
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |