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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 24/05/18 16:08, Jan Beulich wrote:
>>>> On 22.05.18 at 13:20, <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.
>>
>> On VMExit, hardware automatically resets MSR_DEBUGCTL to 0.
> I'm pretty certain that back when I write this, the SDM didn't spell this out.

I wondered as much.  Gen-1 VT-x is well documented as forcing the
VM_EXIT_SAVE_DEBUG_CTLS control to be set in non-root operation, which
clearly suggests that it has always had this behaviour.

>
>>  The only case
>> where different behaviour is needed is if Xen is debugging itself, and this
>> needs setting up unconditionally for the lifetime of the VM.
>>
>> The `ler` command line boolean is the only way to configure any use of
>> MSR_DEBUGCTL for Xen, so tie the host load list entry to this setting in
>> construct_vmcs().  Any runtime update of Xen's MSR_DEBUGCTL setting requires
>> more complicated synchronisation across all the running VMs.
>>
>> In the exceedingly common case, this avoids the unnecessary overhead of 
>> having
>> a host load entry performing the same zeroing operation that hardware has
>> already performed as part of the VMExit.
>>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
> Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>
>
>> Notes for backporting: This change probably does want backporting, but 
>> depends
>> on the previous patch "Support remote access to the MSR lists", and adds an
>> extra rdmsr to the vcpu construction path (resolved in a later patch).
> I wonder if for backporting purposes we couldn't stick this function
> invocation somewhere else, like vmx_ctxt_switch_to() or
> vmx_do_resume(). I realize the potential allocation failure is a problem here,
> but for that we could either allocate the memory at the place you now
> invoke vmx_add_host_load_msr(), or take the brute force approach and
> crash the domain upon failure (the net effect won't be much different to
> allocation failing during domain destruction - the domain won't start in 
> either
> case).
>
> I mention this because it seems to me that pulling in the previous patch
> would in turn require pulling in earlier ones.

Yeah - to backport this change, you need 6 patches from the series.

That said, I think I've come up with a much safer, faster, alternative
which I can disentangle completely from this series.

I was already planning to clean up the host debugctl handling to have a
single read_mostly value.  With a trivial alternative block in the vmx
vmexit handler, we can do away with the host load entry entirely (and,
as I've been reliably informed, doing things this way is faster than
having microcode walk the host/guest load/save lists).

~Andrew

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