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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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@xxxxxxxxxxx>
---
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 was also thinking about a middle ground between choices 1 and 2 from
Dan in case the machine types are not versioned any time soon.  We
would, by default add no pci controller when defining unless we think
it's needed.  That would be determined by any of the following:

a) there is a device that we know needs PCI controller

b) there is a device with PCI address

c) <controller type='pci'/> is spotted in the user-supplied XML

In case any of the above is true (notice that users themselves can
override the addition with third option), we add all those controllers
and when starting the machine, we just check that it's supported
(using the capability).

Martin

Attachment: signature.asc
Description: PGP signature

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list

[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]