[libvirt] [PATCH v5 2/4] qemu: Add PCI-Express root to ARM virt machine

[Cole Robinson]

On 08/10/2015 11:09 AM, Daniel P. Berrange wrote:
> On Thu, Aug 06, 2015 at 07:46:58PM +0200, Martin Kletzander wrote:
>> On Thu, Aug 06, 2015 at 06:37:41PM +0200, Martin Kletzander wrote:
>>> On Fri, Jul 17, 2015 at 02:27:45PM +0300, Pavel Fedin wrote:
>>>> Here we assume that if qemu supports generic PCI host controller,
>>>> it is a part of virt machine and can be used for adding PCI devices.
>>>>
>>>> In qemu this is actually a PCIe bus, so we also declare multibus
>>>> capability so that 0'th bus is specified to qemu correctly as 'pcie.0'
>>>>
>>>> Signed-off-by: Pavel Fedin <p.fedin at samsung.com>
>>>> ---
>>>> src/qemu/qemu_capabilities.c |  8 ++++++++
>>>> src/qemu/qemu_domain.c       | 17 +++++++++++++----
>>>> 2 files changed, 21 insertions(+), 4 deletions(-)
>>>>
>>>> diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
>>>> index d570fdd..f3486c7 100644
>>>> --- a/src/qemu/qemu_capabilities.c
>>>> +++ b/src/qemu/qemu_capabilities.c
>>>> @@ -2138,6 +2138,14 @@ bool virQEMUCapsHasPCIMultiBus(virQEMUCapsPtr qemuCaps,
>>>>       return false;
>>>>   }
>>>>
>>>> +    if (ARCH_IS_ARM(def->os.arch)) {
>>>> +        /* If 'virt' supports PCI, it supports multibus.
>>>> +         * No extra conditions here for simplicity.
>>>> +         */
>>>
>>> So every ARM qemu with the "virt" machine type supports both PCI and
>>> multiqueue?  How about those "virt-*" for which you check below.  That
>>> might not be related, I'm just curious.
>>>
>>>> +        if (STREQ(def->os.machine, "virt"))
>>>> +            return true;
>>>> +    }
>>>> +
>>>>   return false;
>>>> }
>>>>
>>>> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
>>>> index 8b050a0..c7d14e4 100644
>>>> --- a/src/qemu/qemu_domain.c
>>>> +++ b/src/qemu/qemu_domain.c
>>>> @@ -981,7 +981,7 @@ virDomainXMLNamespace virQEMUDriverDomainXMLNamespace = {
>>>> static int
>>>> qemuDomainDefPostParse(virDomainDefPtr def,
>>>>                      virCapsPtr caps,
>>>> -                       void *opaque ATTRIBUTE_UNUSED)
>>>> +                       void *opaque)
>>>> {
>>>>   bool addDefaultUSB = true;
>>>>   bool addImplicitSATA = false;
>>>> @@ -1030,12 +1030,21 @@ qemuDomainDefPostParse(virDomainDefPtr def,
>>>>       break;
>>>>
>>>>   case VIR_ARCH_ARMV7L:
>>>> -       addDefaultUSB = false;
>>>> -       addDefaultMemballoon = false;
>>>> -       break;
>>>>   case VIR_ARCH_AARCH64:
>>>>      addDefaultUSB = false;
>>>>      addDefaultMemballoon = false;
>>>> +       if (STREQ(def->os.machine, "virt") ||
>>>> +           STRPREFIX(def->os.machine, "virt-")) {
>>>> +           virQEMUDriverPtr driver = opaque;
>>>> +
>>>> +           /* This condition is actually a (temporary) hack for test suite which
>>>> +            * does not create capabilities cache */
>>>
>>> Few questions here.  a) how "temporary" is this since you're not
>>> removing it in this series?  b) for what tests you need this hack and
>>> what part of the below is the hack?
>>>
>>> Moreover, you cannot use capabilities when defining an XML.  The
>>> emulator can change between the domain is defined and started, so you
>>> cannot know with what emulator this will be started.
>>>
>>> I see Michal (Cc'd) just pushed this, I probably just missed the mail
>>
>> Of course I forgot, Cc'ing now.
> 
> I agree with your core statement that we should not be using the QEMU
> capabilities when defining the XML. With all existing scenarios we have
> been able to determine whether to add the implicit PCI controller based
> on the machine type name only, because with every other QEMU arch when
> doing such a major change as adding a PCI bus, they have created a new
> machine type.  The problem is that arm 'virt' machine type is not stable,
> it is being changed arbitrarily in new QEMU releases :-(
> 
> So AFAIK, that leaves us with 3 choices
> 
>  - Never add PCI controller at time the XML is defined, on the basis
>    that we have to be conservative in what we add to cope with old QEMU
> 
>  - Always add PCI controller at time the XML is defined, on the basis
>    that most people will have new enough QEMU because ARM 'virt' machine
>    type is very much still in development, so no one will serously stick
>    with the older QEMU versions which lack PCI.
> 
>  - Use the capabilities in XML post-parse to conditionally add the
>    PCI controller. This is what was currently merged
> 
> I don't think option 1 makes much sense as it'll harm ARM arch forever
> more, to cope with QEMU versions that will almost never be used in
> practice.
> 
> I'd be inclined to go with option 2, and then if any PCI devices are
> actually used with the guest, check the capability at start time when
> we are doing auto-address assignment.
> 

Another option: add versioned 'virt' machine types to the next qemu release
(like virt-2.5 etc.), and key libvirt enabling pci off of that.

_Eventually_ we are going to need versioned 'virt' machine types for migration
compatibility like we already do with x86 -M pc-*. Might as well make the
change early so libvirt can actually use it.

- Cole