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

[Daniel P. Berrange]

On Mon, Aug 10, 2015 at 07:23:07PM -0400, Cole Robinson wrote:
> 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.

I've raised the idea of versioned 'virt' machine type several times on
qemu-devel and its been clearly rejected saying they're not willing to
guarantee migration compatibility between releases on arm yet.

Regards,
Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|