Re: [libvirt] [PATCH v3 15/18] qemu: auto-add pcie-root-port/dmi-to-pci-bridge controllers as needed
On 10/14/2016 01:20 PM, Andrea Bolognani wrote:
On Thu, 2016-10-13 at 13:43 -0400, Laine Stump wrote:
+if (nbuses > 0 && !addrs->buses[0].model) {
+if (virDomainPCIAddressBusSetModel(&addrs->buses[0],
+ VIR_DOMAIN_CONTROLLER_MODEL_PCI_ROOT)
< 0)
+goto error;
+}
Shouldn't we use either PCI_ROOT or PCIE_ROOT based on the
machine type here? And set hasPCIeRoot *after* doing so?
Sorry for the questions, I guess this is the point in the
patch where I got a bit lost :(
I'm afraid you missed this question :)
Sorry about the omission. I've tried to limit the code that decides
whether or not there should be a pci-root or a pcie-root to the one
place when default controllers are being added (I forget the name of the
function right now),
I guess you mean qemuDomainDefAddDefaultDevices()?
That's the function where pci{,e}-root is added, if not
already present in the configuration.
and after that only decide based on whether a
pci-root or pcie-root really is there in the config. My subconscious
reasoning for this is that the extra calisthenics around supporting
aarch64/virt with PCI(e) vs with mmio has made me wonder if there might
be machinetypes in the future that could support one type of root bus or
another (or none) according to what is in the config, rather than having
a root bus just builtin to the machine. I don't know if that would ever
happen, but if it did, this code would work without change - only the
function adding the default controllers would need to be changed.
(Note that I used the same logic when deciding how to right
qemuDomainMachineHasPCI[e]Root())(still not sure about that name, but it
can always be changed later to remove the "Machine" if someone doesn't
like it)
That makes sense.
My point is that the code above, if I'm reading it correctly,
sets the model of bus 0 to PCI_ROOT if it's not already set.
But
1) qemuDomainDefAddDefaultDevices() mentioned above should
already have added pci{,e}-root to @def
2) if that's not the case, we should use either PCI_ROOT
or PCIE_ROOT based on what's appropriate for the machine
type
Looking at the code in qemuDomainDefAddDefaultDevices() it
seems like we would never find ourselves in the situation
where pci{,e}-root is needed but not present in @def by the
time qemuDomainPCIAddressSetCreate() is called, so I think
that chunk of code should just be removed.
Truthfully the only reason it's there at all is because there was
similar code originally (which also was surely never needed). Even so,
I'm nervous about totally removing the check for unset model even though
a visual inspection of the current code tells us it won't be needed. So
instead, I'm going to turn it into an internal error condition.
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v3 15/18] qemu: auto-add pcie-root-port/dmi-to-pci-bridge controllers as needed
On Thu, 2016-10-13 at 13:43 -0400, Laine Stump wrote:
> > > > > +if (nbuses > 0 && !addrs->buses[0].model) {
> > > > > +if
(virDomainPCIAddressBusSetModel(&addrs->buses[0],
> > > > > +
> > > > > VIR_DOMAIN_CONTROLLER_MODEL_PCI_ROOT) < 0)
> > > > >
+goto error;
> > > > > +}
> > > >
> > > > Shouldn't we use either PCI_ROOT or PCIE_ROOT based on the
> > > > machine type here? And set
hasPCIeRoot *after* doing so?
> > > >
> > > > Sorry for the questions, I guess this is the point in the
> > > > patch where I got a bit lost :(
> >
> > I'm
afraid you missed this question :)
>
> Sorry about the omission. I've tried to limit the code that decides
> whether or not there should be a pci-root or a
pcie-root to the one
> place when default controllers are being added (I forget the name of the
> function right now),
I guess you mean qemuDomainDefAddDefaultDevices()?
That's the function where pci{,e}-root is added, if not
already present in the configuration.
> and after that only decide based on whether a
> pci-root or pcie-root really is there in the config. My subconscious
> reasoning for this is that the extra calisthenics around supporting
> aarch64/virt with PCI(e) vs with mmio has made me wonder if there might
> be machinetypes in the future that could support one type of root bus or
> another (or none) according to what is in the config, rather than having
> a root bus just builtin to the machine. I don't know if that would ever
> happen, but if it did, this code would work without change - only the
> function adding the default controllers would need to be changed.
>
> (Note that I used the same logic when deciding how to right
> qemuDomainMachineHasPCI[e]Root())(still not sure about that name, but it
> can always be changed later to remove the "Machine" if someone doesn't
> like it)
That makes sense.
My point is that the code above, if I'm reading it correctly,
sets the model of bus 0 to PCI_ROOT if it's not already set.
But
1) qemuDomainDefAddDefaultDevices() mentioned above should
already have added pci{,e}-root to @def
2) if that's not the case, we should use either PCI_ROOT
or PCIE_ROOT based on what's appropriate for the machine
type
Looking at the code in qemuDomainDefAddDefaultDevices() it
seems like we would never find ourselves in the situation
where pci{,e}-root is needed but not present in @def by the
time qemuDomainPCIAddressSetCreate() is called, so I think
that chunk of code should just be removed.
> > > > A DMI-to-PCI bridge with index='1' has been added
> > > > automatically here...
> > > >
> > > > ... but it's not being used by any of the devices. So why
> > > > would it be added in the first place?
> > >
> > > That is a *very* good question!
>
> > Can't wait to know the answer! ;)
>
> Oh, now that I've looked in context of the patch ordering, I undestand:
> it's because patch 16/18 hasn't been applied yet. I'd forgotten the
> ordering...
Right you are :)
--
Andrea Bolognani / Red Hat / Virtualization
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v3 15/18] qemu: auto-add pcie-root-port/dmi-to-pci-bridge controllers as needed
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:
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):
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) {
Re: [libvirt] [PATCH v3 15/18] qemu: auto-add pcie-root-port/dmi-to-pci-bridge controllers as needed
On 10/10/2016 02:14 PM, Andrea Bolognani wrote:
+} else if (flags & VIR_PCI_CONNECT_TYPE_PCI_BRIDGE) {
>+model = VIR_DOMAIN_CONTROLLER_MODEL_DMI_TO_PCI_BRIDGE;
I'm not so clear on this bit: if we're growing the address
set to plug in a pci-bridge, can we just go ahead and assume
a dmi-to-pci-bridge is what we need? What about eg. pseries
machine types, where dmi-to-pci-bridge is not usable?
What can you plug a pci-bridge into on a pseries machine? For all
machinetypes, if there is an unaddressed pci-bridge in the config and
there is a slot available that accepts a pci-bridge (i.e. pci-root,
pci-bridge, or dmi-to-pci-bridge) then we won't be calling
virDomainPCIAddressSetGrow() in the first place (since it's only called
if no empty slot with correct flags can be found). And I don't know
about pseries, but for 440fx, if there isn't a matching empty slot that
accepts a pci-bridge at that point, then it's not possible to add one
(you already have the one and only pci-root, and you're already trying
to add a pci-bridge --> if you have to add a pci-bridge in order to add
a pci-bridge, then you're in an infinite recursion).
So a dmi-to-pci-bridge is the only thing that could possibly help, and
in the cases where there is no pcie-root, it would just mean that you
would get an error later when you're told there's no place to plug in
the dmi-to-pci-bridge. So it makes sense for me to add a check here to
see if the model of addrs->buses[0] is PCIE_ROOT, and only try adding
the dmi-to-pci-bridge in that case.
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v3 15/18] qemu: auto-add pcie-root-port/dmi-to-pci-bridge controllers as needed
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:
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):
(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.
@@ -82,6 +82,30 @@
virDomainPCIControllerModelToConnectType(virDomainControllerModelPCI model)
return 0;
}
+
+static int
s/int/virDomainControllerModelPCI/
But then we can't return -1 when there isn't a perfect match (that's why
I made it int)
Right. Disregard my comments then :)
Alternatively you could turn it into
if ((flags & VIR_PCI_CONNECT_PCIE_DEVICE) ||
(flags & VIR_PCI_CONNECT_PCIE_SWITCH_UPSTREAM_PORT))
which is more obviously correct and also nicer to look at :)
but takes two operations instead of one.
I hardly think this would turn out to be a bottleneck, but
feel free to stick to the original implementation - after
fixing it, of course :)
+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;
+}
Shouldn't we use either PCI_ROOT or PCIE_ROOT based on the
machine type here? And set hasPCIeRoot *after* doing so?
Sorry for the questions, I guess this is the point in the
patch where I got a bit lost :(
I'm afraid you missed this question :)
Sorry about the omission. I've tried to limit the code that decides
whether or not there should be a pci-root or a pcie-root to the one
place when default controllers are being added (I forget the name of the
function right now), and after that only decide based on whether a
pci-root or pcie-root really is there in the config. My subconscious
reasoning for this is that the extra calisthenics around supporting
aarch64/virt with PCI(e) vs with mmio has made me wonder if there might
be machinetypes in the future that could support one type of root bus or
another (or none) according to what is in the config, rather than having
a root bus just builtin to the machine.
Re: [libvirt] [PATCH v3 15/18] qemu: auto-add pcie-root-port/dmi-to-pci-bridge controllers as needed
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:
> > >
> > >
> > >
> > >
> > > 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):
> > >
> > >
> > >
> > > (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.
> > > @@ -82,6 +82,30 @@
> > > virDomainPCIControllerModelToConnectType(virDomainControllerModelPCI
> > > model)
> > >return 0;
> > >}
> > >
> > > +
> > > +static int
> > s/int/virDomainControllerModelPCI/
>
> But then we can't return -1 when there isn't a perfect match (that's why
> I made it int)
Right. Disregard my comments then :)
> > Alternatively you could turn it into
> >
> >if ((flags & VIR_PCI_CONNECT_PCIE_DEVICE) ||
> >(flags & VIR_PCI_CONNECT_PCIE_SWITCH_UPSTREAM_PORT))
> >
> > which is more obviously correct and also nicer to look at :)
>
> but takes two operations instead of one.
I hardly think this would turn out to be a bottleneck, but
feel free to stick to the original implementation - after
fixing it, of course :)
> > > +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;
> > > +}
> >
> > Shouldn't we use either PCI_ROOT or PCIE_ROOT based on the
> > machine type here? And set hasPCIeRoot *after* doing so?
> >
> > Sorry for the questions, I guess this is the point in the
> > patch where I got a bit lost :(
I'm afraid you missed this question :)
> > > +else if (lowestUnaddressedPCIBridge < MIN(lowestAddressedPCIBridge,
> > > + lowestDMIToPCIBridge))
> > > +defaultModel = VIR_DOMAIN_CONTROLLER_MODEL_DMI_TO_PCI_BRIDGE;
> >
> > Again, a bit lost here, sorry :(
> >
> > Basically if we've found a PCI bridge without address that
> > can't be plugged into any existing PCI bridge or DMI-to-PCI
> > bridge, we want to add a DMI-to-PCI bridge so that we have
> > somewhere to plug it in, right?
And I'm pretty sure I got it right here, but just to be on
the safe side, it would be great if you could confirm or
deny :)
> > > +
Re: [libvirt] [PATCH v3 15/18] qemu: auto-add pcie-root-port/dmi-to-pci-bridge controllers as needed
On 10/10/2016 02:14 PM, Andrea Bolognani wrote: On Tue, 2016-09-20 at 15:14 -0400, Laine Stump wrote: Previously libvirt would only add pci-bridge devices automatically when an address was requested for a device that required a legacy PCI slot and none was available. This patch expands that support to dmi-to-pci-bridge (which is needed in order to add a pci-bridge on a machine with a pcie-root), and pcie-root-port (which is needed to add a hotpluggable PCIe device). It does *not* automatically add pcie-switch-upstream-ports or pcie-switch-downstream-ports (and currently there are no plans for that). Given the existing code to auto-add pci-bridge devices, automatically adding pcie-root-ports is fairly straightforward. The dmi-to-pci-bridge support is a bit tricky though, for a few reasons: 1) Although the only reason to add a dmi-to-pci-bridge is so that there is a reasonable place to plug in a pci-bridge controller, most of the time it's not the presence of a pci-bridge *in the config* that triggers the requirement to add a dmi-to-pci-bridge. Rather, it is the presence of a legacy-PCI device in the config, which triggers auto-add of a pci-bridge, which triggers auto-add of a dmi-to-pci-bridge (this is handled in virDomainPCIAddressSetGrow() - if there's a request to add a pci-bridge we'll check if there is a suitable bus to plug it into; if not, we first add a dmi-to-pci-bridge). 2) Once there is already a single dmi-to-pci-bridge on the system, there won't be a need for any more, even if it's full, as long as there is a pci-bridge with an open slot - you can also plug pci-bridges into existing pci-bridges. So we have to make sure we don't add a dmi-to-pci-bridge unless there aren't any dmi-to-pci-bridges *or* any pci-bridges. 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: 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): (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! 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? Although libvirt will now automatically create a dmi-to-pci-bridge when it's needed, the code still remains for now that forces a dmi-to-pci-bridge on all domains with pcie-root (in qemuDomainDefAddDefaultDevices()). That will be removed in the next patch. For now, the pcie-root-ports are added one to a slot, which is a bit wasteful and means it will fail after 31 total PCIe devices (30 if there are also some PCI devices), but helps keep the changeset down for this patch. A future patch will have 8 pcie-root-ports sharing the functions on a single slot. --- src/conf/domain_addr.c | 88 ++-- src/qemu/qemu_domain_address.c | 91 ++--- .../qemuxml2argv-q35-pcie-autoadd.args | 57 .../qemuxml2argv-q35-pcie-autoadd.xml | 51 +++ tests/qemuxml2argvtest.c | 23 .../qemuxml2xmlout-q35-pcie-autoadd.xml| 147 + tests/qemuxml2xmltest.c| 23 7 files changed, 448 insertions(+), 32 deletions(-) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-q35-pcie-autoadd.args create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-q35-pcie-autoadd.
Re: [libvirt] [PATCH v3 15/18] qemu: auto-add pcie-root-port/dmi-to-pci-bridge controllers as needed
On Tue, 2016-09-20 at 15:14 -0400, Laine Stump wrote:
> Previously libvirt would only add pci-bridge devices automatically
> when an address was requested for a device that required a legacy PCI
> slot and none was available. This patch expands that support to
> dmi-to-pci-bridge (which is needed in order to add a pci-bridge on a
> machine with a pcie-root), and pcie-root-port (which is needed to add
> a hotpluggable PCIe device). It does *not* automatically add
> pcie-switch-upstream-ports or pcie-switch-downstream-ports (and
> currently there are no plans for that).
>
> Given the existing code to auto-add pci-bridge devices, automatically
> adding pcie-root-ports is fairly straightforward. The
> dmi-to-pci-bridge support is a bit tricky though, for a few reasons:
>
> 1) Although the only reason to add a dmi-to-pci-bridge is so that
>there is a reasonable place to plug in a pci-bridge controller,
>most of the time it's not the presence of a pci-bridge *in the
>config* that triggers the requirement to add a dmi-to-pci-bridge.
>Rather, it is the presence of a legacy-PCI device in the config,
>which triggers auto-add of a pci-bridge, which triggers auto-add of
>a dmi-to-pci-bridge (this is handled in
>virDomainPCIAddressSetGrow() - if there's a request to add a
>pci-bridge we'll check if there is a suitable bus to plug it into;
>if not, we first add a dmi-to-pci-bridge).
>
> 2) Once there is already a single dmi-to-pci-bridge on the system,
>there won't be a need for any more, even if it's full, as long as
>there is a pci-bridge with an open slot - you can also plug
>pci-bridges into existing pci-bridges. So we have to make sure we
>don't add a dmi-to-pci-bridge unless there aren't any
>dmi-to-pci-bridges *or* any pci-bridges.
>
> 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:
>
>
>
>
>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):
>
>
>
>(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 :(
> Although libvirt will now automatically create a dmi-to-pci-bridge
> when it's needed, the code still remains for now that forces a
> dmi-to-pci-bridge on all domains with pcie-root (in
> qemuDomainDefAddDefaultDevices()). That will be removed in the next
> patch.
>
> For now, the pcie-root-ports are added one to a slot, which is a bit
> wasteful and means it will fail after 31 total PCIe devices (30 if
> there are also some PCI devices), but helps keep the changeset down
> for this patch. A future patch will have 8 pcie-root-ports sharing the
> functions on a single slot.
> ---
> src/conf/domain_addr.c | 88 ++--
> src/qemu/qemu_domain_address.c | 91 ++---
> .../qemuxml2argv-q35-pcie-autoadd.args | 57
> .../qemuxml2argv-q35-pcie-autoadd.xml | 51 +++
> tests/qemuxml2argvtest.c | 23
> .../qemuxml2xmlout-q35-pcie-autoadd.xml| 147
>+
> tests/qemuxml2xmltest.c| 23
> 7 files changed, 448 insertions(+), 32 deletions(-)
> create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-q35-pcie-autoadd.args
> create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-q35-pcie-autoadd.xml
> create mode 100644
>tests/qemuxml2xmloutdata/qemuxml2xmlout-q35-pcie-autoadd.xml
>
> diff --git a/src/conf/domain_addr.c b/src/conf/domain_addr.c
> index 12b5cb2..5f6f6f7 100644
> --- a/src/conf/domain_addr.c
> +++ b/src/conf/domain_addr.c
> @@ -82,6 +82,30 @@
> virDomainPCIControllerModelToConnectType(virDomainControllerModelPCI model)
> return 0;
> }
>
> +
> +static int
s/int/virDomainControllerModelPCI/
> +virDomainPCIControllerConnectTypeToModel(virDomainPCIConnectFlags flags)
> +{
> +if (flags & VIR_PCI_CONNECT_TYPE_PCIE_ROOT_PORT)
> +return VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT_PORT;
> +if (flags & VIR_PCI_CONNECT_TYPE_PCIE_SWITCH_UPSTREAM_PORT)
> +return VIR_DOMAIN_CONTROLLER_MODEL_PCIE_SWITCH_UPSTREAM_PORT;
> +if (flags & VIR_PCI_CONNECT_TYPE_PCIE_SWITCH_DOWNSTREAM_PORT)
> +return VIR_DOMAIN_CONTROLLER_MODEL_PCIE_SWITCH_DOWNSTREAM_PO
[libvirt] [PATCH v3 15/18] qemu: auto-add pcie-root-port/dmi-to-pci-bridge controllers as needed
Previously libvirt would only add pci-bridge devices automatically
when an address was requested for a device that required a legacy PCI
slot and none was available. This patch expands that support to
dmi-to-pci-bridge (which is needed in order to add a pci-bridge on a
machine with a pcie-root), and pcie-root-port (which is needed to add
a hotpluggable PCIe device). It does *not* automatically add
pcie-switch-upstream-ports or pcie-switch-downstream-ports (and
currently there are no plans for that).
Given the existing code to auto-add pci-bridge devices, automatically
adding pcie-root-ports is fairly straightforward. The
dmi-to-pci-bridge support is a bit tricky though, for a few reasons:
1) Although the only reason to add a dmi-to-pci-bridge is so that
there is a reasonable place to plug in a pci-bridge controller,
most of the time it's not the presence of a pci-bridge *in the
config* that triggers the requirement to add a dmi-to-pci-bridge.
Rather, it is the presence of a legacy-PCI device in the config,
which triggers auto-add of a pci-bridge, which triggers auto-add of
a dmi-to-pci-bridge (this is handled in
virDomainPCIAddressSetGrow() - if there's a request to add a
pci-bridge we'll check if there is a suitable bus to plug it into;
if not, we first add a dmi-to-pci-bridge).
2) Once there is already a single dmi-to-pci-bridge on the system,
there won't be a need for any more, even if it's full, as long as
there is a pci-bridge with an open slot - you can also plug
pci-bridges into existing pci-bridges. So we have to make sure we
don't add a dmi-to-pci-bridge unless there aren't any
dmi-to-pci-bridges *or* any pci-bridges.
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:
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):
(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.
Although libvirt will now automatically create a dmi-to-pci-bridge
when it's needed, the code still remains for now that forces a
dmi-to-pci-bridge on all domains with pcie-root (in
qemuDomainDefAddDefaultDevices()). That will be removed in the next
patch.
For now, the pcie-root-ports are added one to a slot, which is a bit
wasteful and means it will fail after 31 total PCIe devices (30 if
there are also some PCI devices), but helps keep the changeset down
for this patch. A future patch will have 8 pcie-root-ports sharing the
functions on a single slot.
---
src/conf/domain_addr.c | 88 ++--
src/qemu/qemu_domain_address.c | 91 ++---
.../qemuxml2argv-q35-pcie-autoadd.args | 57
.../qemuxml2argv-q35-pcie-autoadd.xml | 51 +++
tests/qemuxml2argvtest.c | 23
.../qemuxml2xmlout-q35-pcie-autoadd.xml| 147 +
tests/qemuxml2xmltest.c| 23
7 files changed, 448 insertions(+), 32 deletions(-)
create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-q35-pcie-autoadd.args
create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-q35-pcie-autoadd.xml
create mode 100644 tests/qemuxml2xmloutdata/qemuxml2xmlout-q35-pcie-autoadd.xml
diff --git a/src/conf/domain_addr.c b/src/conf/domain_addr.c
index 12b5cb2..5f6f6f7 100644
--- a/src/conf/domain_addr.c
+++ b/src/conf/domain_addr.c
@@ -82,6 +82,30 @@
virDomainPCIControllerModelToConnectType(virDomainControllerModelPCI model)
return 0;
}
+
+static int
+virDomainPCIControllerConnectTypeToModel(virDomainPCIConnectFlags flags)
+{
+if (flags & VIR_PCI_CONNECT_TYPE_PCIE_ROOT_PORT)
+return VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT_PORT;
+if (flags & VIR_PCI_CONNECT_TYPE_PCIE_SWITCH_UPSTREAM_PORT)
+return VIR_DOMAIN_CONTROLLER_MODEL_PCIE_SWITCH_UPSTREAM_PORT;
+if (flags & VIR_PCI_CONNECT_TYPE_PCIE_SWITCH_DOWNSTREAM_PORT)
+return VIR_DOMAIN_CONTROLLER_MODEL_PCIE_SWITCH_DOWNSTREAM_PORT;
+if (flags & VIR_PCI_CONNECT_TYPE_DMI_TO_PCI_BRIDGE)
+return VIR_DOMAIN_CONTROLLER_MODEL_DMI_TO_PCI_BRIDGE;
+if (flags & VIR_PCI_CONNECT_TYPE_PCI_EXPANDER_BUS)
+return VIR_DOMAIN_CONTROLLER_MODEL_PCI_EXPANDER_BUS;
+if (flags & VIR_PCI_CONNECT_TYPE_PCIE_EXPANDER_BUS)
+return VIR_DOMAIN_CONTROLLER_MODEL_PCIE_EXPANDER_BUS;
+if (flags & VIR_PCI_CONNECT_TYPE_PCI_BRIDGE)
+return VIR_DOMAIN_CONTROLLER_MODEL_PCI_BRIDGE;
+
+/* some connect types do
