[RFC PATCH] PCI: Remove End-End TLP as PASID dependency

[RFC PATCH] PCI: Remove End-End TLP as PASID dependency

[Zhangfei Gao]

Some platform devices appear as PCI and have PCI cfg space,
but are actually on the AMBA bus.
They can support PASID via smmu stall feature, but does not
support tlp since they are not real pci devices.
So remove tlp as a PASID dependency.

Signed-off-by: Zhangfei Gao <zhangfei.gao@linaro.org>
---
 drivers/pci/ats.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/drivers/pci/ats.c b/drivers/pci/ats.c
index 390e92f..8e31278 100644
--- a/drivers/pci/ats.c
+++ b/drivers/pci/ats.c
@@ -344,9 +344,6 @@ int pci_enable_pasid(struct pci_dev *pdev, int features)
 	if (WARN_ON(pdev->pasid_enabled))
 		return -EBUSY;
 
-	if (!pdev->eetlp_prefix_path)
-		return -EINVAL;
-
 	if (!pasid)
 		return -EINVAL;
 
-- 
2.7.4

Re: [RFC PATCH] PCI: Remove End-End TLP as PASID dependency

[Jean-Philippe Brucker]

On Wed, Jun 10, 2020 at 12:18:14PM +0800, Zhangfei Gao wrote:
> Some platform devices appear as PCI and have PCI cfg space,
> but are actually on the AMBA bus.
> They can support PASID via smmu stall feature, but does not
> support tlp since they are not real pci devices.
> So remove tlp as a PASID dependency.
> 
> Signed-off-by: Zhangfei Gao <zhangfei.gao@linaro.org>
> ---
>  drivers/pci/ats.c | 3 ---
>  1 file changed, 3 deletions(-)
> 
> diff --git a/drivers/pci/ats.c b/drivers/pci/ats.c
> index 390e92f..8e31278 100644
> --- a/drivers/pci/ats.c
> +++ b/drivers/pci/ats.c
> @@ -344,9 +344,6 @@ int pci_enable_pasid(struct pci_dev *pdev, int features)
>  	if (WARN_ON(pdev->pasid_enabled))
>  		return -EBUSY;
>  
> -	if (!pdev->eetlp_prefix_path)
> -		return -EINVAL;
> -

This check is useful, and follows the PCI specification (4.0r1.0
2.2.10.2 End-End TLP Prefix Processing: "Software should ensure that TLPs
containing End-End TLP Prefixes are not sent to components that do not
support them.")

Why not set the eetlp_prefix_path bit from a PCI quirk?  Unlike the stall
problem from the other thread, this one looks like a simple design mistake
that can be fixed easily in future iterations of the platform: just set
the "End-End TLP Prefix Supported" bit in the Device Capability 2 Register
of all bridges.

Thanks,
Jean

Re: [RFC PATCH] PCI: Remove End-End TLP as PASID dependency

[Zhangfei Gao]

On 2020/6/10 下午3:46, Jean-Philippe Brucker wrote:
> On Wed, Jun 10, 2020 at 12:18:14PM +0800, Zhangfei Gao wrote:
>> Some platform devices appear as PCI and have PCI cfg space,
>> but are actually on the AMBA bus.
>> They can support PASID via smmu stall feature, but does not
>> support tlp since they are not real pci devices.
>> So remove tlp as a PASID dependency.
>>
>> Signed-off-by: Zhangfei Gao <zhangfei.gao@linaro.org>
>> ---
>>   drivers/pci/ats.c | 3 ---
>>   1 file changed, 3 deletions(-)
>>
>> diff --git a/drivers/pci/ats.c b/drivers/pci/ats.c
>> index 390e92f..8e31278 100644
>> --- a/drivers/pci/ats.c
>> +++ b/drivers/pci/ats.c
>> @@ -344,9 +344,6 @@ int pci_enable_pasid(struct pci_dev *pdev, int features)
>>   	if (WARN_ON(pdev->pasid_enabled))
>>   		return -EBUSY;
>>   
>> -	if (!pdev->eetlp_prefix_path)
>> -		return -EINVAL;
>> -
> This check is useful, and follows the PCI specification (4.0r1.0
> 2.2.10.2 End-End TLP Prefix Processing: "Software should ensure that TLPs
> containing End-End TLP Prefixes are not sent to components that do not
> support them.")
Thanks Jean,
>
> Why not set the eetlp_prefix_path bit from a PCI quirk?  Unlike the stall
> problem from the other thread, this one looks like a simple design mistake
> that can be fixed easily in future iterations of the platform: just set
> the "End-End TLP Prefix Supported" bit in the Device Capability 2 Register
> of all bridges.
Yes, we can still set eetlp_prefix_path bit from a PCI quirk.

And we also have considered adding this bit in Device Capability 2 
Register in future silicon.
But we hesitated that it does reflect the real function: from register, 
it can support tlp, but in fact, it does not.

Thanks

Re: [RFC PATCH] PCI: Remove End-End TLP as PASID dependency

[Sinan Kaya]

On 6/10/2020 4:00 AM, Zhangfei Gao wrote:
>> Why not set the eetlp_prefix_path bit from a PCI quirk?  Unlike the stall
>> problem from the other thread, this one looks like a simple design
>> mistake
>> that can be fixed easily in future iterations of the platform: just set
>> the "End-End TLP Prefix Supported" bit in the Device Capability 2
>> Register
>> of all bridges.
> Yes, we can still set eetlp_prefix_path bit from a PCI quirk.

I agree. Vendor specific quirk should be the way to go here if it is not
compliant with the spec.

Re: [RFC PATCH] PCI: Remove End-End TLP as PASID dependency

[Bjorn Helgaas]

[+cc Sinan]

On Wed, Jun 10, 2020 at 12:18:14PM +0800, Zhangfei Gao wrote:
> Some platform devices appear as PCI and have PCI cfg space,
> but are actually on the AMBA bus.
> They can support PASID via smmu stall feature, but does not
> support tlp since they are not real pci devices.
> So remove tlp as a PASID dependency.

When you iterate on this, pay attention to things like:

  - Wrap paragraphs to 75 columns or so, so they fill the whole line
    but don't overflow when "git show" adds 4 spaces.

  - Leave a blank line between paragraphs.

  - Capitalize consistently: "SMMU", "PCI", "TLP".

  - Provide references to relevant spec sections, e.g., for the SMMU
    stall feature.

> Signed-off-by: Zhangfei Gao <zhangfei.gao@linaro.org>
> ---
>  drivers/pci/ats.c | 3 ---
>  1 file changed, 3 deletions(-)
> 
> diff --git a/drivers/pci/ats.c b/drivers/pci/ats.c
> index 390e92f..8e31278 100644
> --- a/drivers/pci/ats.c
> +++ b/drivers/pci/ats.c
> @@ -344,9 +344,6 @@ int pci_enable_pasid(struct pci_dev *pdev, int features)
>  	if (WARN_ON(pdev->pasid_enabled))
>  		return -EBUSY;
>  
> -	if (!pdev->eetlp_prefix_path)
> -		return -EINVAL;

No.  This would mean we might enable PASID on actual PCIe devices when
it is not safe to do so, as Jean-Philippe pointed out.

You cannot break actual PCIe devices just to make your non-PCIe device
work.

These devices do not support PASID as defined in the PCIe spec.  They
might support something *like* PASID, and you might be able to make
parts of the PCI core work with them, but you're going to have to deal
with the parts that don't follow the PCIe spec on your own.  That
might be quirks, it might be some sort of AMBA adaptation shim, I
don't know.  But it's not the responsibility of the PCI core to adapt
to them.

>  	if (!pasid)
>  		return -EINVAL;
>  
> -- 
> 2.7.4
> 

Re: [RFC PATCH] PCI: Remove End-End TLP as PASID dependency

[Zhangfei Gao]

On 2020/6/12 上午1:41, Bjorn Helgaas wrote:
> [+cc Sinan]
>
> On Wed, Jun 10, 2020 at 12:18:14PM +0800, Zhangfei Gao wrote:
>> Some platform devices appear as PCI and have PCI cfg space,
>> but are actually on the AMBA bus.
>> They can support PASID via smmu stall feature, but does not
>> support tlp since they are not real pci devices.
>> So remove tlp as a PASID dependency.
> When you iterate on this, pay attention to things like:
>
>    - Wrap paragraphs to 75 columns or so, so they fill the whole line
>      but don't overflow when "git show" adds 4 spaces.
>
>    - Leave a blank line between paragraphs.
>
>    - Capitalize consistently: "SMMU", "PCI", "TLP".
>
>    - Provide references to relevant spec sections, e.g., for the SMMU
>      stall feature.
OK, Thanks Bjorn
>
>> Signed-off-by: Zhangfei Gao <zhangfei.gao@linaro.org>
>> ---
>>   drivers/pci/ats.c | 3 ---
>>   1 file changed, 3 deletions(-)
>>
>> diff --git a/drivers/pci/ats.c b/drivers/pci/ats.c
>> index 390e92f..8e31278 100644
>> --- a/drivers/pci/ats.c
>> +++ b/drivers/pci/ats.c
>> @@ -344,9 +344,6 @@ int pci_enable_pasid(struct pci_dev *pdev, int features)
>>   	if (WARN_ON(pdev->pasid_enabled))
>>   		return -EBUSY;
>>   
>> -	if (!pdev->eetlp_prefix_path)
>> -		return -EINVAL;
> No.  This would mean we might enable PASID on actual PCIe devices when
> it is not safe to do so, as Jean-Philippe pointed out.
>
> You cannot break actual PCIe devices just to make your non-PCIe device
> work.
>
> These devices do not support PASID as defined in the PCIe spec.  They
> might support something *like* PASID, and you might be able to make
> parts of the PCI core work with them, but you're going to have to deal
> with the parts that don't follow the PCIe spec on your own.  That
> might be quirks, it might be some sort of AMBA adaptation shim, I
> don't know.  But it's not the responsibility of the PCI core to adapt
> to them.
Understand now.
Will continue use quirk for this.

Thanks