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 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



[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]