[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-ppc] [PATCH v2 3/3] pcie: Simplify pci_adjust_config_limit()
From: |
David Gibson |
Subject: |
Re: [Qemu-ppc] [PATCH v2 3/3] pcie: Simplify pci_adjust_config_limit() |
Date: |
Mon, 13 May 2019 16:20:00 +1000 |
User-agent: |
Mutt/1.11.4 (2019-03-13) |
On Sun, May 12, 2019 at 02:13:30PM -0400, Michael S. Tsirkin wrote:
> On Tue, May 07, 2019 at 02:48:38PM +1000, David Gibson wrote:
> > On Fri, Apr 26, 2019 at 04:40:17PM +1000, Alexey Kardashevskiy wrote:
> > >
> > >
> > > On 24/04/2019 14:19, David Gibson wrote:
> > > > Since c2077e2c "pci: Adjust PCI config limit based on bus topology",
> > > > pci_adjust_config_limit() has been used in the config space read and
> > > > write
> > > > paths to only permit access to extended config space on buses which
> > > > permit
> > > > it. Specifically it prevents access on devices below a vanilla-PCI bus
> > > > via
> > > > some combination of bridges, even if both the host bridge and the device
> > > > itself are PCI-E.
> > > >
> > > > It accomplishes this with a somewhat complex call up the chain of
> > > > bridges
> > > > to see if any of them prohibit extended config space access. This is
> > > > overly complex, since we can always know if the bus will support such
> > > > access at the point it is constructed.
> > > >
> > > > This patch simplifies the test by using a flag in the PCIBus instance
> > > > indicating whether extended configuration space is accessible. It is
> > > > false for vanilla PCI buses. For PCI-E buses, it is true for root
> > > > buses and equal to the parent bus's's capability otherwise.
> > > >
> > > > For the special case of sPAPR's paravirtualized PCI root bus, which
> > > > acts mostly like vanilla PCI, but does allow extended config space
> > > > access, we override the default value of the flag from the host bridge
> > > > code.
> > > >
> > > > This should cause no behavioural change.
> > > >
> > > > Signed-off-by: David Gibson <address@hidden>cd
> > > > ---
> > > > hw/pci/pci.c | 41 ++++++++++++++++++++++------------------
> > > > hw/pci/pci_host.c | 13 +++----------
> > > > hw/ppc/spapr_pci.c | 34 ++++++++++-----------------------
> > > > include/hw/pci/pci.h | 1 -
> > > > include/hw/pci/pci_bus.h | 9 ++++++++-
> > > > 5 files changed, 44 insertions(+), 54 deletions(-)
> > > >
> > > > diff --git a/hw/pci/pci.c b/hw/pci/pci.c
> > > > index ea5941fb22..59ee034331 100644
> > > > --- a/hw/pci/pci.c
> > > > +++ b/hw/pci/pci.c
> > > > @@ -120,6 +120,27 @@ static void pci_bus_realize(BusState *qbus, Error
> > > > **errp)
> > > > vmstate_register(NULL, -1, &vmstate_pcibus, bus);
> > > > }
> > > >
> > > > +static void pcie_bus_realize(BusState *qbus, Error **errp)
> > > > +{
> > > > + PCIBus *bus = PCI_BUS(qbus);
> > > > +
> > > > + pci_bus_realize(qbus, errp);
> > > > +
> > > > + /*
> > > > + * A PCI-E bus can support extended config space if it's the root
> > > > + * bus, or if the bus/bridge above it does as well
> > > > + */
> > > > + if (pci_bus_is_root(bus)) {
> > > > + bus->flags |= PCI_BUS_EXTENDED_CONFIG_SPACE;
> > > > + } else {
> > > > + PCIBus *parent_bus = pci_get_bus(bus->parent_dev);
> > >
> > >
> > > g_assert(bus->parent_dev) ?
> > >
> > > Slightly confusingly bus->parent_dev is not the same as bus->qbus.parent
> > > and can be NULL, I'd even look into ditching parent_dev and using
> > > bus->qbus.parent instead (if possible).
> >
> > Oh boy, the can of worms I reached into following up that simple
> > comment. Yes, they're subtly different, and yes it's confusing. In
> > practice parent_dev is NULL when on a root bus, that's not a PXB bus,
> > and otherwise equal to qbus.parent.
> >
> > After a *lot* of thinking about this, I think parent_dev is actually
> > correct here - we're explicitly looking at the parent as a P2P bridge,
> > not anything else.
> >
> > But, I'll try to do some later cleanups making the parent_dev /
> > qbus.parent confusion a bit better.
> > > > +static inline bool pci_bus_allows_extended_config_space(PCIBus *bus)
> > > > +{
> > > > + return !!(bus->flags & PCI_BUS_EXTENDED_CONFIG_SPACE);
> > > > +}
> > > > +
> > > > +
> > >
> > > An extra empty line.
> > >
> > > Anyway, these are minor comments, so
> > >
> > > Reviewed-by: Alexey Kardashevskiy <address@hidden>
> > >
> > >
> > >
> > >
> > > > #endif /* QEMU_PCI_BUS_H */
> > > >
> > >
>
> Pls address comments and repost ok?
Done.
--
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