Re: [libvirt] [PATCHv2 01/16] qemu: Add KVM CPUs into cache only if KVM is present
First, I'd like to apologize for such a late reply.
...
> > > 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).
> >
>
> accelCPU will be present on a host where an accelerator is avaialable.
> You said can't have host CPU definitions present twice. I agree with
> that. But if we call virQEMUCapsFormatCPUModels twice for
> VIR_DOMAIN_VIRT_KVM and VIR_DOMAIN_VIRT_HVF without the checks, host cpu
> definitions will be present twice for each accelerator because accelCPU
> is not NULL.
I see. I was thinking about several options. In general I think we
should keep the loader and formatter code as simple and stupid as
possible, we should not wire any fancy logic in them. Thus we have two
options:
First option:
> So we need to call it only once for the supported accelerator.
> The checks help in that. Alternative approach to do only one call is:
>
> virDomainVirtType acceleratedDomain = VIR_DOMAIN_VIRT_KVM;
> if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_HVF))
> acceleratedDomain = VIR_DOMAIN_VIRT_HVF;
>
> virQEMUCapsFormatHostCPUModelInfo(qemuCaps, &buf, acceleratedDomain);
> virQEMUCapsFormatHostCPUModelInfo(qemuCaps, &buf, VIR_DOMAIN_VIRT_QEMU);
>
> virQEMUCapsFormatCPUModels(qemuCaps, &buf, acceleratedDomain);
> virQEMUCapsFormatCPUModels(qemuCaps, &buf, VIR_DOMAIN_VIRT_QEMU);
Or even using a dedicated enum instead of virDomainVirtType for
accessing the CPU data. Either way, the callers would need to contain
the extra code to request the data they want.
Second option: store CPU data separately for each virtType (qemu, kvm,
hvf), which would effectively add support for multiple accelerators on a
single host. While we don't currently need to support multiple
accelerators, I think this solution would be the cleanest one. It would
be pretty clear what data are stored in the cache and what the callers
want without having to copy paste some special handling for individual
accelerators. The formatting/loading code would just be called for all
three virtTypes unconditionally.
Thanks for the patience.
Jirka