[prev in list] [next in list] [prev in thread] [next in thread] 

List:       qemu-ppc
Subject:    Re: [Qemu-ppc] [RFC PATCH qemu] spapr_pci: Create PCI-express root bus by default
From:       Alexey Kardashevskiy <aik () ozlabs ! ru>
Date:       2016-10-31 4:10:23
Message-ID: ee1d7da4-373c-4be0-7e67-897a1c90042f () ozlabs ! ru
[Download RAW message or body]

[Attachment #2 (multipart/mixed)]


On 31/10/16 13:53, David Gibson wrote:
> On Fri, Oct 28, 2016 at 12:07:12PM +0200, Greg Kurz wrote:
> > On Fri, 28 Oct 2016 18:56:40 +1100
> > Alexey Kardashevskiy <aik@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 <aik@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.


Sure, a "pcie-root" boolean property should do.

However this is not my main concern, I rather wonder if we have to have
pci.0 when we pick PCIe for the root.



> > 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
> > 
> 


-- 
Alexey


["signature.asc" (application/pgp-signature)]

[prev in list] [next in list] [prev in thread] [next in thread]