Re: [PATCH] PCI: Add a quirk to enable SVA for HiSilicon chip

From: Zhangfei Gao
Date: Wed Jan 13 2021 - 07:06:47 EST

Next message: Aswath Govindraju: "Re: [PATCH 1/2] dt-bindings: mmc: sdhci-am654: Add compatible string for AM64 SoC"
Previous message: Pintu Agarwal: "Re: [BUG]: Kernel 4.9: Kernel crash at fs/sysfs/file.c : sysfs_kf_seq_show"
In reply to: Bjorn Helgaas: "Re: [PATCH] PCI: Add a quirk to enable SVA for HiSilicon chip"
Next in thread: Jean-Philippe Brucker: "Re: [PATCH] PCI: Add a quirk to enable SVA for HiSilicon chip"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

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@xxxxxxxxxx>
Signed-off-by: Jean-Philippe Brucker <jean-philippe@xxxxxxxxxx>
Signed-off-by: Zhou Wang <wangzhou1@xxxxxxxxxxxxx>
---
Property dma-can-stall depends on patchset
https://lore.kernel.org/linux-iommu/20210108145217.2254447-1-jean-philippe@xxxxxxxxxx/

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

Next message: Aswath Govindraju: "Re: [PATCH 1/2] dt-bindings: mmc: sdhci-am654: Add compatible string for AM64 SoC"
Previous message: Pintu Agarwal: "Re: [BUG]: Kernel 4.9: Kernel crash at fs/sysfs/file.c : sysfs_kf_seq_show"
In reply to: Bjorn Helgaas: "Re: [PATCH] PCI: Add a quirk to enable SVA for HiSilicon chip"
Next in thread: Jean-Philippe Brucker: "Re: [PATCH] PCI: Add a quirk to enable SVA for HiSilicon chip"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]