Re: [PATCH v7 1/3] iommufd: Add data structure for Intel VT-d stage-1 cache invalidation

[Jason Gunthorpe]

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