On Tue, Dec 12, 2023 at 11:13:37AM -0800, Nicolin Chen wrote:
On Tue, Dec 12, 2023 at 10:44:21AM -0400, Jason Gunthorpe wrote:
On Mon, Dec 11, 2023 at 11:30:00PM -0800, Nicolin Chen wrote:
Could the structure just look like this? struct iommu_dev_assign_virtual_id { __u32 size; __u32 dev_id; __u32 id_type; __u32 id; };
It needs to take in the viommu_id also, and I'd make the id 64 bits just for good luck.
What is viommu_id required for in this context? I thought we already know which SMMU instance to issue commands via dev_id?
The viommu_id would be the container that holds the xarray that maps the vRID to pRID
Logically we could have multiple mappings per iommufd as we could have multiple iommu instances working here.
I see. This is the object to hold a shared stage-2 HWPT/domain then.
It could be done like that, yes. I wasn't thinking about linking the stage two so tightly but perhaps? If we can avoid putting the hwpt here that might be more general.
// iommufd_private.h
enum iommufd_object_type { ...
- IOMMUFD_OBJ_VIOMMU, ...
};
+struct iommufd_viommu {
- struct iommufd_object obj;
- struct iommufd_hwpt_paging *hwpt;
- struct xarray devices;
+};
struct iommufd_hwpt_paging hwpt { ...
- struct list_head viommu_list; ...
};
I'd probably first try to go backwards and link the hwpt to the viommu.
struct iommufd_group { ...
- struct iommufd_viommu *viommu; // should we attach to viommu instead of hwpt? ...
};
No. Attach is a statement of translation so you still attach to the HWPT.
Question to finalize how we maps vRID-pRID in the xarray: how should IOMMUFD_DEV_INVALIDATE work? The ioctl structure has a dev_id and a list of commands that belongs to the device. So, it forwards the struct device pointer to the driver along with the commands. Then, doesn't the driver already know the pRID from the dev pointer without looking up a vRID-pRID table?
The first version of DEV_INVALIDATE should have no xarray. The invalidate commands are stripped of the SID and executed on the given dev_id period. VMM splits up the invalidate command list.
The second version maybe we have the xarray, or maybe we just push the xarray to the eventual viommu series.
struct iommu_hwpt_alloc { ...
- __u32 viommu_id;
};
+enum iommu_dev_virtual_id_type {
- IOMMU_DEV_VIRTUAL_ID_TYPE_AMD_VIOMMU_DID, // not sure how this fits the xarray in viommu obj.
- IOMMU_DEV_VIRTUAL_ID_TYPE_AMD_VIOMMU_RID,
It is just DID. In both cases the ID is the index to the "STE" radix tree, whatever the driver happens to call it.
Then, I think that we also need an iommu_viommu_alloc structure and ioctl to allocate an object, and that VMM should know if it needs to allocate multiple viommu objects -- this probably needs the hw_info ioctl to return a piommu_id so VMM gets the list of piommus from the attached devices?
Yes and yes
Another question, introducing the viommu obj complicates things a lot. Do we want it to come with iommu_dev_assign_virtual_id, or maybe put in a later series? We could stage the xarray in the iommu_hwpt_paging struct for now, so a single-IOMMU system could still work with that.
All this would be in its own series to enable HW accelerated viommu support on ARM & AMD as we've been doing so far.
I imagine it after we get the basic invalidation done
And should we rename the "cache_invalidate_user"? Would VT-d still uses it for device cache?
I think vt-d will not implement it
Then should we "s/cache_invalidate_user/iotlb_sync_user"?
I think cache_invalidate is still a fine name.. vt-d will generate ATC invalidations under that function too.
Jason