On 2022/5/12 01:25, Jacob Pan wrote:
Hi Jason,
On Wed, 11 May 2022 14:00:25 -0300, Jason Gunthorpe <jgg@xxxxxxxxxx> wrote:
On Wed, May 11, 2022 at 10:02:16AM -0700, Jacob Pan wrote:
If not global, perhaps we could have a list of pasids (e.g. xarray)
attached to the device_domain_info. The TLB flush logic would just
go through the list w/o caring what the PASIDs are for. Does it
make sense to you?
Sort of, but we shouldn't duplicate xarrays - the group already has
this xarray - need to find some way to allow access to it from the
driver.
I am not following, here are the PASIDs for devTLB flush which is per
device. Why group?
Because group is where the core code stores it.
I see, with singleton group. I guess I can let dma-iommu code call
iommu_attach_dma_pasid {
iommu_attach_device_pasid();
Then the PASID will be stored in the group xa.
The flush code can retrieve PASIDs from device_domain_info.device -> group
-> pasid_array.
Thanks for pointing it out, I missed the new pasid_array.
We could retrieve PASIDs from the device PASID table but xa would be
more efficient.
Are you suggesting the dma-iommu API should be called
iommu_set_dma_pasid instead of iommu_attach_dma_pasid?
No that API is Ok - the driver ops API should be 'set' not
attach/detach
Sounds good, this operation has little in common with
domain_ops.dev_attach_pasid() used by SVA domain. So I will add a
new domain_ops.dev_set_pasid()
What? No, their should only be one operation, 'dev_set_pasid' and it
is exactly the same as the SVA operation. It configures things so that
any existing translation on the PASID is removed and the PASID
translates according to the given domain.
SVA given domain or UNMANAGED given domain doesn't matter to the
higher level code. The driver should implement per-domain ops as
required to get the different behaviors.
Perhaps some code to clarify, we have
sva_domain_ops.dev_attach_pasid() = intel_svm_attach_dev_pasid;
default_domain_ops.dev_attach_pasid() = intel_iommu_attach_dev_pasid;
Yes, keep that structure
Consolidate pasid programming into dev_set_pasid() then called by both
intel_svm_attach_dev_pasid() and intel_iommu_attach_dev_pasid(), right?
I was only suggesting that really dev_attach_pasid() op is misnamed,
it should be called set_dev_pasid() and act like a set, not a paired
attach/detach - same as the non-PASID ops.
Got it. Perhaps another patch to rename, Baolu?
Yes. I can rename it in my sva series if others are also happy with this
naming.
Best regards,
baolu
