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
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list