[PATCH] PCI: revert "Enable PASID only when ACS RR & UF enabled on upstream path"

[PATCH] PCI: revert "Enable PASID only when ACS RR & UF enabled on upstream path"

[Christian König]

This reverts commit 201007ef707a8bb5592cd07dd46fc9222c48e0b9.

It's correct that the PCIe fabric routes Memory Requests based on the
TLP address, but enabling the PASID mapping doesn't necessary mean that
Memory Requests will have a PASID associated with them.

The alternative is ATS which lets the device resolve the PASID+addr pair
before a memory request is made into a routeable TLB address through the
TA. Those resolved addresses are then cached on the device instead of
in the IOMMU TLB.

So the assumption that you mandatory need ACS to enabled PASID handling
on a device is simply not correct, we need to take ATS into account as
well.

The patch caused failures with AMDs integrated GPUs because some of them
only enable ATS but not ACS.

For now just revert the patch until this is completely solved.

CC: Jason Gunthorpe <jgg@nvidia.com>
CC: Kevin Tian <kevin.tian@intel.com>
CC: Lu Baolu <baolu.lu@linux.intel.com>
CC: Bjorn Helgaas <bhelgaas@google.com>
CC: Tony Zhu <tony.zhu@intel.com>
CC: Joerg Roedel <jroedel@suse.de>
Signed-off-by: Christian König <christian.koenig@amd.com>
Bug: https://bugzilla.kernel.org/show_bug.cgi?id=216865
---
 drivers/pci/ats.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/drivers/pci/ats.c b/drivers/pci/ats.c
index f9cc2e10b676..c967ad6e2626 100644
--- a/drivers/pci/ats.c
+++ b/drivers/pci/ats.c
@@ -382,9 +382,6 @@ int pci_enable_pasid(struct pci_dev *pdev, int features)
 	if (!pasid)
 		return -EINVAL;
 
-	if (!pci_acs_path_enabled(pdev, NULL, PCI_ACS_RR | PCI_ACS_UF))
-		return -EINVAL;
-
 	pci_read_config_word(pdev, pasid + PCI_PASID_CAP, &supported);
 	supported &= PCI_PASID_CAP_EXEC | PCI_PASID_CAP_PRIV;
 
-- 
2.25.1

Re: [PATCH] PCI: revert "Enable PASID only when ACS RR & UF enabled on upstream path"

[Christian König]

Forgot to add Felix as CC as well.

Am 11.01.23 um 09:57 schrieb Christian König:
> This reverts commit 201007ef707a8bb5592cd07dd46fc9222c48e0b9.
>
> It's correct that the PCIe fabric routes Memory Requests based on the
> TLP address, but enabling the PASID mapping doesn't necessary mean that
> Memory Requests will have a PASID associated with them.
>
> The alternative is ATS which lets the device resolve the PASID+addr pair
> before a memory request is made into a routeable TLB address through the
> TA. Those resolved addresses are then cached on the device instead of
> in the IOMMU TLB.
>
> So the assumption that you mandatory need ACS to enabled PASID handling
> on a device is simply not correct, we need to take ATS into account as
> well.
>
> The patch caused failures with AMDs integrated GPUs because some of them
> only enable ATS but not ACS.
>
> For now just revert the patch until this is completely solved.
>
> CC: Jason Gunthorpe <jgg@nvidia.com>
> CC: Kevin Tian <kevin.tian@intel.com>
> CC: Lu Baolu <baolu.lu@linux.intel.com>
> CC: Bjorn Helgaas <bhelgaas@google.com>
> CC: Tony Zhu <tony.zhu@intel.com>
> CC: Joerg Roedel <jroedel@suse.de>
> Signed-off-by: Christian König <christian.koenig@amd.com>
> Bug: https://bugzilla.kernel.org/show_bug.cgi?id=216865
> ---
>   drivers/pci/ats.c | 3 ---
>   1 file changed, 3 deletions(-)
>
> diff --git a/drivers/pci/ats.c b/drivers/pci/ats.c
> index f9cc2e10b676..c967ad6e2626 100644
> --- a/drivers/pci/ats.c
> +++ b/drivers/pci/ats.c
> @@ -382,9 +382,6 @@ int pci_enable_pasid(struct pci_dev *pdev, int features)
>   	if (!pasid)
>   		return -EINVAL;
>   
> -	if (!pci_acs_path_enabled(pdev, NULL, PCI_ACS_RR | PCI_ACS_UF))
> -		return -EINVAL;
> -
>   	pci_read_config_word(pdev, pasid + PCI_PASID_CAP, &supported);
>   	supported &= PCI_PASID_CAP_EXEC | PCI_PASID_CAP_PRIV;
>   

Re: [PATCH] PCI: revert "Enable PASID only when ACS RR & UF enabled on upstream path"

[Linux kernel regression tracking (Thorsten Leemhuis)]

On 11.01.23 09:57, Christian König wrote:
> This reverts commit 201007ef707a8bb5592cd07dd46fc9222c48e0b9.
> 
> It's correct that the PCIe fabric routes Memory Requests based on the
> TLP address, but enabling the PASID mapping doesn't necessary mean that
> Memory Requests will have a PASID associated with them.
> 
> The alternative is ATS which lets the device resolve the PASID+addr pair
> before a memory request is made into a routeable TLB address through the
> TA. Those resolved addresses are then cached on the device instead of
> in the IOMMU TLB.
> 
> So the assumption that you mandatory need ACS to enabled PASID handling
> on a device is simply not correct, we need to take ATS into account as
> well.
> 
> The patch caused failures with AMDs integrated GPUs because some of them
> only enable ATS but not ACS.
> 
> For now just revert the patch until this is completely solved.
> 
> CC: Jason Gunthorpe <jgg@nvidia.com>
> CC: Kevin Tian <kevin.tian@intel.com>
> CC: Lu Baolu <baolu.lu@linux.intel.com>
> CC: Bjorn Helgaas <bhelgaas@google.com>
> CC: Tony Zhu <tony.zhu@intel.com>
> CC: Joerg Roedel <jroedel@suse.de>
> Signed-off-by: Christian König <christian.koenig@amd.com>

One small thing to improve:

> Bug: https://bugzilla.kernel.org/show_bug.cgi?id=216865

s/Bug:/Link:/ here, otherwise you might get mails from Linus like these:
https://lore.kernel.org/all/CAHk-=wjMmSZzMJ3Xnskdg4+GGz=5p5p+GSYyFBTh0f-DgvdBWg@mail.gmail.com/
https://lore.kernel.org/all/CAHk-=wgs38ZrfPvy=nOwVkVzjpM3VFU1zobP37Fwd_h9iAD5JQ@mail.gmail.com/
https://lore.kernel.org/all/CAHk-=wjxzafG-=J8oT30s7upn4RhBs6TX-uVFZ5rME+L5_DoJA@mail.gmail.com/

This usage is also explained in
Documentation/process/submitting-patches.rst
(http://docs.kernel.org/process/submitting-patches.html) and
Documentation/process/5.Posting.rst
(https://docs.kernel.org/process/5.Posting.html)

Ciao, Thorsten (wearing his 'the Linux kernel's regression tracker' hat)
--
Everything you wanna know about Linux kernel regression tracking:
https://linux-regtracking.leemhuis.info/about/#tldr
If I did something stupid, please tell me, as explained on that page.

P.S.: let me tell regzbot to monitor this thread:

#regzbot ^backmonitor: https://bugzilla.kernel.org/show_bug.cgi?id=216865

Re: [PATCH] PCI: revert "Enable PASID only when ACS RR & UF enabled on upstream path"

[Jason Gunthorpe]

On Wed, Jan 11, 2023 at 09:57:45AM +0100, Christian König wrote:
> This reverts commit 201007ef707a8bb5592cd07dd46fc9222c48e0b9.
> 
> It's correct that the PCIe fabric routes Memory Requests based on the
> TLP address, but enabling the PASID mapping doesn't necessary mean that
> Memory Requests will have a PASID associated with them.

It is true, the routine assumes the device will use untranslated
requests with the PASID.

> The alternative is ATS which lets the device resolve the PASID+addr pair
> before a memory request is made into a routeable TLB address through the
> TA. Those resolved addresses are then cached on the device instead of
> in the IOMMU TLB.

We should pass in a flag "device always sets the translated bit for
PASID" and skip the ACS check in that case.

The ACS check is not wrong, and it is definately necessary for devices
that do not guarentee ATS and PASID are used together, we should not
be removing it.

Given adding the flag is trivial we should just fix it, not revert this.

Jason

Re: [PATCH] PCI: revert "Enable PASID only when ACS RR & UF enabled on upstream path"

[Christian König]

Am 11.01.23 um 14:07 schrieb Jason Gunthorpe:
> On Wed, Jan 11, 2023 at 09:57:45AM +0100, Christian König wrote:
>> This reverts commit 201007ef707a8bb5592cd07dd46fc9222c48e0b9.
>>
>> It's correct that the PCIe fabric routes Memory Requests based on the
>> TLP address, but enabling the PASID mapping doesn't necessary mean that
>> Memory Requests will have a PASID associated with them.
> It is true, the routine assumes the device will use untranslated
> requests with the PASID.
>
>> The alternative is ATS which lets the device resolve the PASID+addr pair
>> before a memory request is made into a routeable TLB address through the
>> TA. Those resolved addresses are then cached on the device instead of
>> in the IOMMU TLB.
> We should pass in a flag "device always sets the translated bit for
> PASID" and skip the ACS check in that case.
>
> The ACS check is not wrong, and it is definately necessary for devices
> that do not guarentee ATS and PASID are used together, we should not
> be removing it.
>
> Given adding the flag is trivial we should just fix it, not revert this.

Well exactly that's the point, adding this flag is absolutely not 
trivial as far as I can see. We need to go through multiple layers of 
abstraction since this is the low level function and nothing high level.

Additional to that the check doesn't seem to make much sense to me. 
pci_enable_pasid() is called by three functions:

pdev_pri_ats_enable() in the AMD IOMMU driver while enabling ATS. As far 
as I can see we absolutely don't need the ACS check here because ATS is 
a must have.

iommu_enable_pci_caps() in the Intel IOMMU driver. This also does some 
handling for ATS, so here we could check the info->ats_supported flag if 
ACS needs to be checked or not.

arm_smmu_enable_pasid() in the ARM IOMMU driver code. No idea what this 
one does with ATS. Here is the only place where the ACS check might make 
sense.

So even if we have some need for this check this seems to be the wrong 
place for the check since not all necessary information from the higher 
level is available.

Regards,
Christian.

>
> Jason

Re: [PATCH] PCI: revert "Enable PASID only when ACS RR & UF enabled on upstream path"

[Jason Gunthorpe]

On Wed, Jan 11, 2023 at 02:38:34PM +0100, Christian König wrote:
> Am 11.01.23 um 14:07 schrieb Jason Gunthorpe:
> > On Wed, Jan 11, 2023 at 09:57:45AM +0100, Christian König wrote:
> > > This reverts commit 201007ef707a8bb5592cd07dd46fc9222c48e0b9.
> > > 
> > > It's correct that the PCIe fabric routes Memory Requests based on the
> > > TLP address, but enabling the PASID mapping doesn't necessary mean that
> > > Memory Requests will have a PASID associated with them.
> > It is true, the routine assumes the device will use untranslated
> > requests with the PASID.
> > 
> > > The alternative is ATS which lets the device resolve the PASID+addr pair
> > > before a memory request is made into a routeable TLB address through the
> > > TA. Those resolved addresses are then cached on the device instead of
> > > in the IOMMU TLB.
> > We should pass in a flag "device always sets the translated bit for
> > PASID" and skip the ACS check in that case.
> > 
> > The ACS check is not wrong, and it is definately necessary for devices
> > that do not guarentee ATS and PASID are used together, we should not
> > be removing it.
> > 
> > Given adding the flag is trivial we should just fix it, not revert this.
> 
> Well exactly that's the point, adding this flag is absolutely not trivial as
> far as I can see. We need to go through multiple layers of abstraction since
> this is the low level function and nothing high level.
> 
> Additional to that the check doesn't seem to make much sense to me.

AFAICT it is the only solution that makes any sense..

> pci_enable_pasid() is called by three functions:
> 
> pdev_pri_ats_enable() in the AMD IOMMU driver while enabling ATS. As far as
> I can see we absolutely don't need the ACS check here because ATS is a must
> have.

Enabling ATS does not mean every PASID TLP will use ATS. It just means
that some transactions might.

We cannot do this properly without driver sourced device-specific
knowledge.

> iommu_enable_pci_caps() in the Intel IOMMU driver. This also does some
> handling for ATS, so here we could check the info->ats_supported flag if ACS
> needs to be checked or not.

*groan* this is seems wrong :( Lu why are we doing this inside iommu
drivers instead of in the device drivers to declare they want to use
PASID?

> arm_smmu_enable_pasid() in the ARM IOMMU driver code. No idea what this one
> does with ATS. Here is the only place where the ACS check might make sense.
> 
> So even if we have some need for this check this seems to be the wrong place
> for the check since not all necessary information from the higher level is
> available.

IIRC we only have 1 driver using the standard pasid support, so we
could just move these things to IDXD.

The AMD sidechannel thing is only use for AMDGPU so it can just assume
things until it gets fixed to use the standard API.

Jason

Re: [PATCH] PCI: revert "Enable PASID only when ACS RR & UF enabled on upstream path"

[Baolu Lu]

On 2023/1/11 21:44, Jason Gunthorpe wrote:
>> iommu_enable_pci_caps() in the Intel IOMMU driver. This also does some
>> handling for ATS, so here we could check the info->ats_supported flag if ACS
>> needs to be checked or not.
> *groan*  this is seems wrong 🙁 Lu why are we doing this inside iommu
> drivers instead of in the device drivers to declare they want to use
> PASID?

Currently it's common to enable pasid in the IOMMU drivers, but device
driver has more knowledge of the device, hence it makes more sense to
move pci_enable_pasid() to the device driver.

--
Best regards,
baolu

Re: [PATCH] PCI: revert "Enable PASID only when ACS RR & UF enabled on upstream path"

[Jason Gunthorpe]

On Wed, Jan 11, 2023 at 09:54:03PM +0800, Baolu Lu wrote:
> On 2023/1/11 21:44, Jason Gunthorpe wrote:
> > > iommu_enable_pci_caps() in the Intel IOMMU driver. This also does some
> > > handling for ATS, so here we could check the info->ats_supported flag if ACS
> > > needs to be checked or not.
> > *groan*  this is seems wrong 🙁 Lu why are we doing this inside iommu
> > drivers instead of in the device drivers to declare they want to use
> > PASID?
> 
> Currently it's common to enable pasid in the IOMMU drivers, but device
> driver has more knowledge of the device, hence it makes more sense to
> move pci_enable_pasid() to the device driver.

So, lets fix it that way.

Add the flag to the pci_enable_pasid(), set the flag in the AMD
IOMMU's special AMD GPU only path assuming the device will always use
ATS

Do not set the flag in the other iommu drivers

Baolu will send a series to move the pasid enabling from the common
path to the drivers

Jason

Re: [PATCH] PCI: revert "Enable PASID only when ACS RR & UF enabled on upstream path"

[Christian König]

Am 11.01.23 um 15:14 schrieb Jason Gunthorpe:
> On Wed, Jan 11, 2023 at 09:54:03PM +0800, Baolu Lu wrote:
>> On 2023/1/11 21:44, Jason Gunthorpe wrote:
>>>> iommu_enable_pci_caps() in the Intel IOMMU driver. This also does some
>>>> handling for ATS, so here we could check the info->ats_supported flag if ACS
>>>> needs to be checked or not.
>>> *groan*  this is seems wrong 🙁 Lu why are we doing this inside iommu
>>> drivers instead of in the device drivers to declare they want to use
>>> PASID?
>> Currently it's common to enable pasid in the IOMMU drivers, but device
>> driver has more knowledge of the device, hence it makes more sense to
>> move pci_enable_pasid() to the device driver.
> So, lets fix it that way.
>
> Add the flag to the pci_enable_pasid(), set the flag in the AMD
> IOMMU's special AMD GPU only path assuming the device will always use
> ATS

That will fix at least this the AMD use case.

> Do not set the flag in the other iommu drivers

Don't we have other hardware which supports ATS as well and might run 
into the same problem?

Regards,
Christian.

>
> Baolu will send a series to move the pasid enabling from the common
> path to the drivers
>
> Jason

Re: [PATCH] PCI: revert "Enable PASID only when ACS RR & UF enabled on upstream path"

[Jason Gunthorpe]

On Wed, Jan 11, 2023 at 03:17:03PM +0100, Christian König wrote:
> Am 11.01.23 um 15:14 schrieb Jason Gunthorpe:
> > On Wed, Jan 11, 2023 at 09:54:03PM +0800, Baolu Lu wrote:
> > > On 2023/1/11 21:44, Jason Gunthorpe wrote:
> > > > > iommu_enable_pci_caps() in the Intel IOMMU driver. This also does some
> > > > > handling for ATS, so here we could check the info->ats_supported flag if ACS
> > > > > needs to be checked or not.
> > > > *groan*  this is seems wrong 🙁 Lu why are we doing this inside iommu
> > > > drivers instead of in the device drivers to declare they want to use
> > > > PASID?
> > > Currently it's common to enable pasid in the IOMMU drivers, but device
> > > driver has more knowledge of the device, hence it makes more sense to
> > > move pci_enable_pasid() to the device driver.
> > So, lets fix it that way.
> > 
> > Add the flag to the pci_enable_pasid(), set the flag in the AMD
> > IOMMU's special AMD GPU only path assuming the device will always use
> > ATS
> 
> That will fix at least this the AMD use case.
> 
> > Do not set the flag in the other iommu drivers
> 
> Don't we have other hardware which supports ATS as well and might run into
> the same problem?

As I said, I think we have only 1 user of the common PASID API and it
was happy with things as-is, so I think for v6.2 we are fine.

Honestly, not declaring ACS in a 'enterprise' multi-function device is
already kind of sketchy/rare - even if ATS saves things for the PASID
case.

Jason

Re: [PATCH] PCI: revert "Enable PASID only when ACS RR & UF enabled on upstream path"

[Christian König]

Am 11.01.23 um 15:20 schrieb Jason Gunthorpe:
> On Wed, Jan 11, 2023 at 03:17:03PM +0100, Christian König wrote:
>> Am 11.01.23 um 15:14 schrieb Jason Gunthorpe:
>>> On Wed, Jan 11, 2023 at 09:54:03PM +0800, Baolu Lu wrote:
>>>> On 2023/1/11 21:44, Jason Gunthorpe wrote:
>>>>>> iommu_enable_pci_caps() in the Intel IOMMU driver. This also does some
>>>>>> handling for ATS, so here we could check the info->ats_supported flag if ACS
>>>>>> needs to be checked or not.
>>>>> *groan*  this is seems wrong 🙁 Lu why are we doing this inside iommu
>>>>> drivers instead of in the device drivers to declare they want to use
>>>>> PASID?
>>>> Currently it's common to enable pasid in the IOMMU drivers, but device
>>>> driver has more knowledge of the device, hence it makes more sense to
>>>> move pci_enable_pasid() to the device driver.
>>> So, lets fix it that way.
>>>
>>> Add the flag to the pci_enable_pasid(), set the flag in the AMD
>>> IOMMU's special AMD GPU only path assuming the device will always use
>>> ATS
>> That will fix at least this the AMD use case.
>>
>>> Do not set the flag in the other iommu drivers
>> Don't we have other hardware which supports ATS as well and might run into
>> the same problem?
> As I said, I think we have only 1 user of the common PASID API and it
> was happy with things as-is, so I think for v6.2 we are fine.
>
> Honestly, not declaring ACS in a 'enterprise' multi-function device is
> already kind of sketchy/rare - even if ATS saves things for the PASID
> case.

Yeah, as I said I've checked a couple of different AMD hardware. And 
what I have available always has ACS, ATS, PRI and PASID enabled together.

That there is a Carizzo variant which enables ATS/PASID but not ACS is 
indeed a bit strange.

Christian.

>
> Jason

Re: [PATCH] PCI: revert "Enable PASID only when ACS RR & UF enabled on upstream path"

[Jason Gunthorpe]

On Wed, Jan 11, 2023 at 03:07:36PM +0100, Christian König wrote:

>    Well no, we can perfectly fine enable this as we have done in the past.
>    What we can't do is rejecting it without driver specific knowledge,
>    because the hardware might still work correctly.

It is an interesting point that any device using PASID for SVA must
necessary also be doing ATS/PRI and thus must always be setting the
translated bit in their Mem TLPs

So, at least at this instant in the kernel we have no need for the ACS
check as everything is SVA.

This is forward looking where we are going to have non SVA uses of
PASIDs and we cannot guarantee translated TLPs.

Keep in mind this is pretty much an integrity problem, eg if I allow
iommufd to assign page tables to a PASID without PRI we can get bad
behaviors if the HW does not route properly.

So we are justified to be conservative here to prevent data corruption
in bad cases.

Jason

Re: [PATCH] PCI: revert "Enable PASID only when ACS RR & UF enabled on upstream path"

[Baolu Lu]

On 2023/1/11 22:17, Christian König wrote:
> Am 11.01.23 um 15:14 schrieb Jason Gunthorpe:
>> On Wed, Jan 11, 2023 at 09:54:03PM +0800, Baolu Lu wrote:
>>> On 2023/1/11 21:44, Jason Gunthorpe wrote:
>>>>> iommu_enable_pci_caps() in the Intel IOMMU driver. This also does some
>>>>> handling for ATS, so here we could check the info->ats_supported 
>>>>> flag if ACS
>>>>> needs to be checked or not.
>>>> *groan*  this is seems wrong 🙁 Lu why are we doing this inside iommu
>>>> drivers instead of in the device drivers to declare they want to use
>>>> PASID?
>>> Currently it's common to enable pasid in the IOMMU drivers, but device
>>> driver has more knowledge of the device, hence it makes more sense to
>>> move pci_enable_pasid() to the device driver.
>> So, lets fix it that way.
>>
>> Add the flag to the pci_enable_pasid(), set the flag in the AMD
>> IOMMU's special AMD GPU only path assuming the device will always use
>> ATS
> 
> That will fix at least this the AMD use case.

I've post a patch for discussion.

https://lore.kernel.org/linux-iommu/20230112084629.737653-1-baolu.lu@linux.intel.com/

--
Best regards,
baolu