[Qemu-devel] [PATCH] spapr: make default PHB optionnal

[Qemu-devel] [PATCH] spapr: make default PHB optionnal

[Greg Kurz]

The sPAPR machine always create a default PHB during initialization, even
if -nodefaults was passed on the command line. This forces the user to
rely on -global if she wants to set properties of the default PHB, such
as numa_node.

This patch introduces a new machine create-default-phb property to control
whether the default PHB must be created or not. It defaults to on in order
to preserve old setups (which is also the motivation to not alter the
current behavior of -nodefaults).

If create-default-phb is set to off, the default PHB isn't created, nor
any other device usually created with it. It is mandatory to provide
a PHB on the command line to be able to use PCI devices (otherwise QEMU
won't start). For example, the following creates a PHB with the same
mappings as the default PHB and also sets the NUMA affinity:

    -machine type=pseries,create-default-phb=off \
    -numa node,nodeid=0 -device spapr-pci-host-bridge,index=0,numa_node=0

Signed-off-by: Greg Kurz <groug@kaod.org>
---
 hw/ppc/spapr.c         |   63 ++++++++++++++++++++++++++++++++++--------------
 include/hw/ppc/spapr.h |    2 ++
 2 files changed, 47 insertions(+), 18 deletions(-)

diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index ba8f57a5a054..3395bb3774b9 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -2103,7 +2103,7 @@ static void ppc_spapr_init(MachineState *machine)
     sPAPRMachineClass *smc = SPAPR_MACHINE_GET_CLASS(machine);
     const char *kernel_filename = machine->kernel_filename;
     const char *initrd_filename = machine->initrd_filename;
-    PCIHostState *phb;
+    PCIHostState *phb = NULL;
     int i;
     MemoryRegion *sysmem = get_system_memory();
     MemoryRegion *ram = g_new(MemoryRegion, 1);
@@ -2294,7 +2294,9 @@ static void ppc_spapr_init(MachineState *machine)
     /* Set up PCI */
     spapr_pci_rtas_init();
 
-    phb = spapr_create_phb(spapr, 0);
+    if (spapr->create_default_phb) {
+        phb = spapr_create_phb(spapr, 0);
+    }
 
     for (i = 0; i < nb_nics; i++) {
         NICInfo *nd = &nd_table[i];
@@ -2305,7 +2307,7 @@ static void ppc_spapr_init(MachineState *machine)
 
         if (strcmp(nd->model, "ibmveth") == 0) {
             spapr_vlan_create(spapr->vio_bus, nd);
-        } else {
+        } else if (phb) {
             pci_nic_init_nofail(&nd_table[i], phb->bus, nd->model, NULL);
         }
     }
@@ -2314,24 +2316,26 @@ static void ppc_spapr_init(MachineState *machine)
         spapr_vscsi_create(spapr->vio_bus);
     }
 
-    /* Graphics */
-    if (spapr_vga_init(phb->bus, &error_fatal)) {
-        spapr->has_graphics = true;
-        machine->usb |= defaults_enabled() && !machine->usb_disabled;
-    }
-
-    if (machine->usb) {
-        if (smc->use_ohci_by_default) {
-            pci_create_simple(phb->bus, -1, "pci-ohci");
-        } else {
-            pci_create_simple(phb->bus, -1, "nec-usb-xhci");
+    if (phb) {
+        /* Graphics */
+        if (spapr_vga_init(phb->bus, &error_fatal)) {
+            spapr->has_graphics = true;
+            machine->usb |= defaults_enabled() && !machine->usb_disabled;
         }
 
-        if (spapr->has_graphics) {
-            USBBus *usb_bus = usb_bus_find(-1);
+        if (machine->usb) {
+            if (smc->use_ohci_by_default) {
+                pci_create_simple(phb->bus, -1, "pci-ohci");
+            } else {
+                pci_create_simple(phb->bus, -1, "nec-usb-xhci");
+            }
+
+            if (spapr->has_graphics) {
+                USBBus *usb_bus = usb_bus_find(-1);
 
-            usb_create_simple(usb_bus, "usb-kbd");
-            usb_create_simple(usb_bus, "usb-mouse");
+                usb_create_simple(usb_bus, "usb-kbd");
+                usb_create_simple(usb_bus, "usb-mouse");
+            }
         }
     }
 
@@ -2544,12 +2548,27 @@ static void spapr_set_modern_hotplug_events(Object *obj, bool value,
     spapr->use_hotplug_event_source = value;
 }
 
+static bool spapr_get_create_default_phb(Object *obj, Error **errp)
+{
+    sPAPRMachineState *spapr = SPAPR_MACHINE(obj);
+
+    return spapr->create_default_phb;
+}
+
+static void spapr_set_create_default_phb(Object *obj, bool value, Error **errp)
+{
+    sPAPRMachineState *spapr = SPAPR_MACHINE(obj);
+
+    spapr->create_default_phb = value;
+}
+
 static void spapr_machine_initfn(Object *obj)
 {
     sPAPRMachineState *spapr = SPAPR_MACHINE(obj);
 
     spapr->htab_fd = -1;
     spapr->use_hotplug_event_source = true;
+    spapr->create_default_phb = true;
     object_property_add_str(obj, "kvm-type",
                             spapr_get_kvm_type, spapr_set_kvm_type, NULL);
     object_property_set_description(obj, "kvm-type",
@@ -2568,6 +2587,14 @@ static void spapr_machine_initfn(Object *obj)
     ppc_compat_add_property(obj, "max-cpu-compat", &spapr->max_compat_pvr,
                             "Maximum permitted CPU compatibility mode",
                             &error_fatal);
+
+    object_property_add_bool(obj, "create-default-phb",
+                            spapr_get_create_default_phb,
+                            spapr_set_create_default_phb,
+                            NULL);
+    object_property_set_description(obj, "create-default-phb",
+                                    "Create a default PCI Host Bridge",
+                                    NULL);
 }
 
 static void spapr_machine_finalizefn(Object *obj)
diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
index a184ffab0ebc..6ee914764807 100644
--- a/include/hw/ppc/spapr.h
+++ b/include/hw/ppc/spapr.h
@@ -93,6 +93,8 @@ struct sPAPRMachineState {
     bool use_hotplug_event_source;
     sPAPREventSource *event_sources;
 
+    bool create_default_phb;
+
     /* ibm,client-architecture-support option negotiation */
     bool cas_reboot;
     bool cas_legacy_guest_workaround;

Re: [Qemu-devel] [PATCH] spapr: make default PHB optionnal

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 6941 bytes --]

On Mon, Jul 03, 2017 at 06:48:25PM +0200, Greg Kurz wrote:
> The sPAPR machine always create a default PHB during initialization, even
> if -nodefaults was passed on the command line. This forces the user to
> rely on -global if she wants to set properties of the default PHB, such
> as numa_node.
> 
> This patch introduces a new machine create-default-phb property to control
> whether the default PHB must be created or not. It defaults to on in order
> to preserve old setups (which is also the motivation to not alter the
> current behavior of -nodefaults).
> 
> If create-default-phb is set to off, the default PHB isn't created, nor
> any other device usually created with it. It is mandatory to provide
> a PHB on the command line to be able to use PCI devices (otherwise QEMU
> won't start). For example, the following creates a PHB with the same
> mappings as the default PHB and also sets the NUMA affinity:
> 
>     -machine type=pseries,create-default-phb=off \
>     -numa node,nodeid=0 -device spapr-pci-host-bridge,index=0,numa_node=0
> 
> Signed-off-by: Greg Kurz <groug@kaod.org>

So, I agree that the distinction between default devices that are
disabled with -nodefaults and default devices that aren't is a big
mess in qemu configuration.  But on the other hand this only addresses
one tiny aspect of that, and in the meantime means we will silently
ignore some other configuration options in some conditions.

So, what's the immediate benefit / use case for this?

> ---
>  hw/ppc/spapr.c         |   63 ++++++++++++++++++++++++++++++++++--------------
>  include/hw/ppc/spapr.h |    2 ++
>  2 files changed, 47 insertions(+), 18 deletions(-)
> 
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index ba8f57a5a054..3395bb3774b9 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -2103,7 +2103,7 @@ static void ppc_spapr_init(MachineState *machine)
>      sPAPRMachineClass *smc = SPAPR_MACHINE_GET_CLASS(machine);
>      const char *kernel_filename = machine->kernel_filename;
>      const char *initrd_filename = machine->initrd_filename;
> -    PCIHostState *phb;
> +    PCIHostState *phb = NULL;
>      int i;
>      MemoryRegion *sysmem = get_system_memory();
>      MemoryRegion *ram = g_new(MemoryRegion, 1);
> @@ -2294,7 +2294,9 @@ static void ppc_spapr_init(MachineState *machine)
>      /* Set up PCI */
>      spapr_pci_rtas_init();
>  
> -    phb = spapr_create_phb(spapr, 0);
> +    if (spapr->create_default_phb) {
> +        phb = spapr_create_phb(spapr, 0);
> +    }
>  
>      for (i = 0; i < nb_nics; i++) {
>          NICInfo *nd = &nd_table[i];
> @@ -2305,7 +2307,7 @@ static void ppc_spapr_init(MachineState *machine)
>  
>          if (strcmp(nd->model, "ibmveth") == 0) {
>              spapr_vlan_create(spapr->vio_bus, nd);
> -        } else {
> +        } else if (phb) {
>              pci_nic_init_nofail(&nd_table[i], phb->bus, nd->model, NULL);
>          }
>      }
> @@ -2314,24 +2316,26 @@ static void ppc_spapr_init(MachineState *machine)
>          spapr_vscsi_create(spapr->vio_bus);
>      }
>  
> -    /* Graphics */
> -    if (spapr_vga_init(phb->bus, &error_fatal)) {
> -        spapr->has_graphics = true;
> -        machine->usb |= defaults_enabled() && !machine->usb_disabled;
> -    }
> -
> -    if (machine->usb) {
> -        if (smc->use_ohci_by_default) {
> -            pci_create_simple(phb->bus, -1, "pci-ohci");
> -        } else {
> -            pci_create_simple(phb->bus, -1, "nec-usb-xhci");
> +    if (phb) {
> +        /* Graphics */
> +        if (spapr_vga_init(phb->bus, &error_fatal)) {
> +            spapr->has_graphics = true;
> +            machine->usb |= defaults_enabled() && !machine->usb_disabled;
>          }
>  
> -        if (spapr->has_graphics) {
> -            USBBus *usb_bus = usb_bus_find(-1);
> +        if (machine->usb) {
> +            if (smc->use_ohci_by_default) {
> +                pci_create_simple(phb->bus, -1, "pci-ohci");
> +            } else {
> +                pci_create_simple(phb->bus, -1, "nec-usb-xhci");
> +            }
> +
> +            if (spapr->has_graphics) {
> +                USBBus *usb_bus = usb_bus_find(-1);
>  
> -            usb_create_simple(usb_bus, "usb-kbd");
> -            usb_create_simple(usb_bus, "usb-mouse");
> +                usb_create_simple(usb_bus, "usb-kbd");
> +                usb_create_simple(usb_bus, "usb-mouse");
> +            }
>          }
>      }
>  
> @@ -2544,12 +2548,27 @@ static void spapr_set_modern_hotplug_events(Object *obj, bool value,
>      spapr->use_hotplug_event_source = value;
>  }
>  
> +static bool spapr_get_create_default_phb(Object *obj, Error **errp)
> +{
> +    sPAPRMachineState *spapr = SPAPR_MACHINE(obj);
> +
> +    return spapr->create_default_phb;
> +}
> +
> +static void spapr_set_create_default_phb(Object *obj, bool value, Error **errp)
> +{
> +    sPAPRMachineState *spapr = SPAPR_MACHINE(obj);
> +
> +    spapr->create_default_phb = value;
> +}
> +
>  static void spapr_machine_initfn(Object *obj)
>  {
>      sPAPRMachineState *spapr = SPAPR_MACHINE(obj);
>  
>      spapr->htab_fd = -1;
>      spapr->use_hotplug_event_source = true;
> +    spapr->create_default_phb = true;
>      object_property_add_str(obj, "kvm-type",
>                              spapr_get_kvm_type, spapr_set_kvm_type, NULL);
>      object_property_set_description(obj, "kvm-type",
> @@ -2568,6 +2587,14 @@ static void spapr_machine_initfn(Object *obj)
>      ppc_compat_add_property(obj, "max-cpu-compat", &spapr->max_compat_pvr,
>                              "Maximum permitted CPU compatibility mode",
>                              &error_fatal);
> +
> +    object_property_add_bool(obj, "create-default-phb",
> +                            spapr_get_create_default_phb,
> +                            spapr_set_create_default_phb,
> +                            NULL);
> +    object_property_set_description(obj, "create-default-phb",
> +                                    "Create a default PCI Host Bridge",
> +                                    NULL);
>  }
>  
>  static void spapr_machine_finalizefn(Object *obj)
> diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
> index a184ffab0ebc..6ee914764807 100644
> --- a/include/hw/ppc/spapr.h
> +++ b/include/hw/ppc/spapr.h
> @@ -93,6 +93,8 @@ struct sPAPRMachineState {
>      bool use_hotplug_event_source;
>      sPAPREventSource *event_sources;
>  
> +    bool create_default_phb;
> +
>      /* ibm,client-architecture-support option negotiation */
>      bool cas_reboot;
>      bool cas_legacy_guest_workaround;
> 

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [Qemu-devel] [PATCH] spapr: make default PHB optionnal

[Greg Kurz]

[-- Attachment #1: Type: text/plain, Size: 7868 bytes --]

On Tue, 4 Jul 2017 17:29:01 +1000
David Gibson <david@gibson.dropbear.id.au> wrote:

> On Mon, Jul 03, 2017 at 06:48:25PM +0200, Greg Kurz wrote:
> > The sPAPR machine always create a default PHB during initialization, even
> > if -nodefaults was passed on the command line. This forces the user to
> > rely on -global if she wants to set properties of the default PHB, such
> > as numa_node.
> > 
> > This patch introduces a new machine create-default-phb property to control
> > whether the default PHB must be created or not. It defaults to on in order
> > to preserve old setups (which is also the motivation to not alter the
> > current behavior of -nodefaults).
> > 
> > If create-default-phb is set to off, the default PHB isn't created, nor
> > any other device usually created with it. It is mandatory to provide
> > a PHB on the command line to be able to use PCI devices (otherwise QEMU
> > won't start). For example, the following creates a PHB with the same
> > mappings as the default PHB and also sets the NUMA affinity:
> > 
> >     -machine type=pseries,create-default-phb=off \
> >     -numa node,nodeid=0 -device spapr-pci-host-bridge,index=0,numa_node=0
> > 
> > Signed-off-by: Greg Kurz <groug@kaod.org>  
> 
> So, I agree that the distinction between default devices that are
> disabled with -nodefaults and default devices that aren't is a big
> mess in qemu configuration.  But on the other hand this only addresses
> one tiny aspect of that, and in the meantime means we will silently
> ignore some other configuration options in some conditions.
> 
> So, what's the immediate benefit / use case for this?
> 

With the current code base, the only way to set properties of the default
PHB, is to pass -global spapr-pci-host-bridge.prop=value for each property.
The immediate benefit of this patch is to unify the way libvirt passes
PHB description to the command line:

ie, do:

    -machine type=pseries,create-default-phb=off \
    -device spapr-pci-host-bridge,prop1=a,prop2=b,prop3=c \
    -device spapr-pci-host-bridge,prop1=d,prop2=e,prop3=f

instead of:

    -machine type=pseries \
    -global spapr-pci-host-bridge.prop1=a \
    -global spapr-pci-host-bridge.prop2=b \
    -global spapr-pci-host-bridge.prop3=c \
    -device spapr-pci-host-bridge,prop1=d,prop2=e,prop3=f

> > ---
> >  hw/ppc/spapr.c         |   63 ++++++++++++++++++++++++++++++++++--------------
> >  include/hw/ppc/spapr.h |    2 ++
> >  2 files changed, 47 insertions(+), 18 deletions(-)
> > 
> > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> > index ba8f57a5a054..3395bb3774b9 100644
> > --- a/hw/ppc/spapr.c
> > +++ b/hw/ppc/spapr.c
> > @@ -2103,7 +2103,7 @@ static void ppc_spapr_init(MachineState *machine)
> >      sPAPRMachineClass *smc = SPAPR_MACHINE_GET_CLASS(machine);
> >      const char *kernel_filename = machine->kernel_filename;
> >      const char *initrd_filename = machine->initrd_filename;
> > -    PCIHostState *phb;
> > +    PCIHostState *phb = NULL;
> >      int i;
> >      MemoryRegion *sysmem = get_system_memory();
> >      MemoryRegion *ram = g_new(MemoryRegion, 1);
> > @@ -2294,7 +2294,9 @@ static void ppc_spapr_init(MachineState *machine)
> >      /* Set up PCI */
> >      spapr_pci_rtas_init();
> >  
> > -    phb = spapr_create_phb(spapr, 0);
> > +    if (spapr->create_default_phb) {
> > +        phb = spapr_create_phb(spapr, 0);
> > +    }
> >  
> >      for (i = 0; i < nb_nics; i++) {
> >          NICInfo *nd = &nd_table[i];
> > @@ -2305,7 +2307,7 @@ static void ppc_spapr_init(MachineState *machine)
> >  
> >          if (strcmp(nd->model, "ibmveth") == 0) {
> >              spapr_vlan_create(spapr->vio_bus, nd);
> > -        } else {
> > +        } else if (phb) {
> >              pci_nic_init_nofail(&nd_table[i], phb->bus, nd->model, NULL);
> >          }
> >      }
> > @@ -2314,24 +2316,26 @@ static void ppc_spapr_init(MachineState *machine)
> >          spapr_vscsi_create(spapr->vio_bus);
> >      }
> >  
> > -    /* Graphics */
> > -    if (spapr_vga_init(phb->bus, &error_fatal)) {
> > -        spapr->has_graphics = true;
> > -        machine->usb |= defaults_enabled() && !machine->usb_disabled;
> > -    }
> > -
> > -    if (machine->usb) {
> > -        if (smc->use_ohci_by_default) {
> > -            pci_create_simple(phb->bus, -1, "pci-ohci");
> > -        } else {
> > -            pci_create_simple(phb->bus, -1, "nec-usb-xhci");
> > +    if (phb) {
> > +        /* Graphics */
> > +        if (spapr_vga_init(phb->bus, &error_fatal)) {
> > +            spapr->has_graphics = true;
> > +            machine->usb |= defaults_enabled() && !machine->usb_disabled;
> >          }
> >  
> > -        if (spapr->has_graphics) {
> > -            USBBus *usb_bus = usb_bus_find(-1);
> > +        if (machine->usb) {
> > +            if (smc->use_ohci_by_default) {
> > +                pci_create_simple(phb->bus, -1, "pci-ohci");
> > +            } else {
> > +                pci_create_simple(phb->bus, -1, "nec-usb-xhci");
> > +            }
> > +
> > +            if (spapr->has_graphics) {
> > +                USBBus *usb_bus = usb_bus_find(-1);
> >  
> > -            usb_create_simple(usb_bus, "usb-kbd");
> > -            usb_create_simple(usb_bus, "usb-mouse");
> > +                usb_create_simple(usb_bus, "usb-kbd");
> > +                usb_create_simple(usb_bus, "usb-mouse");
> > +            }
> >          }
> >      }
> >  
> > @@ -2544,12 +2548,27 @@ static void spapr_set_modern_hotplug_events(Object *obj, bool value,
> >      spapr->use_hotplug_event_source = value;
> >  }
> >  
> > +static bool spapr_get_create_default_phb(Object *obj, Error **errp)
> > +{
> > +    sPAPRMachineState *spapr = SPAPR_MACHINE(obj);
> > +
> > +    return spapr->create_default_phb;
> > +}
> > +
> > +static void spapr_set_create_default_phb(Object *obj, bool value, Error **errp)
> > +{
> > +    sPAPRMachineState *spapr = SPAPR_MACHINE(obj);
> > +
> > +    spapr->create_default_phb = value;
> > +}
> > +
> >  static void spapr_machine_initfn(Object *obj)
> >  {
> >      sPAPRMachineState *spapr = SPAPR_MACHINE(obj);
> >  
> >      spapr->htab_fd = -1;
> >      spapr->use_hotplug_event_source = true;
> > +    spapr->create_default_phb = true;
> >      object_property_add_str(obj, "kvm-type",
> >                              spapr_get_kvm_type, spapr_set_kvm_type, NULL);
> >      object_property_set_description(obj, "kvm-type",
> > @@ -2568,6 +2587,14 @@ static void spapr_machine_initfn(Object *obj)
> >      ppc_compat_add_property(obj, "max-cpu-compat", &spapr->max_compat_pvr,
> >                              "Maximum permitted CPU compatibility mode",
> >                              &error_fatal);
> > +
> > +    object_property_add_bool(obj, "create-default-phb",
> > +                            spapr_get_create_default_phb,
> > +                            spapr_set_create_default_phb,
> > +                            NULL);
> > +    object_property_set_description(obj, "create-default-phb",
> > +                                    "Create a default PCI Host Bridge",
> > +                                    NULL);
> >  }
> >  
> >  static void spapr_machine_finalizefn(Object *obj)
> > diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
> > index a184ffab0ebc..6ee914764807 100644
> > --- a/include/hw/ppc/spapr.h
> > +++ b/include/hw/ppc/spapr.h
> > @@ -93,6 +93,8 @@ struct sPAPRMachineState {
> >      bool use_hotplug_event_source;
> >      sPAPREventSource *event_sources;
> >  
> > +    bool create_default_phb;
> > +
> >      /* ibm,client-architecture-support option negotiation */
> >      bool cas_reboot;
> >      bool cas_legacy_guest_workaround;
> >   
> 


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: [Qemu-devel] [PATCH] spapr: make default PHB optionnal

[Andrea Bolognani]

[libvir-list added to the loop]

On Tue, 2017-07-04 at 10:47 +0200, Greg Kurz wrote:
> On Tue, 4 Jul 2017 17:29:01 +1000 David Gibson <david@gibson.dropbear.id.au> wrote:
> > On Mon, Jul 03, 2017 at 06:48:25PM +0200, Greg Kurz wrote:
> > > 
> > > The sPAPR machine always create a default PHB during initialization, even
> > > if -nodefaults was passed on the command line. This forces the user to
> > > rely on -global if she wants to set properties of the default PHB, such
> > > as numa_node.
> > > 
> > > This patch introduces a new machine create-default-phb property to control
> > > whether the default PHB must be created or not. It defaults to on in order
> > > to preserve old setups (which is also the motivation to not alter the
> > > current behavior of -nodefaults).
> > > 
> > > If create-default-phb is set to off, the default PHB isn't created, nor
> > > any other device usually created with it. It is mandatory to provide
> > > a PHB on the command line to be able to use PCI devices (otherwise QEMU
> > > won't start). For example, the following creates a PHB with the same
> > > mappings as the default PHB and also sets the NUMA affinity:
> > > 
> > >     -machine type=pseries,create-default-phb=off \
> > >     -numa node,nodeid=0 -device spapr-pci-host-bridge,index=0,numa_node=0
> > 
> > So, I agree that the distinction between default devices that are
> > disabled with -nodefaults and default devices that aren't is a big
> > mess in qemu configuration.  But on the other hand this only addresses
> > one tiny aspect of that, and in the meantime means we will silently
> > ignore some other configuration options in some conditions.
> > 
> > So, what's the immediate benefit / use case for this?
> 
> With the current code base, the only way to set properties of the default
> PHB, is to pass -global spapr-pci-host-bridge.prop=value for each property.
> The immediate benefit of this patch is to unify the way libvirt passes
> PHB description to the command line:
> 
> ie, do:
> 
>     -machine type=pseries,create-default-phb=off \
>     -device spapr-pci-host-bridge,prop1=a,prop2=b,prop3=c \
>     -device spapr-pci-host-bridge,prop1=d,prop2=e,prop3=f
> 
> instead of:
> 
>     -machine type=pseries \
>     -global spapr-pci-host-bridge.prop1=a \
>     -global spapr-pci-host-bridge.prop2=b \
>     -global spapr-pci-host-bridge.prop3=c \
>     -device spapr-pci-host-bridge,prop1=d,prop2=e,prop3=f

So, I'm thinking about this mostly in terms of NUMA nodes
because that's the use case I'm aware of.

The problem with using -global is not that it requires using
a different syntax to set properties for the default PHB,
but rather that such properties are then inherited by all
other PHBs unless explicitly overridden. Not creating the
default PHB at all would solve the issue.

On the other hand, libvirt would then need to either

  1) only allow setting NUMA nodes for PHBs if QEMU supports
     the new option, leaving QEMU < 2.10 users behind; or

  2) implement handling for both the new and old behavior.

I'm not sure we could get away with 1), and going for 2)
means more work both for QEMU and libvirt developers for
very little actual gain, so I'd be inclined to scrap this
and just build the libvirt glue on top of the existing
interface.

That is, of course, unless

  1) having a random selection of PHBs not assigned to any
     NUMA node is a sensible use case. This is something
     we just can't do reliably with the current interface:
     we can decide to set the NUMA node only for say, PHBs
     1 and 3 leaving 0 and 2 alone, but once we set it for
     the default PHB we *have* to set it for all remaining
     ones as well. libvirt will by default assign emulated
     devices to the default PHB, so I would rather expect
     users to leave that one alone and set a NUMA node for
     all other PHBs; or

  2) there are other properties outside of numa_node we
     might want to deal with; or

  3) it turns out it's okay to require a recent QEMU :)

-- 
Andrea Bolognani / Red Hat / Virtualization

Re: [Qemu-devel] [PATCH] spapr: make default PHB optionnal

[Shivaprasad G Bhat]

On 07/12/2017 04:25 PM, Andrea Bolognani wrote:
> [libvir-list added to the loop]
>
> On Tue, 2017-07-04 at 10:47 +0200, Greg Kurz wrote:
>> On Tue, 4 Jul 2017 17:29:01 +1000 David Gibson <david@gibson.dropbear.id.au> wrote:
>>> On Mon, Jul 03, 2017 at 06:48:25PM +0200, Greg Kurz wrote:
>>>>   
>>>> The sPAPR machine always create a default PHB during initialization, even
>>>> if -nodefaults was passed on the command line. This forces the user to
>>>> rely on -global if she wants to set properties of the default PHB, such
>>>> as numa_node.
>>>>   
>>>> This patch introduces a new machine create-default-phb property to control
>>>> whether the default PHB must be created or not. It defaults to on in order
>>>> to preserve old setups (which is also the motivation to not alter the
>>>> current behavior of -nodefaults).
>>>>   
>>>> If create-default-phb is set to off, the default PHB isn't created, nor
>>>> any other device usually created with it. It is mandatory to provide
>>>> a PHB on the command line to be able to use PCI devices (otherwise QEMU
>>>> won't start). For example, the following creates a PHB with the same
>>>> mappings as the default PHB and also sets the NUMA affinity:
>>>>   
>>>>       -machine type=pseries,create-default-phb=off \
>>>>       -numa node,nodeid=0 -device spapr-pci-host-bridge,index=0,numa_node=0
>>>   
>>> So, I agree that the distinction between default devices that are
>>> disabled with -nodefaults and default devices that aren't is a big
>>> mess in qemu configuration.  But on the other hand this only addresses
>>> one tiny aspect of that, and in the meantime means we will silently
>>> ignore some other configuration options in some conditions.
>>>   
>>> So, what's the immediate benefit / use case for this?

Setting numa_node for emulated devices is the benefit for now. On x86, I 
figured there is
no way to set the numa_node for the root controller and the emulated 
devices sitting there
all have numa_node set to -1. Only the devices on the pxb can have a 
sensible value specified.
Does it mean, the emulated devices/drivers don't care about the 
numa_node they are on?

Would it be fine on PPC to disallow setting the NUMA node for the 
default PHB because that is where
all the emulated devices sit ?

>>   
>> With the current code base, the only way to set properties of the default
>> PHB, is to pass -global spapr-pci-host-bridge.prop=value for each property.
>> The immediate benefit of this patch is to unify the way libvirt passes
>> PHB description to the command line:
>>   
>> ie, do:
>>   
>>       -machine type=pseries,create-default-phb=off \
>>       -device spapr-pci-host-bridge,prop1=a,prop2=b,prop3=c \
>>       -device spapr-pci-host-bridge,prop1=d,prop2=e,prop3=f
>>   
>> instead of:
>>   
>>       -machine type=pseries \
>>       -global spapr-pci-host-bridge.prop1=a \
>>       -global spapr-pci-host-bridge.prop2=b \
>>       -global spapr-pci-host-bridge.prop3=c \
>>       -device spapr-pci-host-bridge,prop1=d,prop2=e,prop3=f
> So, I'm thinking about this mostly in terms of NUMA nodes
> because that's the use case I'm aware of.
>
> The problem with using -global is not that it requires using
> a different syntax to set properties for the default PHB,
> but rather that such properties are then inherited by all
> other PHBs unless explicitly overridden. Not creating the
> default PHB at all would solve the issue.
>
> On the other hand, libvirt would then need to either
>
>    1) only allow setting NUMA nodes for PHBs if QEMU supports
>       the new option, leaving QEMU < 2.10 users behind; or
>
>    2) implement handling for both the new and old behavior.
>
> I'm not sure we could get away with 1), and going for 2)
> means more work both for QEMU and libvirt developers for
> very little actual gain, so I'd be inclined to scrap this
> and just build the libvirt glue on top of the existing
> interface.
>
> That is, of course, unless
>
>    1) having a random selection of PHBs not assigned to any
>       NUMA node is a sensible use case. This is something
>       we just can't do reliably with the current interface:
>       we can decide to set the NUMA node only for say, PHBs
>       1 and 3 leaving 0 and 2 alone, but once we set it for
>       the default PHB we *have* to set it for all remaining
>       ones as well. libvirt will by default assign emulated
>       devices to the default PHB, so I would rather expect
>       users to leave that one alone and set a NUMA node for
>       all other PHBs; or
>
>    2) there are other properties outside of numa_node we
>       might want to deal with; or
>
>    3) it turns out it's okay to require a recent QEMU :)
>
> -- 
> Andrea Bolognani / Red Hat / Virtualization
>

Re: [Qemu-devel] [PATCH] spapr: make default PHB optionnal

[Andrea Bolognani]

On Wed, 2017-07-12 at 17:09 +0530, Shivaprasad G Bhat wrote:
> Setting numa_node for emulated devices is the benefit for now. On x86, I
> figured there is
> no way to set the numa_node for the root controller and the emulated
> devices sitting there
> all have numa_node set to -1. Only the devices on the pxb can have a
> sensible value specified.
> Does it mean, the emulated devices/drivers don't care about the
> numa_node they are on?
> 
> Would it be fine on PPC to disallow setting the NUMA node for the
> default PHB because that is where
> all the emulated devices sit ?

I was not aware of that limitation for x86 guests.

If that's the case, then I would say it's perfectly okay to
just disallow setting the NUMA node for the default PHB, at
least for the time being.

-- 
Andrea Bolognani / Red Hat / Virtualization

Re: [Qemu-devel] [PATCH] spapr: make default PHB optionnal

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 5772 bytes --]

On Wed, Jul 12, 2017 at 05:09:37PM +0530, Shivaprasad G Bhat wrote:
> 
> 
> On 07/12/2017 04:25 PM, Andrea Bolognani wrote:
> > [libvir-list added to the loop]
> > 
> > On Tue, 2017-07-04 at 10:47 +0200, Greg Kurz wrote:
> > > On Tue, 4 Jul 2017 17:29:01 +1000 David Gibson <david@gibson.dropbear.id.au> wrote:
> > > > On Mon, Jul 03, 2017 at 06:48:25PM +0200, Greg Kurz wrote:
> > > > > The sPAPR machine always create a default PHB during initialization, even
> > > > > if -nodefaults was passed on the command line. This forces the user to
> > > > > rely on -global if she wants to set properties of the default PHB, such
> > > > > as numa_node.
> > > > > This patch introduces a new machine create-default-phb property to control
> > > > > whether the default PHB must be created or not. It defaults to on in order
> > > > > to preserve old setups (which is also the motivation to not alter the
> > > > > current behavior of -nodefaults).
> > > > > If create-default-phb is set to off, the default PHB isn't created, nor
> > > > > any other device usually created with it. It is mandatory to provide
> > > > > a PHB on the command line to be able to use PCI devices (otherwise QEMU
> > > > > won't start). For example, the following creates a PHB with the same
> > > > > mappings as the default PHB and also sets the NUMA affinity:
> > > > >       -machine type=pseries,create-default-phb=off \
> > > > >       -numa node,nodeid=0 -device spapr-pci-host-bridge,index=0,numa_node=0
> > > > So, I agree that the distinction between default devices that are
> > > > disabled with -nodefaults and default devices that aren't is a big
> > > > mess in qemu configuration.  But on the other hand this only addresses
> > > > one tiny aspect of that, and in the meantime means we will silently
> > > > ignore some other configuration options in some conditions.
> > > > So, what's the immediate benefit / use case for this?
> 
> Setting numa_node for emulated devices is the benefit for now. On x86, I
> figured there is
> no way to set the numa_node for the root controller and the emulated devices
> sitting there
> all have numa_node set to -1. Only the devices on the pxb can have a
> sensible value specified.

Given that we have the equivalent restriction on x86, I'm not seeing
removing it on Power as a priority.

> Does it mean, the emulated devices/drivers don't care about the numa_node
> they are on?

Probably not.  If nothing else, I expect the slowdown of bad NUMA
affinity is probably not significant compared to the general slowness
of an emulated device. Since the device is also in software, the
standard NUMA stuff on the host may already migrate the relevant
things to match the (host) NUMA node where the guest code is running,
so it may be strictly irrelevant in that sense as well.

> Would it be fine on PPC to disallow setting the NUMA node for the default
> PHB because that is where
> all the emulated devices sit ?
> 
> > > With the current code base, the only way to set properties of the default
> > > PHB, is to pass -global spapr-pci-host-bridge.prop=value for each property.
> > > The immediate benefit of this patch is to unify the way libvirt passes
> > > PHB description to the command line:
> > > ie, do:
> > >       -machine type=pseries,create-default-phb=off \
> > >       -device spapr-pci-host-bridge,prop1=a,prop2=b,prop3=c \
> > >       -device spapr-pci-host-bridge,prop1=d,prop2=e,prop3=f
> > > instead of:
> > >       -machine type=pseries \
> > >       -global spapr-pci-host-bridge.prop1=a \
> > >       -global spapr-pci-host-bridge.prop2=b \
> > >       -global spapr-pci-host-bridge.prop3=c \
> > >       -device spapr-pci-host-bridge,prop1=d,prop2=e,prop3=f
> > So, I'm thinking about this mostly in terms of NUMA nodes
> > because that's the use case I'm aware of.
> > 
> > The problem with using -global is not that it requires using
> > a different syntax to set properties for the default PHB,
> > but rather that such properties are then inherited by all
> > other PHBs unless explicitly overridden. Not creating the
> > default PHB at all would solve the issue.
> > 
> > On the other hand, libvirt would then need to either
> > 
> >    1) only allow setting NUMA nodes for PHBs if QEMU supports
> >       the new option, leaving QEMU < 2.10 users behind; or
> > 
> >    2) implement handling for both the new and old behavior.
> > 
> > I'm not sure we could get away with 1), and going for 2)
> > means more work both for QEMU and libvirt developers for
> > very little actual gain, so I'd be inclined to scrap this
> > and just build the libvirt glue on top of the existing
> > interface.
> > 
> > That is, of course, unless
> > 
> >    1) having a random selection of PHBs not assigned to any
> >       NUMA node is a sensible use case. This is something
> >       we just can't do reliably with the current interface:
> >       we can decide to set the NUMA node only for say, PHBs
> >       1 and 3 leaving 0 and 2 alone, but once we set it for
> >       the default PHB we *have* to set it for all remaining
> >       ones as well. libvirt will by default assign emulated
> >       devices to the default PHB, so I would rather expect
> >       users to leave that one alone and set a NUMA node for
> >       all other PHBs; or
> > 
> >    2) there are other properties outside of numa_node we
> >       might want to deal with; or
> > 
> >    3) it turns out it's okay to require a recent QEMU :)
> > 
> 

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [Qemu-devel] [PATCH] spapr: make default PHB optionnal

[Greg Kurz]

[-- Attachment #1: Type: text/plain, Size: 4930 bytes --]

On Wed, 12 Jul 2017 12:55:34 +0200
Andrea Bolognani <abologna@redhat.com> wrote:

> [libvir-list added to the loop]
> 
> On Tue, 2017-07-04 at 10:47 +0200, Greg Kurz wrote:
> > On Tue, 4 Jul 2017 17:29:01 +1000 David Gibson <david@gibson.dropbear.id.au> wrote:  
> > > On Mon, Jul 03, 2017 at 06:48:25PM +0200, Greg Kurz wrote:  
> > > > 
> > > > The sPAPR machine always create a default PHB during initialization, even
> > > > if -nodefaults was passed on the command line. This forces the user to
> > > > rely on -global if she wants to set properties of the default PHB, such
> > > > as numa_node.
> > > > 
> > > > This patch introduces a new machine create-default-phb property to control
> > > > whether the default PHB must be created or not. It defaults to on in order
> > > > to preserve old setups (which is also the motivation to not alter the
> > > > current behavior of -nodefaults).
> > > > 
> > > > If create-default-phb is set to off, the default PHB isn't created, nor
> > > > any other device usually created with it. It is mandatory to provide
> > > > a PHB on the command line to be able to use PCI devices (otherwise QEMU
> > > > won't start). For example, the following creates a PHB with the same
> > > > mappings as the default PHB and also sets the NUMA affinity:
> > > > 
> > > >     -machine type=pseries,create-default-phb=off \
> > > >     -numa node,nodeid=0 -device spapr-pci-host-bridge,index=0,numa_node=0  
> > > 
> > > So, I agree that the distinction between default devices that are
> > > disabled with -nodefaults and default devices that aren't is a big
> > > mess in qemu configuration.  But on the other hand this only addresses
> > > one tiny aspect of that, and in the meantime means we will silently
> > > ignore some other configuration options in some conditions.
> > > 
> > > So, what's the immediate benefit / use case for this?  
> > 
> > With the current code base, the only way to set properties of the default
> > PHB, is to pass -global spapr-pci-host-bridge.prop=value for each property.
> > The immediate benefit of this patch is to unify the way libvirt passes
> > PHB description to the command line:
> > 
> > ie, do:
> > 
> >     -machine type=pseries,create-default-phb=off \
> >     -device spapr-pci-host-bridge,prop1=a,prop2=b,prop3=c \
> >     -device spapr-pci-host-bridge,prop1=d,prop2=e,prop3=f
> > 
> > instead of:
> > 
> >     -machine type=pseries \
> >     -global spapr-pci-host-bridge.prop1=a \
> >     -global spapr-pci-host-bridge.prop2=b \
> >     -global spapr-pci-host-bridge.prop3=c \
> >     -device spapr-pci-host-bridge,prop1=d,prop2=e,prop3=f  
> 
> So, I'm thinking about this mostly in terms of NUMA nodes
> because that's the use case I'm aware of.
> 

This is the initial motivation behind this patch indeed.

> The problem with using -global is not that it requires using
> a different syntax to set properties for the default PHB,
> but rather that such properties are then inherited by all
> other PHBs unless explicitly overridden.

Yeah, I should have mentioned that.

> Not creating the
> default PHB at all would solve the issue.
> 
> On the other hand, libvirt would then need to either
> 
>   1) only allow setting NUMA nodes for PHBs if QEMU supports
>      the new option, leaving QEMU < 2.10 users behind; or
> 
>   2) implement handling for both the new and old behavior.
> 
> I'm not sure we could get away with 1), and going for 2)
> means more work both for QEMU and libvirt developers for
> very little actual gain, so I'd be inclined to scrap this
> and just build the libvirt glue on top of the existing
> interface.
> 

Makes sense.

> That is, of course, unless
> 
>   1) having a random selection of PHBs not assigned to any
>      NUMA node is a sensible use case. This is something
>      we just can't do reliably with the current interface:
>      we can decide to set the NUMA node only for say, PHBs
>      1 and 3 leaving 0 and 2 alone, but once we set it for
>      the default PHB we *have* to set it for all remaining
>      ones as well. libvirt will by default assign emulated
>      devices to the default PHB, so I would rather expect
>      users to leave that one alone and set a NUMA node for
>      all other PHBs; or
> 

I agree.

>   2) there are other properties outside of numa_node we
>      might want to deal with; or
> 

Dunno... and anyway, since we can have several PHBs, it is
probably easier to leave the default one alone.

>   3) it turns out it's okay to require a recent QEMU :)
> 

I guess having a recent QEMU is always better :) but you're the one
to say if it is okay for libvirt users.

> -- 
> Andrea Bolognani / Red Hat / Virtualization


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: [Qemu-devel] [PATCH] spapr: make default PHB optionnal

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 8644 bytes --]

On Tue, Jul 04, 2017 at 10:47:22AM +0200, Greg Kurz wrote:
> On Tue, 4 Jul 2017 17:29:01 +1000
> David Gibson <david@gibson.dropbear.id.au> wrote:
> 
> > On Mon, Jul 03, 2017 at 06:48:25PM +0200, Greg Kurz wrote:
> > > The sPAPR machine always create a default PHB during initialization, even
> > > if -nodefaults was passed on the command line. This forces the user to
> > > rely on -global if she wants to set properties of the default PHB, such
> > > as numa_node.
> > > 
> > > This patch introduces a new machine create-default-phb property to control
> > > whether the default PHB must be created or not. It defaults to on in order
> > > to preserve old setups (which is also the motivation to not alter the
> > > current behavior of -nodefaults).
> > > 
> > > If create-default-phb is set to off, the default PHB isn't created, nor
> > > any other device usually created with it. It is mandatory to provide
> > > a PHB on the command line to be able to use PCI devices (otherwise QEMU
> > > won't start). For example, the following creates a PHB with the same
> > > mappings as the default PHB and also sets the NUMA affinity:
> > > 
> > >     -machine type=pseries,create-default-phb=off \
> > >     -numa node,nodeid=0 -device spapr-pci-host-bridge,index=0,numa_node=0
> > > 
> > > Signed-off-by: Greg Kurz <groug@kaod.org>  
> > 
> > So, I agree that the distinction between default devices that are
> > disabled with -nodefaults and default devices that aren't is a big
> > mess in qemu configuration.  But on the other hand this only addresses
> > one tiny aspect of that, and in the meantime means we will silently
> > ignore some other configuration options in some conditions.
> > 
> > So, what's the immediate benefit / use case for this?
> > 
> 
> With the current code base, the only way to set properties of the default
> PHB, is to pass -global spapr-pci-host-bridge.prop=value for each property.
> The immediate benefit of this patch is to unify the way libvirt passes
> PHB description to the command line:

I thought you could use -set to set things more explicitly.  But I'll
admit, I can never remember how the syntax of that works.
> 
> ie, do:
> 
>     -machine type=pseries,create-default-phb=off \
>     -device spapr-pci-host-bridge,prop1=a,prop2=b,prop3=c \
>     -device spapr-pci-host-bridge,prop1=d,prop2=e,prop3=f
> 
> instead of:
> 
>     -machine type=pseries \
>     -global spapr-pci-host-bridge.prop1=a \
>     -global spapr-pci-host-bridge.prop2=b \
>     -global spapr-pci-host-bridge.prop3=c \
>     -device spapr-pci-host-bridge,prop1=d,prop2=e,prop3=f
> 
> > > ---
> > >  hw/ppc/spapr.c         |   63 ++++++++++++++++++++++++++++++++++--------------
> > >  include/hw/ppc/spapr.h |    2 ++
> > >  2 files changed, 47 insertions(+), 18 deletions(-)
> > > 
> > > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> > > index ba8f57a5a054..3395bb3774b9 100644
> > > --- a/hw/ppc/spapr.c
> > > +++ b/hw/ppc/spapr.c
> > > @@ -2103,7 +2103,7 @@ static void ppc_spapr_init(MachineState *machine)
> > >      sPAPRMachineClass *smc = SPAPR_MACHINE_GET_CLASS(machine);
> > >      const char *kernel_filename = machine->kernel_filename;
> > >      const char *initrd_filename = machine->initrd_filename;
> > > -    PCIHostState *phb;
> > > +    PCIHostState *phb = NULL;
> > >      int i;
> > >      MemoryRegion *sysmem = get_system_memory();
> > >      MemoryRegion *ram = g_new(MemoryRegion, 1);
> > > @@ -2294,7 +2294,9 @@ static void ppc_spapr_init(MachineState *machine)
> > >      /* Set up PCI */
> > >      spapr_pci_rtas_init();
> > >  
> > > -    phb = spapr_create_phb(spapr, 0);
> > > +    if (spapr->create_default_phb) {
> > > +        phb = spapr_create_phb(spapr, 0);
> > > +    }
> > >  
> > >      for (i = 0; i < nb_nics; i++) {
> > >          NICInfo *nd = &nd_table[i];
> > > @@ -2305,7 +2307,7 @@ static void ppc_spapr_init(MachineState *machine)
> > >  
> > >          if (strcmp(nd->model, "ibmveth") == 0) {
> > >              spapr_vlan_create(spapr->vio_bus, nd);
> > > -        } else {
> > > +        } else if (phb) {
> > >              pci_nic_init_nofail(&nd_table[i], phb->bus, nd->model, NULL);
> > >          }
> > >      }
> > > @@ -2314,24 +2316,26 @@ static void ppc_spapr_init(MachineState *machine)
> > >          spapr_vscsi_create(spapr->vio_bus);
> > >      }
> > >  
> > > -    /* Graphics */
> > > -    if (spapr_vga_init(phb->bus, &error_fatal)) {
> > > -        spapr->has_graphics = true;
> > > -        machine->usb |= defaults_enabled() && !machine->usb_disabled;
> > > -    }
> > > -
> > > -    if (machine->usb) {
> > > -        if (smc->use_ohci_by_default) {
> > > -            pci_create_simple(phb->bus, -1, "pci-ohci");
> > > -        } else {
> > > -            pci_create_simple(phb->bus, -1, "nec-usb-xhci");
> > > +    if (phb) {
> > > +        /* Graphics */
> > > +        if (spapr_vga_init(phb->bus, &error_fatal)) {
> > > +            spapr->has_graphics = true;
> > > +            machine->usb |= defaults_enabled() && !machine->usb_disabled;
> > >          }
> > >  
> > > -        if (spapr->has_graphics) {
> > > -            USBBus *usb_bus = usb_bus_find(-1);
> > > +        if (machine->usb) {
> > > +            if (smc->use_ohci_by_default) {
> > > +                pci_create_simple(phb->bus, -1, "pci-ohci");
> > > +            } else {
> > > +                pci_create_simple(phb->bus, -1, "nec-usb-xhci");
> > > +            }
> > > +
> > > +            if (spapr->has_graphics) {
> > > +                USBBus *usb_bus = usb_bus_find(-1);
> > >  
> > > -            usb_create_simple(usb_bus, "usb-kbd");
> > > -            usb_create_simple(usb_bus, "usb-mouse");
> > > +                usb_create_simple(usb_bus, "usb-kbd");
> > > +                usb_create_simple(usb_bus, "usb-mouse");
> > > +            }
> > >          }
> > >      }
> > >  
> > > @@ -2544,12 +2548,27 @@ static void spapr_set_modern_hotplug_events(Object *obj, bool value,
> > >      spapr->use_hotplug_event_source = value;
> > >  }
> > >  
> > > +static bool spapr_get_create_default_phb(Object *obj, Error **errp)
> > > +{
> > > +    sPAPRMachineState *spapr = SPAPR_MACHINE(obj);
> > > +
> > > +    return spapr->create_default_phb;
> > > +}
> > > +
> > > +static void spapr_set_create_default_phb(Object *obj, bool value, Error **errp)
> > > +{
> > > +    sPAPRMachineState *spapr = SPAPR_MACHINE(obj);
> > > +
> > > +    spapr->create_default_phb = value;
> > > +}
> > > +
> > >  static void spapr_machine_initfn(Object *obj)
> > >  {
> > >      sPAPRMachineState *spapr = SPAPR_MACHINE(obj);
> > >  
> > >      spapr->htab_fd = -1;
> > >      spapr->use_hotplug_event_source = true;
> > > +    spapr->create_default_phb = true;
> > >      object_property_add_str(obj, "kvm-type",
> > >                              spapr_get_kvm_type, spapr_set_kvm_type, NULL);
> > >      object_property_set_description(obj, "kvm-type",
> > > @@ -2568,6 +2587,14 @@ static void spapr_machine_initfn(Object *obj)
> > >      ppc_compat_add_property(obj, "max-cpu-compat", &spapr->max_compat_pvr,
> > >                              "Maximum permitted CPU compatibility mode",
> > >                              &error_fatal);
> > > +
> > > +    object_property_add_bool(obj, "create-default-phb",
> > > +                            spapr_get_create_default_phb,
> > > +                            spapr_set_create_default_phb,
> > > +                            NULL);
> > > +    object_property_set_description(obj, "create-default-phb",
> > > +                                    "Create a default PCI Host Bridge",
> > > +                                    NULL);
> > >  }
> > >  
> > >  static void spapr_machine_finalizefn(Object *obj)
> > > diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
> > > index a184ffab0ebc..6ee914764807 100644
> > > --- a/include/hw/ppc/spapr.h
> > > +++ b/include/hw/ppc/spapr.h
> > > @@ -93,6 +93,8 @@ struct sPAPRMachineState {
> > >      bool use_hotplug_event_source;
> > >      sPAPREventSource *event_sources;
> > >  
> > > +    bool create_default_phb;
> > > +
> > >      /* ibm,client-architecture-support option negotiation */
> > >      bool cas_reboot;
> > >      bool cas_legacy_guest_workaround;
> > >   
> > 
> 



-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [Qemu-devel] [PATCH] spapr: make default PHB optionnal

[Greg Kurz]

[-- Attachment #1: Type: text/plain, Size: 9155 bytes --]

On Wed, 19 Jul 2017 14:34:42 +1000
David Gibson <david@gibson.dropbear.id.au> wrote:

> On Tue, Jul 04, 2017 at 10:47:22AM +0200, Greg Kurz wrote:
> > On Tue, 4 Jul 2017 17:29:01 +1000
> > David Gibson <david@gibson.dropbear.id.au> wrote:
> >   
> > > On Mon, Jul 03, 2017 at 06:48:25PM +0200, Greg Kurz wrote:  
> > > > The sPAPR machine always create a default PHB during initialization, even
> > > > if -nodefaults was passed on the command line. This forces the user to
> > > > rely on -global if she wants to set properties of the default PHB, such
> > > > as numa_node.
> > > > 
> > > > This patch introduces a new machine create-default-phb property to control
> > > > whether the default PHB must be created or not. It defaults to on in order
> > > > to preserve old setups (which is also the motivation to not alter the
> > > > current behavior of -nodefaults).
> > > > 
> > > > If create-default-phb is set to off, the default PHB isn't created, nor
> > > > any other device usually created with it. It is mandatory to provide
> > > > a PHB on the command line to be able to use PCI devices (otherwise QEMU
> > > > won't start). For example, the following creates a PHB with the same
> > > > mappings as the default PHB and also sets the NUMA affinity:
> > > > 
> > > >     -machine type=pseries,create-default-phb=off \
> > > >     -numa node,nodeid=0 -device spapr-pci-host-bridge,index=0,numa_node=0
> > > > 
> > > > Signed-off-by: Greg Kurz <groug@kaod.org>    
> > > 
> > > So, I agree that the distinction between default devices that are
> > > disabled with -nodefaults and default devices that aren't is a big
> > > mess in qemu configuration.  But on the other hand this only addresses
> > > one tiny aspect of that, and in the meantime means we will silently
> > > ignore some other configuration options in some conditions.
> > > 
> > > So, what's the immediate benefit / use case for this?
> > >   
> > 
> > With the current code base, the only way to set properties of the default
> > PHB, is to pass -global spapr-pci-host-bridge.prop=value for each property.
> > The immediate benefit of this patch is to unify the way libvirt passes
> > PHB description to the command line:  
> 
> I thought you could use -set to set things more explicitly.  But I'll
> admit, I can never remember how the syntax of that works.

IIUC, -set is handled by qemu_set_option() which gets called way
earlier than ppc_spapr_init->spapr_create_phb(), ie, it only works
for devices created with -device, not for the default PHB.

> > 
> > ie, do:
> > 
> >     -machine type=pseries,create-default-phb=off \
> >     -device spapr-pci-host-bridge,prop1=a,prop2=b,prop3=c \
> >     -device spapr-pci-host-bridge,prop1=d,prop2=e,prop3=f
> > 
> > instead of:
> > 
> >     -machine type=pseries \
> >     -global spapr-pci-host-bridge.prop1=a \
> >     -global spapr-pci-host-bridge.prop2=b \
> >     -global spapr-pci-host-bridge.prop3=c \
> >     -device spapr-pci-host-bridge,prop1=d,prop2=e,prop3=f
> >   
> > > > ---
> > > >  hw/ppc/spapr.c         |   63 ++++++++++++++++++++++++++++++++++--------------
> > > >  include/hw/ppc/spapr.h |    2 ++
> > > >  2 files changed, 47 insertions(+), 18 deletions(-)
> > > > 
> > > > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> > > > index ba8f57a5a054..3395bb3774b9 100644
> > > > --- a/hw/ppc/spapr.c
> > > > +++ b/hw/ppc/spapr.c
> > > > @@ -2103,7 +2103,7 @@ static void ppc_spapr_init(MachineState *machine)
> > > >      sPAPRMachineClass *smc = SPAPR_MACHINE_GET_CLASS(machine);
> > > >      const char *kernel_filename = machine->kernel_filename;
> > > >      const char *initrd_filename = machine->initrd_filename;
> > > > -    PCIHostState *phb;
> > > > +    PCIHostState *phb = NULL;
> > > >      int i;
> > > >      MemoryRegion *sysmem = get_system_memory();
> > > >      MemoryRegion *ram = g_new(MemoryRegion, 1);
> > > > @@ -2294,7 +2294,9 @@ static void ppc_spapr_init(MachineState *machine)
> > > >      /* Set up PCI */
> > > >      spapr_pci_rtas_init();
> > > >  
> > > > -    phb = spapr_create_phb(spapr, 0);
> > > > +    if (spapr->create_default_phb) {
> > > > +        phb = spapr_create_phb(spapr, 0);
> > > > +    }
> > > >  
> > > >      for (i = 0; i < nb_nics; i++) {
> > > >          NICInfo *nd = &nd_table[i];
> > > > @@ -2305,7 +2307,7 @@ static void ppc_spapr_init(MachineState *machine)
> > > >  
> > > >          if (strcmp(nd->model, "ibmveth") == 0) {
> > > >              spapr_vlan_create(spapr->vio_bus, nd);
> > > > -        } else {
> > > > +        } else if (phb) {
> > > >              pci_nic_init_nofail(&nd_table[i], phb->bus, nd->model, NULL);
> > > >          }
> > > >      }
> > > > @@ -2314,24 +2316,26 @@ static void ppc_spapr_init(MachineState *machine)
> > > >          spapr_vscsi_create(spapr->vio_bus);
> > > >      }
> > > >  
> > > > -    /* Graphics */
> > > > -    if (spapr_vga_init(phb->bus, &error_fatal)) {
> > > > -        spapr->has_graphics = true;
> > > > -        machine->usb |= defaults_enabled() && !machine->usb_disabled;
> > > > -    }
> > > > -
> > > > -    if (machine->usb) {
> > > > -        if (smc->use_ohci_by_default) {
> > > > -            pci_create_simple(phb->bus, -1, "pci-ohci");
> > > > -        } else {
> > > > -            pci_create_simple(phb->bus, -1, "nec-usb-xhci");
> > > > +    if (phb) {
> > > > +        /* Graphics */
> > > > +        if (spapr_vga_init(phb->bus, &error_fatal)) {
> > > > +            spapr->has_graphics = true;
> > > > +            machine->usb |= defaults_enabled() && !machine->usb_disabled;
> > > >          }
> > > >  
> > > > -        if (spapr->has_graphics) {
> > > > -            USBBus *usb_bus = usb_bus_find(-1);
> > > > +        if (machine->usb) {
> > > > +            if (smc->use_ohci_by_default) {
> > > > +                pci_create_simple(phb->bus, -1, "pci-ohci");
> > > > +            } else {
> > > > +                pci_create_simple(phb->bus, -1, "nec-usb-xhci");
> > > > +            }
> > > > +
> > > > +            if (spapr->has_graphics) {
> > > > +                USBBus *usb_bus = usb_bus_find(-1);
> > > >  
> > > > -            usb_create_simple(usb_bus, "usb-kbd");
> > > > -            usb_create_simple(usb_bus, "usb-mouse");
> > > > +                usb_create_simple(usb_bus, "usb-kbd");
> > > > +                usb_create_simple(usb_bus, "usb-mouse");
> > > > +            }
> > > >          }
> > > >      }
> > > >  
> > > > @@ -2544,12 +2548,27 @@ static void spapr_set_modern_hotplug_events(Object *obj, bool value,
> > > >      spapr->use_hotplug_event_source = value;
> > > >  }
> > > >  
> > > > +static bool spapr_get_create_default_phb(Object *obj, Error **errp)
> > > > +{
> > > > +    sPAPRMachineState *spapr = SPAPR_MACHINE(obj);
> > > > +
> > > > +    return spapr->create_default_phb;
> > > > +}
> > > > +
> > > > +static void spapr_set_create_default_phb(Object *obj, bool value, Error **errp)
> > > > +{
> > > > +    sPAPRMachineState *spapr = SPAPR_MACHINE(obj);
> > > > +
> > > > +    spapr->create_default_phb = value;
> > > > +}
> > > > +
> > > >  static void spapr_machine_initfn(Object *obj)
> > > >  {
> > > >      sPAPRMachineState *spapr = SPAPR_MACHINE(obj);
> > > >  
> > > >      spapr->htab_fd = -1;
> > > >      spapr->use_hotplug_event_source = true;
> > > > +    spapr->create_default_phb = true;
> > > >      object_property_add_str(obj, "kvm-type",
> > > >                              spapr_get_kvm_type, spapr_set_kvm_type, NULL);
> > > >      object_property_set_description(obj, "kvm-type",
> > > > @@ -2568,6 +2587,14 @@ static void spapr_machine_initfn(Object *obj)
> > > >      ppc_compat_add_property(obj, "max-cpu-compat", &spapr->max_compat_pvr,
> > > >                              "Maximum permitted CPU compatibility mode",
> > > >                              &error_fatal);
> > > > +
> > > > +    object_property_add_bool(obj, "create-default-phb",
> > > > +                            spapr_get_create_default_phb,
> > > > +                            spapr_set_create_default_phb,
> > > > +                            NULL);
> > > > +    object_property_set_description(obj, "create-default-phb",
> > > > +                                    "Create a default PCI Host Bridge",
> > > > +                                    NULL);
> > > >  }
> > > >  
> > > >  static void spapr_machine_finalizefn(Object *obj)
> > > > diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
> > > > index a184ffab0ebc..6ee914764807 100644
> > > > --- a/include/hw/ppc/spapr.h
> > > > +++ b/include/hw/ppc/spapr.h
> > > > @@ -93,6 +93,8 @@ struct sPAPRMachineState {
> > > >      bool use_hotplug_event_source;
> > > >      sPAPREventSource *event_sources;
> > > >  
> > > > +    bool create_default_phb;
> > > > +
> > > >      /* ibm,client-architecture-support option negotiation */
> > > >      bool cas_reboot;
> > > >      bool cas_legacy_guest_workaround;
> > > >     
> > >   
> >   
> 
> 
> 


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]