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

[Igor Mammedov <imammedo@redhat.com>]

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