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

[Laine Stump]

On 04/18/2013 08:09 AM, Ján Tomko wrote:
> 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.


Well, we're going to need to figure those out. We can either do it for
all, and let people interested in particular machinetypes explicitly
exclude theirs, or we can do it only for those machinetypes that we know
have a PCI bus, and wait for the others to complain; I guess it all
depends on if we want to minimize pain, or minimize the amount of time
that passes before we get it right.

I'm not sure if the presence of "PCI" in the output of qemu-system-*
-help is an accurate indicator of which platforms have a PCI bus, but I
tried running that on all the qemu emulators on my Fedora 18 system and
got the following list of qemu binaries that have devices claiming to
live on a PCI bus:

for e in qemu-system-*; do if $e -device \? 2>&1| grep -q PCI; then echo
"YES - $e"; else echo "NO  - $e"; fi; done
YES - qemu-system-alpha
YES - qemu-system-arm
NO  - qemu-system-cris
YES - qemu-system-i386
NO  - qemu-system-lm32
YES - qemu-system-m68k
NO  - qemu-system-microblaze
NO  - qemu-system-microblazeel
YES - qemu-system-mips
YES - qemu-system-mips64
YES - qemu-system-mips64el
YES - qemu-system-mipsel
NO  - qemu-system-or32
YES - qemu-system-ppc
YES - qemu-system-ppc64
YES - qemu-system-ppcemb
NO  - qemu-system-s390x
YES - qemu-system-sh4
YES - qemu-system-sh4eb
NO  - qemu-system-sparc
YES - qemu-system-sparc64
NO  - qemu-system-unicore32
YES - qemu-system-x86_64
NO  - qemu-system-xtensa
NO  - qemu-system-xtensaeb

So within any binary that supports PCI devices, does that necessarily
mean it's supported for *all* machinetypes? If so, we've got a pretty
long list of machinetypes to check for:

( for e in qemu-system-*; do if $e -device \? 2>&1| grep -q PCI; then $e
-M ? | cut -f1 -d' ' | grep -v Supported | grep -v none; fi; done ) |
sort | uniq | wc
     72      72     590

(note that it's bad enough we have to eliminate the opacity of
machinetype; we *certainly* don't want to start trying to make decisions
based on the emulator binary name!)

And of course, just because a machinetype can support pci devices,
doesn't necessarily mean that it has a builtin pci-root bus :-O


>> (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.


If I understand you correctly, you're suggesting that we always do the
auto-scan for PCI devices, even if there are no buses, because it would
then generate an error for any PCI device it encountered. That sounds
reasonable to me.



>>>      }
>>>  
>>> @@ -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.


Right, right. Nothing to see here; move along :-)