Re: [Qemu-devel] [PATCH] i386: Allow monitor / mwait cpuid override

[Eduardo Habkost]

On Mon, Mar 27, 2017 at 06:42:49PM +0200, Alexander Graf wrote:
> 
> 
> On 27/03/2017 18:33, Eduardo Habkost wrote:
> > On Mon, Mar 27, 2017 at 06:10:43PM +0200, Alexander Graf wrote:
> > > 
> > > 
> > > On 27/03/2017 17:46, Eduardo Habkost wrote:
> > > > On Mon, Mar 27, 2017 at 04:26:50PM +0200, Alexander Graf wrote:
> > > > > KVM allows trap and emulate (read: NOP) of the MONITOR and MWAIT
> > > > > instructions. There is work undergoing to enable actual execution
> > > > > of these inside of KVM, but nobody really wants to expose the feature
> > > > > to the guest by default, as it would eat up all of the host CPU.
> > > > 
> > > > Isn't this something that should be reported using
> > > > KVM_GET_EMULATED_CPUID? (QEMU still doesn't know how to use
> > > > KVM_GET_EMULATED_CPUID, however.)
> > > 
> > > Depends how you look at it. In KVM land there are basically 3 ways to deal
> > > with MONITOR/MWAIT:
> > > 
> > >   1) #VMEXIT on every execution, treat them as NOP
> > >   2) let the guest natively execute them (looks like a busy loop for the
> > > host, but saves power)
> > >   3) be smart in KVM about it, add actual emulation and adaptively allow 
> > > for
> > > native mwait execution or emulated mwait which means we can run inside 
> > > host
> > > context
> > 
> > Which case is going to be enabled when using your patch? Don't
> > you want to make that configurable?

If the answer to this question is "no", then we can ignore all
the rest of my previous message. :)

> > 
> > This looks like configuration that can't be easily represented
> > using the GET_SUPPORTED_CPUID/KVM_SET_CPUID abstraction, as the
> > ability to enable the feature can't be represented by a single
> > bit.
> > 
> > We already have a few cases where we make
> > kvm_arch_get_supported_cpuid() look at something other than just
> > GET_SUPPORTED_CPUID return values (most cases are related to
> > in-kernel irqchip). We can change kvm_arch_get_supported_cpuid()
> > to look at other flags (like one that would configure mwait
> > behavior), and decide to report CPUID_EXT_MONITOR on those cases.
> > 
> > Would something like the following make sense?
> > 
> > * Having a "mwait=(none|nop|native|smart)" option, to choose
> >   mwait behavior
> > * Making kvm_arch_get_supported_cpuid() return CPUID_EXT_MONITOR
> >   as supported only if the "mwait" option is not "none".
> > 
> > Then:
> > * "-cpu ...,monitor=on" would fail, because mwait=none would be
> >   the default
> > * "-cpu ...,mwait=none,monitor=on" would fail
> > * "-cpu ...,mwait=(nop|native|smart)" would fail if
> >   host KVM doesn't report the corresponding mwait behavior as
> >   supported
> > * "-cpu ...,mwait=(nop|native|smart),monitor=on" would work
> > * "-cpu ...,mwait=(nop|native|smart)" would also work, and
> >   probably should enable CPUID_EXT_MONITOR flag by default
> > 
> > We could still implement "monitor=force" for debugging, but I
> > think we need something safer than "just ignore what KVM is
> > telling us" for production.
> 
> I personally think we want the =force for any cpuid bit that we want to
> randomly poke into the expose CPUID bitmap and if a customer is daring
> enough, leave that to him as an option.
> 
> For anything else, I wouldn't overengineer this interface. I think
> eventually we want to just natively support emulated mwait with proper
> adaptive logic on when to trap vs execute mwait inside the guest (as
> mentioned in the kvm thread on this). Once that's implemented, KVM can just
> expose the MONITOR bit as supported and everyone gets it automatically.
> 
> If at that point, the adaptive interface performs much worse than the naive
> one, we can still think about pushing this into an enable_cap and add a
> specific mwait= option.

Sounds good to me.

I thought the user still had to choose between the options above
somehow, and that KVM was not going to return MONITOR on
GET_SUPPORTED_CPUID because of that. If getting adaptative logic
that's good enough to add MONITOR to GET_SUPPORTED_CPUID is still
a possibility, then that's even better.

-- 
Eduardo