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

[Roman Bolshakov]

On Wed, Nov 21, 2018 at 07:43:43PM +0100, Jiri Denemark wrote:
> On Wed, Nov 21, 2018 at 20:50:50 +0300, Roman Bolshakov wrote:
> > On Wed, Nov 21, 2018 at 05:04:07PM +0100, Jiri Denemark wrote:
> > > On Wed, Nov 21, 2018 at 17:01:44 +0300, Roman Bolshakov wrote:
> > > > diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
> > > > index fde27010e4..4ba8369e3a 100644
> > > > --- a/src/qemu/qemu_capabilities.c
> > > > +++ b/src/qemu/qemu_capabilities.c
> > > > @@ -3467,11 +3467,13 @@ virQEMUCapsLoadCache(virArch hostArch,
> > > >      }
> > > >      VIR_FREE(str);
> > > >  
> > > > -    if (virQEMUCapsLoadHostCPUModelInfo(qemuCaps, ctxt, VIR_DOMAIN_VIRT_KVM) < 0 ||
> > > > +    if ((virQEMUCapsGet(qemuCaps, QEMU_CAPS_KVM) &&
> > > > +         virQEMUCapsLoadHostCPUModelInfo(qemuCaps, ctxt, VIR_DOMAIN_VIRT_KVM) < 0) ||
> > > >          virQEMUCapsLoadHostCPUModelInfo(qemuCaps, ctxt, VIR_DOMAIN_VIRT_QEMU) < 0)
> > > >          goto cleanup;
> > > 
> > > I don't think we should introduce these guards in all the places. All
> > > the loading and formatting functions should return success if the
> > > appropriate info is not available, so you should just make sure the
> > > relevant info is NULL in qemuCaps.
> > 
> > Do you mean the capabilities checks should be moved inside the
> > functions?
> 
> virQEMUCapsLoadHostCPUModelInfo does (not literally, but effectively)
> 
>     hostCPUNode = virXPathNode("./hostCPU[@type='kvm']", ctxt);
>     if (!hostCPUNode)
>         return 0;
> 
> virQEMUCapsLoadCPUModels does
>     n = virXPathNodeSet("./cpu[@type='kvm']", ctxt, &nodes);
>     if (n == 0)
>         return 0;
> 

I agree, virQEMUCapsLoadHostCPUModelInfo and virQEMUCapsLoadCPUModels
don't need the check.

> 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.

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.

Thank you,
Roman