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
