QEMU 2.9 introduces the pcie-root-port device, which is
a generic version of the existing ioh3420 device.

Make the new device available to libvirt users.

Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1408808
---
 docs/schemas/domaincommon.rng                    |  1 +
 src/conf/domain_conf.c                           |  4 +++-
 src/conf/domain_conf.h                           |  1 +
 src/qemu/qemu_capabilities.c                     |  2 ++
 src/qemu/qemu_capabilities.h                     |  1 +
 src/qemu/qemu_command.c                          | 18 +++++++++++++++---
 tests/qemucapabilitiesdata/caps_2.9.0.x86_64.xml |  1 +
 7 files changed, 24 insertions(+), 4 deletions(-)

diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng
index 5e59328..e4cf990 100644
--- a/docs/schemas/domaincommon.rng
+++ b/docs/schemas/domaincommon.rng
@@ -1904,6 +1904,7 @@
                     <value>i82801b11-bridge</value>
                     <!-- implementations of 'pcie-root-port' -->
                     <value>ioh3420</value>
+                    <value>pcie-root-port</value>
                     <!-- implementations of 'pcie-switch-upstream-port' -->
                     <value>x3130-upstream</value>
                     <!-- implementations of 'pcie-switch-downstream-port' -->
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 88d419e..f110c31 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -338,7 +338,9 @@ VIR_ENUM_IMPL(virDomainControllerPCIModelName,
               "x3130-upstream",
               "xio3130-downstream",
               "pxb",
-              "pxb-pcie")
+              "pxb-pcie",
+              "pcie-root-port",
+);
 
 VIR_ENUM_IMPL(virDomainControllerModelSCSI, VIR_DOMAIN_CONTROLLER_MODEL_SCSI_LAST,
               "auto",
diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
index b788a82..72901ca 100644
--- a/src/conf/domain_conf.h
+++ b/src/conf/domain_conf.h
@@ -694,6 +694,7 @@ typedef enum {
     VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_XIO3130_DOWNSTREAM,
     VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_PXB,
     VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_PXB_PCIE,
+    VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_PCIE_ROOT_PORT,
 
     VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_LAST
 } virDomainControllerPCIModelName;
diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
index 70f9ed7..453bc8a 100644
--- a/src/qemu/qemu_capabilities.c
+++ b/src/qemu/qemu_capabilities.c
@@ -359,6 +359,7 @@ VIR_ENUM_IMPL(virQEMUCaps, QEMU_CAPS_LAST,
               "query-cpu-model-expansion", /* 245 */
               "virtio-net.host_mtu",
               "spice-rendernode",
+              "pcie-root-port",
     );
 
 
@@ -1619,6 +1620,7 @@ struct virQEMUCapsStringFlags virQEMUCapsObjectTypes[] = {
     { "ivshmem-plain", QEMU_CAPS_DEVICE_IVSHMEM_PLAIN },
     { "ivshmem-doorbell", QEMU_CAPS_DEVICE_IVSHMEM_DOORBELL },
     { "vhost-scsi", QEMU_CAPS_DEVICE_VHOST_SCSI },
+    { "pcie-root-port", QEMU_CAPS_DEVICE_PCIE_ROOT_PORT },
 };
 
 static struct virQEMUCapsStringFlags virQEMUCapsObjectPropsVirtioBalloon[] = {
diff --git a/src/qemu/qemu_capabilities.h b/src/qemu/qemu_capabilities.h
index cc9f46e..79de977 100644
--- a/src/qemu/qemu_capabilities.h
+++ b/src/qemu/qemu_capabilities.h
@@ -394,6 +394,7 @@ typedef enum {
     QEMU_CAPS_QUERY_CPU_MODEL_EXPANSION, /* qmp query-cpu-model-expansion */
     QEMU_CAPS_VIRTIO_NET_HOST_MTU, /* virtio-net-*.host_mtu */
     QEMU_CAPS_SPICE_RENDERNODE, /* -spice rendernode */
+    QEMU_CAPS_DEVICE_PCIE_ROOT_PORT, /* -device pcie-root-port */
 
     QEMU_CAPS_LAST /* this must always be the last item */
 } virQEMUCapsFlags;
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index b484b7b..7e5f727 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -2874,20 +2874,32 @@ qemuBuildControllerDevStr(const virDomainDef *domainDef,
                                def->opts.pciopts.modelName);
                 goto error;
             }
-            if (def->opts.pciopts.modelName
-                != VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_IOH3420) {
+            if ((def->opts.pciopts.modelName !=
+                 VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_IOH3420) &&
+                (def->opts.pciopts.modelName !=
+                 VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_PCIE_ROOT_PORT)) {
                 virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
                                _("PCI controller model name '%s' "
                                  "is not valid for a pcie-root-port"),
                                modelName);
                 goto error;
             }
-            if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_IOH3420)) {
+            if ((def->opts.pciopts.modelName ==
+                 VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_IOH3420) &&
+                !virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_IOH3420)) {
                 virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
                                _("the pcie-root-port (ioh3420) "
                                  "controller is not supported in this QEMU binary"));
                 goto error;
             }
+            if ((def->opts.pciopts.modelName ==
+                 VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_PCIE_ROOT_PORT) &&
+                !virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_PCIE_ROOT_PORT)) {
+                virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+                               _("the pcie-root-port (pcie-root-port) "
+                                 "controller is not supported in this QEMU binary"));
+                goto error;
+            }
 
             virBufferAsprintf(&buf, "%s,port=0x%x,chassis=%d,id=%s",
                               modelName, def->opts.pciopts.port,
diff --git a/tests/qemucapabilitiesdata/caps_2.9.0.x86_64.xml b/tests/qemucapabilitiesdata/caps_2.9.0.x86_64.xml
index 334f8e7..a397615 100644
--- a/tests/qemucapabilitiesdata/caps_2.9.0.x86_64.xml
+++ b/tests/qemucapabilitiesdata/caps_2.9.0.x86_64.xml
@@ -202,6 +202,7 @@
   <flag name='drive-iotune-group'/>
   <flag name='query-cpu-model-expansion'/>
   <flag name='virtio-net.host_mtu'/>
+  <flag name='pcie-root-port'/>
   <version>2008050</version>
   <kvmVersion>0</kvmVersion>
   <package> (v2.8.0-1961-g5b10b94bd5)</package>
-- 
2.7.4

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

On 03/14/2017 12:58 PM, Andrea Bolognani wrote:
> QEMU 2.9 introduces the pcie-root-port device, which is
> a generic version of the existing ioh3420 device.
> 
> Make the new device available to libvirt users.
> 
> Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1408808

Yes, but kind of no. It makes it available, but still difficult and
cumbersome to use. I would say it *partially* resolves that BZ, with
full resolution coming from the followup patches that make it the
default. (I'm only pointing this out because we wouldn't want some
distro maintainer to be looking through patches for backport and
erroneously believe, based on the commit log, that this was the only
patch they needed).


> ---
>  docs/schemas/domaincommon.rng                    |  1 +
>  src/conf/domain_conf.c                           |  4 +++-
>  src/conf/domain_conf.h                           |  1 +
>  src/qemu/qemu_capabilities.c                     |  2 ++
>  src/qemu/qemu_capabilities.h                     |  1 +
>  src/qemu/qemu_command.c                          | 18 +++++++++++++++---
>  tests/qemucapabilitiesdata/caps_2.9.0.x86_64.xml |  1 +

Lately everyone has gotten in the habit of making a separate patch for
1) caps, 2) config+docs, and 3) functionality in hypervisor, but I'm
okay with the old school style of combining them all (I think that the
documentation change should be included in this same patch though, or if
you split them then the convention seems to be that the docs change is
included in the patch that updates the XML).

BTW, although you added an entry to your new-fangled "news" file in
patch 4, you never added new info to formatdomain.html.in - at least
that should be included in this patch (with a small addition to its text
when you change the default for aarch64).

Other than the missing documentation, ACK.

>  7 files changed, 24 insertions(+), 4 deletions(-)
> 
> diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng
> index 5e59328..e4cf990 100644
> --- a/docs/schemas/domaincommon.rng
> +++ b/docs/schemas/domaincommon.rng
> @@ -1904,6 +1904,7 @@
>                      <value>i82801b11-bridge</value>
>                      <!-- implementations of 'pcie-root-port' -->
>                      <value>ioh3420</value>
> +                    <value>pcie-root-port</value>
>                      <!-- implementations of 'pcie-switch-upstream-port' -->
>                      <value>x3130-upstream</value>
>                      <!-- implementations of 'pcie-switch-downstream-port' -->
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index 88d419e..f110c31 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -338,7 +338,9 @@ VIR_ENUM_IMPL(virDomainControllerPCIModelName,
>                "x3130-upstream",
>                "xio3130-downstream",
>                "pxb",
> -              "pxb-pcie")
> +              "pxb-pcie",
> +              "pcie-root-port",

Sigh. As this becomes the norm, it's going to make libvirt config look
redundant (and is also likely to confuse people about which attribute to
change if they want to use iohh3420 instead of the generic one), but
there's nothing that can be done about it. (I agree that this was the
best choice for device name in qemu btw).


> +);
>  
>  VIR_ENUM_IMPL(virDomainControllerModelSCSI, VIR_DOMAIN_CONTROLLER_MODEL_SCSI_LAST,
>                "auto",
> diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
> index b788a82..72901ca 100644
> --- a/src/conf/domain_conf.h
> +++ b/src/conf/domain_conf.h
> @@ -694,6 +694,7 @@ typedef enum {
>      VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_XIO3130_DOWNSTREAM,
>      VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_PXB,
>      VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_PXB_PCIE,
> +    VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_PCIE_ROOT_PORT,
>  
>      VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_LAST
>  } virDomainControllerPCIModelName;
> diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
> index 70f9ed7..453bc8a 100644
> --- a/src/qemu/qemu_capabilities.c
> +++ b/src/qemu/qemu_capabilities.c
> @@ -359,6 +359,7 @@ VIR_ENUM_IMPL(virQEMUCaps, QEMU_CAPS_LAST,
>                "query-cpu-model-expansion", /* 245 */
>                "virtio-net.host_mtu",
>                "spice-rendernode",
> +              "pcie-root-port",
>      );
>  
>  
> @@ -1619,6 +1620,7 @@ struct virQEMUCapsStringFlags virQEMUCapsObjectTypes[] = {
>      { "ivshmem-plain", QEMU_CAPS_DEVICE_IVSHMEM_PLAIN },
>      { "ivshmem-doorbell", QEMU_CAPS_DEVICE_IVSHMEM_DOORBELL },
>      { "vhost-scsi", QEMU_CAPS_DEVICE_VHOST_SCSI },
> +    { "pcie-root-port", QEMU_CAPS_DEVICE_PCIE_ROOT_PORT },
>  };
>  
>  static struct virQEMUCapsStringFlags virQEMUCapsObjectPropsVirtioBalloon[] = {
> diff --git a/src/qemu/qemu_capabilities.h b/src/qemu/qemu_capabilities.h
> index cc9f46e..79de977 100644
> --- a/src/qemu/qemu_capabilities.h
> +++ b/src/qemu/qemu_capabilities.h
> @@ -394,6 +394,7 @@ typedef enum {
>      QEMU_CAPS_QUERY_CPU_MODEL_EXPANSION, /* qmp query-cpu-model-expansion */
>      QEMU_CAPS_VIRTIO_NET_HOST_MTU, /* virtio-net-*.host_mtu */
>      QEMU_CAPS_SPICE_RENDERNODE, /* -spice rendernode */
> +    QEMU_CAPS_DEVICE_PCIE_ROOT_PORT, /* -device pcie-root-port */
>  
>      QEMU_CAPS_LAST /* this must always be the last item */
>  } virQEMUCapsFlags;
> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> index b484b7b..7e5f727 100644
> --- a/src/qemu/qemu_command.c
> +++ b/src/qemu/qemu_command.c
> @@ -2874,20 +2874,32 @@ qemuBuildControllerDevStr(const virDomainDef *domainDef,
>                                 def->opts.pciopts.modelName);
>                  goto error;
>              }
> -            if (def->opts.pciopts.modelName
> -                != VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_IOH3420) {
> +            if ((def->opts.pciopts.modelName !=
> +                 VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_IOH3420) &&
> +                (def->opts.pciopts.modelName !=
> +                 VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_PCIE_ROOT_PORT)) {
>                  virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
>                                 _("PCI controller model name '%s' "
>                                   "is not valid for a pcie-root-port"),
>                                 modelName);
>                  goto error;
>              }
> -            if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_IOH3420)) {
> +            if ((def->opts.pciopts.modelName ==
> +                 VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_IOH3420) &&
> +                !virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_IOH3420)) {
>                  virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
>                                 _("the pcie-root-port (ioh3420) "
>                                   "controller is not supported in this QEMU binary"));
>                  goto error;
>              }
> +            if ((def->opts.pciopts.modelName ==
> +                 VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_PCIE_ROOT_PORT) &&
> +                !virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_PCIE_ROOT_PORT)) {
> +                virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> +                               _("the pcie-root-port (pcie-root-port) "
> +                                 "controller is not supported in this QEMU binary"));
> +                goto error;
> +            }
>  
>              virBufferAsprintf(&buf, "%s,port=0x%x,chassis=%d,id=%s",
>                                modelName, def->opts.pciopts.port,
> diff --git a/tests/qemucapabilitiesdata/caps_2.9.0.x86_64.xml b/tests/qemucapabilitiesdata/caps_2.9.0.x86_64.xml
> index 334f8e7..a397615 100644
> --- a/tests/qemucapabilitiesdata/caps_2.9.0.x86_64.xml
> +++ b/tests/qemucapabilitiesdata/caps_2.9.0.x86_64.xml
> @@ -202,6 +202,7 @@
>    <flag name='drive-iotune-group'/>
>    <flag name='query-cpu-model-expansion'/>
>    <flag name='virtio-net.host_mtu'/>
> +  <flag name='pcie-root-port'/>
>    <version>2008050</version>
>    <kvmVersion>0</kvmVersion>
>    <package> (v2.8.0-1961-g5b10b94bd5)</package>
> 

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

On Wed, 2017-03-15 at 15:18 -0400, Laine Stump wrote:
> > Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1408808
> 
> Yes, but kind of no. It makes it available, but still difficult and
> cumbersome to use. I would say it *partially* resolves that BZ, with
> full resolution coming from the followup patches that make it the
> default. (I'm only pointing this out because we wouldn't want some
> distro maintainer to be looking through patches for backport and
> erroneously believe, based on the commit log, that this was the only
> patch they needed).

I was debating about this myself.

Strictly speaking, the bug report is about adding support
for generic PCIe Root Ports, which this patch does.

That said, I see your point, and since I don't have any
specific objection to moving the Resolves: to the next
patch I'll do just that.

[...]
> BTW, although you added an entry to your new-fangled "news" file in
> patch 4, you never added new info to formatdomain.html.in - at least
> that should be included in this patch (with a small addition to its text
> when you change the default for aarch64).

The existing documentation is fairly vague about PCI
controller models:

  PCI controllers also have an optional subelement <model>
  with an attribute name. The name attribute holds the
  name of the specific device that qemu is emulating (e.g.
  "i82801b11-bridge") rather than simply the class of device
  ("dmi-to-pci-bridge", "pci-bridge"), which is set in the
  controller element's model attribute. In almost all cases,
  you should not manually add a <model> subelement to a
  controller, nor should you modify one that is automatically
  generated by libvirt. Since 1.2.19 (QEMU only).

Nowhere are the valid models for each PCI controllers listed,
and that's fine in my book: the documentation explicitly
tells the user that they should let libvirt do its thing in
basically all cases.

Do you really think we should make that more explicit?

[...]
> > @@ -338,7 +338,9 @@ VIR_ENUM_IMPL(virDomainControllerPCIModelName,
> >                "x3130-upstream",
> >                "xio3130-downstream",
> >                "pxb",
> > -              "pxb-pcie")
> > +              "pxb-pcie",
> > +              "pcie-root-port",
> 
> Sigh. As this becomes the norm, it's going to make libvirt config look
> redundant (and is also likely to confuse people about which attribute to
> change if they want to use iohh3420 instead of the generic one), but
> there's nothing that can be done about it. (I agree that this was the
> best choice for device name in qemu btw).

You just have to change the model, as opposed to, you
know... The model. What's so confusing about that? :D

-- 
Andrea Bolognani / Red Hat / Virtualization

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

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



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

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


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

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

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list