[v4,04/19] target/i386/cpu: Remove X86CPU::check_cpuid field

The X86CPU::check_cpuid boolean was only set in the
pc_compat_2_4[] array, via the 'check=off' property.
We removed all machines using that array, lets remove
that CPU property and simplify x86_cpu_realizefn().

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
Reviewed-by: Zhao Liu <zhao1.liu@intel.com>
---
 target/i386/cpu.h | 1 -
 target/i386/cpu.c | 3 +--
 2 files changed, 1 insertion(+), 3 deletions(-)

On 5/12/2025 4:39 PM, Philippe Mathieu-Daudé wrote:
> The X86CPU::check_cpuid boolean was only set in the
> pc_compat_2_4[] array, via the 'check=off' property.
> We removed all machines using that array, lets remove
> that CPU property and simplify x86_cpu_realizefn().

No.

We cannot do this. Because it changes the behavior of QEMU.

'check_cpuid' is true by default while 'enforce_cpuid' is false. So that 
QEMU emits warnings in x86_cpu_filter_features() by default when user 
requests unsupported CPU features. If remove "check" property and the 
internal 'check_cpuid', QEMU will not do it unless user sets 
enforce_cpuid explicitly.

> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
> Reviewed-by: Zhao Liu <zhao1.liu@intel.com>
> ---
>   target/i386/cpu.h | 1 -
>   target/i386/cpu.c | 3 +--
>   2 files changed, 1 insertion(+), 3 deletions(-)
> 
> diff --git a/target/i386/cpu.h b/target/i386/cpu.h
> index 4f8ed8868e9..0db70a70439 100644
> --- a/target/i386/cpu.h
> +++ b/target/i386/cpu.h
> @@ -2164,7 +2164,6 @@ struct ArchCPU {
>       uint8_t hyperv_ver_id_sb;
>       uint32_t hyperv_ver_id_sn;
>   
> -    bool check_cpuid;
>       bool enforce_cpuid;
>       /*
>        * Force features to be enabled even if the host doesn't support them.
> diff --git a/target/i386/cpu.c b/target/i386/cpu.c
> index 1ca6307c72e..cd4361b4227 100644
> --- a/target/i386/cpu.c
> +++ b/target/i386/cpu.c
> @@ -8174,7 +8174,7 @@ static void x86_cpu_realizefn(DeviceState *dev, Error **errp)
>           }
>       }
>   
> -    if (x86_cpu_filter_features(cpu, cpu->check_cpuid || cpu->enforce_cpuid)) {
> +    if (x86_cpu_filter_features(cpu, cpu->enforce_cpuid)) {
>           if (cpu->enforce_cpuid) {
>               error_setg(&local_err,
>                          accel_uses_host_cpuid() ?
> @@ -8813,7 +8813,6 @@ static const Property x86_cpu_properties[] = {
>       DEFINE_PROP_UINT8("hv-version-id-sbranch", X86CPU, hyperv_ver_id_sb, 0),
>       DEFINE_PROP_UINT32("hv-version-id-snumber", X86CPU, hyperv_ver_id_sn, 0),
>   
> -    DEFINE_PROP_BOOL("check", X86CPU, check_cpuid, true),
>       DEFINE_PROP_BOOL("enforce", X86CPU, enforce_cpuid, false),
>       DEFINE_PROP_BOOL("x-force-features", X86CPU, force_features, false),
>       DEFINE_PROP_BOOL("kvm", X86CPU, expose_kvm, true),

On Wed, May 28, 2025 at 10:09:56AM +0800, Xiaoyao Li wrote:
> Date: Wed, 28 May 2025 10:09:56 +0800
> From: Xiaoyao Li <xiaoyao.li@intel.com>
> Subject: Re: [PATCH v4 04/19] target/i386/cpu: Remove X86CPU::check_cpuid
>  field
> 
> On 5/12/2025 4:39 PM, Philippe Mathieu-Daudé wrote:
> > The X86CPU::check_cpuid boolean was only set in the
> > pc_compat_2_4[] array, via the 'check=off' property.
> > We removed all machines using that array, lets remove
> > that CPU property and simplify x86_cpu_realizefn().
> 
> No.
> 
> We cannot do this. Because it changes the behavior of QEMU.
> 
> 'check_cpuid' is true by default while 'enforce_cpuid' is false. So that
> QEMU emits warnings in x86_cpu_filter_features() by default when user
> requests unsupported CPU features. If remove "check" property and the
> internal 'check_cpuid', QEMU will not do it unless user sets enforce_cpuid
> explicitly.

One option would be to have x86_cpu_filter_features() unconditionally
turn on verbose and print warnings, but some people might want to turn
off these warning prints, I don't know if anyone would, but it would be
possible.

The other option is still to keep the “check” property.

IMO, the latter option is the better way to reduce Philippe's burden.

Regards,
Zhao

On Mon, 12 May 2025 10:39:33 +0200
Philippe Mathieu-Daudé <philmd@linaro.org> wrote:

> The X86CPU::check_cpuid boolean was only set in the
> pc_compat_2_4[] array, via the 'check=off' property.
> We removed all machines using that array, lets remove
> that CPU property and simplify x86_cpu_realizefn().
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
> Reviewed-by: Zhao Liu <zhao1.liu@intel.com>

Reviewed-by: Igor Mammedov <imammedo@redhat.com>

> ---
>  target/i386/cpu.h | 1 -
>  target/i386/cpu.c | 3 +--
>  2 files changed, 1 insertion(+), 3 deletions(-)
> 
> diff --git a/target/i386/cpu.h b/target/i386/cpu.h
> index 4f8ed8868e9..0db70a70439 100644
> --- a/target/i386/cpu.h
> +++ b/target/i386/cpu.h
> @@ -2164,7 +2164,6 @@ struct ArchCPU {
>      uint8_t hyperv_ver_id_sb;
>      uint32_t hyperv_ver_id_sn;
>  
> -    bool check_cpuid;
>      bool enforce_cpuid;
>      /*
>       * Force features to be enabled even if the host doesn't support them.
> diff --git a/target/i386/cpu.c b/target/i386/cpu.c
> index 1ca6307c72e..cd4361b4227 100644
> --- a/target/i386/cpu.c
> +++ b/target/i386/cpu.c
> @@ -8174,7 +8174,7 @@ static void x86_cpu_realizefn(DeviceState *dev, Error **errp)
>          }
>      }
>  
> -    if (x86_cpu_filter_features(cpu, cpu->check_cpuid || cpu->enforce_cpuid)) {
> +    if (x86_cpu_filter_features(cpu, cpu->enforce_cpuid)) {
>          if (cpu->enforce_cpuid) {
>              error_setg(&local_err,
>                         accel_uses_host_cpuid() ?
> @@ -8813,7 +8813,6 @@ static const Property x86_cpu_properties[] = {
>      DEFINE_PROP_UINT8("hv-version-id-sbranch", X86CPU, hyperv_ver_id_sb, 0),
>      DEFINE_PROP_UINT32("hv-version-id-snumber", X86CPU, hyperv_ver_id_sn, 0),
>  
> -    DEFINE_PROP_BOOL("check", X86CPU, check_cpuid, true),
>      DEFINE_PROP_BOOL("enforce", X86CPU, enforce_cpuid, false),
>      DEFINE_PROP_BOOL("x-force-features", X86CPU, force_features, false),
>      DEFINE_PROP_BOOL("kvm", X86CPU, expose_kvm, true),

On Wed, 28 May 2025 13:23:49 +0800
Zhao Liu <zhao1.liu@intel.com> wrote:

> On Wed, May 28, 2025 at 10:09:56AM +0800, Xiaoyao Li wrote:
> > Date: Wed, 28 May 2025 10:09:56 +0800
> > From: Xiaoyao Li <xiaoyao.li@intel.com>
> > Subject: Re: [PATCH v4 04/19] target/i386/cpu: Remove X86CPU::check_cpuid
> >  field
> > 
> > On 5/12/2025 4:39 PM, Philippe Mathieu-Daudé wrote:  
> > > The X86CPU::check_cpuid boolean was only set in the
> > > pc_compat_2_4[] array, via the 'check=off' property.
> > > We removed all machines using that array, lets remove
> > > that CPU property and simplify x86_cpu_realizefn().  
> > 
> > No.
> > 
> > We cannot do this. Because it changes the behavior of QEMU.
> > 
> > 'check_cpuid' is true by default while 'enforce_cpuid' is false. So that
> > QEMU emits warnings in x86_cpu_filter_features() by default when user
> > requests unsupported CPU features. If remove "check" property and the
> > internal 'check_cpuid', QEMU will not do it unless user sets enforce_cpuid
> > explicitly.  
> 
> One option would be to have x86_cpu_filter_features() unconditionally
> turn on verbose and print warnings, but some people might want to turn
> off these warning prints, I don't know if anyone would, but it would be
> possible.
> 
> The other option is still to keep the “check” property.
> 
> IMO, the latter option is the better way to reduce Philippe's burden.

we essentially loose warnings by default when some features aren't available,
qemu still continues to run though.

Given that Daniel acked it from libvirt side, libvirt doesn't care about warnings
(it does its has its own cpu model calculation). Likely other mgmt do not care
about it either, and if they do they probably doing something wrong and
should use QMP to get that data.
That leaves us with human users, for that case I'd say one should use
enforce_cpuid if feature availability matters.

so +1 to removal
 
> 
> Regards,
> Zhao
> 
>

On 6/3/2025 11:02 PM, Igor Mammedov wrote:
> On Wed, 28 May 2025 13:23:49 +0800
> Zhao Liu <zhao1.liu@intel.com> wrote:
> 
>> On Wed, May 28, 2025 at 10:09:56AM +0800, Xiaoyao Li wrote:
>>> Date: Wed, 28 May 2025 10:09:56 +0800
>>> From: Xiaoyao Li <xiaoyao.li@intel.com>
>>> Subject: Re: [PATCH v4 04/19] target/i386/cpu: Remove X86CPU::check_cpuid
>>>   field
>>>
>>> On 5/12/2025 4:39 PM, Philippe Mathieu-Daudé wrote:
>>>> The X86CPU::check_cpuid boolean was only set in the
>>>> pc_compat_2_4[] array, via the 'check=off' property.
>>>> We removed all machines using that array, lets remove
>>>> that CPU property and simplify x86_cpu_realizefn().
>>>
>>> No.
>>>
>>> We cannot do this. Because it changes the behavior of QEMU.
>>>
>>> 'check_cpuid' is true by default while 'enforce_cpuid' is false. So that
>>> QEMU emits warnings in x86_cpu_filter_features() by default when user
>>> requests unsupported CPU features. If remove "check" property and the
>>> internal 'check_cpuid', QEMU will not do it unless user sets enforce_cpuid
>>> explicitly.
>>
>> One option would be to have x86_cpu_filter_features() unconditionally
>> turn on verbose and print warnings, but some people might want to turn
>> off these warning prints, I don't know if anyone would, but it would be
>> possible.
>>
>> The other option is still to keep the “check” property.
>>
>> IMO, the latter option is the better way to reduce Philippe's burden.
> 
> we essentially loose warnings by default when some features aren't available,
> qemu still continues to run though.
> 
> Given that Daniel acked it from libvirt side, libvirt doesn't care about warnings
> (it does its has its own cpu model calculation). Likely other mgmt do not care
> about it either, and if they do they probably doing something wrong and
> should use QMP to get that data.
> That leaves us with human users, for that case I'd say one should use
> enforce_cpuid if feature availability matters.

But with "check", it allows the VM to continue running with the 
unsupported bits cleared and warnings to inform users. This is really 
friendly.

> so +1 to removal
>   
>>
>> Regards,
>> Zhao
>>
>>
>

On Tue, 3 Jun 2025 23:54:19 +0800
Xiaoyao Li <xiaoyao.li@intel.com> wrote:

> On 6/3/2025 11:02 PM, Igor Mammedov wrote:
> > On Wed, 28 May 2025 13:23:49 +0800
> > Zhao Liu <zhao1.liu@intel.com> wrote:
> >   
> >> On Wed, May 28, 2025 at 10:09:56AM +0800, Xiaoyao Li wrote:  
> >>> Date: Wed, 28 May 2025 10:09:56 +0800
> >>> From: Xiaoyao Li <xiaoyao.li@intel.com>
> >>> Subject: Re: [PATCH v4 04/19] target/i386/cpu: Remove X86CPU::check_cpuid
> >>>   field
> >>>
> >>> On 5/12/2025 4:39 PM, Philippe Mathieu-Daudé wrote:  
> >>>> The X86CPU::check_cpuid boolean was only set in the
> >>>> pc_compat_2_4[] array, via the 'check=off' property.
> >>>> We removed all machines using that array, lets remove
> >>>> that CPU property and simplify x86_cpu_realizefn().  
> >>>
> >>> No.
> >>>
> >>> We cannot do this. Because it changes the behavior of QEMU.
> >>>
> >>> 'check_cpuid' is true by default while 'enforce_cpuid' is false. So that
> >>> QEMU emits warnings in x86_cpu_filter_features() by default when user
> >>> requests unsupported CPU features. If remove "check" property and the
> >>> internal 'check_cpuid', QEMU will not do it unless user sets enforce_cpuid
> >>> explicitly.  
> >>
> >> One option would be to have x86_cpu_filter_features() unconditionally
> >> turn on verbose and print warnings, but some people might want to turn
> >> off these warning prints, I don't know if anyone would, but it would be
> >> possible.
> >>
> >> The other option is still to keep the “check” property.
> >>
> >> IMO, the latter option is the better way to reduce Philippe's burden.  
> > 
> > we essentially loose warnings by default when some features aren't available,
> > qemu still continues to run though.
> > 
> > Given that Daniel acked it from libvirt side, libvirt doesn't care about warnings
> > (it does its has its own cpu model calculation). Likely other mgmt do not care
> > about it either, and if they do they probably doing something wrong and
> > should use QMP to get that data.
> > That leaves us with human users, for that case I'd say one should use
> > enforce_cpuid if feature availability matters.  
> 
> But with "check", it allows the VM to continue running with the 
> unsupported bits cleared and warnings to inform users. This is really 
> friendly.

it's friendly for human users (mostly developers), but for upper layers
is doesn't make a difference, since it's noise in logs nobody reads until
qemu somehow works.

the 1st category can and should use enforce flag instead to get what they ask for
if they care about it.
If missing feature bits do not matter then it warnings shouldn't matter either.
What I'm advocating for is being more strict/deterministic on QEMU side,
  1. you get what you asked for with enforce or fix you CLI explicitly
     to be clear on what you are missing out.
  2. you don't care if some features are missing, but then you don't really care
     about warnings either. (I'd still get missing features filtered out though,
     just silently). If one cares about missing features, one can use #1

I admit, It's a tiny bit of code but removing it, cleans up code a little bit
and helps readability/in reviews. Doing such small cleanups here and there
have a cumulative effect on the codebase.

Anyways it's not something I'd fight for, so if you insist on keeping it
it's ok as well.

> > so +1 to removal
> >     
> >>
> >> Regards,
> >> Zhao
> >>
> >>  
> >   
>

On Tue, Jun 03, 2025 at 05:02:38PM +0200, Igor Mammedov wrote:
> On Wed, 28 May 2025 13:23:49 +0800
> Zhao Liu <zhao1.liu@intel.com> wrote:
> 
> > On Wed, May 28, 2025 at 10:09:56AM +0800, Xiaoyao Li wrote:
> > > Date: Wed, 28 May 2025 10:09:56 +0800
> > > From: Xiaoyao Li <xiaoyao.li@intel.com>
> > > Subject: Re: [PATCH v4 04/19] target/i386/cpu: Remove X86CPU::check_cpuid
> > >  field
> > > 
> > > On 5/12/2025 4:39 PM, Philippe Mathieu-Daudé wrote:  
> > > > The X86CPU::check_cpuid boolean was only set in the
> > > > pc_compat_2_4[] array, via the 'check=off' property.
> > > > We removed all machines using that array, lets remove
> > > > that CPU property and simplify x86_cpu_realizefn().  
> > > 
> > > No.
> > > 
> > > We cannot do this. Because it changes the behavior of QEMU.
> > > 
> > > 'check_cpuid' is true by default while 'enforce_cpuid' is false. So that
> > > QEMU emits warnings in x86_cpu_filter_features() by default when user
> > > requests unsupported CPU features. If remove "check" property and the
> > > internal 'check_cpuid', QEMU will not do it unless user sets enforce_cpuid
> > > explicitly.  
> > 
> > One option would be to have x86_cpu_filter_features() unconditionally
> > turn on verbose and print warnings, but some people might want to turn
> > off these warning prints, I don't know if anyone would, but it would be
> > possible.
> > 
> > The other option is still to keep the “check” property.
> > 
> > IMO, the latter option is the better way to reduce Philippe's burden.
> 
> we essentially loose warnings by default when some features aren't available,
> qemu still continues to run though.
> 
> Given that Daniel acked it from libvirt side, libvirt doesn't care about warnings
> (it does its has its own cpu model calculation). Likely other mgmt do not care
> about it either, and if they do they probably doing something wrong and
> should use QMP to get that data.

Acking it was a mistake on my part - I mis-interpreted the patch and so
didn't notice we were loosing the verbose printing of missing features
by default.

I'm actually curious why we made the 'check' feature tied to machine
types at all. If it doesn't affect guest ABI, just causes verbose
info on stderr, it feels like something we could have just had on
all machine types new & old. Git history brings us back to

  commit 3e68482224129c3ddc061af7c9d438b882ecfdd1
  Author: Eduardo Habkost <ehabkost@redhat.com>
  Date:   Tue Nov 3 17:18:50 2015 -0200

    target-i386: Set "check=off" by default on pc-*-2.4 and older
    
    The default CPU model (qemu64) have some issues today: it enables some
    features (ABM and SSE4a) that are not present in many host CPUs. That
    means many hosts (but not all of them) had those features silently
    disabled in the default configuration in QEMU 2.4 and older.
    
    With the new "check=on" default, this causes warnings to be printed in
    the default configuration, because of the lack of SSE4A on all Intel
    hosts, and the lack of ABM on Sandy Bridge and older hosts:
    
      $ qemu-system-x86_64 -machine pc,accel=kvm
      warning: host doesn't support requested feature: CPUID.80000001H:ECX.abm [bit 5]
      warning: host doesn't support requested feature: CPUID.80000001H:ECX.sse4a [bit 6]
    
    Those issues will be fixed in pc-*-2.5 and newer. But as we can't change
    the guest ABI in pc-*-2.4, disable "check" mode by default in pc-*-2.4
    and older so we don't print spurious warnings.

IOW, we wanted to have 'check' unconditionally on by default, but
had to do a temp hack to avoid spamming all configurations with
the broken 'qemu64' CPU model design.

> That leaves us with human users, for that case I'd say one should use
> enforce_cpuid if feature availability matters.

IMHO even with mgmt apps, it is worth having 'check=on' by default
as the log message has value in debugging scenarios. It could have
the potential to highlight situations where an mgmt app has
unwittingly done something wrong with CPU config. At the very least
though its a warning to humans debugging that they should not trust
the QEMU command line as a expressing the full CPU featureset.

With regards,
Daniel

On Thu, Jun 05, 2025 at 11:09:30AM +0200, Igor Mammedov wrote:
> On Tue, 3 Jun 2025 23:54:19 +0800
> Xiaoyao Li <xiaoyao.li@intel.com> wrote:
> 
> > On 6/3/2025 11:02 PM, Igor Mammedov wrote:
> > > On Wed, 28 May 2025 13:23:49 +0800
> > > Zhao Liu <zhao1.liu@intel.com> wrote:
> > >   
> > >> On Wed, May 28, 2025 at 10:09:56AM +0800, Xiaoyao Li wrote:  
> > >>> Date: Wed, 28 May 2025 10:09:56 +0800
> > >>> From: Xiaoyao Li <xiaoyao.li@intel.com>
> > >>> Subject: Re: [PATCH v4 04/19] target/i386/cpu: Remove X86CPU::check_cpuid
> > >>>   field
> > >>>
> > >>> On 5/12/2025 4:39 PM, Philippe Mathieu-Daudé wrote:  
> > >>>> The X86CPU::check_cpuid boolean was only set in the
> > >>>> pc_compat_2_4[] array, via the 'check=off' property.
> > >>>> We removed all machines using that array, lets remove
> > >>>> that CPU property and simplify x86_cpu_realizefn().  
> > >>>
> > >>> No.
> > >>>
> > >>> We cannot do this. Because it changes the behavior of QEMU.
> > >>>
> > >>> 'check_cpuid' is true by default while 'enforce_cpuid' is false. So that
> > >>> QEMU emits warnings in x86_cpu_filter_features() by default when user
> > >>> requests unsupported CPU features. If remove "check" property and the
> > >>> internal 'check_cpuid', QEMU will not do it unless user sets enforce_cpuid
> > >>> explicitly.  
> > >>
> > >> One option would be to have x86_cpu_filter_features() unconditionally
> > >> turn on verbose and print warnings, but some people might want to turn
> > >> off these warning prints, I don't know if anyone would, but it would be
> > >> possible.
> > >>
> > >> The other option is still to keep the “check” property.
> > >>
> > >> IMO, the latter option is the better way to reduce Philippe's burden.  
> > > 
> > > we essentially loose warnings by default when some features aren't available,
> > > qemu still continues to run though.
> > > 
> > > Given that Daniel acked it from libvirt side, libvirt doesn't care about warnings
> > > (it does its has its own cpu model calculation). Likely other mgmt do not care
> > > about it either, and if they do they probably doing something wrong and
> > > should use QMP to get that data.
> > > That leaves us with human users, for that case I'd say one should use
> > > enforce_cpuid if feature availability matters.  
> > 
> > But with "check", it allows the VM to continue running with the 
> > unsupported bits cleared and warnings to inform users. This is really 
> > friendly.
> 
> it's friendly for human users (mostly developers), but for upper layers
> is doesn't make a difference, since it's noise in logs nobody reads until
> qemu somehow works.

The latter statement is accurate. In the libvirt case, this info ends up
in the per-VM logfile, which is one of the key files humans look at first
when triaging any QEMU related bug reports.

> 
> the 1st category can and should use enforce flag instead to get what they ask for
> if they care about it.
> If missing feature bits do not matter then it warnings shouldn't matter either.

I don't think that's correct. The mgmt app can decide the missing features
don't matter, but at the same time someone looking at the QEMU command line
doesn't know the mgmt apps' intent and it is easy to overlook that QEMU will
silently drop features. So the precense of the warnings in the log alerts the
person debugging that the actual QEMU guest visible CPU is subtly different
from what's on the command line, and that's useful IMHO for troubleshooting.

> What I'm advocating for is being more strict/deterministic on QEMU side,
>   1. you get what you asked for with enforce or fix you CLI explicitly
>      to be clear on what you are missing out.
>   2. you don't care if some features are missing, but then you don't really care
>      about warnings either. (I'd still get missing features filtered out though,
>      just silently). If one cares about missing features, one can use #1
> 
> I admit, It's a tiny bit of code but removing it, cleans up code a little bit
> and helps readability/in reviews. Doing such small cleanups here and there
> have a cumulative effect on the codebase.
> 
> Anyways it's not something I'd fight for, so if you insist on keeping it
> it's ok as well.

With regards,
Daniel