On Wed, 19 Jul 2017 14:34:42 +1000 David Gibson <da...@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 <da...@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 <gr...@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; > > > > > > > > > > > >
pgpgiXpbMpf2P.pgp
Description: OpenPGP digital signature