On 08/11/2015 01:23 AM, Martin Kletzander wrote:
> 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),
Yeah, that's an expansion of what I was talking about in the "On the
other hand" paragraph in my reply to the grandparent of your message (I
had said allow for adding <controller type='pci' model='pcie-root'/>
manually); your idea has that plus some useful extra (also looking for
devices that need a pci controller).
This sounds like the safest thing to do for the virt-* machinetypes
where we don't know whether or not pcie-root exists. But then how many
of these will there be? I think we should lobby fairly strongly for qemu
to version the virt machinetypes right away.
> we add all those controllers
> and when starting the machine, we just check that it's supported
> (using the capability).
lacking cooperation from qemu on the versioning + stability from, +1
from me on this idea. If they're nice about it and start versioning
right away, then it may be a lot of extra ugly code that will only be
useful for a very short while, and after that just burden us with
maintenance for ever.
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list