Additional PHBs (pci-root controllers) will be created for
the guest using the spapr-pci-host-bridge QEMU device, if
available; the implicit default PHB, while present in the
guest configuration, will be skipped.

Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1431193

Signed-off-by: Andrea Bolognani <abologna@redhat.com>
---
 src/qemu/qemu_command.c | 47 ++++++++++++++++++++++++++++++++++++++++++++---
 1 file changed, 44 insertions(+), 3 deletions(-)

diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index dba2519..fb0beed 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -3164,6 +3164,40 @@ qemuBuildControllerDevStr(const virDomainDef *domainDef,
                                  def->opts.pciopts.numaNode);
             break;
         case VIR_DOMAIN_CONTROLLER_MODEL_PCI_ROOT:
+            if (def->opts.pciopts.modelName == VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_NONE ||
+                def->opts.pciopts.idx == -1) {
+                virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+                               _("autogenerated pci-root options not set"));
+                goto error;
+            }
+
+            /* Skip the implicit one */
+            if (def->opts.pciopts.idx == 0)
+                goto done;
+
+            modelName = virDomainControllerPCIModelNameTypeToString(def->opts.pciopts.modelName);
+            if (!modelName) {
+                virReportError(VIR_ERR_INTERNAL_ERROR,
+                               _("unknown pci-root model name value %d"),
+                               def->opts.pciopts.modelName);
+                goto error;
+            }
+            if (def->opts.pciopts.modelName != VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_SPAPR_PCI_HOST_BRIDGE) {
+                virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+                               _("PCI controller model name '%s' is not valid for a pci-root"),
+                               modelName);
+                goto error;
+            }
+            if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_SPAPR_PCI_HOST_BRIDGE)) {
+                virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+                               _("the spapr-pci-host-bridge controller "
+                                 "is not supported in this QEMU binary"));
+                goto error;
+            }
+            virBufferAsprintf(&buf, "%s,index=%d,id=%s",
+                              modelName, def->opts.pciopts.idx,
+                              def->info.alias);
+            break;
         case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT:
         case VIR_DOMAIN_CONTROLLER_MODEL_PCI_LAST:
             virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
@@ -3213,6 +3247,7 @@ qemuBuildControllerDevStr(const virDomainDef *domainDef,
     if (virBufferCheckError(&buf) < 0)
         goto error;
 
+ done:
     *devstr = virBufferContentAndReset(&buf);
     return 0;
 
@@ -3270,10 +3305,16 @@ qemuBuildControllerDevCommandLine(virCommandPtr cmd,
                 continue;
             }
 
-            /* skip pci-root/pcie-root */
+            /* skip pcie-root */
             if (cont->type == VIR_DOMAIN_CONTROLLER_TYPE_PCI &&
-                (cont->model == VIR_DOMAIN_CONTROLLER_MODEL_PCI_ROOT ||
-                 cont->model == VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT))
+                cont->model == VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT)
+                continue;
+
+            /* Skip pci-root, except for pSeries guests (which actually
+             * support more than one PCI Host Bridge per guest) */
+            if (!qemuDomainIsPSeries(def) &&
+                cont->type == VIR_DOMAIN_CONTROLLER_TYPE_PCI &&
+                cont->model == VIR_DOMAIN_CONTROLLER_MODEL_PCI_ROOT)
                 continue;
 
             /* first SATA controller on Q35 machines is implicit */
-- 
2.7.5

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

Hi Andrea,

We need to also adjust the memlock limits along with this patch.

I have the changes here if you want to append to this patch.

https://paste.fedoraproject.org/paste/JrI4stQTIYiaecuXPyXh8g

Thanks,
Shivaprasad


On Fri, Jun 23, 2017 at 8:33 PM, Andrea Bolognani <abologna@redhat.com>
wrote:

> Additional PHBs (pci-root controllers) will be created for
> the guest using the spapr-pci-host-bridge QEMU device, if
> available; the implicit default PHB, while present in the
> guest configuration, will be skipped.
>
> Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1431193
>
> Signed-off-by: Andrea Bolognani <abologna@redhat.com>
> ---
>  src/qemu/qemu_command.c | 47 ++++++++++++++++++++++++++++++
> ++++++++++++++---
>  1 file changed, 44 insertions(+), 3 deletions(-)
>
> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> index dba2519..fb0beed 100644
> --- a/src/qemu/qemu_command.c
> +++ b/src/qemu/qemu_command.c
> @@ -3164,6 +3164,40 @@ qemuBuildControllerDevStr(const virDomainDef
> *domainDef,
>                                   def->opts.pciopts.numaNode);
>              break;
>          case VIR_DOMAIN_CONTROLLER_MODEL_PCI_ROOT:
> +            if (def->opts.pciopts.modelName == VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_NONE
> ||
> +                def->opts.pciopts.idx == -1) {
> +                virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> +                               _("autogenerated pci-root options not
> set"));
> +                goto error;
> +            }
> +
> +            /* Skip the implicit one */
> +            if (def->opts.pciopts.idx == 0)
> +                goto done;
> +
> +            modelName = virDomainControllerPCIModelNam
> eTypeToString(def->opts.pciopts.modelName);
> +            if (!modelName) {
> +                virReportError(VIR_ERR_INTERNAL_ERROR,
> +                               _("unknown pci-root model name value %d"),
> +                               def->opts.pciopts.modelName);
> +                goto error;
> +            }
> +            if (def->opts.pciopts.modelName != VIR_DOMAIN_CONTROLLER_PCI_
> MODEL_NAME_SPAPR_PCI_HOST_BRIDGE) {
> +                virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> +                               _("PCI controller model name '%s' is not
> valid for a pci-root"),
> +                               modelName);
> +                goto error;
> +            }
> +            if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_SPAPR_PCI_HOST_BRIDGE))
> {
> +                virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> +                               _("the spapr-pci-host-bridge controller "
> +                                 "is not supported in this QEMU binary"));
> +                goto error;
> +            }
> +            virBufferAsprintf(&buf, "%s,index=%d,id=%s",
> +                              modelName, def->opts.pciopts.idx,
> +                              def->info.alias);
> +            break;
>          case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT:
>          case VIR_DOMAIN_CONTROLLER_MODEL_PCI_LAST:
>              virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> @@ -3213,6 +3247,7 @@ qemuBuildControllerDevStr(const virDomainDef
> *domainDef,
>      if (virBufferCheckError(&buf) < 0)
>          goto error;
>
> + done:
>      *devstr = virBufferContentAndReset(&buf);
>      return 0;
>
> @@ -3270,10 +3305,16 @@ qemuBuildControllerDevCommandLine(virCommandPtr
> cmd,
>                  continue;
>              }
>
> -            /* skip pci-root/pcie-root */
> +            /* skip pcie-root */
>              if (cont->type == VIR_DOMAIN_CONTROLLER_TYPE_PCI &&
> -                (cont->model == VIR_DOMAIN_CONTROLLER_MODEL_PCI_ROOT ||
> -                 cont->model == VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT))
> +                cont->model == VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT)
> +                continue;
> +
> +            /* Skip pci-root, except for pSeries guests (which actually
> +             * support more than one PCI Host Bridge per guest) */
> +            if (!qemuDomainIsPSeries(def) &&
> +                cont->type == VIR_DOMAIN_CONTROLLER_TYPE_PCI &&
> +                cont->model == VIR_DOMAIN_CONTROLLER_MODEL_PCI_ROOT)
>                  continue;
>
>              /* first SATA controller on Q35 machines is implicit */
> --
> 2.7.5
>
> --
> libvir-list mailing list
> libvir-list@redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list
>
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

On Tue, 2017-06-27 at 13:53 +0530, Shivaprasad bhat wrote:
> Hi Andrea,
> 
> We need to also adjust the memlock limits along with this patch. 

Good point. I must have remembered about that at least
five during development of the feature - and then promptly
forgotten just as many times :/

> I have the changes here if you want to append to this patch.
> 
> https://paste.fedoraproject.org/paste/JrI4stQTIYiaecuXPyXh8g

Please send this as a proper patch, mentioning that it's
meant to be applied on top of my series.

-- 
Andrea Bolognani / Red Hat / Virtualization

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

On Thu, Jun 29, 2017 at 1:03 PM, Andrea Bolognani <abologna@redhat.com>
wrote:

> On Tue, 2017-06-27 at 13:53 +0530, Shivaprasad bhat wrote:
> > Hi Andrea,
> >
> > We need to also adjust the memlock limits along with this patch.
>
> Good point. I must have remembered about that at least
> five during development of the feature - and then promptly
> forgotten just as many times :/
>
>
I guessed so.. :)


> > I have the changes here if you want to append to this patch.
> >
> > https://paste.fedoraproject.org/paste/JrI4stQTIYiaecuXPyXh8g
>
> Please send this as a proper patch, mentioning that it's
> meant to be applied on top of my series.
>
>
Just sent the patch.

Thanks,
Shivaprasad

-- 
> Andrea Bolognani / Red Hat / Virtualization
>
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

The subject line implies (to me) that this patch somehow automagically
creates multiple PHBs for the domain. It looks like what it does is to
add the proper commandline args to the qemu command when multiple PHBs
have already been added to the config.

As for the functionality:

Reviewed-by: Laine Stump <laine@laine.org>

On 06/23/2017 11:03 AM, Andrea Bolognani wrote:
> Additional PHBs (pci-root controllers) will be created for
> the guest using the spapr-pci-host-bridge QEMU device, if
> available; the implicit default PHB, while present in the
> guest configuration, will be skipped.
> 
> Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1431193
> 
> Signed-off-by: Andrea Bolognani <abologna@redhat.com>
> ---
>  src/qemu/qemu_command.c | 47 ++++++++++++++++++++++++++++++++++++++++++++---
>  1 file changed, 44 insertions(+), 3 deletions(-)
> 
> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> index dba2519..fb0beed 100644
> --- a/src/qemu/qemu_command.c
> +++ b/src/qemu/qemu_command.c
> @@ -3164,6 +3164,40 @@ qemuBuildControllerDevStr(const virDomainDef *domainDef,
>                                   def->opts.pciopts.numaNode);
>              break;
>          case VIR_DOMAIN_CONTROLLER_MODEL_PCI_ROOT:
> +            if (def->opts.pciopts.modelName == VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_NONE ||
> +                def->opts.pciopts.idx == -1) {
> +                virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> +                               _("autogenerated pci-root options not set"));
> +                goto error;
> +            }
> +
> +            /* Skip the implicit one */
> +            if (def->opts.pciopts.idx == 0)
> +                goto done;
> +
> +            modelName = virDomainControllerPCIModelNameTypeToString(def->opts.pciopts.modelName);
> +            if (!modelName) {
> +                virReportError(VIR_ERR_INTERNAL_ERROR,
> +                               _("unknown pci-root model name value %d"),
> +                               def->opts.pciopts.modelName);
> +                goto error;
> +            }
> +            if (def->opts.pciopts.modelName != VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_SPAPR_PCI_HOST_BRIDGE) {
> +                virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> +                               _("PCI controller model name '%s' is not valid for a pci-root"),
> +                               modelName);
> +                goto error;
> +            }
> +            if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_SPAPR_PCI_HOST_BRIDGE)) {
> +                virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> +                               _("the spapr-pci-host-bridge controller "
> +                                 "is not supported in this QEMU binary"));
> +                goto error;
> +            }
> +            virBufferAsprintf(&buf, "%s,index=%d,id=%s",
> +                              modelName, def->opts.pciopts.idx,
> +                              def->info.alias);
> +            break;
>          case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT:
>          case VIR_DOMAIN_CONTROLLER_MODEL_PCI_LAST:
>              virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> @@ -3213,6 +3247,7 @@ qemuBuildControllerDevStr(const virDomainDef *domainDef,
>      if (virBufferCheckError(&buf) < 0)
>          goto error;
>  
> + done:
>      *devstr = virBufferContentAndReset(&buf);
>      return 0;
>  
> @@ -3270,10 +3305,16 @@ qemuBuildControllerDevCommandLine(virCommandPtr cmd,
>                  continue;
>              }
>  
> -            /* skip pci-root/pcie-root */
> +            /* skip pcie-root */
>              if (cont->type == VIR_DOMAIN_CONTROLLER_TYPE_PCI &&
> -                (cont->model == VIR_DOMAIN_CONTROLLER_MODEL_PCI_ROOT ||
> -                 cont->model == VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT))
> +                cont->model == VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT)
> +                continue;
> +
> +            /* Skip pci-root, except for pSeries guests (which actually
> +             * support more than one PCI Host Bridge per guest) */
> +            if (!qemuDomainIsPSeries(def) &&
> +                cont->type == VIR_DOMAIN_CONTROLLER_TYPE_PCI &&
> +                cont->model == VIR_DOMAIN_CONTROLLER_MODEL_PCI_ROOT)
>                  continue;
>  
>              /* first SATA controller on Q35 machines is implicit */
> 

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

On Mon, 2017-06-26 at 22:00 -0400, Laine Stump wrote:
> The subject line implies (to me) that this patch somehow automagically
> creates multiple PHBs for the domain. It looks like what it does is to
> add the proper commandline args to the qemu command when multiple PHBs
> have already been added to the config.

Would

  qemu: Format PHBs on the command line

be better?

-- 
Andrea Bolognani / Red Hat / Virtualization

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