| Message ID | 1371668174-32115-1-git-send-email-rkrcmar@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Delegated to: | Bjorn Helgaas |
| Headers | show |
[+cc Michael, Alex, Isaku] On Wed, Jun 19, 2013 at 12:56 PM, Radim Kr?má? <rkrcmar@redhat.com> 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. > 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. > > 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@redhat.com> > --- > 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 || > + !(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. Bjorn -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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@redhat.com> 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@redhat.com> > > --- > > 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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Jun 24, 2013 at 8:58 PM, Alex Williamson <alex.williamson@redhat.com> wrote: > 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@redhat.com> 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. I don't think that would be the same topology Radim described. In Radim's case, we have this: 00:03.0 upstream port 01:00.0 downstream port and when we call alloc_pcie_link_state() for 01:00.0, pdev is 01:00.0 pdev->bus is bus 01 pdev->bus->parent is bus 00 pdev->bus->parent->self (the bridge device leading to bus 00) is NULL But in the case of a quad 82576 plugged into a slot, there would be a root port or a downstream port leading to the slot's link, so my guess is we'd have something like this (based on lspci output I found at [1]): 00:05.0 root port leading to slot (bridge to [bus 01-08]) 01:00.0 upstream port of switch on card (bridge to [bus 02-08]) 02:02.0 downstream port (bridge to [bus 03-05]) 02:04.0 downstream port (bridge to [bus 06-08]) 03:00.0 82576 port 0 03:00.1 82576 port 1 06:00.0 82576 port 2 06:00.1 82576 port 3 So when we call alloc_pcie_link_state() for 02:02.0, pdev is 02:02.0 pdev->bus is bus 02 pdev->bus->parent is bus 01 pdev->bus->parent->self (the bridge leading to bus 01) is 00:05.0 Bjorn [1] http://sourceforge.net/p/e1000/bugs/112/ -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, 2013-06-24 at 21:35 -0600, Bjorn Helgaas wrote: > On Mon, Jun 24, 2013 at 8:58 PM, Alex Williamson > <alex.williamson@redhat.com> wrote: > > 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@redhat.com> 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. > > I don't think that would be the same topology Radim described. In > Radim's case, we have this: > > 00:03.0 upstream port > 01:00.0 downstream port > > and when we call alloc_pcie_link_state() for 01:00.0, > > pdev is 01:00.0 > pdev->bus is bus 01 > pdev->bus->parent is bus 00 > pdev->bus->parent->self (the bridge device leading to bus 00) is NULL > > But in the case of a quad 82576 plugged into a slot, there would be a > root port or a downstream port leading to the slot's link, so my guess > is we'd have something like this (based on lspci output I found at > [1]): > > 00:05.0 root port leading to slot (bridge to [bus 01-08]) > 01:00.0 upstream port of switch on card (bridge to [bus 02-08]) > 02:02.0 downstream port (bridge to [bus 03-05]) > 02:04.0 downstream port (bridge to [bus 06-08]) > 03:00.0 82576 port 0 > 03:00.1 82576 port 1 > 06:00.0 82576 port 2 > 06:00.1 82576 port 3 > > So when we call alloc_pcie_link_state() for 02:02.0, > > pdev is 02:02.0 > pdev->bus is bus 02 > pdev->bus->parent is bus 01 > pdev->bus->parent->self (the bridge leading to bus 01) is 00:05.0 Oops, I misread his statement. I don't think an upstream port connected directly to a root complex is valid. I thought he was having problems with an upstream port connected to a root port, which is obviously valid. QEMU lets us attach nearly anything to the root complex, most of them aren't valid. Thanks, Alex -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Jun 24, 2013 at 07:38:45PM -0600, Bjorn Helgaas wrote: > [+cc Michael, Alex, Isaku] > > On Wed, Jun 19, 2013 at 12:56 PM, Radim Kr?má? <rkrcmar@redhat.com> 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 don't think it's a spec compliant topology either. Anything connected to an RC is either an integrated endpoint or a root port. There's no way to have an upstream port that is also an integrated endpoint or a root port - these are distinct device types. So I don't think Linux needs to support it. Having said that, there's all kind of broken hardware out there - crashing is not a friendly way to tell users that their hardware is not spec compliant. Maybe linux can print a friendly warning and ignore this port? > > 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. > > > > 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@redhat.com> > > --- > > 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 || > > + !(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. > > Bjorn -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Jun 25, 2013 at 5:23 AM, Michael S. Tsirkin <mst@redhat.com> wrote: > On Mon, Jun 24, 2013 at 07:38:45PM -0600, Bjorn Helgaas wrote: >> [+cc Michael, Alex, Isaku] >> >> On Wed, Jun 19, 2013 at 12:56 PM, Radim Kr?má? <rkrcmar@redhat.com> 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 don't think it's a spec compliant topology either. > > Anything connected to an RC is either an integrated endpoint > or a root port. > There's no way to have an upstream port that is also > an integrated endpoint or a root port - these are distinct > device types. > > So I don't think Linux needs to support it. > > Having said that, there's all kind of broken hardware > out there - crashing is not a friendly way to tell users > that their hardware is not spec compliant. > Maybe linux can print a friendly warning and ignore > this port? Indeed, that would be nicer. I booted Win7 on the same config, and it came up fine. It did complain that it couldn't start the PCI-to-PCI bridge driver on 00:03.0, the upstream port. I'd like it if Linux could similarly tolerate that. But since this is a relatively low-priority issue, I'm going to hold out for a more straightforward solution. Checking for a null "pdev->bus->parent->self" pointer is not very obvious. I think we can probably come up with a more direct check that reads better and possibly even detects the issue at the upstream port, not the downstream port. I opened a bugzilla for this issue: https://bugzilla.kernel.org/show_bug.cgi?id=60111 Radim, can you please attach a complete dmesg log showing the oops, i.e., console output with "ignore_loglevel" and lspci -vv output (have to use your patch so it boots, I guess)? I tried to reproduce it and I know the problem is there, but I haven't quite found the right recipe yet. Bjorn -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
2013-06-25 11:17-0600, Bjorn Helgaas: > On Tue, Jun 25, 2013 at 5:23 AM, Michael S. Tsirkin <mst@redhat.com> wrote: > > On Mon, Jun 24, 2013 at 07:38:45PM -0600, Bjorn Helgaas wrote: > >> [+cc Michael, Alex, Isaku] > >> > >> On Wed, Jun 19, 2013 at 12:56 PM, Radim Kr?má? <rkrcmar@redhat.com> 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 don't think it's a spec compliant topology either. > > > > Anything connected to an RC is either an integrated endpoint > > or a root port. > > There's no way to have an upstream port that is also > > an integrated endpoint or a root port - these are distinct > > device types. > > > > So I don't think Linux needs to support it. > > > > Having said that, there's all kind of broken hardware > > out there - crashing is not a friendly way to tell users > > that their hardware is not spec compliant. > > Maybe linux can print a friendly warning and ignore > > this port? > > Indeed, that would be nicer. I booted Win7 on the same config, and it > came up fine. It did complain that it couldn't start the PCI-to-PCI > bridge driver on 00:03.0, the upstream port. True, would something like printk(KERN_WARNING "pcie: %s connected directly to root bus[complex?]:" " aspm disabled\n", dev_name(pdev)); be enough? > I'd like it if Linux could similarly tolerate that. But since this is > a relatively low-priority issue, I'm going to hold out for a more > straightforward solution. Checking for a null > "pdev->bus->parent->self" pointer is not very obvious. I think we can > probably come up with a more direct check that reads better and > possibly even detects the issue at the upstream port, not the > downstream port. The check could be in pcie_aspm_init_link_state, where a "strange" VIA chipset is already being detected and using "pci_is_root_bus()" instead of "->self" is probably clearer as well. /* QEMU can connect upstream port directly to the root complex */ if (pci_pcie_type(pdev) == PCI_EXP_TYPE_DOWNSTREAM && !pci_is_root_bus(pdev->bus->parent)) return; If we were to check for the, even worse, downstream<->root complex, it would be "!pci_is_root_bus(pdev->bus)". Upstream port is a bit neglected in our aspm implementation, so refactoring might be required to get detection in it. > I opened a bugzilla for this issue: > https://bugzilla.kernel.org/show_bug.cgi?id=60111 > > Radim, can you please attach a complete dmesg log showing the oops, > i.e., console output with "ignore_loglevel" and lspci -vv output (have > to use your patch so it boots, I guess)? I tried to reproduce it and > I know the problem is there, but I haven't quite found the right > recipe yet. Sorry, I have not mentioned that device must be connected to the switch, so "pci_scan_slot" does not end with "nr == 0", skipping the link setup. I connected the root disk; whole command for qemu-kvm 1.5: qemu-kvm -m 2048 -M q35 -serial stdio -vnc :0 \ -device x3130-upstream,bus=pcie.0,id=upstream \ -device xio3130-downstream,bus=upstream,id=downstream,chassis=1 \ -drive file=fc19.img,id=disk1,if=none,format=raw,media=disk,cache=none \ -device virtio-blk-pci,bus=downstream,id=virtio-disk1,drive=disk1 "pcie_aspm=off" should do exactly the same as patch. Both logs attached in bugzilla. -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 || + !(parent = pdev->bus->parent->self->link_state)) { kfree(link); return NULL; }
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. 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. 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@redhat.com> --- drivers/pci/pcie/aspm.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)