Re: [Qemu-arm] [Qemu-devel] [PATCH RFC 0/8] cpus: make "-cpu cpux, features" global properties

[Eduardo Habkost]

On Fri, Jun 03, 2016 at 08:36:21AM +0200, David Hildenbrand wrote:
> > On Thu, Jun 02, 2016 at 10:44:49PM +0200, David Hildenbrand wrote:
> > > > Current CLI option -cpu cpux,features serves as template
> > > > for all created cpus of type: cpux. However QEMU parses
> > > > "features" every time it creates a cpu instance and applies
> > > > them to it while doing parsing.
> > > > 
> > > > That doesn't work well with -device/device_add infrastructure
> > > > as it has no idea about cpu specific hooks that's used for
> > > > parsing "features".
> > > > In order to make -device/device_add utilize "-cpu features"
> > > > template convert it into a set of global properties, so that
> > > > every new CPU created will have them applied automatically.
> > > > 
> > > > That also allows to parse features only once, instread of
> > > > doing it for every CPU instance created.  
> > > 
> > > While I think this makes sense for most cases, we (s390x) are
> > > currently working on a mechanism to compare and baseline cpu models via
> > > a qmp interface, to not have to replicate CPU models in libvirt
> > > every time we do some changes.
> > > 
> > > To do that, we are creating temporary CPUs to handle the model
> > > parsing. So, with our current prototype, we rely on the mechanism
> > > to parse properties multiple time, as we are really creating
> > > different CPUs.  
> > 
> > This series only changes the code that exists for parsing the
> > -cpu option, and nothing else. Is this (the code that parses
> > -cpu) really what you need to reuse?
> 
> I was reading "every new CPU created will have them applied automatically".
> If I was having a basic understanding problem here, very good :)

Sorry, I misunderstood you:

So, you won't reuse the code for -cpu, but with global
properties, you are going to be affected even if you do a simple
object_new()/device_add. I see the problem, now.

I assume Igor explained it, already, and his suggestion sounds OK
to you. But I will answer your questions to confirm that this is
really the case:

> 
> The problematic part is when the properties are applied where the
> "changed" data is stored (class. vs. instance).
> 
> e.g. in terms of s390x: z13 includes both vx and tx
> -cpu z13,vx=off,tx=off

This will be translated to:
-global z13-s390-cpu.vx=off -global z13-s390-cpu.tx=off

> 
> Now, what would happen on
> a) device_add z13-s390-cpu // I assume vx=off, tx=off ?

Yes.

> 
> b) device_add z13-s390-cpu,vx=on // vx=on suddenly for all vcpus or one
> instance? I assume just this instance

It would affect all z13-s390-cpu instances.

> 
> c) device_add zBC12-s390-cpu // will I suddenly get a z13?
> Or a zBC12 without tx and vx? I assume the latter.

A zBC12 with the default values (not affected by -cpu).

> 
> d) object_new("z13-s390-cpu")); // will I get a clean z13 with tx and vx on?

The same as device_add z13-s390-cpu (a z13 without
tx and vx).

> 
> d) has to work for us. Otherwise we will have to fallback to manual
> property parsing.

It will be affected by the globals, but I assume management code
is not going to use add extra -cpu arguments when probing for CPU
model information, right?

Users will need to be aware that -cpu is equivalent to -global,
and will affect CPU information returned by query-cpu-definitions
(or similar commands).

> 
> > 
> > If all you need is to parse properties, why can't you reuse the
> > existing QOM/Device mechanisms to handle properties (the one used
> > by -device and device_add), instead of the -cpu code?
> 
> We can, if my given example works. And the global properties
> don't interfere with cpus.

They do, but only the model specified in -cpu.

> 
> > 
> > We need to use less of the infrastructure that exists for the
> > legacy -cpu option (and use more of the generic QOM/Device
> > mechanisms), not more of it.
> 
> It is better to have one way of creating cpus that two.

Unfortunately we already have two ways of creating CPUs: -cpu and
device_add. We are trying to translate -cpu to something
equivalent to generic mechanisms (-device and -global), so we
have only one underlying mechanism.

> 
> > 
> > 
> > > 
> > > While we could somehow change our mechanism I don't think this is
> > > the right thing to do.
> > >   
> > 
> > If reusing the existing parsing code is something you absolutely
> > need, we could split the process in two parts: 1) converting the
> > feature string to a list of property=value pairs; 2) registering
> > the property=value pairs as global properties. Then you coulde
> > reuse (1) only. But do you really need to reuse the parser for
> > the legacy -cpu option in your mechanism?
> 
> It's really not about the parser, more about the global properties.

Understood.

> 
> > 
> > > We will have to support heterogeneous cpu models (I think arm was one of
> > > the guys requesting this if I'm not mistaking) and it somehow
> > > contradicts to the general mechanism of device_add fully specifying
> > > parameters. These would now be implicit parameters.  
> > 
> > The -cpu interface really does contradict the general mechanism
> > of device_add. This whole series is about translating the
> > handling of -cpu to a more generic mechanism (-global), to allow
> > us to deprecate -cpu in the future. Why is that a bad thing?
> 
> It is a bad thing as soon as they affect other devices.
> If I did a -cpu z13,tx=off, I don't expect
> 
> a) a hot-plugged z13 to suddenly have tx=off

It will.

(Igor, can you confirm?)


> b) a hot-plugged zBC12 to suddenly have tx off

It won't.


I understand that this may be confusing, but that's because -smp
and -cpu don't fit the QEMU device/object models. We are moving
towards allowing CPU topologies to be created using only -device.

To do that, we are gradually translating -cpu to the generic
mechanisms used by -device/-global, so command-line options could
be easily converted to use the new mechanisms in the future.

Does that make sense?

> 
> Won't libvirt have to specify the cpu name either way in device-add?
> And your plan seems to be that the properties are suddenly implicit.
> I don't see a problem with libvirt having to specify the properties
> manually on device add.
> 
> I agree, cleaning up the parsing function indeed makes sense.
> 
> David
> 

-- 
Eduardo