On Thu, Jan 04, 2024 at 11:38:40PM -0800, Nicolin Chen wrote:
On Wed, Jan 03, 2024 at 08:02:04PM -0400, Jason Gunthorpe wrote:
On Wed, Jan 03, 2024 at 12:18:35PM -0800, Nicolin Chen wrote:
The driver would have to create it and there would be some driver specific enclosing struct to go with it
Perhaps device_ids goes in the driver specific struct, I don't know.
+struct iommufd_viommu {
- struct iommufd_object obj;
- struct iommufd_ctx *ictx;
- struct iommu_device *iommu_dev;
- struct iommufd_hwpt_paging *hwpt; /* maybe unneeded */
- int vmid;
- union iommu_driver_user_data {
struct iommu_driver_user_vtd;struct iommu_driver_user_arm_smmuv3;struct iommu_driver_user_amd_viommu;- };
Not like that, in the usual container_of way
How about this:
// iommu.h @@ -490,6 +490,16 @@ struct iommu_ops {
/* User space instance allocation and freeing by the iommu driver */struct iommu_device_user *(*iommu_alloc_user)(struct iommu_device *iommu);
struct iommufd_viommu *iommu_alloc_viommu(struct device *dev);
+/**
- struct iommu_device_user - IOMMU core representation of one IOMMU virtual
instance
- @iommu_dev: Underlying IOMMU hardware instance
- @id: Virtual instance ID, e.g. a vmid
- */
+struct iommu_device_user {
struct iommu_device *iommu_dev;struct xarray virtual_ids;u32 id;+};
// iommufd_private.h +struct iommufd_viommu {
struct iommufd_object obj;struct iommufd_ctx *ictx;struct iommu_device *iommu_dev;struct iommu_device_user *iommu_user;struct iommufd_hwpt_paging *hwpt;+};
And you probably don't need two structs, just combine them together
And don't repeat the iommu_domain misdesign, there should be some helper to init (or maybe allocate and init) the core structure along with the driver part.
The set/unset_dev_id ops probably need a type argument if there can be a multi-xarray case, then the virtual_ids xarray should be moved to the driver private structure too?
Yeah, probably. No need to add things that are not used right now, but it does make some sense that the driver could control the datastructure used for mapping. eg AMD has a HW backed datastructure so they may not need an xarray.
Also, a 64-bit virtual id in the uAPI was suggested previously. But xarray is limited to a 32-bit index? Should we compromise the uAPI to 32-bit or is there an alternative for a 64-bit id lookup?
You can use 64 bits in the uapi and forbid values that are too large. xarry uses an unsigned long for the index so it is 64 bit on 64 bit systems.
Jason