Re: [Qemu-devel] [RFC 0/2] GET_EMULATED_CPUID support with "allow-emulation" option

[Eduardo Habkost]

On Thu, Jun 05, 2014 at 06:40:25PM +0200, Alexander Graf wrote:
> 
> On 05.06.14 18:26, Paolo Bonzini wrote:
> >Il 05/06/2014 18:24, Alexander Graf ha scritto:
> >>
> >>On 05.06.14 18:12, Eduardo Habkost wrote:
> >>>This implements GET_SUPPORTED_CPUID support using an explicit option
> >>>for it:
> >>>"allow-emulation". We don't want any emulated feature to be enabled by
> >>>accident,
> >>>so they will be enabled only if the user explicitly wants to
> >>>allow them.
> >>
> >>So is this an all-or-nothing approach? I would really prefer to override
> >>individual bits.
> >
> >You can still disable them with "cpu foo,-movbe,allow-emulation".
> >
> >>Also, I don't think the line "emulated" is the right one to draw. We
> >>"emulate" SVM or VMX too, but still enable them by default as soon as we
> >>think they're ready enough.
> >
> >Well, I disagreed with the whole KVM_GET_EMULATED_CPUID concept for
> >MOVBE too for example.  It seemed overengineered to me, sooner or
> >later we might graduate MOVBE out of KVM_GET_EMULATED_CPUID as
> >well.
> >
> >However, for MONITOR/MWAIT it makes some sense.
> 
> I honestly think what we want for MONITOR/MWAIT is a cpuid-override bit.
> 
>   cpuid = user_specified_cpuids();
>   cpuid &= kvm_capable_cpuids()
>   cpuid |= user_override_cpuids();
> 
>   kvm_set_cpuid(cpuid);

Note that the above is not how it works today. It works this way:

    requested_cpuid = cpu_model_cpuids(cpu_model);
    requested_cpuid |= user_enabled_flags();
    requested_cpuid &= ~user_disabled_flags();
    possible_cpuid = cpuid & kvm_capable_cpuids();
    if (possible_cpuid != requested_cpuid) {
          if (check || enforce) fprintf(stderr, "some features are not 
available. help!");
          if (enforce) exit(1);
          cpu.filtered_features = cpuid & ~possible_cpuid;
    }
    cpu.cpuids = possible_cpuid;

This patch only affects the result of kvm_capable_cpuids(), only.

The semantics of a "-cpu" option is completely defined by
requested_cpuid. And this is not changing. The only way you can have a
different result depending on GET_SUPPORTED_CPUID or GET_EMULATE_CPUID,
is if you are _not_ using the "enforce" flag. But you should be using
it, if you care about stable CPUID data.

> 
> If the user knows what he's doing, he can set the force bit. If the
> kernel happens to emulate that instruction, he's happy. If the kernel
> doesn't emulate it, it will fail and he will realize that he did
> something stupid.

If the user knows what he's doing, he simply sets allow-emulation, which
will _not_ affect requested_cpuid.

> 
> But ok, we do have this awesome GET_EMULATE_CPUID ioctl now, so we
> can as well use it - even though I consider it superfluous:
> 
>   cpuid = user_specified_cpuids();
>   cpuid &= kvm_capable_cpuids()
>   cpuid |= user_override_cpuids() & kvm_emulated_cpuid();
> 
>   kvm_set_cpuid(cpuid);
> 
> but enabling all experimental features inside KVM just because we
> want one or two of them is very counter-intuitive. Imagine we'd
> introduce emulation support for AVX. Suddenly allow-emulation (which
> I'd need for Mac OS X 10.5) would enable AVX as well which I really
> don't want enabled.

If allow-emulation can suddenly enable a feature you don't want, your
command-line is already broken because changes to GET_SUPPORTED_CPUID
could also break your setup.

> 
> Also, while we can't change the ioctl name anymore, please let's use
> "experimental" rather than "emulated" as the name everywhere. Maybe
> we'll never bump individual features from experimental to fully
> supported, but there's no reason we wouldn't have emulated features
> that are not part of this bitmap and there's no reason we wouldn't
> have real hardware features that are not ready yet and part of this
> bitmap.

Agreed. The main point of GET_EMULATED_CPUID is to report features we
will never want to enable accidentally. Calling them "experimental"
makes sense to me.

-- 
Eduardo