| Message ID | 1626144876-11352-4-git-send-email-zhangfei.gao@linaro.org (mailing list archive) |
|---|---|
| State | Accepted |
| Delegated to: | Bjorn Helgaas |
| Headers | show |
| Series | PCI: Add a quirk to enable SVA for HiSilicon chip | expand |
[+cc Will, Robin, Joerg, hoping for an ack] On Tue, Jul 13, 2021 at 10:54:36AM +0800, Zhangfei Gao wrote: > HiSilicon KunPeng920 and KunPeng930 have devices appear as PCI but are > actually on the AMBA bus. These fake PCI devices can support SVA via > SMMU stall feature, by setting dma-can-stall for ACPI platforms. > > Signed-off-by: Zhangfei Gao <zhangfei.gao@linaro.org> > Signed-off-by: Jean-Philippe Brucker <jean-philippe@linaro.org> > Signed-off-by: Zhou Wang <wangzhou1@hisilicon.com> > --- > drivers/pci/quirks.c | 13 +++++++++++++ > 1 file changed, 13 insertions(+) > > diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c > index 5d46ac6..03b0f98 100644 > --- a/drivers/pci/quirks.c > +++ b/drivers/pci/quirks.c > @@ -1823,10 +1823,23 @@ DECLARE_PCI_FIXUP_CLASS_FINAL(PCI_VENDOR_ID_HUAWEI, 0x1610, PCI_CLASS_BRIDGE_PCI > > static void quirk_huawei_pcie_sva(struct pci_dev *pdev) > { > + struct property_entry properties[] = { > + PROPERTY_ENTRY_BOOL("dma-can-stall"), "dma-can-stall" is used in arm_smmu_probe_device() to help set master->stall_enabled. I don't know the implications, so it'd be nice to get an ack from a maintainer of that code. > + {}, > + }; > + > if (pdev->revision != 0x21 && pdev->revision != 0x30) > return; > > pdev->pasid_no_tlp = 1; > + > + /* > + * Set the dma-can-stall property on ACPI platforms. Device tree > + * can set it directly. > + */ > + if (!pdev->dev.of_node && > + device_add_properties(&pdev->dev, properties)) > + pci_warn(pdev, "could not add stall property"); > } > DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_HUAWEI, 0xa250, quirk_huawei_pcie_sva); > DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_HUAWEI, 0xa251, quirk_huawei_pcie_sva); > -- > 2.7.4 >
On 2021-08-26 19:26, Bjorn Helgaas wrote: > [+cc Will, Robin, Joerg, hoping for an ack] > > On Tue, Jul 13, 2021 at 10:54:36AM +0800, Zhangfei Gao wrote: >> HiSilicon KunPeng920 and KunPeng930 have devices appear as PCI but are >> actually on the AMBA bus. These fake PCI devices can support SVA via >> SMMU stall feature, by setting dma-can-stall for ACPI platforms. >> >> Signed-off-by: Zhangfei Gao <zhangfei.gao@linaro.org> >> Signed-off-by: Jean-Philippe Brucker <jean-philippe@linaro.org> >> Signed-off-by: Zhou Wang <wangzhou1@hisilicon.com> >> --- >> drivers/pci/quirks.c | 13 +++++++++++++ >> 1 file changed, 13 insertions(+) >> >> diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c >> index 5d46ac6..03b0f98 100644 >> --- a/drivers/pci/quirks.c >> +++ b/drivers/pci/quirks.c >> @@ -1823,10 +1823,23 @@ DECLARE_PCI_FIXUP_CLASS_FINAL(PCI_VENDOR_ID_HUAWEI, 0x1610, PCI_CLASS_BRIDGE_PCI >> >> static void quirk_huawei_pcie_sva(struct pci_dev *pdev) >> { >> + struct property_entry properties[] = { >> + PROPERTY_ENTRY_BOOL("dma-can-stall"), > > "dma-can-stall" is used in arm_smmu_probe_device() to help set > master->stall_enabled. > > I don't know the implications, so it'd be nice to get an ack from a > maintainer of that code. If it helps, Acked-by: Robin Murphy <robin.murphy@arm.com> Normally stalling must not be enabled for PCI devices, since it would break the PCI requirement for free-flowing writes and may lead to deadlock. We expect PCI devices to support ATS and PRI if they want to be fault-tolerant, so there's no ACPI binding to describe anything else, even when a "PCI" device turns out to be a regular old SoC device dressed up as a RCiEP and normal rules don't apply. I'm taking it on trust that stalling really is safe for all possible matching devices here (in general, deadlock may still be possible in the SoC interconnect depending on topology, hence why it's an explicit opt-in even for platform devices), but TBH either way I think I'd rather have this as a quirk in the kernel under our control, than have vendors attempt to play tricks with _DSD properties out in the field :) Cheers, Robin. >> + {}, >> + }; >> + >> if (pdev->revision != 0x21 && pdev->revision != 0x30) >> return; >> >> pdev->pasid_no_tlp = 1; >> + >> + /* >> + * Set the dma-can-stall property on ACPI platforms. Device tree >> + * can set it directly. >> + */ >> + if (!pdev->dev.of_node && >> + device_add_properties(&pdev->dev, properties)) >> + pci_warn(pdev, "could not add stall property"); >> } >> DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_HUAWEI, 0xa250, quirk_huawei_pcie_sva); >> DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_HUAWEI, 0xa251, quirk_huawei_pcie_sva); >> -- >> 2.7.4 >>
On Thu, Aug 26, 2021 at 08:12:12PM +0100, Robin Murphy wrote: > On 2021-08-26 19:26, Bjorn Helgaas wrote: > > [+cc Will, Robin, Joerg, hoping for an ack] > > > > On Tue, Jul 13, 2021 at 10:54:36AM +0800, Zhangfei Gao wrote: > > > HiSilicon KunPeng920 and KunPeng930 have devices appear as PCI but are > > > actually on the AMBA bus. These fake PCI devices can support SVA via > > > SMMU stall feature, by setting dma-can-stall for ACPI platforms. > > > > > > Signed-off-by: Zhangfei Gao <zhangfei.gao@linaro.org> > > > Signed-off-by: Jean-Philippe Brucker <jean-philippe@linaro.org> > > > Signed-off-by: Zhou Wang <wangzhou1@hisilicon.com> > > > --- > > > drivers/pci/quirks.c | 13 +++++++++++++ > > > 1 file changed, 13 insertions(+) > > > > > > diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c > > > index 5d46ac6..03b0f98 100644 > > > --- a/drivers/pci/quirks.c > > > +++ b/drivers/pci/quirks.c > > > @@ -1823,10 +1823,23 @@ DECLARE_PCI_FIXUP_CLASS_FINAL(PCI_VENDOR_ID_HUAWEI, 0x1610, PCI_CLASS_BRIDGE_PCI > > > static void quirk_huawei_pcie_sva(struct pci_dev *pdev) > > > { > > > + struct property_entry properties[] = { > > > + PROPERTY_ENTRY_BOOL("dma-can-stall"), > > > > "dma-can-stall" is used in arm_smmu_probe_device() to help set > > master->stall_enabled. > > > > I don't know the implications, so it'd be nice to get an ack from a > > maintainer of that code. > > If it helps, > > Acked-by: Robin Murphy <robin.murphy@arm.com> > > Normally stalling must not be enabled for PCI devices, since it would break > the PCI requirement for free-flowing writes and may lead to deadlock. We > expect PCI devices to support ATS and PRI if they want to be fault-tolerant, > so there's no ACPI binding to describe anything else, even when a "PCI" > device turns out to be a regular old SoC device dressed up as a RCiEP and > normal rules don't apply. > > I'm taking it on trust that stalling really is safe for all possible > matching devices here (in general, deadlock may still be possible in the SoC > interconnect depending on topology, hence why it's an explicit opt-in even > for platform devices), but TBH either way I think I'd rather have this as a > quirk in the kernel under our control, than have vendors attempt to play > tricks with _DSD properties out in the field :) I think a comment next to the quirk echoing the commit message and your first paragraph above would be really helpful here. Will
diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c index 5d46ac6..03b0f98 100644 --- a/drivers/pci/quirks.c +++ b/drivers/pci/quirks.c @@ -1823,10 +1823,23 @@ DECLARE_PCI_FIXUP_CLASS_FINAL(PCI_VENDOR_ID_HUAWEI, 0x1610, PCI_CLASS_BRIDGE_PCI static void quirk_huawei_pcie_sva(struct pci_dev *pdev) { + struct property_entry properties[] = { + PROPERTY_ENTRY_BOOL("dma-can-stall"), + {}, + }; + if (pdev->revision != 0x21 && pdev->revision != 0x30) return; pdev->pasid_no_tlp = 1; + + /* + * Set the dma-can-stall property on ACPI platforms. Device tree + * can set it directly. + */ + if (!pdev->dev.of_node && + device_add_properties(&pdev->dev, properties)) + pci_warn(pdev, "could not add stall property"); } DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_HUAWEI, 0xa250, quirk_huawei_pcie_sva); DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_HUAWEI, 0xa251, quirk_huawei_pcie_sva);