Re: [libvirt] [PATCH v1 0/4] CPU Model Comparsion via QEMU

On 04/17/2018 04:18 AM, Jiri Denemark wrote: > On Mon, Apr 16, 2018 at 19:16:05 -0400, Collin Walling wrote: >> [Overview] >> >> This patch series implements an interface to "query-cpu-model-comparison" >> (available QEMU ~2.8.0) via virsh cpu-compare. >> >> [Using This Feature] >> >> Run virsh cpu-compare (ensure you are running the virsh in your build dir) and >> pass it an xml file describing a cpu definition. You can copy the cpu xml from >> virsh capabilities (if you want to compare the host cpu to itself), or a cpu >> defined in any guest xml. Additionally, you can create a cpu xml as such (e.g. >> for s390x): >> >> <cpu> >> <arch>s390x</arch> >> <model fallback='forbid'>model_name</model> >> <feature policy='require|disable' name='feature_name'/> >> </cpu> >> >> NOTE: the presence of <arch> is optional and it will treat the cpu defined in >> the xml as a host cpu. This will disregard all feature policies (i.e. >> all features listed will behave with policy='require', even if disable >> is specified). >> >> NOTE: as s390x only supports feature policies 'require' and 'disable', I am >> uncertain how to handle the other policies when parsing CPUDef to JSON. >> >> [Example Output] >> >> On an s390x system running a z13.2, this is the expected output (where each file >> describes a CPU model corresponding to the name of the file): >> >> $ virsh cpu-compare zEC12.xml >> Host CPU is a superset of CPU described in zEC12.xml >> >> $ virsh cpu-compare z13.2.xml >> CPU described in z13.2.xml is identical to host CPU >> >> $ virsh cpu-compare z14.xml >> CPU described in z14.xml is incompatible with host CPU >> >> $ virsh cpu-compare z14.xml --error >> error: Failed to compare host CPU with z14.xml >> error: the CPU is incompatible with host CPU >> >> $ virsh cpu-compare z12345.xml >> error: Failed to compare host CPU with z12345cpu.xml >> error: internal error: unable to execute QEMU command 'query-cpu-model-comparison': The CPU definition 'z12345-base' is unknown. >> >> [Patch Rundown] >> >> The first patch copies the host CPU definition from qemuCaps to virCaps so >> we can easily access the host CPU model and features when we handle the CPU >> comparison in qemu_driver. Note that we take care to not clobber anything >> already stored in the host CPU definition until we have successfully >> constructed a new host CPU definition. > > I think this is a wrong approach. You'd be basically giving random > answers depending on which QEMU binary is probed first. The reason for > storing the CPU model in qemuCaps is that it is tight to a particular > QEMU binary rather than the host itself. The model reported by one > binary may not be usable with other binaries and this applies to any > comparisons or other operations done with this CPU model. > > In other words, we need to introduce a new set of CPU related APIs which > will take more arguments so that the caller may specify what binary, > virt type, and machine type they want to use. In other words, the APIs > should support parameters similar to virConnectGetDomainCapabilities(). > > I'm currently starting to work on these new APIs. > > Jirka I see your concern. I understand your points behind having multiple arguments to finely control which qemu we probe, but what do you think of the current code within "virQEMUCapsInitGuest"? If I understand it correctly, then it has a way of querying the "native qemu binary" capabilities (e.g. qemu-kvm). We could refactor this code to get these "kvmbinCaps" when we need it, and from that we can retrieve the host CPU model. We would not need to specify a binary for this, as we already have a list of "native binaries" that we can test. As for virt type, we can still specify this via "virQEMUCapsGetHostModel".

I think that would suffice, at least enough for what this patch series needs. I could spin up a patch for this if you'd like and we can see if it makes sense?