Messages in this thread | | | Date | Tue, 7 Apr 2020 13:01:07 +0200 | From | Jean-Philippe Brucker <> | Subject | Re: [PATCH 03/10] iommu/ioasid: Introduce per set allocation APIs |
| |
On Mon, Apr 06, 2020 at 01:02:45PM -0700, Jacob Pan wrote: > > > + sdata = kzalloc(sizeof(*sdata), GFP_KERNEL); > > > + if (!sdata) > > > + return -ENOMEM; > > > > I don't understand why we need this structure at all, nor why we need > > the SID. Users have already allocated an ioasid_set, so why not just > > stick the content of ioasid_set_data in there, and pass the > > ioasid_set pointer to ioasid_alloc()? > > > > My thinking was that ioasid_set is an opaque user token, e.g. we use mm > to identify a common set belong to a VM. > > This sdata is an IOASID internal structure for managing & servicing per > set data. If we let user fill in the content, some of the entries need > to be managed by the IOASID code under a lock.
We don't have to let users fill the content. A bit like iommu_domain: device drivers don't modify it, they pass it to iommu_map() rather than passing a domain ID.
> IMO, not suitable to let user allocate and manage. > > Perhaps we should rename struct ioasid_set to ioasid_set_token?
Is the token actually used anywhere? As far as I can tell VFIO does its own uniqueness check before calling ioasid_alloc_set(), and consumers of notifications don't read the token.
> > /** > * struct ioasid_set_data - Meta data about ioasid_set > * > * @token: Unique to identify an IOASID set > * @xa: XArray to store ioasid_set private ID to > system-wide IOASID > * mapping > * @max_id: Max number of IOASIDs can be allocated within the set > * @nr_id Number of IOASIDs allocated in the set > * @sid ID of the set > */ > struct ioasid_set_data { > struct ioasid_set *token; > struct xarray xa; > int size; > int nr_ioasids; > int sid; > struct rcu_head rcu; > };
How about we remove the current ioasid_set, call this structure ioasid_set instead of ioasid_set_data, and have ioasid_alloc_set() return it, rather than requiring users to allocate the ioasid_set themselves?
struct ioasid_set *ioasid_alloc_set(ioasid_t quota):
This way ioasid_set is opaque to users (we could have the definition in ioasid.c), but it can be passed to ioasid_alloc() and avoids the lookup by SID. Could also add the unique token as a void * argument to ioasid_alloc_set(), if needed.
Thanks, Jean
| |