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
