On Fri, Nov 23, 2018 at 09:46:36PM +0300, Roman Bolshakov wrote: > On Fri, Nov 23, 2018 at 06:16:46PM +0100, Jiri Denemark wrote: > > 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). > > > > 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. > > 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); > > Would that work for you? > > -- > Thank you, > Roman > Hi Jiri, That's a kind reminder. Thank you, Roman -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list
