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

[Eduardo Habkost]

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

> 
> So today there is no streamlined way to actually notify the guest that
> it's ok to execute MONITOR / MWAIT, even when we want to explicitly
> leave the guest in guest context.

I'm not familiar with the variables involved in this decision.
How exactly would somebody (human or software) determine if it's
really ok to let the guest execute MONITOR / MWAIT?

Under what circumstances do you expect this to be used? Is this
just for debugging and development?

> 
> This patch adds a new -cpu parameter called "mwait" which - when
> enabled - force enables the MONITOR / MWAIT CPUID flag, even when
> the underlying accel framework does not explicitly advertise support.
> 

If you really want something that makes QEMU ignore what the
accel code is reporting, I would prefer a syntax that could be
used for other features too, like "-cpu ...,monitor=force".

> With that in place, we can explicitly allow users to specify that
> they want have the guest execute MONITOR / MWAIT in its idle loop.
> 
> Signed-off-by: Alexander Graf <address@hidden>
> ---
>  target/i386/cpu.c | 5 +++++
>  target/i386/cpu.h | 1 +
>  2 files changed, 6 insertions(+)
> 
> diff --git a/target/i386/cpu.c b/target/i386/cpu.c
> index 7aa7622..c44020b 100644
> --- a/target/i386/cpu.c
> +++ b/target/i386/cpu.c
> @@ -3460,6 +3460,10 @@ static int x86_cpu_filter_features(X86CPU *cpu)
>              x86_cpu_get_supported_feature_word(w, false);
>          uint32_t requested_features = env->features[w];
>          env->features[w] &= host_feat;
> +        if (cpu->expose_monitor && (w == FEAT_1_ECX)) {
> +            /* Force monitor feature in */
> +            env->features[w] |= CPUID_EXT_MONITOR;
> +        }
>          cpu->filtered_features[w] = requested_features & ~env->features[w];
>          if (cpu->filtered_features[w]) {
>              rv = 1;
> @@ -3988,6 +3992,7 @@ static Property x86_cpu_properties[] = {
>      DEFINE_PROP_BOOL("check", X86CPU, check_cpuid, true),
>      DEFINE_PROP_BOOL("enforce", X86CPU, enforce_cpuid, false),
>      DEFINE_PROP_BOOL("kvm", X86CPU, expose_kvm, true),
> +    DEFINE_PROP_BOOL("mwait", X86CPU, expose_monitor, false),
>      DEFINE_PROP_UINT32("phys-bits", X86CPU, phys_bits, 0),
>      DEFINE_PROP_BOOL("host-phys-bits", X86CPU, host_phys_bits, false),
>      DEFINE_PROP_BOOL("fill-mtrr-mask", X86CPU, fill_mtrr_mask, true),
> diff --git a/target/i386/cpu.h b/target/i386/cpu.h
> index 07401ad..7400d00 100644
> --- a/target/i386/cpu.h
> +++ b/target/i386/cpu.h
> @@ -1214,6 +1214,7 @@ struct X86CPU {
>      bool check_cpuid;
>      bool enforce_cpuid;
>      bool expose_kvm;
> +    bool expose_monitor;
>      bool migratable;
>      bool max_features; /* Enable all supported features automatically */
>      uint32_t apic_id;
> -- 
> 1.8.5.6
> 

-- 
Eduardo