Re: [PATCH 3/5] target-i386: Slim conversion to X86CPU subclasses

From: Eduardo Habkost <ehabkost@redhat.com>
To: Igor Mammedov <imammedo@redhat.com>
Cc: aliguori@us.ibm.com,
	"kvm@vger.kernel.org list" <kvm@vger.kernel.org>,
	gleb@redhat.com, qemu-devel@nongnu.org, blauwirbel@gmail.com,
	anthony@codemonkey.ws, pbonzini@redhat.com,
	"Andreas Färber" <afaerber@suse.de>,
	rth@twiddle.net
Subject: Re: [PATCH 3/5] target-i386: Slim conversion to X86CPU subclasses
Date: Fri, 8 Feb 2013 13:08:39 -0200	[thread overview]
Message-ID: <20130208150839.GN9964@otherpad.lan.raisama.net> (raw)
In-Reply-To: <20130208145231.GK4138@otherpad.lan.raisama.net>

On Fri, Feb 08, 2013 at 12:52:31PM -0200, Eduardo Habkost wrote:
> On Fri, Feb 08, 2013 at 01:58:42PM +0100, Igor Mammedov wrote:
[...]
> > Continuing on theoretical issue:
> > > We could add an inited field to X86CPUClass that gets checked at initfn
> > > time (only ever getting set to true by the pre-defined models). Then it
> > > would be per model. And if we add a prototype for the ..._class_init we
> > > could even call it late as proposed for -cpu host if we take some
> >              ^^^^^^^^^^^^ is a tricky part, for global properties to work it
> > would require, calling this hook after kvm_init() is succeeds and before
> > any initfn() is called in general or as minimum before Device.initfn(). And it
> > anyway will not make all CPU classes to have correct defaults in KVM mode,
> > since only CPU class of created CPU instance will be fixed up.
> > 
> > 1. One way to make sure that built-in CPU classes have fixed up defaults is to
> > iterate over them in kvm_arch_init() and possibly calling their class_init()
> > again to reinitialize. It's still hack (due fixing something up), but it would
> > give at least correct KVM mode defaults, regardless of the order classes are
> > initialized.
> 
> Can't we do that more easily with the tcg-vendor/vendor properties?
> 
> It looks we are burning too much brain cycles trying to force a model
> that's really unintuitive to the outside, where the default-value of a
> class property depends on the options given to the QEMU command-line. We
> don't have to do that.
> 
> The point of initializing stuff in class_init is to make introspection
> easy. If we make the classes change how they look like depending on the
> command-line configuration, the classes and the class introspection
> system get less useful.
> 

Sorry for replying to myself, but extending my answer:

> > 
> > 2. But more correct way from POV of OOP would be one without any fixups, i.e.
> > create extra KVM-builtin-CPU-classes that are derived from host class.
> > and in object_class_by_name() lookup for them if kvm is enabled. But we could
> > do this as follow-up to #1.

Solution #2 would be 100% correct, strictly speaking, but isn't it
overkill to create separate classes if we could just add one additional
property in X86CPUClass, and let the CPU object choose which property is
important for the CPUID setup, depending if KVM is enabled or not?

-- 
Eduardo