[libvirt] [PATCHv2 01/16] qemu: Add KVM CPUs into cache only if KVM is present

[Jiri Denemark]

On Fri, Nov 23, 2018 at 18:55:00 +0300, Roman Bolshakov wrote:
> On Fri, Nov 23, 2018 at 04:30:13PM +0100, Jiri Denemark wrote:
> > On Fri, Nov 23, 2018 at 17:16:12 +0300, Roman Bolshakov wrote:
> > > On Wed, Nov 21, 2018 at 07:43:43PM +0100, Jiri Denemark wrote:
> > > > virQEMUCapsInitHostCPUModel always fills in something and your check
> > > > should probably remain in place for it
> > > > 
> > > > virQEMUCapsFormatHostCPUModelInfo does
> > > >     virQEMUCapsHostCPUDataPtr cpuData = &qemuCaps->kvmCPU;
> > > >     qemuMonitorCPUModelInfoPtr model = cpuData->info;
> > > >     if (!model)
> > > >         return;
> > > > 
> > > > virQEMUCapsFormatCPUModels
> > > >     cpus = qemuCaps->kvmCPUModels;
> > > >     if (!cpus)
> > > >         return;
> > > > 
> > > > So to me it looks like all functions are ready to see NULL pointers and
> > > > just do nothing if that's the case. Thus the only thing this patch
> > > > should need to do is to make sure virQEMUCapsInitHostCPUModel does not
> > > > set something non-NULL there.
> > > 
> > > Unfortunately, that won't work for the patch series. kvmCPUModels is renamed to
> > > accelCPUModels and kvmCPU is renamed to accelCPU in PATCH 6.
> > 
> > And how does different name change the behavior?
> > 
> > > So, virQEMUCapsFormatHostCPUModelInfo looks like:
> > >     if (virQEMUCapsTypeIsAccelerated(type))
> > >         cpuData = qemuCaps->accelCPU;
> > >     else
> > >         cpuData = qemuCaps->tcgCPU;
> > > 
> > > and virQEMUCapsFormatCPUModels looks like:
> > >     if (virQEMUCapsTypeIsAccelerated(type))
> > >         cpus = qemuCaps->accelCPUModels;
> > >     else
> > >         cpus = qemuCaps->tcgCPUModels;
> > > 
> > > Without the check we'd return CPUs for KVM domain on the platform that doesn't
> > > support it.
> > 
> > It won't return anything because the code will make sure accelCPUModels
> > and accelCPU will be NULL when no accel method is supported.
> 
> But accelCPU is not NULL on macOS with QEMU_CAPS_HVF and on Linux with
> QEMU_CAPS_KVM. That's where the problem arises.

Right, and that's what I think should be changed. Rather then adding
checks to the formatting and loading code to ignore something which
shouldn't be present in the first place.

> We're going to get additional kvm CPUs on mac and hvf CPUs on Linux
> and that will break qemucapabilitiestest.

I think I'm missing something here. There's only one CPU definition
describing the host CPU. There are hosts which have several different
CPUs, but libvirt is not really prepared to see that and I believe this
is not what you're addressing with this series, is it? Or are you
talking about some other CPUs?

> In fact they will be the same accelCPUs of the supported accelerator
> but with hostCPU's type attribute of the other accelerator.

How would this happen? We have a single accelerator enabled on a host
and we generate a host CPU model for it (and just for it, there's no
reason to generate a CPU model for something that is not supported on
the host).

> If you wish I can try to rework the patchset. Instead of generalizing
> kvmCPU, I'd just add hvfCPU to qemuCaps. It might have a good side
> effect that libvirt will be able to support multiple accelerators on the
> same platform.

I think we can leave this for the future :-)

Jirka