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

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(a)samsung.com&gt; >>>> --- >>>> 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.