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

[Igor Mammedov <imammedo@redhat.com>]

On Thu, 5 Jun 2025 10:24:34 +0100
Daniel P. Berrangé <berrange@redhat.com> wrote:

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

in that line of thought, maybe hardcode 'check=on' and drop conditional/property?

> 
> With regards,
> Daniel