lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
Open Source and information security mailing list archives
| ||
|
Date: Tue, 21 Apr 2020 14:51:14 -0700 From: Jacob Pan <jacob.jun.pan@...ux.intel.com> To: Jean-Philippe Brucker <jean-philippe@...aro.org> Cc: Joerg Roedel <joro@...tes.org>, Alex Williamson <alex.williamson@...hat.com>, Lu Baolu <baolu.lu@...ux.intel.com>, iommu@...ts.linux-foundation.org, LKML <linux-kernel@...r.kernel.org>, David Woodhouse <dwmw2@...radead.org>, Jean-Philippe Brucker <jean-philippe@...aro.com>, Yi Liu <yi.l.liu@...el.com>, "Tian, Kevin" <kevin.tian@...el.com>, Raj Ashok <ashok.raj@...el.com>, Christoph Hellwig <hch@...radead.org>, Jonathan Cameron <jic23@...nel.org>, Eric Auger <eric.auger@...hat.com>, jacob.jun.pan@...ux.intel.com 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@...aro.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]
Powered by blists - more mailing lists