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

[Jiri Denemark]

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

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.

Jirka