Re: [libvirt] [PATCH v2 11/11] qemu: auto-add pci-root controller for pc machine types
On 04/18/2013 07:22 AM, Laine Stump wrote:
On 04/17/2013 03:00 PM, Ján Tomko wrote:
>
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index 68518a7..a2179aa 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -10849,9 +10849,15 @@ virDomainDefParseXML(xmlDocPtr xml,
>
> if (def->virtType == VIR_DOMAIN_VIRT_QEMU ||
> def->virtType == VIR_DOMAIN_VIRT_KQEMU ||
> - def->virtType == VIR_DOMAIN_VIRT_KVM)
> + def->virtType == VIR_DOMAIN_VIRT_KVM) {
> if (virDomainDefMaybeAddController(def, VIR_DOMAIN_CONTROLLER_TYPE_USB, 0,
-1) < 0)
> goto error;
> + if (STRPREFIX(def->os.machine,"pc")) {
You also need to do this for all rhel-* machine types. Even though that
machine type only shows up in RHEL-specific versions of qemu, it is
possible for someone with a stock RHEL qemu to install upstream libvirt,
and we don't want to break that.
It seems these are not the only machines with a PCI bus (PPC for example
has one), which makes me wonder how many things this will break.
(side note - we need to make another patch that moves *all* of these
implicit controller additions to a hypervisor-specific post-parse
callback and only for certain machinetypes; of course this could break
non-qemu hypervisors if you move the MaybeAddController() calls out of
here and into a qemu-only callback, so I guess we should add a callback
for all of them, then let the maintainers remove that which is
inappropriate).
> + if (virDomainDefMaybeAddController(def, VIR_DOMAIN_CONTROLLER_TYPE_PCI,
0,
> + VIR_DOMAIN_CONTROLLER_MODEL_PCI_ROOT)
< 0)
> + goto error;
> + }
> + }
>
> /* analysis of the resource leases */
> if ((n = virXPathNodeSet("./devices/lease", ctxt, &nodes)) < 0)
{
> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> index df0077a..21da6b6 100644
> --- a/src/qemu/qemu_command.c
> +++ b/src/qemu/qemu_command.c
> @@ -1398,7 +1402,7 @@ qemuDomainAssignPCIAddresses(virDomainDefPtr def,
> if (!(addrs = qemuDomainPCIAddressSetCreate(def, addrs->nbuses, false)))
> goto cleanup;
>
> - if (qemuAssignDevicePCISlots(def, qemuCaps, addrs) < 0)
> + if (nbuses && qemuAssignDevicePCISlots(def, qemuCaps, addrs) <
0)
> goto cleanup;
Is it okay to simply not call qemuAssignDevicePCISlots() if there are no
PCI buses? Does this properly return success if there are no PCI devices
and no PCI buses? Does it properly return a failure when there is at
least one PCI device in the config but no PCI buses?
Now I see there are (at least) two errors in 10/11:
I reserve a slot for a future bridge addition even if there is no PCI
bus and I added a last-minute change that accesses addrs right after
setting it to NULL.
I thought PCI devices would be caught by qemuCollectPCIAddress but that
would only catch those with explicitly specified addresses.
I think if we change AssignDevicePCISlots not to reserve slot 1
unconditionally, it would be good to call it even if we have no PCI bus:
this would expose PCI devices on a machine with no PCI bus and it would
expose machine types that do have an implicit PCI bus but we don't add
the controller for them.
> }
>
> @@ -10146,6 +10150,12 @@ virDomainDefPtr qemuParseCommandLine(virCapsPtr qemuCaps,
> if (virDomainDefAddImplicitControllers(def) < 0)
> goto error;
>
> + if (STRPREFIX(def->os.machine,"pc")) {
> + if (virDomainDefMaybeAddController(def, VIR_DOMAIN_CONTROLLER_TYPE_PCI, 0,
> + VIR_DOMAIN_CONTROLLER_MODEL_PCI_ROOT)
< 0)
> + goto error;
> + }
> +
Didn't you already do this in virDomainDefParseXML?
I did. This is for getting the domain definition from a qemu command
line, as opposed to the XML.
Jan