On Fri, Oct 28, 2016 at 12:07:12PM +0200, Greg Kurz wrote: > On Fri, 28 Oct 2016 18:56:40 +1100 > Alexey Kardashevskiy <a...@ozlabs.ru> wrote: > > > At the moment sPAPR PHB creates a root buf of TYPE_PCI_BUS type. > > This means that vfio-pci devices attached to it (and this is > > a default behaviour) hide PCIe extended capabilities as > > the bus does not pass a pci_bus_is_express(pdev->bus) check. > > > > This changes adds a default PCI bus type property to sPAPR PHB > > and uses TYPE_PCIE_BUS if none passed; older machines get TYPE_PCI_BUS > > for backward compatibility as a bus type is used in the bus name > > so the root bus name becomes "pcie.0" instead of "pci.0". > > > > Signed-off-by: Alexey Kardashevskiy <a...@ozlabs.ru> > > --- > > > > What can possibly go wrong with such change of a name? > > From devices prospective, I cannot see any. > > > > libvirt might get upset as "pci.0" will not be available, > > will it make sense to create pcie.0 as a root bus and always > > add a PCIe->PCI bridge and name its bus "pci.0"? > > > > Or create root bus from TYPE_PCIE_BUS and force name to "pci.0"? > > pci_register_bus() can do this. > > > > > > --- > > hw/ppc/spapr.c | 5 +++++ > > hw/ppc/spapr_pci.c | 5 ++++- > > include/hw/pci-host/spapr.h | 1 + > > 3 files changed, 10 insertions(+), 1 deletion(-) > > > > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c > > index 0b3820b..a268511 100644 > > --- a/hw/ppc/spapr.c > > +++ b/hw/ppc/spapr.c > > @@ -2541,6 +2541,11 @@ DEFINE_SPAPR_MACHINE(2_8, "2.8", true); > > .driver = TYPE_SPAPR_PCI_HOST_BRIDGE, \ > > .property = "mem64_win_size", \ > > .value = "0", \ > > + }, \ > > + { \ > > + .driver = TYPE_SPAPR_PCI_HOST_BRIDGE, \ > > + .property = "root_bus_type", \ > > + .value = TYPE_PCI_BUS, \ > > }, > > > > static void phb_placement_2_7(sPAPRMachineState *spapr, uint32_t index, > > diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c > > index 7cde30e..2fa1f22 100644 > > --- a/hw/ppc/spapr_pci.c > > +++ b/hw/ppc/spapr_pci.c > > @@ -1434,7 +1434,9 @@ static void spapr_phb_realize(DeviceState *dev, Error > > **errp) > > bus = pci_register_bus(dev, NULL, > > pci_spapr_set_irq, pci_spapr_map_irq, sphb, > > &sphb->memspace, &sphb->iospace, > > - PCI_DEVFN(0, 0), PCI_NUM_PINS, TYPE_PCI_BUS); > > + PCI_DEVFN(0, 0), PCI_NUM_PINS, > > + sphb->root_bus_type ? sphb->root_bus_type : > > + TYPE_PCIE_BUS); > > Shouldn't we ensure that sphb->root_bus_type is either TYPE_PCIE_BUS or > TYPE_PCI_BUS ?
Yes, I think so. In fact, I think it would be better to make the property a boolean that just selects PCI-E, rather than this which exposes qemu (semi-)internal type names on the comamnd line. > Otherwise, if we pass something like: > > -global spapr-pci-host-bridge.root_bus_type=pcie > > we get the following not really explicit error: > > ERROR:/home/greg/Work/qemu/qemu-spapr/qom/object.c:474:object_new_with_type: > assertion failed: (type != NULL) > > > phb->bus = bus; > > qbus_set_hotplug_handler(BUS(phb->bus), DEVICE(sphb), NULL); > > > > @@ -1590,6 +1592,7 @@ static Property spapr_phb_properties[] = { > > DEFINE_PROP_UINT64("pgsz", sPAPRPHBState, page_size_mask, > > (1ULL << 12) | (1ULL << 16)), > > DEFINE_PROP_UINT32("numa_node", sPAPRPHBState, numa_node, -1), > > + DEFINE_PROP_STRING("root_bus_type", sPAPRPHBState, root_bus_type), > > DEFINE_PROP_END_OF_LIST(), > > }; > > > > diff --git a/include/hw/pci-host/spapr.h b/include/hw/pci-host/spapr.h > > index b92c1b5..a4ee873 100644 > > --- a/include/hw/pci-host/spapr.h > > +++ b/include/hw/pci-host/spapr.h > > @@ -79,6 +79,7 @@ struct sPAPRPHBState { > > uint64_t dma64_win_addr; > > > > uint32_t numa_node; > > + char *root_bus_type; > > }; > > > > #define SPAPR_PCI_MEM_WIN_BUS_OFFSET 0x80000000ULL > -- 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
signature.asc
Description: PGP signature