| Message ID | 1610434192-27995-1-git-send-email-zhangfei.gao@linaro.org (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Bjorn Helgaas |
| Headers | show |
| Series | PCI: Add a quirk to enable SVA for HiSilicon chip | expand |
On Tue, Jan 12, 2021 at 02:49:52PM +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 not support tlp > and have to enable SMMU stall mode to use the SVA feature. > > Add a quirk to set dma-can-stall property and enable tlp for these devices. s/tlp/TLP/ I don't think "enable TLP" really captures what's going on here. You must be referring to the fact that you set pdev->eetlp_prefix_path. That is normally set by pci_configure_eetlp_prefix() if the Device Capabilities 2 register has the End-End TLP Prefix Supported bit set *and* all devices in the upstream path also have it set. The only place we currently test eetlp_prefix_path is in pci_enable_pasid(). In PCIe, PASID is implemented using the PASID TLP prefix, so we only enable PASID if TLP prefixes are supported. If I understand correctly, a PASID-like feature is implemented on AMBA without using TLP prefixes, and setting eetlp_prefix_path makes that work. I don't think you should do this by setting eetlp_prefix_path because TLP prefixes are used for other features, e.g., TPH. Setting eetlp_prefix_path implies these devices can also support things like TLP, and I don't think that's necessarily true. Apparently these devices have a PASID capability. I think you should add a new pci_dev bit that is specific to this idea of "PASID works without TLP prefixes" and then change pci_enable_pasid() to look at that bit as well as eetlp_prefix_path. It seems like dma-can-stall is a separate thing from PASID? If so, this should be two separate patches. If they can be separated, I would probably make the PASID thing the first patch, and then the "dma-can-stall" can be on its own as a broken DT workaround (if that's what it is) and it's easier to remove that if it become obsolete. > 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> > --- > Property dma-can-stall depends on patchset > https://lore.kernel.org/linux-iommu/20210108145217.2254447-1-jean-philippe@linaro.org/ > > drivers/pci/quirks.c | 25 +++++++++++++++++++++++++ > 1 file changed, 25 insertions(+) > > diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c > index 653660e..a27f327 100644 > --- a/drivers/pci/quirks.c > +++ b/drivers/pci/quirks.c > @@ -1825,6 +1825,31 @@ DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_E7525_MCH, quir > > DECLARE_PCI_FIXUP_CLASS_FINAL(PCI_VENDOR_ID_HUAWEI, 0x1610, PCI_CLASS_BRIDGE_PCI, 8, quirk_pcie_mch); > > +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->eetlp_prefix_path = 1; > + > + /* Device-tree can set the stall property */ > + if (!pdev->dev.of_node && > + device_add_properties(&pdev->dev, properties)) Does this mean "dma-can-stall" *can* be set via DT, and if it is, this quirk is not needed? So is this quirk basically a workaround for an old or broken DT? > + pci_warn(pdev, "could not add stall property"); > +} > + Remove this blank line to follow the style of the rest of the file. > +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); > +DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_HUAWEI, 0xa255, quirk_huawei_pcie_sva); > +DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_HUAWEI, 0xa256, quirk_huawei_pcie_sva); > +DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_HUAWEI, 0xa258, quirk_huawei_pcie_sva); > +DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_HUAWEI, 0xa259, quirk_huawei_pcie_sva); > + > /* > * It's possible for the MSI to get corrupted if SHPC and ACPI are used > * together on certain PXH-based systems. > -- > 2.7.4 >
Hi, Bjorn Thanks for the suggestion. On 2021/1/13 上午1:02, Bjorn Helgaas wrote: > On Tue, Jan 12, 2021 at 02:49:52PM +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 not support tlp >> and have to enable SMMU stall mode to use the SVA feature. >> >> Add a quirk to set dma-can-stall property and enable tlp for these devices. > s/tlp/TLP/ > > I don't think "enable TLP" really captures what's going on here. You > must be referring to the fact that you set pdev->eetlp_prefix_path. > > That is normally set by pci_configure_eetlp_prefix() if the Device > Capabilities 2 register has the End-End TLP Prefix Supported bit set > *and* all devices in the upstream path also have it set. > > The only place we currently test eetlp_prefix_path is in > pci_enable_pasid(). In PCIe, PASID is implemented using the PASID TLP > prefix, so we only enable PASID if TLP prefixes are supported. > > If I understand correctly, a PASID-like feature is implemented on AMBA > without using TLP prefixes, and setting eetlp_prefix_path makes that > work. Yes, that's the requirement. > > I don't think you should do this by setting eetlp_prefix_path because > TLP prefixes are used for other features, e.g., TPH. Setting > eetlp_prefix_path implies these devices can also support things like > TLP, and I don't think that's necessarily true. Thanks for the remainder. > > Apparently these devices have a PASID capability. I think you should > add a new pci_dev bit that is specific to this idea of "PASID works > without TLP prefixes" and then change pci_enable_pasid() to look at > that bit as well as eetlp_prefix_path. That's great, this solution is much simpler. we can set the bit before pci_enable_pasid. > > It seems like dma-can-stall is a separate thing from PASID? If so, > this should be two separate patches. > > If they can be separated, I would probably make the PASID thing the > first patch, and then the "dma-can-stall" can be on its own as a > broken DT workaround (if that's what it is) and it's easier to remove > that if it become obsolete. > >> 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> >> --- >> Property dma-can-stall depends on patchset >> https://lore.kernel.org/linux-iommu/20210108145217.2254447-1-jean-philippe@linaro.org/ >> >> drivers/pci/quirks.c | 25 +++++++++++++++++++++++++ >> 1 file changed, 25 insertions(+) >> >> diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c >> index 653660e..a27f327 100644 >> --- a/drivers/pci/quirks.c >> +++ b/drivers/pci/quirks.c >> @@ -1825,6 +1825,31 @@ DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_E7525_MCH, quir >> >> DECLARE_PCI_FIXUP_CLASS_FINAL(PCI_VENDOR_ID_HUAWEI, 0x1610, PCI_CLASS_BRIDGE_PCI, 8, quirk_pcie_mch); >> >> +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->eetlp_prefix_path = 1; >> + >> + /* Device-tree can set the stall property */ >> + if (!pdev->dev.of_node && >> + device_add_properties(&pdev->dev, properties)) > Does this mean "dma-can-stall" *can* be set via DT, and if it is, this > quirk is not needed? So is this quirk basically a workaround for an > old or broken DT? The quirk is still needed for uefi case, since uefi can not describe the endpoints (peripheral devices). > >> + pci_warn(pdev, "could not add stall property"); >> +} >> + > Remove this blank line to follow the style of the rest of the file. > >> +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); >> +DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_HUAWEI, 0xa255, quirk_huawei_pcie_sva); >> +DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_HUAWEI, 0xa256, quirk_huawei_pcie_sva); >> +DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_HUAWEI, 0xa258, quirk_huawei_pcie_sva); >> +DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_HUAWEI, 0xa259, quirk_huawei_pcie_sva); >> + >> /* >> * It's possible for the MSI to get corrupted if SHPC and ACPI are used >> * together on certain PXH-based systems. How about changes like this diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c index 68f53f7..886ea26 100644 --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c @@ -2466,6 +2466,9 @@ static int arm_smmu_enable_pasid(struct arm_smmu_master *master) if (num_pasids <= 0) return num_pasids; + if (master->stall_enabled) + pdev->pasid_no_tlp = 1; + ret = pci_enable_pasid(pdev, features); if (ret) { dev_err(&pdev->dev, "Failed to enable PASID\n"); @@ -2860,6 +2863,11 @@ static struct iommu_device *arm_smmu_probe_device(struct device *dev) device_property_read_u32(dev, "pasid-num-bits", &master->ssid_bits); master->ssid_bits = min(smmu->ssid_bits, master->ssid_bits); + if ((smmu->features & ARM_SMMU_FEAT_STALLS && + device_property_read_bool(dev, "dma-can-stall")) || + smmu->features & ARM_SMMU_FEAT_STALL_FORCE) + master->stall_enabled = true; + /* * Note that PASID must be enabled before, and disabled after ATS: * PCI Express Base 4.0r1.0 - 10.5.1.3 ATS Control Register @@ -2874,11 +2882,6 @@ static struct iommu_device *arm_smmu_probe_device(struct device *dev) master->ssid_bits = min_t(u8, master->ssid_bits, CTXDESC_LINEAR_CDMAX); - if ((smmu->features & ARM_SMMU_FEAT_STALLS && - device_property_read_bool(dev, "dma-can-stall")) || - smmu->features & ARM_SMMU_FEAT_STALL_FORCE) - master->stall_enabled = true; - arm_smmu_init_pri(master); return &smmu->iommu; diff --git a/drivers/pci/ats.c b/drivers/pci/ats.c index e36d601..fe604b5 100644 --- a/drivers/pci/ats.c +++ b/drivers/pci/ats.c @@ -386,7 +386,7 @@ int pci_enable_pasid(struct pci_dev *pdev, int features) if (WARN_ON(pdev->pasid_enabled)) return -EBUSY; - if (!pdev->eetlp_prefix_path) + if ((!pdev->eetlp_prefix_path) && (!pdev->pasid_no_tlp)) return -EINVAL; if (!pasid) diff --git a/include/linux/pci.h b/include/linux/pci.h index f1f26f8..fbee7fe 100644 --- a/include/linux/pci.h +++ b/include/linux/pci.h @@ -388,6 +388,7 @@ struct pci_dev { supported from root to here */ u16 l1ss; /* L1SS Capability pointer */ #endif + unsigned int pasid_no_tlp:1; /* PASID can be supported without TLP Prefix */ unsigned int eetlp_prefix_path:1; /* End-to-End TLP Prefix */ pci_channel_state_t error_state; /* Current connectivity state */ Thanks
On Wed, Jan 13, 2021 at 08:05:11PM +0800, Zhangfei Gao wrote: > > > + /* Device-tree can set the stall property */ > > > + if (!pdev->dev.of_node && > > > + device_add_properties(&pdev->dev, properties)) > > Does this mean "dma-can-stall" *can* be set via DT, and if it is, this > > quirk is not needed? So is this quirk basically a workaround for an > > old or broken DT? > The quirk is still needed for uefi case, since uefi can not describe the > endpoints (peripheral devices). Yes, this comment isn't very clear. How about /* * Set the dma-can-stall property on ACPI platforms. Device tree * can set it directly. */ > > > > > + pci_warn(pdev, "could not add stall property"); > > > +} > > > + > > Remove this blank line to follow the style of the rest of the file. > > > > > +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); > > > +DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_HUAWEI, 0xa255, quirk_huawei_pcie_sva); > > > +DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_HUAWEI, 0xa256, quirk_huawei_pcie_sva); > > > +DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_HUAWEI, 0xa258, quirk_huawei_pcie_sva); > > > +DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_HUAWEI, 0xa259, quirk_huawei_pcie_sva); > > > + > > > /* > > > * It's possible for the MSI to get corrupted if SHPC and ACPI are used > > > * together on certain PXH-based systems. > > How about changes like this > > diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c > b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c > index 68f53f7..886ea26 100644 > --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c > +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c > @@ -2466,6 +2466,9 @@ static int arm_smmu_enable_pasid(struct > arm_smmu_master *master) > if (num_pasids <= 0) > return num_pasids; > > + if (master->stall_enabled) > + pdev->pasid_no_tlp = 1; > + From the SMMU perspective there is no relation between stall and pasid, so I don't think this makes a lot of sense. Could we instead set pasid_no_tlp for the list of device IDs above? I agree with splitting the patches. PASID support for SMMUv3 is upstream, but the introduction of dma-can-stall, which this depends on, is still pending on the list. Thanks, Jean
diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c index 653660e..a27f327 100644 --- a/drivers/pci/quirks.c +++ b/drivers/pci/quirks.c @@ -1825,6 +1825,31 @@ DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_E7525_MCH, quir DECLARE_PCI_FIXUP_CLASS_FINAL(PCI_VENDOR_ID_HUAWEI, 0x1610, PCI_CLASS_BRIDGE_PCI, 8, quirk_pcie_mch); +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->eetlp_prefix_path = 1; + + /* Device-tree can set the stall property */ + 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); +DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_HUAWEI, 0xa255, quirk_huawei_pcie_sva); +DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_HUAWEI, 0xa256, quirk_huawei_pcie_sva); +DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_HUAWEI, 0xa258, quirk_huawei_pcie_sva); +DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_HUAWEI, 0xa259, quirk_huawei_pcie_sva); + /* * It's possible for the MSI to get corrupted if SHPC and ACPI are used * together on certain PXH-based systems.