[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-ppc] [PATCH] spapr: make default PHB optionnal
From: |
Greg Kurz |
Subject: |
Re: [Qemu-ppc] [PATCH] spapr: make default PHB optionnal |
Date: |
Mon, 24 Jul 2017 20:59:59 +0200 |
On Wed, 19 Jul 2017 14:34:42 +1000
David Gibson <address@hidden> 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 <address@hidden> 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 <address@hidden>
> > >
> > > 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;
> > > >
> > >
> >
>
>
>
pgpsF1uy0y0wp.pgp
Description: OpenPGP digital signature
- [Qemu-ppc] [PATCH] spapr: make default PHB optionnal, Greg Kurz, 2017/07/03
- Re: [Qemu-ppc] [PATCH] spapr: make default PHB optionnal, David Gibson, 2017/07/04
- Re: [Qemu-ppc] [PATCH] spapr: make default PHB optionnal, Greg Kurz, 2017/07/04
- Re: [Qemu-ppc] [PATCH] spapr: make default PHB optionnal, Andrea Bolognani, 2017/07/12
- Re: [Qemu-ppc] [PATCH] spapr: make default PHB optionnal, Greg Kurz, 2017/07/12
- Re: [Qemu-ppc] [PATCH] spapr: make default PHB optionnal, Shivaprasad G Bhat, 2017/07/12
- Re: [Qemu-ppc] [PATCH] spapr: make default PHB optionnal, Andrea Bolognani, 2017/07/18
- Re: [Qemu-ppc] [PATCH] spapr: make default PHB optionnal, David Gibson, 2017/07/19
- Re: [Qemu-ppc] [PATCH] spapr: make default PHB optionnal, David Gibson, 2017/07/19
- Re: [Qemu-ppc] [PATCH] spapr: make default PHB optionnal,
Greg Kurz <=