[libvirt] [PATCH v2 11/11] qemu: auto-add pci-root controller for pc machine types

[Ján Tomko]

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