Messages in this thread | | | Date | Tue, 21 Apr 2020 14:51:14 -0700 | From | Jacob Pan <> | Subject | Re: [PATCH 03/10] iommu/ioasid: Introduce per set allocation APIs |
| |
Hi Jean,
Sorry for the late reply, been trying to redesign the notification part.
On Tue, 7 Apr 2020 13:01:07 +0200 Jean-Philippe Brucker <jean-philippe@linaro.org> wrote:
> 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. > much better.
> > 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. > for vt-d, the per vm token (preferrably mm) will be used by kvm to manage its PASID translation table. when kvm receives a notification about a new guest-host PASID mapping, it needs to know which vm it belongs to. So if mm is used as token, both vfio and kvm can identify PASID ownership.
> > > > /** > > * 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. > Sounds good. still pass a token. Thanks for the idea.
> Thanks, > Jean
[Jacob Pan]
| |