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

[Zhangfei Gao <zhangfei.gao@linaro.org>]

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