On Mon, 2013-06-24 at 19:38 -0600, Bjorn Helgaas wrote:
> [+cc Michael, Alex, Isaku]
>
> On Wed, Jun 19, 2013 at 12:56 PM, Radim Krčmář <rkrcmar@xxxxxxxxxx> wrote:
> > PCIe switch upstream port can be connected directly to the PCIe root bus
> > in QEMU; ASPM does not expect this topology and dereferences NULL pointer
> > when initializing.
> >
> > I have not confirmed this can happen on real hardware, but it is presented
> > as a feature in QEMU, so there is no reason to panic if we can recover.
>
> This doesn't seem like a valid hardware topology to me. If this *can*
> occur on real hardware, we should fix it in Linux. If not, maybe QEMU
> should be changed to disallow it.
I think a quad-port 82576 plugged into an express slot is likely the
same topology.
> > The dereference happens with topology defined by
> > -M q35 -device x3130-upstream,bus=pcie.0,id=upstream \
> > -device xio3130-downstream,bus=upstream,id=downstream,chassis=1
> > where on line drivers/pci/pcie/aspm.c:530 (alloc_pcie_link_state+13):
> > parent = pdev->bus->parent->self->link_state;
> > "pdev->bus->parent->self == NULL", because "pdev->bus->parent" has no
> > "->parent", hence no "->self".
> >
> > Even though discouraged by QEMU documentation, one can set up even
> > topology without the upstream port
> > -M q35 -device xio3130-downstream,bus=pcie.0,id=downstream,chassis=1
> > so "pdev->bus->parent == NULL", because "pdev->bus" is the root bus.
> > The patch checks for this too, because I do not like *NULL.
I don't think this is legal on real hardware.
> > Right now, PCIe switch has to connect to the root port
> > -M q35 -device ioh3420,bus=pcie.0,id=root.0 \
> > -device x3130-upstream,bus=root.0,id=upstream \
> > -device xio3130-downstream,bus=upstream,id=downstream,chassis=1
> >
> > Signed-off-by: Radim Krčmář <rkrcmar@xxxxxxxxxx>
> > ---
> > drivers/pci/pcie/aspm.c | 4 ++--
> > 1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c
> > index 403a443..1ad1514 100644
> > --- a/drivers/pci/pcie/aspm.c
> > +++ b/drivers/pci/pcie/aspm.c
> > @@ -527,8 +527,8 @@ static struct pcie_link_state *alloc_pcie_link_state(struct pci_dev *pdev)
> > link->pdev = pdev;
> > if (pci_pcie_type(pdev) == PCI_EXP_TYPE_DOWNSTREAM) {
> > struct pcie_link_state *parent;
> > - parent = pdev->bus->parent->self->link_state;
> > - if (!parent) {
> > + if (!pdev->bus->parent || !pdev->bus->parent->self ||
I think there's an extra test in here. Elsewhere we seem to assume that
if parent exists, then so does parent->self. So this could be
simplified to just add:
if (pci_is_root_bus(pdev->bus) {
kfree(link);
return NULL;
}
> > + !(parent = pdev->bus->parent->self->link_state)) {
> > kfree(link);
> > return NULL;
> > }
> > --
> > 1.8.1.4
>
> I don't really want to further complicate the "if" statement you're
> changing. The link state allocation is pretty obtuse already, and if
> this situation only occurs in QEMU, we're likely to break it again
> when somebody refactors this code.
Maybe the above plus a common exit to avoid duplicate free/return.
Thanks,
Alex
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
