[libvirt] [PATCH 1/4] qemu: Add support for generic PCIe Root Ports

[Laine Stump]

On 03/16/2017 10:18 AM, Andrea Bolognani wrote:
> On Wed, 2017-03-15 at 15:18 -0400, Laine Stump wrote:
>>> Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1408808
>>  
>> Yes, but kind of no. It makes it available, but still difficult and
>> cumbersome to use. I would say it *partially* resolves that BZ, with
>> full resolution coming from the followup patches that make it the
>> default. (I'm only pointing this out because we wouldn't want some
>> distro maintainer to be looking through patches for backport and
>> erroneously believe, based on the commit log, that this was the only
>> patch they needed).
> 
> I was debating about this myself.
> 
> Strictly speaking, the bug report is about adding support
> for generic PCIe Root Ports, which this patch does.
> 
> That said, I see your point, and since I don't have any
> specific objection to moving the Resolves: to the next
> patch I'll do just that.
> 
> [...]
>> BTW, although you added an entry to your new-fangled "news" file in
>> patch 4, you never added new info to formatdomain.html.in - at least
>> that should be included in this patch (with a small addition to its text
>> when you change the default for aarch64).
> 
> The existing documentation is fairly vague about PCI
> controller models:
> 
>   PCI controllers also have an optional subelement <model>
>   with an attribute name. The name attribute holds the
>   name of the specific device that qemu is emulating (e.g.
>   "i82801b11-bridge") rather than simply the class of device
>   ("dmi-to-pci-bridge", "pci-bridge"), which is set in the
>   controller element's model attribute. In almost all cases,
>   you should not manually add a <model> subelement to a
>   controller, nor should you modify one that is automatically
>   generated by libvirt. Since 1.2.19 (QEMU only).
> 
> Nowhere are the valid models for each PCI controllers listed,
> and that's fine in my book: the documentation explicitly
> tells the user that they should let libvirt do its thing in
> basically all cases.
> 
> Do you really think we should make that more explicit?



Well, "yes" because everything should be documented, but "no" because
once it's documented then people will start playing with it and whine
when something breaks because they played with it "in the wrong way".

I'm undecided about this, since there are some other cases where we
describe some things that users should never have to set and say "if you
don't know what this is for, you shouldn't change it" which seems like
an adequate warning, but then we get bug reports due to people changing
it anyway. On the other hand, when we *don't* document things that show
up in the XML, then people start crafting cargo-cult solutions that use
those undocumented things in maybe not the right way. (for example, we
added in a specifier to allow choosing between vfio and legacy-kvm
device assignment when we first added vfio support because someone was
concerned that there might be some bug in vfio and they would need a way
to fall back to legacy kvm assignment. Very soon after that legacy kvm
assignment was completely disabled in the kernel, but as least a couple
of times I've seen people trying to explicitly set that when assigning
devices, and wondering why their setup doesn't work)


So I guess you can push this without the extra documentation, and we can
discuss it to death in the meantime :-)

> 
> [...]
>>> @@ -338,7 +338,9 @@ VIR_ENUM_IMPL(virDomainControllerPCIModelName,
>>>                 "x3130-upstream",
>>>                 "xio3130-downstream",
>>>                 "pxb",
>>> -              "pxb-pcie")
>>> +              "pxb-pcie",
>>> +              "pcie-root-port",
>>  
>> Sigh. As this becomes the norm, it's going to make libvirt config look
>> redundant (and is also likely to confuse people about which attribute to
>> change if they want to use iohh3420 instead of the generic one), but
>> there's nothing that can be done about it. (I agree that this was the
>> best choice for device name in qemu btw).
> 
> You just have to change the model, as opposed to, you
> know... The model. What's so confusing about that? :D
> 
> -- 
> Andrea Bolognani / Red Hat / Virtualization
>