On Fri, Jun 17, 2016 at 12:01:58PM -0400, Laine Stump wrote:
On 06/17/2016 11:46 AM, Martin Kletzander wrote:
On Fri, Jun 17, 2016 at 11:36:05AM -0400, Laine Stump wrote:
On 06/17/2016 08:43 AM, Andrea Bolognani wrote:
* other than the pcie-root. This is so that there will be hot-pluggable - * PCI slots available + * PCI slots available. + * + * We skip this step for aarch64 mach-virt guests, where we want to + * be able to have a pure virtio-mmio topology */ if (virDomainControllerFind(def, VIR_DOMAIN_CONTROLLER_TYPE_PCI, 1) < 0 && + !qemuDomainMachineIsVirt(def) &&
You're assuming that the only virt* machinetypes will be aarch64, which may be reasonable now, but not in the future (periodically someone from qemu will mention the idea of a "virt" machinetype for x86, which is legacy-free and accepts only virtio devices). Wouldn't a more specific comparison be better here (and in the other places in this patch)?
Just my $.02 here, but since our qemuDomainMachineIsVirt() is made specifically for aarch64 arches, I think the right thing to do would be just add architecture check into that function as using it throughout the codebase ought to actually be what all the callers want.
Sure, a single function would be great. Its name should reflect that it is for *aarch64* virt machines though (in anticipation of other arches getting a virt machinetype).
I thought that was obvious but re-reading what I wrote it isn't. So to be sure, I'll express my feelings with code, so it's more exact =) Something along the lines of: diff --git i/src/qemu/qemu_domain.c w/src/qemu/qemu_domain.c index 2fe59d78fd2f..cc6b7ae10a08 100644 --- i/src/qemu/qemu_domain.c +++ w/src/qemu/qemu_domain.c @@ -4906,10 +4906,11 @@ qemuDomainMachineIsS390CCW(const virDomainDef *def) bool -qemuDomainMachineIsVirt(const virDomainDef *def) +qemuDomainIsAArch64Virt(const virDomainDef *def) { - return STREQ(def->os.machine, "virt") || - STRPREFIX(def->os.machine, "virt-"); + if (def->os.arch != VIR_ARCH_AARCH64) && + STREQ(def->os.machine, "virt") || + STRPREFIX(def->os.machine, "virt-"); } -- Martin