[libvirt] [PATCH v3 15/18] qemu: auto-add pcie-root-port/dmi-to-pci-bridge controllers as needed
Laine Stump
laine at laine.org
Thu Oct 13 18:36:33 UTC 2016
On 10/13/2016 01:43 PM, Laine Stump wrote:
> On 10/11/2016 05:34 AM, Andrea Bolognani wrote:
>> On Mon, 2016-10-10 at 15:43 -0400, Laine Stump wrote:
>>>>> 3) Although it is strongly discouraged, it is legal for a pci-bridge
>>>>> to be directly plugged into pcie-root, and we don't want to
>>>>> auto-add a dmi-to-pci-bridge if there is already a pci-bridge
>>>>> that's been forced directly into pcie-root. Finally,
>>>>> although I
>>>>> fail to see the utility of it, it is legal to have
>>>>> something like
>>>>> this in the xml:
>>>>> <controller type='pci' model='pcie-root' index='0'/>
>>>>> <controller type='pci' model='pci-bridge' index='2'/>
>>>>> and that will lead to an automatically added
>>>>> dmi-to-pci-bridge at
>>>>> index=1 (to give the unaddressed pci-bridge a proper place
>>>>> to plug
>>>>> in):
>>>>> <controller type='pci' model='dmi-to-pci-bridge'
>>>>> index='1'/>
>>>>> (for example, see the existing test case
>>>>> "usb-controller-default-q35"). This is handled in
>>>>> qemuDomainPCIAddressSetCreate() when it's adding in
>>>>> controllers to
>>>>> fill holes in the indexes.
>>>> I wonder how this "feature" came to be... It seems to be the
>>>> reason for quite a bit of convoluted code down below, which
>>>> we could avoid if this had never worked at all. As is so
>>>> often the case, too late for that :(
>>> Maybe not. The only place I ever saw that was in the above test case,
>>> and another named "usb-controller-explicit-q35", and the author of both
>>> of those tests was (wait for it!), no, not Albert Einstein. Andrea
>>> Bolognani!
>> Oh, *that* guy! It's always *that* guy, isn't it? :P
>>
>>> The only reason it worked in the past was because we always
>>> automatically added the dmi-to-pci-bridge very early in the post-parse
>>> processing. This implies that any saved config anywhere will already
>>> have the necessary dmi-to-pci-bridge at index='1', so we only need to
>>> preserve the behavior for new domain definitions that have a pci-bridge
>>> at index='2' but nothing at index='1'.
>>> Since you're the only person documented to have ever created a config
>>> like that, and it would only be problematic if someone tried to create
>>> another new config, maybe we should just stop accidentally
>>> supporting it
>>> and count it as a bug that's being fixed. What's your opinion?
>> Given the evidence you're presenting, I'm all for getting
>> rid of it, especially since it will make the code below
>> much simpler and hence more maintainable.
>>
>> Considering how critical that part of libvirt is, anything
>> we can do to make it leaner and meaner is going to be a huge
>> win in the long run.
>
> I'm looking back through the code and wondering how to simplify it -
> it may be that the alternate method I had initially used (which failed
> that one test) is just as complicated as what I have now;
> unfortunately I didn't save it.
Okay, now I've reminded myself of what I did. Basically everything
necessary for that is the changes to qemuDomainPCIAddressSetCreate()
dealing with "lowest*Bridge" (see the patch excerpt below). I would
still need to patch this function even if we didn't support "auto-adding
dmi-to-pci-bridge when a pci-bridge is present in the config", but it
would be slightly simpler.
I'll split it into a separate patch so that it's more apparent, and we
can decide based on that.
On 09/20/2016 03:14 PM, Laine Stump wrote:
> diff --git a/src/qemu/qemu_domain_address.c b/src/qemu/qemu_domain_address.c
> index 1ff80e3..a9c4c32 100644
> --- a/src/qemu/qemu_domain_address.c
> +++ b/src/qemu/qemu_domain_address.c
> @@ -797,6 +797,11 @@ qemuDomainPCIAddressSetCreate(virDomainDefPtr def,
> {
> virDomainPCIAddressSetPtr addrs;
> size_t i;
> + bool hasPCIeRoot = false;
> + int lowestDMIToPCIBridge = nbuses;
> + int lowestUnaddressedPCIBridge = nbuses;
> + int lowestAddressedPCIBridge = nbuses;
> + virDomainControllerModelPCI defaultModel;
>
> if ((addrs = virDomainPCIAddressSetAlloc(nbuses)) == NULL)
> return NULL;
> @@ -804,38 +809,84 @@ qemuDomainPCIAddressSetCreate(virDomainDefPtr def,
> addrs->nbuses = nbuses;
> addrs->dryRun = dryRun;
>
> - /* As a safety measure, set default model='pci-root' for first pci
> - * controller and 'pci-bridge' for all subsequent. After setting
> - * those defaults, then scan the config and set the actual model
> - * for all addrs[idx]->bus that already have a corresponding
> - * controller in the config.
> - *
> - */
> - if (nbuses > 0)
> - virDomainPCIAddressBusSetModel(&addrs->buses[0],
> - VIR_DOMAIN_CONTROLLER_MODEL_PCI_ROOT);
> - for (i = 1; i < nbuses; i++) {
> - virDomainPCIAddressBusSetModel(&addrs->buses[i],
> - VIR_DOMAIN_CONTROLLER_MODEL_PCI_BRIDGE);
> - }
> -
> for (i = 0; i < def->ncontrollers; i++) {
> - size_t idx = def->controllers[i]->idx;
> + virDomainControllerDefPtr cont = def->controllers[i];
> + size_t idx = cont->idx;
>
> - if (def->controllers[i]->type != VIR_DOMAIN_CONTROLLER_TYPE_PCI)
> + if (cont->type != VIR_DOMAIN_CONTROLLER_TYPE_PCI)
> continue;
>
> if (idx >= addrs->nbuses) {
> virReportError(VIR_ERR_INTERNAL_ERROR,
> _("Inappropriate new pci controller index %zu "
> - "not found in addrs"), idx);
> + "exceeds addrs array length"), idx);
> goto error;
> }
>
> - if (virDomainPCIAddressBusSetModel(&addrs->buses[idx],
> - def->controllers[i]->model) < 0)
> + if (virDomainPCIAddressBusSetModel(&addrs->buses[idx], cont->model) < 0)
> goto error;
> +
> + /* we'll use all this info later to determine if we need
> + * to add a dmi-to-pci-bridge due to unaddressed pci-bridge controllers
> + */
> + if (cont->model == VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT) {
> + hasPCIeRoot = true;
> + } else if (cont->model ==
> + VIR_DOMAIN_CONTROLLER_MODEL_DMI_TO_PCI_BRIDGE) {
> + if (lowestDMIToPCIBridge > idx)
> + lowestDMIToPCIBridge = idx;
> + } else if (cont->model == VIR_DOMAIN_CONTROLLER_MODEL_PCI_BRIDGE) {
> + if (virDeviceInfoPCIAddressWanted(&cont->info)) {
> + if (lowestUnaddressedPCIBridge > idx)
> + lowestUnaddressedPCIBridge = idx;
> + } else {
> + if (lowestAddressedPCIBridge > idx)
> + lowestAddressedPCIBridge = idx;
> + }
> }
> + }
> +
> + if (nbuses > 0 && !addrs->buses[0].model) {
> + if (virDomainPCIAddressBusSetModel(&addrs->buses[0],
> + VIR_DOMAIN_CONTROLLER_MODEL_PCI_ROOT) < 0)
> + goto error;
> + }
> +
> + /* Now fill in a reasonable model for all the buses in the set
> + * that don't yet have a corresponding controller in the domain
> + * config. In the rare (and actually fairly idiotic, but still
> + * allowed for some reason) case that a domain has 1) a pcie-root
> + * at index 0, 2)*no* dmi-to-pci-bridge (or pci-bridge that was
> + * manually addressed to sit directly on pcie-root), and 3) does
> + * have an unaddressed pci-bridge at an index > 1, then we need to
> + * add a dmi-to-pci-bridge.
> + */
> +
> + if (!hasPCIeRoot)
> + defaultModel = VIR_DOMAIN_CONTROLLER_MODEL_PCI_BRIDGE;
> +
> + else if (lowestUnaddressedPCIBridge < MIN(lowestAddressedPCIBridge,
> + lowestDMIToPCIBridge))
> + defaultModel = VIR_DOMAIN_CONTROLLER_MODEL_DMI_TO_PCI_BRIDGE;
> + else
> + defaultModel = VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT_PORT;
> +
> + for (i = 1; i < addrs->nbuses; i++) {
> +
> + if (addrs->buses[i].model)
> + continue;
> +
> + if (virDomainPCIAddressBusSetModel(&addrs->buses[i], defaultModel) < 0)
> + goto error;
> +
> + VIR_DEBUG("Auto-adding <controller type='pci' model='%s' index='%zu'/>",
> + virDomainControllerModelPCITypeToString(defaultModel), i);
> + /* only add a single dmi-to-pci-bridge, then add pcie-root-port
> + * for any other unspecified controller indexes.
> + */
> + if (hasPCIeRoot)
> + defaultModel = VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT_PORT;
> + }
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20161013/0d97479a/attachment-0001.htm>
More information about the libvir-list
mailing list