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