[libvirt] [PATCH 00/11] qemu: Implement pcie-to-pci-bridge controller
Patches 1-4 are cleanups and preparation. For more information, take a peek at patch 8's commit message, which also includes a link to the relevant Bugzilla entry. Andrea Bolognani (11): docs: Tweak PCI controller model documentation tests: Add aarch64-traditional-pci test conf: Remove dubious code from virDomainPCIAddressSetGrow() conf: Rename virDomainPCIAddressSet.areMultipleRootsSupported qemu: Add QEMU_CAPS_DEVICE_PCIE_PCI_BRIDGE qemu: Implement pcie-to-pci-bridge controller conf: Add virDomainPCIAddressSet.isPCIeToPCIBridgeSupported conf: Prefer pcie-to-pci-bridge to dmi-to-pci-bridge tests: Use pcie-to-pci-bridge for aarch64-traditional-pci news: Add 4.3.0 release news: Update for pcie-to-pci-bridge support docs/formatdomain.html.in | 21 ++--- docs/news.xml | 17 ++++ docs/schemas/domaincommon.rng | 3 + src/conf/domain_addr.c | 95 ++++++++++++++++------ src/conf/domain_addr.h | 8 +- src/conf/domain_conf.c | 3 + src/conf/domain_conf.h | 2 + src/qemu/qemu_capabilities.c | 2 + src/qemu/qemu_capabilities.h | 1 + src/qemu/qemu_command.c | 1 + src/qemu/qemu_domain.c | 17 ++++ src/qemu/qemu_domain_address.c | 14 +++- tests/qemucapabilitiesdata/caps_2.12.0.aarch64.xml | 1 + tests/qemucapabilitiesdata/caps_2.12.0.x86_64.xml | 1 + .../qemuxml2argvdata/aarch64-traditional-pci.args | 27 ++++++ tests/qemuxml2argvdata/aarch64-traditional-pci.xml | 19 +++++ tests/qemuxml2argvtest.c | 9 ++ .../qemuxml2xmloutdata/aarch64-traditional-pci.xml | 43 ++++++++++ tests/qemuxml2xmltest.c | 9 ++ 19 files changed, 251 insertions(+), 42 deletions(-) create mode 100644 tests/qemuxml2argvdata/aarch64-traditional-pci.args create mode 100644 tests/qemuxml2argvdata/aarch64-traditional-pci.xml create mode 100644 tests/qemuxml2xmloutdata/aarch64-traditional-pci.xml -- 2.14.3
Instead of first listing the models on their own, and then listing them again grouped by the libvirt release they were introduced in, have a single list. Signed-off-by: Andrea Bolognani <abologna@redhat.com> --- docs/formatdomain.html.in | 20 ++++++++------------ 1 file changed, 8 insertions(+), 12 deletions(-) diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index 08dc74b6b9..299d7b9d42 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -3936,18 +3936,14 @@ <p> PCI controllers have an optional <code>model</code> attribute with - possible values <code>pci-root</code>, <code>pcie-root</code>, - <code>pcie-root-port</code>, <code>pci-bridge</code>, - <code>dmi-to-pci-bridge</code>, <code>pcie-switch-upstream-port</code>, - <code>pcie-switch-downstream-port</code>, <code>pci-expander-bus</code>, - or <code>pcie-expander-bus</code>. (pci-root and - pci-bridge <span class="since">since 1.0.5</span>, pcie-root and - dmi-to-pci-bridge <span class="since">since 1.1.2</span>, - pcie-root-port, pcie-switch-upstream-port, - pcie-switch-downstream-port <span class="since">since - 1.2.19</span>, and pci-expander-bus and - pcie-expander-bus <span class="since">since 1.3.4</span>) The - root controllers (<code>pci-root</code> + possible values <code>pci-root</code> and <code>pci-bridge</code> + (<span class="since">since 1.0.5</span>), <code>pcie-root</code> and + <code>dmi-to-pci-bridge</code> (<span class="since">since 1.1.2</span>), + <code>pcie-root-port</code>, <code>pcie-switch-upstream-port</code> and + <code>pcie-switch-downstream-port</code> (<span class="since">since + 1.2.19</span>), <code>pci-expander-bus</code> and + <code>pcie-expander-bus</code> (<span class="since">since 1.3.4</span>). + The root controllers (<code>pci-root</code> and <code>pcie-root</code>) have an optional <code>pcihole64</code> element specifying how big (in kilobytes, or in the unit specified by <code>pcihole64</code>'s -- 2.14.3
On 03/28/2018 10:06 AM, Andrea Bolognani wrote:
Instead of first listing the models on their own, and then listing them again grouped by the libvirt release they were introduced in, have a single list.
Signed-off-by: Andrea Bolognani <abologna@redhat.com> --- docs/formatdomain.html.in | 20 ++++++++------------ 1 file changed, 8 insertions(+), 12 deletions(-)
Personally I don't find it any easier to read ;-) - it's all a mash of letters and dashes... Of course true of other parts on formatdomain too. Still, how about something like this: <table class="top_table"> <tr> <th> Value </th> <th> Since </th> </tr> <tr> <td> pci-root </td> <td> <span class="since">since 1.0.5</span> </td> </tr> <tr> <td> pci-bridge </td> <td> <span class="since">since 1.0.5</span> </td> </tr> <tr> <td> pcie-root </td> <td> <span class="since">since 1.1.2</span> </td> </tr> <tr> <td> dmi-to-pci-bridge </td> <td> <span class="since">since 1.1.2</span> </td> </tr> <tr> <td> pcie-root-port </td> <td> <span class="since">since 1.2.19</span> </td> </tr> <tr> <td> pcie-switch-upstream-port </td> <td> <span class="since">since 1.2.19</span> </td> </tr> <tr> <td> pcie-switch-downstream-port </td> <td> <span class="since">since 1.2.19</span> </td> </tr> <tr> <td> pci-expander-bus </td> <td> <span class="since">since 1.3.4</span> </td> </tr> <tr> <td> pcie-expander-bus </td> <td> <span class="since">since 1.3.4</span> </td> </tr> </table> </p> <p> You could choose different labels if you desired... Also consider the possibilities for expansion w/r/t providing even a *little* bit more "Description" details about what each of these things are and when/how/why they'd be used (far beyond the scope of this patch though). I would think some of that could be drawn from the descriptions that exist "just above" the "Device leases" section. IOW: a relationship matrix... With at least the table, Reviewed-by: John Ferlan <jferlan@redhat.com> John
diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index 08dc74b6b9..299d7b9d42 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -3936,18 +3936,14 @@
<p> PCI controllers have an optional <code>model</code> attribute with - possible values <code>pci-root</code>, <code>pcie-root</code>, - <code>pcie-root-port</code>, <code>pci-bridge</code>, - <code>dmi-to-pci-bridge</code>, <code>pcie-switch-upstream-port</code>, - <code>pcie-switch-downstream-port</code>, <code>pci-expander-bus</code>, - or <code>pcie-expander-bus</code>. (pci-root and - pci-bridge <span class="since">since 1.0.5</span>, pcie-root and - dmi-to-pci-bridge <span class="since">since 1.1.2</span>, - pcie-root-port, pcie-switch-upstream-port, - pcie-switch-downstream-port <span class="since">since - 1.2.19</span>, and pci-expander-bus and - pcie-expander-bus <span class="since">since 1.3.4</span>) The - root controllers (<code>pci-root</code> + possible values <code>pci-root</code> and <code>pci-bridge</code> + (<span class="since">since 1.0.5</span>), <code>pcie-root</code> and + <code>dmi-to-pci-bridge</code> (<span class="since">since 1.1.2</span>), + <code>pcie-root-port</code>, <code>pcie-switch-upstream-port</code> and + <code>pcie-switch-downstream-port</code> (<span class="since">since + 1.2.19</span>), <code>pci-expander-bus</code> and + <code>pcie-expander-bus</code> (<span class="since">since 1.3.4</span>). + The root controllers (<code>pci-root</code> and <code>pcie-root</code>) have an optional <code>pcihole64</code> element specifying how big (in kilobytes, or in the unit specified by <code>pcihole64</code>'s
On Tue, 2018-04-03 at 19:11 -0400, John Ferlan wrote:
On 03/28/2018 10:06 AM, Andrea Bolognani wrote:
Instead of first listing the models on their own, and then listing them again grouped by the libvirt release they were introduced in, have a single list.
Signed-off-by: Andrea Bolognani <abologna@redhat.com> --- docs/formatdomain.html.in | 20 ++++++++------------ 1 file changed, 8 insertions(+), 12 deletions(-)
Personally I don't find it any easier to read ;-) - it's all a mash of letters and dashes... Of course true of other parts on formatdomain too.
The goal was avoiding repeated information, not necessarily making the whole thing easier to read. But I agree that's a laudable goal as well :)
Still, how about something like this:
<table class="top_table"> <tr> <th> Value </th> <th> Since </th> </tr> <tr> <td> pci-root </td> <td> <span class="since">since 1.0.5</span> </td> [...] </tr>
You could choose different labels if you desired...
Using a table for this seems like a waste of vertical space to me, and there are no existing examples of anything like that in the documentation AFAICT. Maybe a <ul> instead?
Also consider the possibilities for expansion w/r/t providing even a *little* bit more "Description" details about what each of these things are and when/how/why they'd be used (far beyond the scope of this patch though). I would think some of that could be drawn from the descriptions that exist "just above" the "Device leases" section. IOW: a relationship matrix...
Is the information already present below in free-text format not enough? I don't think we can compress that information in a format that would be suitable for a table. In any case, I agree it would be entirely out of scope here. -- Andrea Bolognani / Red Hat / Virtualization
On 04/04/2018 04:39 AM, Andrea Bolognani wrote:
On Tue, 2018-04-03 at 19:11 -0400, John Ferlan wrote:
On 03/28/2018 10:06 AM, Andrea Bolognani wrote:
Instead of first listing the models on their own, and then listing them again grouped by the libvirt release they were introduced in, have a single list.
Signed-off-by: Andrea Bolognani <abologna@redhat.com> --- docs/formatdomain.html.in | 20 ++++++++------------ 1 file changed, 8 insertions(+), 12 deletions(-)
Personally I don't find it any easier to read ;-) - it's all a mash of letters and dashes... Of course true of other parts on formatdomain too.
The goal was avoiding repeated information, not necessarily making the whole thing easier to read. But I agree that's a laudable goal as well :)
Understood - to the degree of keeping since w/ the possible values is better than the previous format. I guess I still found it difficult to read so looked for something else more readable.
Still, how about something like this:
<table class="top_table"> <tr> <th> Value </th> <th> Since </th> </tr> <tr> <td> pci-root </td> <td> <span class="since">since 1.0.5</span> </td> [...] </tr>
You could choose different labels if you desired...
Using a table for this seems like a waste of vertical space to me, and there are no existing examples of anything like that in the documentation AFAICT. Maybe a <ul> instead?
Table or list is fine - I guess I just landed on a part of the page with a table and thought this would be so much nicer than a paragraph of data where there's multiple since labels. FWIW: The hyperv description of the power management section uses a table w/ a since column.
Also consider the possibilities for expansion w/r/t providing even a *little* bit more "Description" details about what each of these things are and when/how/why they'd be used (far beyond the scope of this patch though). I would think some of that could be drawn from the descriptions that exist "just above" the "Device leases" section. IOW: a relationship matrix...
Is the information already present below in free-text format not enough? I don't think we can compress that information in a format that would be suitable for a table.
In any case, I agree it would be entirely out of scope here.
The information free form is sufficient of course it's easy to miss too, but at this point baby steps and out of context. In any case, if you want to not make any extra changes, then I guess that's fine since your patch is better than the original; however, taking the extra step for list or table I think would be better. The R-b can stick with for whatever option you choose. John
This test shows what happens when you add a traditional PCI device such as pci-serial to a pure PCIe machine type such as aarch64/virt. Signed-off-by: Andrea Bolognani <abologna@redhat.com> --- .../qemuxml2argvdata/aarch64-traditional-pci.args | 26 +++++++++++++ tests/qemuxml2argvdata/aarch64-traditional-pci.xml | 19 ++++++++++ tests/qemuxml2argvtest.c | 8 ++++ .../qemuxml2xmloutdata/aarch64-traditional-pci.xml | 43 ++++++++++++++++++++++ tests/qemuxml2xmltest.c | 8 ++++ 5 files changed, 104 insertions(+) create mode 100644 tests/qemuxml2argvdata/aarch64-traditional-pci.args create mode 100644 tests/qemuxml2argvdata/aarch64-traditional-pci.xml create mode 100644 tests/qemuxml2xmloutdata/aarch64-traditional-pci.xml diff --git a/tests/qemuxml2argvdata/aarch64-traditional-pci.args b/tests/qemuxml2argvdata/aarch64-traditional-pci.args new file mode 100644 index 0000000000..f10252f729 --- /dev/null +++ b/tests/qemuxml2argvdata/aarch64-traditional-pci.args @@ -0,0 +1,26 @@ +LC_ALL=C \ +PATH=/bin \ +HOME=/home/test \ +USER=test \ +LOGNAME=test \ +QEMU_AUDIO_DRV=none \ +/usr/bin/qemu-system-aarch64 \ +-name guest \ +-S \ +-M virt \ +-m 1024 \ +-smp 1,sockets=1,cores=1,threads=1 \ +-uuid 496d7ea8-9739-544b-4ebd-ef08be936e8b \ +-nographic \ +-nodefconfig \ +-nodefaults \ +-chardev socket,id=charmonitor,path=/tmp/lib/domain--1-guest/monitor.sock,\ +server,nowait \ +-mon chardev=charmonitor,id=monitor,mode=readline \ +-no-acpi \ +-boot c \ +-device i82801b11-bridge,id=pci.1,bus=pcie.0,addr=0x1 \ +-device pci-bridge,chassis_nr=2,id=pci.2,bus=pci.1,addr=0x0 \ +-device pcie-root-port,port=0x10,chassis=3,id=pci.3,bus=pcie.0,addr=0x2 \ +-chardev pty,id=charserial0 \ +-device pci-serial,chardev=charserial0,id=serial0,bus=pci.2,addr=0x1 diff --git a/tests/qemuxml2argvdata/aarch64-traditional-pci.xml b/tests/qemuxml2argvdata/aarch64-traditional-pci.xml new file mode 100644 index 0000000000..8c87a27f6e --- /dev/null +++ b/tests/qemuxml2argvdata/aarch64-traditional-pci.xml @@ -0,0 +1,19 @@ +<domain type="qemu"> + <name>guest</name> + <uuid>496d7ea8-9739-544b-4ebd-ef08be936e8b</uuid> + <memory>1048576</memory> + <vcpu placement='static'>1</vcpu> + <os> + <type arch="aarch64" machine="virt">hvm</type> + </os> + <devices> + <emulator>/usr/bin/qemu-system-aarch64</emulator> + <controller type='pci' model='pcie-root'/> + <!-- pci-serial is a traditional PCI device, and will cause a + traditional PCI topology to be created for the guest --> + <serial type='pty'> + <target type='pci-serial'/> + </serial> + <memballoon model='none'/> + </devices> +</domain> diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c index 308d71f725..9a0d01e4f4 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -2705,6 +2705,14 @@ mymain(void) QEMU_CAPS_DEVICE_PCI_BRIDGE, QEMU_CAPS_DEVICE_DMI_TO_PCI_BRIDGE, QEMU_CAPS_DEVICE_PCIE_ROOT_PORT); + DO_TEST("aarch64-traditional-pci", + QEMU_CAPS_NODEFCONFIG, + QEMU_CAPS_OBJECT_GPEX, + QEMU_CAPS_PCI_MULTIFUNCTION, + QEMU_CAPS_DEVICE_PCIE_ROOT_PORT, + QEMU_CAPS_DEVICE_DMI_TO_PCI_BRIDGE, + QEMU_CAPS_DEVICE_PCI_BRIDGE, + QEMU_CAPS_DEVICE_PCI_SERIAL); /* Make sure all combinations of ACPI and UEFI behave as expected */ DO_TEST("aarch64-acpi-uefi", NONE); diff --git a/tests/qemuxml2xmloutdata/aarch64-traditional-pci.xml b/tests/qemuxml2xmloutdata/aarch64-traditional-pci.xml new file mode 100644 index 0000000000..70664aa4f9 --- /dev/null +++ b/tests/qemuxml2xmloutdata/aarch64-traditional-pci.xml @@ -0,0 +1,43 @@ +<domain type='qemu'> + <name>guest</name> + <uuid>496d7ea8-9739-544b-4ebd-ef08be936e8b</uuid> + <memory unit='KiB'>1048576</memory> + <currentMemory unit='KiB'>1048576</currentMemory> + <vcpu placement='static'>1</vcpu> + <os> + <type arch='aarch64' machine='virt'>hvm</type> + <boot dev='hd'/> + </os> + <features> + <gic version='2'/> + </features> + <clock offset='utc'/> + <on_poweroff>destroy</on_poweroff> + <on_reboot>restart</on_reboot> + <on_crash>destroy</on_crash> + <devices> + <emulator>/usr/bin/qemu-system-aarch64</emulator> + <controller type='pci' index='0' model='pcie-root'/> + <controller type='pci' index='1' model='dmi-to-pci-bridge'> + <model name='i82801b11-bridge'/> + <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x0'/> + </controller> + <controller type='pci' index='2' model='pci-bridge'> + <model name='pci-bridge'/> + <target chassisNr='2'/> + <address type='pci' domain='0x0000' bus='0x01' slot='0x00' function='0x0'/> + </controller> + <controller type='pci' index='3' model='pcie-root-port'> + <model name='pcie-root-port'/> + <target chassis='3' port='0x10'/> + <address type='pci' domain='0x0000' bus='0x00' slot='0x02' function='0x0'/> + </controller> + <serial type='pty'> + <target type='pci-serial' port='0'> + <model name='pci-serial'/> + </target> + <address type='pci' domain='0x0000' bus='0x02' slot='0x01' function='0x0'/> + </serial> + <memballoon model='none'/> + </devices> +</domain> diff --git a/tests/qemuxml2xmltest.c b/tests/qemuxml2xmltest.c index 0f560290a0..49b61d7647 100644 --- a/tests/qemuxml2xmltest.c +++ b/tests/qemuxml2xmltest.c @@ -1117,6 +1117,14 @@ mymain(void) QEMU_CAPS_DEVICE_PCI_BRIDGE, QEMU_CAPS_DEVICE_DMI_TO_PCI_BRIDGE, QEMU_CAPS_DEVICE_PCIE_ROOT_PORT); + DO_TEST("aarch64-traditional-pci", + QEMU_CAPS_NODEFCONFIG, + QEMU_CAPS_OBJECT_GPEX, + QEMU_CAPS_PCI_MULTIFUNCTION, + QEMU_CAPS_DEVICE_PCIE_ROOT_PORT, + QEMU_CAPS_DEVICE_DMI_TO_PCI_BRIDGE, + QEMU_CAPS_DEVICE_PCI_BRIDGE, + QEMU_CAPS_DEVICE_PCI_SERIAL); DO_TEST("aarch64-video-default", QEMU_CAPS_NODEFCONFIG, QEMU_CAPS_OBJECT_GPEX, -- 2.14.3
On 03/28/2018 10:06 AM, Andrea Bolognani wrote:
This test shows what happens when you add a traditional PCI device such as pci-serial to a pure PCIe machine type such as aarch64/virt.
Signed-off-by: Andrea Bolognani <abologna@redhat.com> --- .../qemuxml2argvdata/aarch64-traditional-pci.args | 26 +++++++++++++ tests/qemuxml2argvdata/aarch64-traditional-pci.xml | 19 ++++++++++ tests/qemuxml2argvtest.c | 8 ++++ .../qemuxml2xmloutdata/aarch64-traditional-pci.xml | 43 ++++++++++++++++++++++ tests/qemuxml2xmltest.c | 8 ++++ 5 files changed, 104 insertions(+) create mode 100644 tests/qemuxml2argvdata/aarch64-traditional-pci.args create mode 100644 tests/qemuxml2argvdata/aarch64-traditional-pci.xml create mode 100644 tests/qemuxml2xmloutdata/aarch64-traditional-pci.xml
Trust is such a wonderful thing... Reviewed-by: John Ferlan <jferlan@redhat.com> John
I haven't been able to come up with a single scenario in which the code in question would be executed; even if there was one, it would be due to the user specifying a *partial* PCI topology in the guest XML, which is of course entirely unsupportable and thus providing even the slightest hint that doing so is in any way a good idea is actively harmful. Signed-off-by: Andrea Bolognani <abologna@redhat.com> --- src/conf/domain_addr.c | 9 --------- 1 file changed, 9 deletions(-) diff --git a/src/conf/domain_addr.c b/src/conf/domain_addr.c index 0c914fe25c..18b6f8d588 100644 --- a/src/conf/domain_addr.c +++ b/src/conf/domain_addr.c @@ -447,15 +447,6 @@ virDomainPCIAddressSetGrow(virDomainPCIAddressSetPtr addrs, addr->bus++; } } - } else if (flags & VIR_PCI_CONNECT_TYPE_PCI_BRIDGE && - addrs->buses[0].model == VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT) { - /* NB: if the root bus is pci-root, and we couldn't find an - * open place to connect a pci-bridge, then there is nothing - * we can do (since the only way to gain a new slot that - * accepts a pci-bridge is to add *a pci-bridge* (which is the - * reason we're here in the first place!) - */ - model = VIR_DOMAIN_CONTROLLER_MODEL_DMI_TO_PCI_BRIDGE; } else if (flags & (VIR_PCI_CONNECT_TYPE_PCIE_DEVICE | VIR_PCI_CONNECT_TYPE_PCIE_SWITCH_UPSTREAM_PORT)) { model = VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT_PORT; -- 2.14.3
On 03/28/2018 10:06 AM, Andrea Bolognani wrote:
I haven't been able to come up with a single scenario in which the code in question would be executed; even if there was one, it would be due to the user specifying a *partial* PCI topology in the guest XML, which is of course entirely unsupportable and thus providing even the slightest hint that doing so is in any way a good idea is actively harmful.
Signed-off-by: Andrea Bolognani <abologna@redhat.com> --- src/conf/domain_addr.c | 9 --------- 1 file changed, 9 deletions(-)
Since Laine added this code - perhaps calling his name out on the CC list will allow him to appear and answer the question. Although it could take 3 such utterances (e.g. beetlejuice)... Personally, it seems reasonable and you didn't have to change any test output, but PCI connection requirements are black boxes to me. I know I cannot plug my US plug into the CZ outlets, but when it comes to PCI connection rules - I'm glad someone else knows them! A soft and mostly unqualified, Reviewed-by: John Ferlan <jferlan@redhat.com> John
diff --git a/src/conf/domain_addr.c b/src/conf/domain_addr.c index 0c914fe25c..18b6f8d588 100644 --- a/src/conf/domain_addr.c +++ b/src/conf/domain_addr.c @@ -447,15 +447,6 @@ virDomainPCIAddressSetGrow(virDomainPCIAddressSetPtr addrs, addr->bus++; } } - } else if (flags & VIR_PCI_CONNECT_TYPE_PCI_BRIDGE && - addrs->buses[0].model == VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT) { - /* NB: if the root bus is pci-root, and we couldn't find an - * open place to connect a pci-bridge, then there is nothing - * we can do (since the only way to gain a new slot that - * accepts a pci-bridge is to add *a pci-bridge* (which is the - * reason we're here in the first place!) - */ - model = VIR_DOMAIN_CONTROLLER_MODEL_DMI_TO_PCI_BRIDGE; } else if (flags & (VIR_PCI_CONNECT_TYPE_PCIE_DEVICE | VIR_PCI_CONNECT_TYPE_PCIE_SWITCH_UPSTREAM_PORT)) { model = VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT_PORT;
On 04/03/2018 07:12 PM, John Ferlan wrote:
On 03/28/2018 10:06 AM, Andrea Bolognani wrote:
I haven't been able to come up with a single scenario in which the code in question would be executed; even if there was one, it would be due to the user specifying a *partial* PCI topology in the guest XML, which is of course entirely unsupportable and thus providing even the slightest hint that doing so is in any way a good idea is actively harmful.
Signed-off-by: Andrea Bolognani <abologna@redhat.com> --- src/conf/domain_addr.c | 9 --------- 1 file changed, 9 deletions(-)
Since Laine added this code - perhaps calling his name out on the CC list will allow him to appear and answer the question. Although it could take 3 such utterances (e.g. beetlejuice)...
I would hope that such a reference wouldn't need to be explained :-)
Personally, it seems reasonable and you didn't have to change any test output, but PCI connection requirements are black boxes to me. I know I cannot plug my US plug into the CZ outlets,
Sure you can - you just need properly sized large bare wires.
but when it comes to PCI connection rules - I'm glad someone else knows them!
A soft and mostly unqualified,
Reviewed-by: John Ferlan <jferlan@redhat.com>
John
diff --git a/src/conf/domain_addr.c b/src/conf/domain_addr.c index 0c914fe25c..18b6f8d588 100644 --- a/src/conf/domain_addr.c +++ b/src/conf/domain_addr.c @@ -447,15 +447,6 @@ virDomainPCIAddressSetGrow(virDomainPCIAddressSetPtr addrs, addr->bus++; } } - } else if (flags & VIR_PCI_CONNECT_TYPE_PCI_BRIDGE && - addrs->buses[0].model == VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT) { - /* NB: if the root bus is pci-root, and we couldn't find an - * open place to connect a pci-bridge, then there is nothing - * we can do (since the only way to gain a new slot that - * accepts a pci-bridge is to add *a pci-bridge* (which is the - * reason we're here in the first place!) - */ - model = VIR_DOMAIN_CONTROLLER_MODEL_DMI_TO_PCI_BRIDGE;
This is saying that if the device you need a slot for is a pci-bridge (or has exactly the same connection requirements as a pci-bridge) and if the root bus is pcie-root then you need to add a dmi-to-pci-bridge (you wouldn't be in this function in the first place if you already *had* a dmi-to-pci-bridge, so there's no need to check if you have one). This would happen if someone manually added a pci-bridge device to a pure pcie topology. Normally I would say that this should stay, as I believe we *should* try to support partially-specified topologies as much as possible (since you've dealt with libvirt-qe bugzilla reports, you too know of some of the odd scenarios they come up with - e.g. removing one controller from a working config.). However, currently when an un-addressed pci-bridge is encountered (which is the only time we should get to this code), the search for a slot that accepts a pci-bridge will find an empty slot on the *that same pci-bridge* and we end up logging an error indicating that (I forget the exact error) - I could *swear* that at some point in the past it wasn't dead code, and that it actually helped to resolve a bug filed by libvirt-qe, but experimentation shows that certainly isn't the case now. In the meantime, if I remove that code (and don't apply any of your patches) then a pure pcie domain *can* be successfully edited to add a single pci-bridge (as long as you specify an index, *and* there is an unused index of a smaller value), but what ends up in the "proper/validated" config is a pci-bridge that is plugged directly into a pcie-root-port (which is also wrong, but silently so). So I guess I reserve my judgement until I see what happens with your entire series applied, which I'll do tomorrow morning.
} else if (flags & (VIR_PCI_CONNECT_TYPE_PCIE_DEVICE | VIR_PCI_CONNECT_TYPE_PCIE_SWITCH_UPSTREAM_PORT)) { model = VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT_PORT;
On Tue, 2018-04-03 at 22:44 -0400, Laine Stump wrote:
On 04/03/2018 07:12 PM, John Ferlan wrote:
Since Laine added this code - perhaps calling his name out on the CC list will allow him to appear and answer the question.
Fair point. I kinda just assumed Laine would be the only one crazy^Wbrave enough to attempt a review for this series :P
- } else if (flags & VIR_PCI_CONNECT_TYPE_PCI_BRIDGE && - addrs->buses[0].model == VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT) { - /* NB: if the root bus is pci-root, and we couldn't find an - * open place to connect a pci-bridge, then there is nothing - * we can do (since the only way to gain a new slot that - * accepts a pci-bridge is to add *a pci-bridge* (which is the - * reason we're here in the first place!) - */ - model = VIR_DOMAIN_CONTROLLER_MODEL_DMI_TO_PCI_BRIDGE;
This is saying that if the device you need a slot for is a pci-bridge (or has exactly the same connection requirements as a pci-bridge) and if the root bus is pcie-root then you need to add a dmi-to-pci-bridge (you wouldn't be in this function in the first place if you already *had* a dmi-to-pci-bridge, so there's no need to check if you have one). This would happen if someone manually added a pci-bridge device to a pure pcie topology.
Normally I would say that this should stay, as I believe we *should* try to support partially-specified topologies as much as possible (since you've dealt with libvirt-qe bugzilla reports, you too know of some of the odd scenarios they come up with - e.g. removing one controller from a working config.).
I'm not sure I agree here. There are just way too many corner cases for us to hope we can deal with all of them reasonably, let alone without introducing heaps of fragile and difficult to understand code, and supporting some of them can give users the impression that *all* of them are going to work, no matter how insane. I'd rather back the reasonable, supportable use cases with rock-solid code and error out when the user is asking too much of libvirt.
However, currently when an un-addressed pci-bridge is encountered (which is the only time we should get to this code), the search for a slot that accepts a pci-bridge will find an empty slot on the *that same pci-bridge* and we end up logging an error indicating that (I forget the exact error) - I could *swear* that at some point in the past it wasn't dead code, and that it actually helped to resolve a bug filed by libvirt-qe, but experimentation shows that certainly isn't the case now.
This matches my understanding, and my experience after playing with it for a fairly long time.
In the meantime, if I remove that code (and don't apply any of your patches) then a pure pcie domain *can* be successfully edited to add a single pci-bridge (as long as you specify an index, *and* there is an unused index of a smaller value), but what ends up in the "proper/validated" config is a pci-bridge that is plugged directly into a pcie-root-port (which is also wrong, but silently so).
So I guess I reserve my judgement until I see what happens with your entire series applied, which I'll do tomorrow morning.
Looking forward to that :) -- Andrea Bolognani / Red Hat / Virtualization
On 04/04/2018 04:54 AM, Andrea Bolognani wrote:
On Tue, 2018-04-03 at 22:44 -0400, Laine Stump wrote:
In the meantime, if I remove that code (and don't apply any of your patches) then a pure pcie domain *can* be successfully edited to add a single pci-bridge (as long as you specify an index, *and* there is an unused index of a smaller value), but what ends up in the "proper/validated" config is a pci-bridge that is plugged directly into a pcie-root-port (which is also wrong, but silently so).
So I guess I reserve my judgement until I see what happens with your entire series applied, which I'll do tomorrow morning. Looking forward to that :)
So it turns out that once all of your patches are applied, the error is the same before or after this code is removed, so I think it may not be executed at all. Since it doesn't solve the issue it originally was intended to solve, and removing it makes no functional difference, I have no problem with you removing it (it would be nice if we could figure out a way to log a more informative error than: Â Â Â unsupported configuration: PCI controller at index 4 (0x04) has bus='0x04', Â Â Â but index must be larger than bus but that's a separate issue.)
On Fri, 2018-04-06 at 14:23 -0400, Laine Stump wrote:
So it turns out that once all of your patches are applied, the error is the same before or after this code is removed, so I think it may not be executed at all.
Since it doesn't solve the issue it originally was intended to solve, and removing it makes no functional difference, I have no problem with you removing it (it would be nice if we could figure out a way to log a more informative error than:
unsupported configuration: PCI controller at index 4 (0x04) has bus='0x04',
but index must be larger than bus
but that's a separate issue.)
Yeah, that would be nice indeed. I've pushed the patch in the meantime, thanks for looking at it :) -- Andrea Bolognani / Red Hat / Virtualization
We're going to add a similarly-named attribute later, and we'd like to be consistent. Signed-off-by: Andrea Bolognani <abologna@redhat.com> --- src/conf/domain_addr.c | 2 +- src/conf/domain_addr.h | 2 +- src/qemu/qemu_domain_address.c | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/src/conf/domain_addr.c b/src/conf/domain_addr.c index 18b6f8d588..e02d7ac614 100644 --- a/src/conf/domain_addr.c +++ b/src/conf/domain_addr.c @@ -413,7 +413,7 @@ virDomainPCIAddressSetGrow(virDomainPCIAddressSetPtr addrs, */ if (flags & VIR_PCI_CONNECT_TYPE_PCI_DEVICE) { - if (addrs->multipleRootsSupported) { + if (addrs->areMultipleRootsSupported) { /* Use a pci-root controller to expand the guest's PCI * topology if it supports having more than one */ model = VIR_DOMAIN_CONTROLLER_MODEL_PCI_ROOT; diff --git a/src/conf/domain_addr.h b/src/conf/domain_addr.h index d3541bab09..87986c2bb7 100644 --- a/src/conf/domain_addr.h +++ b/src/conf/domain_addr.h @@ -115,7 +115,7 @@ struct _virDomainPCIAddressSet { bool dryRun; /* on a dry run, new buses are auto-added and addresses aren't saved in device infos */ /* If true, the guest can have multiple pci-root controllers */ - bool multipleRootsSupported; + bool areMultipleRootsSupported; }; typedef struct _virDomainPCIAddressSet virDomainPCIAddressSet; typedef virDomainPCIAddressSet *virDomainPCIAddressSetPtr; diff --git a/src/qemu/qemu_domain_address.c b/src/qemu/qemu_domain_address.c index 37473ebf13..40fb540adc 100644 --- a/src/qemu/qemu_domain_address.c +++ b/src/qemu/qemu_domain_address.c @@ -1353,7 +1353,7 @@ qemuDomainPCIAddressSetCreate(virDomainDefPtr def, /* pSeries domains support multiple pci-root controllers */ if (qemuDomainIsPSeries(def)) - addrs->multipleRootsSupported = true; + addrs->areMultipleRootsSupported = true; for (i = 0; i < def->ncontrollers; i++) { virDomainControllerDefPtr cont = def->controllers[i]; -- 2.14.3
On 03/28/2018 10:06 AM, Andrea Bolognani wrote:
We're going to add a similarly-named attribute later, and we'd like to be consistent.
Signed-off-by: Andrea Bolognani <abologna@redhat.com> --- src/conf/domain_addr.c | 2 +- src/conf/domain_addr.h | 2 +- src/qemu/qemu_domain_address.c | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-)
Reviewed-by: John Ferlan <jferlan@redhat.com> John
This capability will be set when the pcie-pci-bridge device is available in the QEMU binary. Signed-off-by: Andrea Bolognani <abologna@redhat.com> --- src/qemu/qemu_capabilities.c | 2 ++ src/qemu/qemu_capabilities.h | 1 + tests/qemucapabilitiesdata/caps_2.12.0.aarch64.xml | 1 + tests/qemucapabilitiesdata/caps_2.12.0.x86_64.xml | 1 + 4 files changed, 5 insertions(+) diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c index e54dde69ab..840f520370 100644 --- a/src/qemu/qemu_capabilities.c +++ b/src/qemu/qemu_capabilities.c @@ -466,6 +466,7 @@ VIR_ENUM_IMPL(virQEMUCaps, QEMU_CAPS_LAST, /* 285 */ "virtio-mouse-ccw", "virtio-tablet-ccw", + "pcie-pci-bridge", ); @@ -1705,6 +1706,7 @@ struct virQEMUCapsStringFlags virQEMUCapsObjectTypes[] = { { "virtio-keyboard-ccw", QEMU_CAPS_DEVICE_VIRTIO_KEYBOARD_CCW }, { "virtio-mouse-ccw", QEMU_CAPS_DEVICE_VIRTIO_MOUSE_CCW }, { "virtio-tablet-ccw", QEMU_CAPS_DEVICE_VIRTIO_TABLET_CCW }, + { "pcie-pci-bridge", QEMU_CAPS_DEVICE_PCIE_PCI_BRIDGE }, }; static struct virQEMUCapsStringFlags virQEMUCapsObjectPropsVirtioBalloon[] = { diff --git a/src/qemu/qemu_capabilities.h b/src/qemu/qemu_capabilities.h index 3f3c29f8fb..50f359857c 100644 --- a/src/qemu/qemu_capabilities.h +++ b/src/qemu/qemu_capabilities.h @@ -450,6 +450,7 @@ typedef enum { /* 285 */ QEMU_CAPS_DEVICE_VIRTIO_MOUSE_CCW, /* -device virtio-mouse-ccw */ QEMU_CAPS_DEVICE_VIRTIO_TABLET_CCW, /* -device virtio-tablet-ccw */ + QEMU_CAPS_DEVICE_PCIE_PCI_BRIDGE, /* -device pcie-pci-bridge */ QEMU_CAPS_LAST /* this must always be the last item */ } virQEMUCapsFlags; diff --git a/tests/qemucapabilitiesdata/caps_2.12.0.aarch64.xml b/tests/qemucapabilitiesdata/caps_2.12.0.aarch64.xml index ff48293656..5a42f951dc 100644 --- a/tests/qemucapabilitiesdata/caps_2.12.0.aarch64.xml +++ b/tests/qemucapabilitiesdata/caps_2.12.0.aarch64.xml @@ -187,6 +187,7 @@ <flag name='isa-serial'/> <flag name='pl011'/> <flag name='dump-completed'/> + <flag name='pcie-pci-bridge'/> <version>2011090</version> <kvmVersion>0</kvmVersion> <microcodeVersion>342346</microcodeVersion> diff --git a/tests/qemucapabilitiesdata/caps_2.12.0.x86_64.xml b/tests/qemucapabilitiesdata/caps_2.12.0.x86_64.xml index 334296e213..800b250f3f 100644 --- a/tests/qemucapabilitiesdata/caps_2.12.0.x86_64.xml +++ b/tests/qemucapabilitiesdata/caps_2.12.0.x86_64.xml @@ -225,6 +225,7 @@ <flag name='iscsi.password-secret'/> <flag name='isa-serial'/> <flag name='dump-completed'/> + <flag name='pcie-pci-bridge'/> <version>2011090</version> <kvmVersion>0</kvmVersion> <microcodeVersion>390060</microcodeVersion> -- 2.14.3
On 03/28/2018 10:06 AM, Andrea Bolognani wrote:
This capability will be set when the pcie-pci-bridge device is available in the QEMU binary.
Signed-off-by: Andrea Bolognani <abologna@redhat.com> --- src/qemu/qemu_capabilities.c | 2 ++ src/qemu/qemu_capabilities.h | 1 + tests/qemucapabilitiesdata/caps_2.12.0.aarch64.xml | 1 + tests/qemucapabilitiesdata/caps_2.12.0.x86_64.xml | 1 + 4 files changed, 5 insertions(+)
Reviewed-by: John Ferlan <jferlan@redhat.com> John
The new controller will not yet be used automatically by libvirt, but at this point it's already possible to configure a guest to use it. Signed-off-by: Andrea Bolognani <abologna@redhat.com> --- docs/formatdomain.html.in | 3 ++- docs/schemas/domaincommon.rng | 3 +++ src/conf/domain_addr.c | 25 ++++++++++++++++++++----- src/conf/domain_addr.h | 4 +++- src/conf/domain_conf.c | 3 +++ src/conf/domain_conf.h | 2 ++ src/qemu/qemu_command.c | 1 + src/qemu/qemu_domain.c | 17 +++++++++++++++++ src/qemu/qemu_domain_address.c | 4 ++++ 9 files changed, 55 insertions(+), 7 deletions(-) diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index 299d7b9d42..fa5a0fff8a 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -3942,7 +3942,8 @@ <code>pcie-root-port</code>, <code>pcie-switch-upstream-port</code> and <code>pcie-switch-downstream-port</code> (<span class="since">since 1.2.19</span>), <code>pci-expander-bus</code> and - <code>pcie-expander-bus</code> (<span class="since">since 1.3.4</span>). + <code>pcie-expander-bus</code> (<span class="since">since 1.3.4</span>), + <code>pcie-to-pci-bridge</code> (<span class="since">since 4.3.0</span>). The root controllers (<code>pci-root</code> and <code>pcie-root</code>) have an optional <code>pcihole64</code> element specifying how big (in diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng index 8165e699d6..85ba95d5f6 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -2102,6 +2102,8 @@ <value>pci-bridge</value> <!-- implementations of 'dmi-to-pci-bridge' --> <value>i82801b11-bridge</value> + <!-- implementations of 'pcie-to-pci-bridge' --> + <value>pcie-pci-bridge</value> <!-- implementations of 'pcie-root-port' --> <value>ioh3420</value> <value>pcie-root-port</value> @@ -2172,6 +2174,7 @@ <choice> <value>pci-bridge</value> <value>dmi-to-pci-bridge</value> + <value>pcie-to-pci-bridge</value> <value>pcie-root-port</value> <value>pcie-switch-upstream-port</value> <value>pcie-switch-downstream-port</value> diff --git a/src/conf/domain_addr.c b/src/conf/domain_addr.c index e02d7ac614..b0709f8295 100644 --- a/src/conf/domain_addr.c +++ b/src/conf/domain_addr.c @@ -62,6 +62,9 @@ virDomainPCIControllerModelToConnectType(virDomainControllerModelPCI model) case VIR_DOMAIN_CONTROLLER_MODEL_DMI_TO_PCI_BRIDGE: return VIR_PCI_CONNECT_TYPE_DMI_TO_PCI_BRIDGE; + case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_TO_PCI_BRIDGE: + return VIR_PCI_CONNECT_TYPE_PCIE_TO_PCI_BRIDGE; + case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT_PORT: return VIR_PCI_CONNECT_TYPE_PCIE_ROOT_PORT | VIR_PCI_CONNECT_AGGREGATE_SLOT; @@ -160,6 +163,8 @@ virDomainPCIAddressFlagsCompatible(virPCIDeviceAddressPtr addr, connectStr = "pci-switch-downstream-port"; } else if (devFlags & VIR_PCI_CONNECT_TYPE_DMI_TO_PCI_BRIDGE) { connectStr = "dmi-to-pci-bridge"; + } else if (devFlags & VIR_PCI_CONNECT_TYPE_PCIE_TO_PCI_BRIDGE) { + connectStr = "pcie-to-pci-bridge"; } else if (devFlags & VIR_PCI_CONNECT_TYPE_PCI_EXPANDER_BUS) { connectStr = "pci-expander-bus"; } else if (devFlags & VIR_PCI_CONNECT_TYPE_PCIE_EXPANDER_BUS) { @@ -316,14 +321,24 @@ virDomainPCIAddressBusSetModel(virDomainPCIAddressBusPtr bus, bus->minSlot = 0; bus->maxSlot = VIR_PCI_ADDRESS_SLOT_LAST; break; + case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_TO_PCI_BRIDGE: + /* Same as pci-bridge: 32 hotpluggable traditional PCI slots (0-31), + * the first of which is not usable because of the SHPC */ + bus->flags = (VIR_PCI_CONNECT_HOTPLUGGABLE | + VIR_PCI_CONNECT_TYPE_PCI_DEVICE | + VIR_PCI_CONNECT_TYPE_PCI_BRIDGE); + bus->minSlot = 1; + bus->maxSlot = VIR_PCI_ADDRESS_SLOT_LAST; + break; case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT_PORT: case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_SWITCH_DOWNSTREAM_PORT: /* provides one slot which is pcie, can be used by endpoint - * devices and pcie-switch-upstream-ports, and is hotpluggable - */ - bus->flags = VIR_PCI_CONNECT_TYPE_PCIE_DEVICE - | VIR_PCI_CONNECT_TYPE_PCIE_SWITCH_UPSTREAM_PORT - | VIR_PCI_CONNECT_HOTPLUGGABLE; + * devices, pcie-switch-upstream-ports or pcie-to-pci-bridges, + * and is hotpluggable */ + bus->flags = (VIR_PCI_CONNECT_HOTPLUGGABLE | + VIR_PCI_CONNECT_TYPE_PCIE_DEVICE | + VIR_PCI_CONNECT_TYPE_PCIE_SWITCH_UPSTREAM_PORT | + VIR_PCI_CONNECT_TYPE_PCIE_TO_PCI_BRIDGE); bus->minSlot = 0; bus->maxSlot = 0; break; diff --git a/src/conf/domain_addr.h b/src/conf/domain_addr.h index 87986c2bb7..87248a4fb8 100644 --- a/src/conf/domain_addr.h +++ b/src/conf/domain_addr.h @@ -49,6 +49,7 @@ typedef enum { VIR_PCI_CONNECT_TYPE_PCI_EXPANDER_BUS = 1 << 8, VIR_PCI_CONNECT_TYPE_PCIE_EXPANDER_BUS = 1 << 9, VIR_PCI_CONNECT_TYPE_PCI_BRIDGE = 1 << 10, + VIR_PCI_CONNECT_TYPE_PCIE_TO_PCI_BRIDGE = 1 << 11, } virDomainPCIConnectFlags; /* a combination of all bits that describe the type of connections @@ -62,7 +63,8 @@ typedef enum { VIR_PCI_CONNECT_TYPE_DMI_TO_PCI_BRIDGE | \ VIR_PCI_CONNECT_TYPE_PCI_EXPANDER_BUS | \ VIR_PCI_CONNECT_TYPE_PCIE_EXPANDER_BUS | \ - VIR_PCI_CONNECT_TYPE_PCI_BRIDGE) + VIR_PCI_CONNECT_TYPE_PCI_BRIDGE | \ + VIR_PCI_CONNECT_TYPE_PCIE_TO_PCI_BRIDGE) /* combination of all bits that could be used to connect a normal * endpoint device (i.e. excluding the connection possible between an diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index ae7c0d9b71..5a55d16d59 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -335,6 +335,7 @@ VIR_ENUM_IMPL(virDomainControllerModelPCI, VIR_DOMAIN_CONTROLLER_MODEL_PCI_LAST, "pcie-root", "pci-bridge", "dmi-to-pci-bridge", + "pcie-to-pci-bridge", "pcie-root-port", "pcie-switch-upstream-port", "pcie-switch-downstream-port", @@ -353,6 +354,7 @@ VIR_ENUM_IMPL(virDomainControllerPCIModelName, "pxb-pcie", "pcie-root-port", "spapr-pci-host-bridge", + "pcie-pci-bridge", ); VIR_ENUM_IMPL(virDomainControllerModelSCSI, VIR_DOMAIN_CONTROLLER_MODEL_SCSI_LAST, @@ -10188,6 +10190,7 @@ virDomainControllerDefParseXML(virDomainXMLOptionPtr xmlopt, } case VIR_DOMAIN_CONTROLLER_MODEL_PCI_BRIDGE: case VIR_DOMAIN_CONTROLLER_MODEL_DMI_TO_PCI_BRIDGE: + case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_TO_PCI_BRIDGE: case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT_PORT: case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_SWITCH_UPSTREAM_PORT: case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_SWITCH_DOWNSTREAM_PORT: diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 61379e50fe..b2d2641b05 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -690,6 +690,7 @@ typedef enum { VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT, VIR_DOMAIN_CONTROLLER_MODEL_PCI_BRIDGE, VIR_DOMAIN_CONTROLLER_MODEL_DMI_TO_PCI_BRIDGE, + VIR_DOMAIN_CONTROLLER_MODEL_PCIE_TO_PCI_BRIDGE, VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT_PORT, VIR_DOMAIN_CONTROLLER_MODEL_PCIE_SWITCH_UPSTREAM_PORT, VIR_DOMAIN_CONTROLLER_MODEL_PCIE_SWITCH_DOWNSTREAM_PORT, @@ -710,6 +711,7 @@ typedef enum { VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_PXB_PCIE, VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_PCIE_ROOT_PORT, VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_SPAPR_PCI_HOST_BRIDGE, + VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_PCIE_PCI_BRIDGE, VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_LAST } virDomainControllerPCIModelName; diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 89fd08b642..f1cf1cba5c 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -2800,6 +2800,7 @@ qemuBuildControllerDevStr(const virDomainDef *domainDef, break; case VIR_DOMAIN_CONTROLLER_MODEL_DMI_TO_PCI_BRIDGE: case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_SWITCH_UPSTREAM_PORT: + case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_TO_PCI_BRIDGE: virBufferAsprintf(&buf, "%s,id=%s", modelName, def->info.alias); break; case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT_PORT: diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 580e0f830d..d96475b5c9 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -4355,6 +4355,8 @@ virDomainControllerPCIModelNameToQEMUCaps(int modelName) return QEMU_CAPS_DEVICE_PCIE_ROOT_PORT; case VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_SPAPR_PCI_HOST_BRIDGE: return QEMU_CAPS_DEVICE_SPAPR_PCI_HOST_BRIDGE; + case VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_PCIE_PCI_BRIDGE: + return QEMU_CAPS_DEVICE_PCIE_PCI_BRIDGE; case VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_NONE: return 0; case VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_LAST: @@ -4412,6 +4414,7 @@ qemuDomainDeviceDefValidateControllerPCI(const virDomainControllerDef *cont, case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_SWITCH_DOWNSTREAM_PORT: case VIR_DOMAIN_CONTROLLER_MODEL_PCI_EXPANDER_BUS: case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_EXPANDER_BUS: + case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_TO_PCI_BRIDGE: /* modelName should have been set automatically */ if (pciopts->modelName == VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_NONE) { virReportControllerMissingOption(cont, model, modelName, "modelName"); @@ -4516,6 +4519,13 @@ qemuDomainDeviceDefValidateControllerPCI(const virDomainControllerDef *cont, } break; + case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_TO_PCI_BRIDGE: + if (pciopts->modelName != VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_PCIE_PCI_BRIDGE) { + virReportControllerInvalidValue(cont, model, modelName, "modelName"); + return -1; + } + break; + case VIR_DOMAIN_CONTROLLER_MODEL_PCI_DEFAULT: case VIR_DOMAIN_CONTROLLER_MODEL_PCI_LAST: default: @@ -4532,6 +4542,7 @@ qemuDomainDeviceDefValidateControllerPCI(const virDomainControllerDef *cont, case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_SWITCH_DOWNSTREAM_PORT: case VIR_DOMAIN_CONTROLLER_MODEL_PCI_EXPANDER_BUS: case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_EXPANDER_BUS: + case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_TO_PCI_BRIDGE: if (cont->idx == 0) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("Index for '%s' controllers must be > 0"), @@ -4593,6 +4604,7 @@ qemuDomainDeviceDefValidateControllerPCI(const virDomainControllerDef *cont, case VIR_DOMAIN_CONTROLLER_MODEL_PCI_EXPANDER_BUS: case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_EXPANDER_BUS: case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT: + case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_TO_PCI_BRIDGE: if (pciopts->targetIndex != -1) { virReportControllerInvalidOption(cont, model, modelName, "targetIndex"); return -1; @@ -4626,6 +4638,7 @@ qemuDomainDeviceDefValidateControllerPCI(const virDomainControllerDef *cont, case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_SWITCH_DOWNSTREAM_PORT: case VIR_DOMAIN_CONTROLLER_MODEL_PCI_EXPANDER_BUS: case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_EXPANDER_BUS: + case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_TO_PCI_BRIDGE: if (pciopts->pcihole64 || pciopts->pcihole64size != 0) { virReportControllerInvalidOption(cont, model, modelName, "pcihole64"); @@ -4657,6 +4670,7 @@ qemuDomainDeviceDefValidateControllerPCI(const virDomainControllerDef *cont, case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_SWITCH_UPSTREAM_PORT: case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_SWITCH_DOWNSTREAM_PORT: case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT: + case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_TO_PCI_BRIDGE: if (pciopts->busNr != -1) { virReportControllerInvalidOption(cont, model, modelName, "busNr"); return -1; @@ -4701,6 +4715,7 @@ qemuDomainDeviceDefValidateControllerPCI(const virDomainControllerDef *cont, case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_SWITCH_UPSTREAM_PORT: case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_SWITCH_DOWNSTREAM_PORT: case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT: + case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_TO_PCI_BRIDGE: if (pciopts->numaNode != -1) { virReportControllerInvalidOption(cont, model, modelName, "numaNode"); return -1; @@ -4731,6 +4746,7 @@ qemuDomainDeviceDefValidateControllerPCI(const virDomainControllerDef *cont, case VIR_DOMAIN_CONTROLLER_MODEL_PCI_EXPANDER_BUS: case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_EXPANDER_BUS: case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT: + case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_TO_PCI_BRIDGE: if (pciopts->chassisNr != -1) { virReportControllerInvalidOption(cont, model, modelName, "chassisNr"); return -1; @@ -4765,6 +4781,7 @@ qemuDomainDeviceDefValidateControllerPCI(const virDomainControllerDef *cont, case VIR_DOMAIN_CONTROLLER_MODEL_PCI_EXPANDER_BUS: case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_EXPANDER_BUS: case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT: + case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_TO_PCI_BRIDGE: if (pciopts->chassis != -1) { virReportControllerInvalidOption(cont, model, modelName, "chassis"); return -1; diff --git a/src/qemu/qemu_domain_address.c b/src/qemu/qemu_domain_address.c index 40fb540adc..86d9807908 100644 --- a/src/qemu/qemu_domain_address.c +++ b/src/qemu/qemu_domain_address.c @@ -2171,6 +2171,9 @@ qemuDomainPCIControllerSetDefaultModelName(virDomainControllerDefPtr cont, case VIR_DOMAIN_CONTROLLER_MODEL_DMI_TO_PCI_BRIDGE: *modelName = VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_I82801B11_BRIDGE; break; + case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_TO_PCI_BRIDGE: + *modelName = VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_PCIE_PCI_BRIDGE; + break; case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT_PORT: /* Use generic PCIe Root Ports if available, falling back to * ioh3420 otherwise */ @@ -2582,6 +2585,7 @@ qemuDomainAssignPCIAddresses(virDomainDefPtr def, } break; case VIR_DOMAIN_CONTROLLER_MODEL_DMI_TO_PCI_BRIDGE: + case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_TO_PCI_BRIDGE: case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_SWITCH_UPSTREAM_PORT: case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT: case VIR_DOMAIN_CONTROLLER_MODEL_PCI_DEFAULT: -- 2.14.3
On 03/28/2018 10:06 AM, Andrea Bolognani wrote:
The new controller will not yet be used automatically by libvirt, but at this point it's already possible to configure a guest to use it.
Signed-off-by: Andrea Bolognani <abologna@redhat.com> --- docs/formatdomain.html.in | 3 ++- docs/schemas/domaincommon.rng | 3 +++ src/conf/domain_addr.c | 25 ++++++++++++++++++++----- src/conf/domain_addr.h | 4 +++- src/conf/domain_conf.c | 3 +++ src/conf/domain_conf.h | 2 ++ src/qemu/qemu_command.c | 1 + src/qemu/qemu_domain.c | 17 +++++++++++++++++ src/qemu/qemu_domain_address.c | 4 ++++ 9 files changed, 55 insertions(+), 7 deletions(-)
diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index 299d7b9d42..fa5a0fff8a 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -3942,7 +3942,8 @@ <code>pcie-root-port</code>, <code>pcie-switch-upstream-port</code> and <code>pcie-switch-downstream-port</code> (<span class="since">since 1.2.19</span>), <code>pci-expander-bus</code> and - <code>pcie-expander-bus</code> (<span class="since">since 1.3.4</span>). + <code>pcie-expander-bus</code> (<span class="since">since 1.3.4</span>), + <code>pcie-to-pci-bridge</code> (<span class="since">since 4.3.0</span>). The root controllers (<code>pci-root</code> and <code>pcie-root</code>) have an optional <code>pcihole64</code> element specifying how big (in
Considering my patch 1 comments, there will be obvious adjustments here. Is there something else perhaps a few paragraphs later that could be added to say what this is and how/when/why someone should want to use it? Including the restriction about slot=0 (whatever the FLA SHPC is)... Similar to dmi-to-pci-bridge... Perhaps something that would already be there considering my patch 1 comment for a description field or some sort of relationship matrix... With a few more words... Reviewed-by: John Ferlan <jferlan@redhat.com> John
On Tue, 2018-04-03 at 19:13 -0400, John Ferlan wrote:
Is there something else perhaps a few paragraphs later that could be added to say what this is and how/when/why someone should want to use it? Including the restriction about slot=0 (whatever the FLA SHPC is)... Similar to dmi-to-pci-bridge... Perhaps something that would already be there considering my patch 1 comment for a description field or some sort of relationship matrix...
Yeah, I realized today that there's some documentation for the existing behavior of adding a dmi-to-pci-bridge that will need to tweaked in this patch to address the changes... Once that's been taken care of, I don't think there will be much to add, though. -- Andrea Bolognani / Red Hat / Virtualization
On Wed, 2018-04-04 at 10:58 +0200, Andrea Bolognani wrote:
On Tue, 2018-04-03 at 19:13 -0400, John Ferlan wrote:
Is there something else perhaps a few paragraphs later that could be added to say what this is and how/when/why someone should want to use it? Including the restriction about slot=0 (whatever the FLA SHPC is)... Similar to dmi-to-pci-bridge... Perhaps something that would already be there considering my patch 1 comment for a description field or some sort of relationship matrix...
Yeah, I realized today that there's some documentation for the existing behavior of adding a dmi-to-pci-bridge that will need to tweaked in this patch to address the changes... Once that's been taken care of, I don't think there will be much to add, though.
I've come up with the diff below, which I plan to squash in patch 8/11, when the change in behavior it documents is implemented. I have also converted the list of models to a <ul>, as agreed upon in the review for patch 1/11. Since patch 3/11 is really just an independent cleanup which doesn't need to hold up the new feature, I plan to go ahead and push the rest of the series (with the changes described above) later today. Is that okay with you, or would you rather see a full v2 posted on the list? diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index 84059737d5..a5a83f56c3 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -4114,10 +4114,14 @@ devices (although libvirt will never auto-assign a PCI device to a PCIe slot, it will allow manual specification of such an assignment). Devices connected to pcie-root cannot be - hotplugged. In order to make standard PCI slots available on a - system which has a pcie-root controller, a pci controller - with <code>model='dmi-to-pci-bridge'</code> is automatically - added, usually at the defacto standard location of slot=0x1e. A + hotplugged. If traditional PCI devices are present in the guest + configuration, a <code>pcie-to-pci-bridge</code> controller will + automatically be added: this controller, which plugs into a + <code>pcie-root-port</code>, provides 31 usable PCI slots (1-31) with + hotplug support (<span class="since">since 4.3.0</span>). If the QEMU + binary doesn't support the corresponding device, then a + <code>dmi-to-pci-bridge</code> controller will be added instead, + usually at the defacto standard location of slot=0x1e. A dmi-to-pci-bridge controller plugs into a PCIe slot (as provided by pcie-root), and itself provides 31 standard PCI slots (which also do not support device hotplug). In order to have -- Andrea Bolognani / Red Hat / Virtualization
On 04/06/2018 04:15 AM, Andrea Bolognani wrote:
On Wed, 2018-04-04 at 10:58 +0200, Andrea Bolognani wrote:
On Tue, 2018-04-03 at 19:13 -0400, John Ferlan wrote:
Is there something else perhaps a few paragraphs later that could be added to say what this is and how/when/why someone should want to use it? Including the restriction about slot=0 (whatever the FLA SHPC is)... Similar to dmi-to-pci-bridge... Perhaps something that would already be there considering my patch 1 comment for a description field or some sort of relationship matrix...
Yeah, I realized today that there's some documentation for the existing behavior of adding a dmi-to-pci-bridge that will need to tweaked in this patch to address the changes... Once that's been taken care of, I don't think there will be much to add, though.
I've come up with the diff below, which I plan to squash in patch 8/11, when the change in behavior it documents is implemented. I have also converted the list of models to a <ul>, as agreed upon in the review for patch 1/11.
Since patch 3/11 is really just an independent cleanup which doesn't need to hold up the new feature, I plan to go ahead and push the rest of the series (with the changes described above) later today. Is that okay with you, or would you rather see a full v2 posted on the list?
Your plan is fine by me. John
diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index 84059737d5..a5a83f56c3 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -4114,10 +4114,14 @@ devices (although libvirt will never auto-assign a PCI device to a PCIe slot, it will allow manual specification of such an assignment). Devices connected to pcie-root cannot be - hotplugged. In order to make standard PCI slots available on a - system which has a pcie-root controller, a pci controller - with <code>model='dmi-to-pci-bridge'</code> is automatically - added, usually at the defacto standard location of slot=0x1e. A + hotplugged. If traditional PCI devices are present in the guest + configuration, a <code>pcie-to-pci-bridge</code> controller will + automatically be added: this controller, which plugs into a + <code>pcie-root-port</code>, provides 31 usable PCI slots (1-31) with + hotplug support (<span class="since">since 4.3.0</span>). If the QEMU + binary doesn't support the corresponding device, then a + <code>dmi-to-pci-bridge</code> controller will be added instead, + usually at the defacto standard location of slot=0x1e. A dmi-to-pci-bridge controller plugs into a PCIe slot (as provided by pcie-root), and itself provides 31 standard PCI slots (which also do not support device hotplug). In order to have
Just like the existing areMultipleRootsSupported, this will allow us to change the results of the driver-agnostic PCI address allocation logic based on whether the QEMU binary supports certain features. Signed-off-by: Andrea Bolognani <abologna@redhat.com> --- src/conf/domain_addr.h | 2 ++ src/qemu/qemu_domain_address.c | 8 ++++++-- 2 files changed, 8 insertions(+), 2 deletions(-) diff --git a/src/conf/domain_addr.h b/src/conf/domain_addr.h index 87248a4fb8..3236b7d6de 100644 --- a/src/conf/domain_addr.h +++ b/src/conf/domain_addr.h @@ -118,6 +118,8 @@ struct _virDomainPCIAddressSet { and addresses aren't saved in device infos */ /* If true, the guest can have multiple pci-root controllers */ bool areMultipleRootsSupported; + /* If true, the guest can use the pcie-to-pci-bridge controller */ + bool isPCIeToPCIBridgeSupported; }; typedef struct _virDomainPCIAddressSet virDomainPCIAddressSet; typedef virDomainPCIAddressSet *virDomainPCIAddressSetPtr; diff --git a/src/qemu/qemu_domain_address.c b/src/qemu/qemu_domain_address.c index 86d9807908..7fe9d5926c 100644 --- a/src/qemu/qemu_domain_address.c +++ b/src/qemu/qemu_domain_address.c @@ -1338,6 +1338,7 @@ qemuDomainCollectPCIAddress(virDomainDefPtr def ATTRIBUTE_UNUSED, static virDomainPCIAddressSetPtr qemuDomainPCIAddressSetCreate(virDomainDefPtr def, + virQEMUCapsPtr qemuCaps, unsigned int nbuses, bool dryRun) { @@ -1355,6 +1356,9 @@ qemuDomainPCIAddressSetCreate(virDomainDefPtr def, if (qemuDomainIsPSeries(def)) addrs->areMultipleRootsSupported = true; + if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_PCIE_PCI_BRIDGE)) + addrs->isPCIeToPCIBridgeSupported = true; + for (i = 0; i < def->ncontrollers; i++) { virDomainControllerDefPtr cont = def->controllers[i]; size_t idx = cont->idx; @@ -2361,7 +2365,7 @@ qemuDomainAssignPCIAddresses(virDomainDefPtr def, if (nbuses > 0) { /* 1st pass to figure out how many PCI bridges we need */ - if (!(addrs = qemuDomainPCIAddressSetCreate(def, nbuses, true))) + if (!(addrs = qemuDomainPCIAddressSetCreate(def, qemuCaps, nbuses, true))) goto cleanup; if (qemuDomainValidateDevicePCISlotsChipsets(def, qemuCaps, @@ -2491,7 +2495,7 @@ qemuDomainAssignPCIAddresses(virDomainDefPtr def, addrs = NULL; } - if (!(addrs = qemuDomainPCIAddressSetCreate(def, nbuses, false))) + if (!(addrs = qemuDomainPCIAddressSetCreate(def, qemuCaps, nbuses, false))) goto cleanup; if (qemuDomainSupportsPCI(def, qemuCaps)) { -- 2.14.3
On 03/28/2018 10:06 AM, Andrea Bolognani wrote:
Just like the existing areMultipleRootsSupported, this will allow us to change the results of the driver-agnostic PCI address allocation logic based on whether the QEMU binary supports certain features.
Signed-off-by: Andrea Bolognani <abologna@redhat.com> --- src/conf/domain_addr.h | 2 ++ src/qemu/qemu_domain_address.c | 8 ++++++-- 2 files changed, 8 insertions(+), 2 deletions(-)
Could also pass the bool instead of the qemuCaps to qemuDomainPCIAddressSetCreate - this way is fine though ;-) Reviewed-by: John Ferlan <jferlan@redhat.com> John
Both pcie-to-pci-bridge and dmi-to-pci-bridge can be used to create a traditional PCI topology in a pure PCIe guest such as those using the x86_64/q35 or aarch64/virt machine type; however, the former should be preferred, as it doesn't need to obey limitation of real hardware and is completely architecture-agnostic. Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1520821 Signed-off-by: Andrea Bolognani <abologna@redhat.com> --- src/conf/domain_addr.c | 59 +++++++++++++++++++++++++++++++++++++++++--------- 1 file changed, 49 insertions(+), 10 deletions(-) diff --git a/src/conf/domain_addr.c b/src/conf/domain_addr.c index b0709f8295..8964973e03 100644 --- a/src/conf/domain_addr.c +++ b/src/conf/domain_addr.c @@ -416,6 +416,7 @@ virDomainPCIAddressSetGrow(virDomainPCIAddressSetPtr addrs, size_t i; int model; bool needDMIToPCIBridge = false; + bool needPCIeToPCIBridge = false; add = addr->bus - addrs->nbuses + 1; if (add <= 0) @@ -436,27 +437,41 @@ virDomainPCIAddressSetGrow(virDomainPCIAddressSetPtr addrs, model = VIR_DOMAIN_CONTROLLER_MODEL_PCI_BRIDGE; /* if there aren't yet any buses that will accept a - * pci-bridge, and the caller is asking for one, we'll need to - * add a dmi-to-pci-bridge first. + * pci-bridge, but we need one for the device's PCI address + * to make sense, it means the guest only has a PCIe topology + * configured so far, and we need to create a traditional PCI + * topology to accomodate the new device. */ needDMIToPCIBridge = true; + needPCIeToPCIBridge = true; for (i = 0; i < addrs->nbuses; i++) { if (addrs->buses[i].flags & VIR_PCI_CONNECT_TYPE_PCI_BRIDGE) { needDMIToPCIBridge = false; + needPCIeToPCIBridge = false; break; } } - if (needDMIToPCIBridge && add == 1) { + + /* Prefer pcie-to-pci-bridge, fall back to dmi-to-pci-bridge */ + if (addrs->isPCIeToPCIBridgeSupported) + needDMIToPCIBridge = false; + else + needPCIeToPCIBridge = false; + + if ((needDMIToPCIBridge || needPCIeToPCIBridge) && add == 1) { /* We need to add a single pci-bridge to provide the bus * our legacy PCI device will be plugged into; however, we * have also determined that there isn't yet any proper - * place to connect that pci-bridge we're about to add (on - * a system with pcie-root, that "proper place" would be a - * dmi-to-pci-bridge". So, to give the pci-bridge a place - * to connect, we increase the count of buses to add, - * while also incrementing the bus number in the address - * for the device (since the pci-bridge will now be at an - * index 1 higher than the caller had anticipated). + * place to connect that pci-bridge we're about to add, + * which means we're dealing with a pure PCIe guest. We + * need to create a traditional PCI topology, and for that + * we have two options: dmi-to-pci-bridge + pci-bridge or + * pcie-root-port + pcie-to-pci-bridge (the latter of which + * is pretty much a pci-bridge as far as devices attached + * to it are concerned and as such makes the pci-bridge + * unnecessary). Either way, there's going to be one more + * controller than initially expected, and the 'bus' part + * of the device's address will need to be bumped. */ add++; addr->bus++; @@ -525,6 +540,30 @@ virDomainPCIAddressSetGrow(virDomainPCIAddressSetPtr addrs, } } + if (needPCIeToPCIBridge) { + /* We need a pcie-root-port to plug pcie-to-pci-bridge into; however, + * qemuDomainAssignPCIAddresses() will, in some cases, create a dummy + * PCIe device and reserve an address for it in order to leave the + * user with an empty pcie-root-port ready for hotplugging, and if + * we didn't do anything other than adding the pcie-root-port here + * it would be used for that, which we don't want. So we change the + * connect flags to make sure only the pcie-to-pci-bridge will be + * connected to the pcie-root-port we just added, and another one + * will be allocated for the dummy PCIe device later on. + */ + if (virDomainPCIAddressBusSetModel(&addrs->buses[i], + VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT_PORT) < 0) { + return -1; + } + addrs->buses[i].flags = VIR_PCI_CONNECT_TYPE_PCIE_TO_PCI_BRIDGE; + i++; + + if (virDomainPCIAddressBusSetModel(&addrs->buses[i++], + VIR_DOMAIN_CONTROLLER_MODEL_PCIE_TO_PCI_BRIDGE) < 0) { + return -1; + } + } + for (; i < addrs->nbuses; i++) { if (virDomainPCIAddressBusSetModel(&addrs->buses[i], model) < 0) return -1; -- 2.14.3
On 03/28/2018 10:06 AM, Andrea Bolognani wrote:
Both pcie-to-pci-bridge and dmi-to-pci-bridge can be used to create a traditional PCI topology in a pure PCIe guest such as those using the x86_64/q35 or aarch64/virt machine type; however, the former should be preferred, as it doesn't need to obey limitation of real hardware and is completely architecture-agnostic.
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1520821
Signed-off-by: Andrea Bolognani <abologna@redhat.com> --- src/conf/domain_addr.c | 59 +++++++++++++++++++++++++++++++++++++++++--------- 1 file changed, 49 insertions(+), 10 deletions(-)
diff --git a/src/conf/domain_addr.c b/src/conf/domain_addr.c index b0709f8295..8964973e03 100644 --- a/src/conf/domain_addr.c +++ b/src/conf/domain_addr.c @@ -416,6 +416,7 @@ virDomainPCIAddressSetGrow(virDomainPCIAddressSetPtr addrs, size_t i; int model; bool needDMIToPCIBridge = false; + bool needPCIeToPCIBridge = false;
add = addr->bus - addrs->nbuses + 1; if (add <= 0) @@ -436,27 +437,41 @@ virDomainPCIAddressSetGrow(virDomainPCIAddressSetPtr addrs, model = VIR_DOMAIN_CONTROLLER_MODEL_PCI_BRIDGE;
/* if there aren't yet any buses that will accept a - * pci-bridge, and the caller is asking for one, we'll need to - * add a dmi-to-pci-bridge first. + * pci-bridge, but we need one for the device's PCI address + * to make sense, it means the guest only has a PCIe topology + * configured so far, and we need to create a traditional PCI + * topology to accomodate the new device. */ needDMIToPCIBridge = true; + needPCIeToPCIBridge = true; for (i = 0; i < addrs->nbuses; i++) { if (addrs->buses[i].flags & VIR_PCI_CONNECT_TYPE_PCI_BRIDGE) { needDMIToPCIBridge = false; + needPCIeToPCIBridge = false; break; } } - if (needDMIToPCIBridge && add == 1) { + + /* Prefer pcie-to-pci-bridge, fall back to dmi-to-pci-bridge */ + if (addrs->isPCIeToPCIBridgeSupported) + needDMIToPCIBridge = false; + else + needPCIeToPCIBridge = false;
The above seems a bit extra work and is a bit hard to read... Could the previous for loop change to use a new bool "needXToPCIBridge"... Then, I think it would just be: if (addrs->isPCIeToPCIBridgeSupported) needPCIeToPCIBridge = needXToPCIBridge; else needDMIToPCIBridge = needXToPCIBridge; with the following just being if (needXToPCIBridge && add == 1) What you have works, just seems to be overkill or maybe I'm missing something subtle ;-). You have my Reviewed-by: John Ferlan <jferlan@redhat.com> John
+ + if ((needDMIToPCIBridge || needPCIeToPCIBridge) && add == 1) { /* We need to add a single pci-bridge to provide the bus * our legacy PCI device will be plugged into; however, we * have also determined that there isn't yet any proper - * place to connect that pci-bridge we're about to add (on - * a system with pcie-root, that "proper place" would be a - * dmi-to-pci-bridge". So, to give the pci-bridge a place - * to connect, we increase the count of buses to add, - * while also incrementing the bus number in the address - * for the device (since the pci-bridge will now be at an - * index 1 higher than the caller had anticipated). + * place to connect that pci-bridge we're about to add, + * which means we're dealing with a pure PCIe guest. We + * need to create a traditional PCI topology, and for that + * we have two options: dmi-to-pci-bridge + pci-bridge or + * pcie-root-port + pcie-to-pci-bridge (the latter of which + * is pretty much a pci-bridge as far as devices attached + * to it are concerned and as such makes the pci-bridge + * unnecessary). Either way, there's going to be one more + * controller than initially expected, and the 'bus' part + * of the device's address will need to be bumped. */ add++; addr->bus++; @@ -525,6 +540,30 @@ virDomainPCIAddressSetGrow(virDomainPCIAddressSetPtr addrs, } }
+ if (needPCIeToPCIBridge) { + /* We need a pcie-root-port to plug pcie-to-pci-bridge into; however, + * qemuDomainAssignPCIAddresses() will, in some cases, create a dummy + * PCIe device and reserve an address for it in order to leave the + * user with an empty pcie-root-port ready for hotplugging, and if + * we didn't do anything other than adding the pcie-root-port here + * it would be used for that, which we don't want. So we change the + * connect flags to make sure only the pcie-to-pci-bridge will be + * connected to the pcie-root-port we just added, and another one + * will be allocated for the dummy PCIe device later on. + */ + if (virDomainPCIAddressBusSetModel(&addrs->buses[i], + VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT_PORT) < 0) { + return -1; + } + addrs->buses[i].flags = VIR_PCI_CONNECT_TYPE_PCIE_TO_PCI_BRIDGE; + i++; + + if (virDomainPCIAddressBusSetModel(&addrs->buses[i++], + VIR_DOMAIN_CONTROLLER_MODEL_PCIE_TO_PCI_BRIDGE) < 0) { + return -1; + } + } + for (; i < addrs->nbuses; i++) { if (virDomainPCIAddressBusSetModel(&addrs->buses[i], model) < 0) return -1;
On Tue, 2018-04-03 at 19:13 -0400, John Ferlan wrote: [...]
needDMIToPCIBridge = true; + needPCIeToPCIBridge = true; for (i = 0; i < addrs->nbuses; i++) { if (addrs->buses[i].flags & VIR_PCI_CONNECT_TYPE_PCI_BRIDGE) { needDMIToPCIBridge = false; + needPCIeToPCIBridge = false; break; } } - if (needDMIToPCIBridge && add == 1) { + + /* Prefer pcie-to-pci-bridge, fall back to dmi-to-pci-bridge */ + if (addrs->isPCIeToPCIBridgeSupported) + needDMIToPCIBridge = false; + else + needPCIeToPCIBridge = false;
The above seems a bit extra work and is a bit hard to read... Could the previous for loop change to use a new bool "needXToPCIBridge"...
Then, I think it would just be:
if (addrs->isPCIeToPCIBridgeSupported) needPCIeToPCIBridge = needXToPCIBridge; else needDMIToPCIBridge = needXToPCIBridge;
with the following just being if (needXToPCIBridge && add == 1)
What you have works, just seems to be overkill or maybe I'm missing something subtle ;-).
I don't think you missed something, both version should work just as fine. I happen to find my version easier to read than yours, so I'd stick with that if you're okay with it - I guess it's just a matter of preference at the end of the day... -- Andrea Bolognani / Red Hat / Virtualization
On 04/04/2018 05:04 AM, Andrea Bolognani wrote:
On Tue, 2018-04-03 at 19:13 -0400, John Ferlan wrote: [...]
needDMIToPCIBridge = true; + needPCIeToPCIBridge = true; for (i = 0; i < addrs->nbuses; i++) { if (addrs->buses[i].flags & VIR_PCI_CONNECT_TYPE_PCI_BRIDGE) { needDMIToPCIBridge = false; + needPCIeToPCIBridge = false; break; } } - if (needDMIToPCIBridge && add == 1) { + + /* Prefer pcie-to-pci-bridge, fall back to dmi-to-pci-bridge */ + if (addrs->isPCIeToPCIBridgeSupported) + needDMIToPCIBridge = false; + else + needPCIeToPCIBridge = false;
The above seems a bit extra work and is a bit hard to read... Could the previous for loop change to use a new bool "needXToPCIBridge"...
Then, I think it would just be:
if (addrs->isPCIeToPCIBridgeSupported) needPCIeToPCIBridge = needXToPCIBridge; else needDMIToPCIBridge = needXToPCIBridge;
with the following just being if (needXToPCIBridge && add == 1)
What you have works, just seems to be overkill or maybe I'm missing something subtle ;-).
I don't think you missed something, both version should work just as fine. I happen to find my version easier to read than yours, so I'd stick with that if you're okay with it - I guess it's just a matter of preference at the end of the day...
I guess part of me is thinking of the "next" bridge that gets added where someone just copies what you did and creates a needXXXToPCIBridge boolean, adds another setting to true, another setting to false, and then needs to decide which to prefer and thus has to muck with setting the others based on the new one. In the long run - it seems the setting of the bool is based on need, then the second part is to decide what type of thing to add. That's why I thought it would be easier to read with what I presented. Again I'm fine with the way you had it, so it's your call. John
Now that support for the pcie-to-pci-bridge controller has been implemented, adding the QEMU_CAPS_DEVICE_PCIE_PCI_BRIDGE capability to the existing test is enough to cause the guest to use pcie-to-pci-bridge instead of dmi-to-pci-bridge. Signed-off-by: Andrea Bolognani <abologna@redhat.com> --- tests/qemuxml2argvdata/aarch64-traditional-pci.args | 7 ++++--- tests/qemuxml2argvtest.c | 1 + tests/qemuxml2xmloutdata/aarch64-traditional-pci.xml | 16 ++++++++-------- tests/qemuxml2xmltest.c | 1 + 4 files changed, 14 insertions(+), 11 deletions(-) diff --git a/tests/qemuxml2argvdata/aarch64-traditional-pci.args b/tests/qemuxml2argvdata/aarch64-traditional-pci.args index f10252f729..7ddd03e9e1 100644 --- a/tests/qemuxml2argvdata/aarch64-traditional-pci.args +++ b/tests/qemuxml2argvdata/aarch64-traditional-pci.args @@ -19,8 +19,9 @@ server,nowait \ -mon chardev=charmonitor,id=monitor,mode=readline \ -no-acpi \ -boot c \ --device i82801b11-bridge,id=pci.1,bus=pcie.0,addr=0x1 \ --device pci-bridge,chassis_nr=2,id=pci.2,bus=pci.1,addr=0x0 \ --device pcie-root-port,port=0x10,chassis=3,id=pci.3,bus=pcie.0,addr=0x2 \ +-device pcie-root-port,port=0x8,chassis=1,id=pci.1,bus=pcie.0,multifunction=on,\ +addr=0x1 \ +-device pcie-pci-bridge,id=pci.2,bus=pci.1,addr=0x0 \ +-device pcie-root-port,port=0x9,chassis=3,id=pci.3,bus=pcie.0,addr=0x1.0x1 \ -chardev pty,id=charserial0 \ -device pci-serial,chardev=charserial0,id=serial0,bus=pci.2,addr=0x1 diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c index 9a0d01e4f4..94b82d82d3 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -2711,6 +2711,7 @@ mymain(void) QEMU_CAPS_PCI_MULTIFUNCTION, QEMU_CAPS_DEVICE_PCIE_ROOT_PORT, QEMU_CAPS_DEVICE_DMI_TO_PCI_BRIDGE, + QEMU_CAPS_DEVICE_PCIE_PCI_BRIDGE, QEMU_CAPS_DEVICE_PCI_BRIDGE, QEMU_CAPS_DEVICE_PCI_SERIAL); diff --git a/tests/qemuxml2xmloutdata/aarch64-traditional-pci.xml b/tests/qemuxml2xmloutdata/aarch64-traditional-pci.xml index 70664aa4f9..e0807308d9 100644 --- a/tests/qemuxml2xmloutdata/aarch64-traditional-pci.xml +++ b/tests/qemuxml2xmloutdata/aarch64-traditional-pci.xml @@ -18,19 +18,19 @@ <devices> <emulator>/usr/bin/qemu-system-aarch64</emulator> <controller type='pci' index='0' model='pcie-root'/> - <controller type='pci' index='1' model='dmi-to-pci-bridge'> - <model name='i82801b11-bridge'/> - <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x0'/> + <controller type='pci' index='1' model='pcie-root-port'> + <model name='pcie-root-port'/> + <target chassis='1' port='0x8'/> + <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x0' multifunction='on'/> </controller> - <controller type='pci' index='2' model='pci-bridge'> - <model name='pci-bridge'/> - <target chassisNr='2'/> + <controller type='pci' index='2' model='pcie-to-pci-bridge'> + <model name='pcie-pci-bridge'/> <address type='pci' domain='0x0000' bus='0x01' slot='0x00' function='0x0'/> </controller> <controller type='pci' index='3' model='pcie-root-port'> <model name='pcie-root-port'/> - <target chassis='3' port='0x10'/> - <address type='pci' domain='0x0000' bus='0x00' slot='0x02' function='0x0'/> + <target chassis='3' port='0x9'/> + <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x1'/> </controller> <serial type='pty'> <target type='pci-serial' port='0'> diff --git a/tests/qemuxml2xmltest.c b/tests/qemuxml2xmltest.c index 49b61d7647..3880c1d122 100644 --- a/tests/qemuxml2xmltest.c +++ b/tests/qemuxml2xmltest.c @@ -1123,6 +1123,7 @@ mymain(void) QEMU_CAPS_PCI_MULTIFUNCTION, QEMU_CAPS_DEVICE_PCIE_ROOT_PORT, QEMU_CAPS_DEVICE_DMI_TO_PCI_BRIDGE, + QEMU_CAPS_DEVICE_PCIE_PCI_BRIDGE, QEMU_CAPS_DEVICE_PCI_BRIDGE, QEMU_CAPS_DEVICE_PCI_SERIAL); DO_TEST("aarch64-video-default", -- 2.14.3
On 03/28/2018 10:06 AM, Andrea Bolognani wrote:
Now that support for the pcie-to-pci-bridge controller has been implemented, adding the QEMU_CAPS_DEVICE_PCIE_PCI_BRIDGE capability to the existing test is enough to cause the guest to use pcie-to-pci-bridge instead of dmi-to-pci-bridge.
Signed-off-by: Andrea Bolognani <abologna@redhat.com> --- tests/qemuxml2argvdata/aarch64-traditional-pci.args | 7 ++++--- tests/qemuxml2argvtest.c | 1 + tests/qemuxml2xmloutdata/aarch64-traditional-pci.xml | 16 ++++++++-------- tests/qemuxml2xmltest.c | 1 + 4 files changed, 14 insertions(+), 11 deletions(-)
Surprised this isn't part of some earlier patch, but it's fine here too I suppose. Reviewed-by: John Ferlan <jferlan@redhat.com> John
On Tue, 2018-04-03 at 19:15 -0400, John Ferlan wrote:
On 03/28/2018 10:06 AM, Andrea Bolognani wrote:
Now that support for the pcie-to-pci-bridge controller has been implemented, adding the QEMU_CAPS_DEVICE_PCIE_PCI_BRIDGE capability to the existing test is enough to cause the guest to use pcie-to-pci-bridge instead of dmi-to-pci-bridge.
Signed-off-by: Andrea Bolognani <abologna@redhat.com> --- tests/qemuxml2argvdata/aarch64-traditional-pci.args | 7 ++++--- tests/qemuxml2argvtest.c | 1 + tests/qemuxml2xmloutdata/aarch64-traditional-pci.xml | 16 ++++++++-------- tests/qemuxml2xmltest.c | 1 + 4 files changed, 14 insertions(+), 11 deletions(-)
Surprised this isn't part of some earlier patch, but it's fine here too I suppose.
I could add the test case *after* introducing the capability: then the test results would change automatically with the previous patch. I don't have a strong preference either way. -- Andrea Bolognani / Red Hat / Virtualization
Signed-off-by: Andrea Bolognani <abologna@redhat.com> --- docs/news.xml | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/docs/news.xml b/docs/news.xml index 1088895746..20524bf537 100644 --- a/docs/news.xml +++ b/docs/news.xml @@ -33,6 +33,14 @@ --> <libvirt> + <release version="v4.3.0" date="unreleased"> + <section title="New features"> + </section> + <section title="Improvements"> + </section> + <section title="Bug fixes"> + </section> + </release> <release version="v4.2.0" date="unreleased"> <section title="New features"> <change> -- 2.14.3
Signed-off-by: Andrea Bolognani <abologna@redhat.com> --- docs/news.xml | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/docs/news.xml b/docs/news.xml index 20524bf537..f8b4584f70 100644 --- a/docs/news.xml +++ b/docs/news.xml @@ -35,6 +35,15 @@ <libvirt> <release version="v4.3.0" date="unreleased"> <section title="New features"> + <change> + <summary> + qemu: Add support for the pcie-to-pci-bridge controller + </summary> + <description> + Pure PCIe guests such as x86_64/q35 and aarch64/virt will now + add this controller when traditional PCI devices are in use. + </description> + </change> </section> <section title="Improvements"> </section> -- 2.14.3
On 03/28/2018 10:06 AM, Andrea Bolognani wrote:
Signed-off-by: Andrea Bolognani <abologna@redhat.com> --- docs/news.xml | 9 +++++++++ 1 file changed, 9 insertions(+)
Just make sure there isn't any wierd placement by merges or loss of patch 10 before this series finally gets pushed... Reviewed-by: John Ferlan <jferlan@redhat.com> John
participants (3)
-
Andrea Bolognani -
John Ferlan -
Laine Stump