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 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]
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
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
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH 03/10] iommu/ioasid: Introduce per set allocation APIs
On Wed, 1 Apr 2020 15:47:45 +0200
Jean-Philippe Brucker wrote:
> On Wed, Mar 25, 2020 at 10:55:24AM -0700, Jacob Pan wrote:
> > IOASID set defines a group of IDs that share the same token. The
> > ioasid_set concept helps to do permission checking among users as
> > in the current code.
> >
> > With guest SVA usage, each VM has its own IOASID set. More
> > functionalities are needed:
> > 1. Enforce quota, each guest may be assigned limited quota such
> > that one guest cannot abuse all the system resource.
> > 2. Stores IOASID mapping between guest and host IOASIDs
> > 3. Per set operations, e.g. free the entire set
> >
> > For each ioasid_set token, a unique set ID is assigned. This makes
> > reference of the set and data lookup much easier to implement.
> >
> > Signed-off-by: Liu Yi L
> > Signed-off-by: Jacob Pan
> > ---
> > drivers/iommu/ioasid.c | 147
> > +
> > include/linux/ioasid.h | 13 + 2 files changed, 160
> > insertions(+)
> >
> > diff --git a/drivers/iommu/ioasid.c b/drivers/iommu/ioasid.c
> > index 4026e52855b9..27ee57f7079b 100644
> > --- a/drivers/iommu/ioasid.c
> > +++ b/drivers/iommu/ioasid.c
> > @@ -10,6 +10,25 @@
> > #include
> > #include
> >
> > +static DEFINE_XARRAY_ALLOC(ioasid_sets);
> > +/**
> > + * struct ioasid_set_data - Meta data about ioasid_set
> > + *
> > + * @token: Unique to identify an IOASID set
> > + * @xa:XArray to store subset ID and IOASID mapping
> > + * @size: Max number of IOASIDs can be allocated within the
> > set
> > + * @nr_ioasids Number of IOASIDs allocated in the set
> > + * @sidID 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;
> > +};
> > +
> > struct ioasid_data {
> > ioasid_t id;
> > struct ioasid_set *set;
> > @@ -388,6 +407,111 @@ void ioasid_free(ioasid_t ioasid)
> > EXPORT_SYMBOL_GPL(ioasid_free);
> >
> > /**
> > + * ioasid_alloc_set - Allocate a set of IOASIDs
> > + * @token: Unique token of the IOASID set
> > + * @quota: Quota allowed in this set
> > + * @sid: IOASID set ID to be assigned
> > + *
> > + * Return 0 upon success. Token will be stored internally for
> > lookup,
> > + * IOASID allocation within the set and other per set operations
> > will use
> > + * the @sid assigned.
> > + *
> > + */
> > +int ioasid_alloc_set(struct ioasid_set *token, ioasid_t quota, int
> > *sid) +{
> > + struct ioasid_set_data *sdata;
> > + ioasid_t id;
> > + int ret = 0;
> > +
> > + if (quota > ioasid_capacity_avail) {
> > + pr_warn("Out of IOASID capacity! ask %d, avail
> > %d\n",
> > + quota, ioasid_capacity_avail);
> > + return -ENOSPC;
> > + }
>
> This check should be in the same critical section as the quota
> substraction
>
yes, right.
> > +
> > + 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. IMO, not suitable to let
user allocate and manage.
Perhaps we should rename struct ioasid_set to ioasid_set_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;
};
I agree SID is optional. User could just use the token to reference
the set. I use the SID for performance reason, i.e. quick lookup from
SID to its data. Otherwise, we may have to search through a list of
sets to find a match?
> > +
> > + spin_lock(_allocator_lock);
> > +
> > + ret = xa_alloc(_sets, , sdata,
> > + XA_LIMIT(0, ioasid_capacity_avail - quota),
> > + GFP_KERNEL);
>
> Same as Kevin, I think the limit should be the static
> ioasid_capacity. And perhaps a comment explaining the worst case of
> one PASID per set. I found a little confusing using the same type
> ioasid_t for IOASIDs and IOASID sets, it may be clearer to use an int
> for IOASID set IDs.
>
Re: [PATCH 03/10] iommu/ioasid: Introduce per set allocation APIs
On Wed, Mar 25, 2020 at 10:55:24AM -0700, Jacob Pan wrote:
> IOASID set defines a group of IDs that share the same token. The
> ioasid_set concept helps to do permission checking among users as in the
> current code.
>
> With guest SVA usage, each VM has its own IOASID set. More
> functionalities are needed:
> 1. Enforce quota, each guest may be assigned limited quota such that one
> guest cannot abuse all the system resource.
> 2. Stores IOASID mapping between guest and host IOASIDs
> 3. Per set operations, e.g. free the entire set
>
> For each ioasid_set token, a unique set ID is assigned. This makes
> reference of the set and data lookup much easier to implement.
>
> Signed-off-by: Liu Yi L
> Signed-off-by: Jacob Pan
> ---
> drivers/iommu/ioasid.c | 147
> +
> include/linux/ioasid.h | 13 +
> 2 files changed, 160 insertions(+)
>
> diff --git a/drivers/iommu/ioasid.c b/drivers/iommu/ioasid.c
> index 4026e52855b9..27ee57f7079b 100644
> --- a/drivers/iommu/ioasid.c
> +++ b/drivers/iommu/ioasid.c
> @@ -10,6 +10,25 @@
> #include
> #include
>
> +static DEFINE_XARRAY_ALLOC(ioasid_sets);
> +/**
> + * struct ioasid_set_data - Meta data about ioasid_set
> + *
> + * @token: Unique to identify an IOASID set
> + * @xa: XArray to store subset ID and IOASID mapping
> + * @size:Max number of IOASIDs can be allocated within the set
> + * @nr_ioasids 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;
> +};
> +
> struct ioasid_data {
> ioasid_t id;
> struct ioasid_set *set;
> @@ -388,6 +407,111 @@ void ioasid_free(ioasid_t ioasid)
> EXPORT_SYMBOL_GPL(ioasid_free);
>
> /**
> + * ioasid_alloc_set - Allocate a set of IOASIDs
> + * @token: Unique token of the IOASID set
> + * @quota: Quota allowed in this set
> + * @sid: IOASID set ID to be assigned
> + *
> + * Return 0 upon success. Token will be stored internally for lookup,
> + * IOASID allocation within the set and other per set operations will use
> + * the @sid assigned.
> + *
> + */
> +int ioasid_alloc_set(struct ioasid_set *token, ioasid_t quota, int *sid)
> +{
> + struct ioasid_set_data *sdata;
> + ioasid_t id;
> + int ret = 0;
> +
> + if (quota > ioasid_capacity_avail) {
> + pr_warn("Out of IOASID capacity! ask %d, avail %d\n",
> + quota, ioasid_capacity_avail);
> + return -ENOSPC;
> + }
This check should be in the same critical section as the quota
substraction
> +
> + 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()?
> +
> + spin_lock(_allocator_lock);
> +
> + ret = xa_alloc(_sets, , sdata,
> +XA_LIMIT(0, ioasid_capacity_avail - quota),
> +GFP_KERNEL);
Same as Kevin, I think the limit should be the static ioasid_capacity. And
perhaps a comment explaining the worst case of one PASID per set. I found
a little confusing using the same type ioasid_t for IOASIDs and IOASID
sets, it may be clearer to use an int for IOASID set IDs.
Thanks,
Jean
> + if (ret) {
> + kfree(sdata);
> + goto error;
> + }
> +
> + sdata->token = token;
> + sdata->size = quota;
> + sdata->sid = id;
> +
> + /*
> + * Set Xarray is used to store IDs within the set, get ready for
> + * sub-set ID and system-wide IOASID allocation results.
> + */
> + xa_init_flags(>xa, XA_FLAGS_ALLOC);
> +
> + ioasid_capacity_avail -= quota;
> + *sid = id;
> +
> +error:
> + spin_unlock(_allocator_lock);
> +
> + return ret;
> +}
> +EXPORT_SYMBOL_GPL(ioasid_alloc_set);
> +
> +/**
> + * ioasid_free_set - Free all IOASIDs within the set
> + *
> + * @sid: The IOASID set ID to be freed
> + * @destroy_set: Whether to keep the set for further allocation.
> + * If true, the set will be destroyed.
> + *
> + * All IOASIDs allocated within the set will be freed upon return.
> + */
> +void ioasid_free_set(int sid, bool destroy_set)
> +{
> + struct ioasid_set_data *sdata;
> + struct ioasid_data *entry;
> + unsigned long index;
> +
> + spin_lock(_allocator_lock);
> + sdata = xa_load(_sets, sid);
> + if (!sdata) {
> + pr_err("No IOASID set found to free %d\n", sid);
> + goto done_unlock;
> + }
> +
> + if (xa_empty(>xa)) {
> + pr_warn("No IOASIDs in the set %d\n", sdata->sid);
> + goto done_destroy;
> + }
> +
> +
RE: [PATCH 03/10] iommu/ioasid: Introduce per set allocation APIs
> From: Jacob Pan
> Sent: Saturday, March 28, 2020 12:59 AM
>
> On Fri, 27 Mar 2020 08:38:44 +
> "Tian, Kevin" wrote:
>
> > > From: Jacob Pan
> > > Sent: Thursday, March 26, 2020 1:55 AM
> > >
> > > IOASID set defines a group of IDs that share the same token. The
> > > ioasid_set concept helps to do permission checking among users as
> > > in the current code.
> > >
> > > With guest SVA usage, each VM has its own IOASID set. More
> > > functionalities are needed:
> > > 1. Enforce quota, each guest may be assigned limited quota such
> > > that one guest cannot abuse all the system resource.
> > > 2. Stores IOASID mapping between guest and host IOASIDs
> > > 3. Per set operations, e.g. free the entire set
> > >
> > > For each ioasid_set token, a unique set ID is assigned. This makes
> > > reference of the set and data lookup much easier to implement.
> > >
> > > Signed-off-by: Liu Yi L
> > > Signed-off-by: Jacob Pan
> > > ---
> > > drivers/iommu/ioasid.c | 147
> > > +
> > > include/linux/ioasid.h | 13 +
> > > 2 files changed, 160 insertions(+)
> > >
> > > diff --git a/drivers/iommu/ioasid.c b/drivers/iommu/ioasid.c
> > > index 4026e52855b9..27ee57f7079b 100644
> > > --- a/drivers/iommu/ioasid.c
> > > +++ b/drivers/iommu/ioasid.c
> > > @@ -10,6 +10,25 @@
> > > #include
> > > #include
> > >
> > > +static DEFINE_XARRAY_ALLOC(ioasid_sets);
> > > +/**
> > > + * struct ioasid_set_data - Meta data about ioasid_set
> > > + *
> > > + * @token: Unique to identify an IOASID set
> > > + * @xa: XArray to store subset ID and IOASID
> > > mapping
> >
> > what is a subset? is it a different thing from set?
> >
> Subset is a set, but a subset ID is an ID only valid within the set.
> When we have non-identity Guest-Host PASID mapping, Subset ID is
> the Guest PASID but in more general terms. Or call it "Set Private ID"
>
> This can be confusing, perhaps I rephrase it as:
> "XArray to store ioasid_set private ID to system-wide IOASID mapping"
>
>
> > > + * @size:Max number of IOASIDs can be allocated within the
> > > set
> >
> > 'size' reads more like 'current size' instead of 'max size'. maybe
> > call it 'max_ioasids' to align with 'nr_ioasids'? or simplify both as
> > 'max' and 'nr'?
> >
> Right, how about max_id and nr_id?
sounds good.
>
> > > + * @nr_ioasids 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;
> > > +};
> > > +
> > > struct ioasid_data {
> > > ioasid_t id;
> > > struct ioasid_set *set;
> > > @@ -388,6 +407,111 @@ void ioasid_free(ioasid_t ioasid)
> > > EXPORT_SYMBOL_GPL(ioasid_free);
> > >
> > > /**
> > > + * ioasid_alloc_set - Allocate a set of IOASIDs
> >
> > 'a set of IOASIDS' sounds like 'many IOASIDs'. Just saying 'allocate
> > an IOASID set' is more clear.
> >
> Make sense
>
> > > + * @token: Unique token of the IOASID set
> > > + * @quota: Quota allowed in this set
> > > + * @sid: IOASID set ID to be assigned
> > > + *
> > > + * Return 0 upon success. Token will be stored internally for
> > > lookup,
> > > + * IOASID allocation within the set and other per set operations
> > > will use
> > > + * the @sid assigned.
> > > + *
> > > + */
> > > +int ioasid_alloc_set(struct ioasid_set *token, ioasid_t quota, int
> > > *sid) +{
> > > + struct ioasid_set_data *sdata;
> > > + ioasid_t id;
> > > + int ret = 0;
> > > +
> > > + if (quota > ioasid_capacity_avail) {
> > > + pr_warn("Out of IOASID capacity! ask %d, avail
> > > %d\n",
> > > + quota, ioasid_capacity_avail);
> > > + return -ENOSPC;
> > > + }
> > > +
> > > + sdata = kzalloc(sizeof(*sdata), GFP_KERNEL);
> > > + if (!sdata)
> > > + return -ENOMEM;
> > > +
> > > + spin_lock(_allocator_lock);
> > > +
> > > + ret = xa_alloc(_sets, , sdata,
> > > +XA_LIMIT(0, ioasid_capacity_avail - quota),
> > > +GFP_KERNEL);
> >
> > Interestingly I didn't find the definition of ioasid_sets. and it is
> > not in existing file.
> >
> It is at the beginning of this file
> +static DEFINE_XARRAY_ALLOC(ioasid_sets);
How did I overlook it after several checks...
>
> > I'm not sure how many sets can be created, but anyway the set
> > namespace is different from ioasid name space. Then why do we
> > use ioasid capability as the limitation for allocating set id here?
> >
> I am assuming the worst case scenario which is one IOASID per set, that
> is why the number of sets are limited by the number of system IOASIDs.
I feel using a static max is simpler and clearer here. Anyway the set id
is never used on hardware so it is not necessary to tie it with dynamic
IOAPIC numbers.
>
> > > + if (ret) {
> > > + kfree(sdata);
> > > + goto error;
> > > + }
> > >
Re: [PATCH 03/10] iommu/ioasid: Introduce per set allocation APIs
On Fri, 27 Mar 2020 08:38:44 +
"Tian, Kevin" wrote:
> > From: Jacob Pan
> > Sent: Thursday, March 26, 2020 1:55 AM
> >
> > IOASID set defines a group of IDs that share the same token. The
> > ioasid_set concept helps to do permission checking among users as
> > in the current code.
> >
> > With guest SVA usage, each VM has its own IOASID set. More
> > functionalities are needed:
> > 1. Enforce quota, each guest may be assigned limited quota such
> > that one guest cannot abuse all the system resource.
> > 2. Stores IOASID mapping between guest and host IOASIDs
> > 3. Per set operations, e.g. free the entire set
> >
> > For each ioasid_set token, a unique set ID is assigned. This makes
> > reference of the set and data lookup much easier to implement.
> >
> > Signed-off-by: Liu Yi L
> > Signed-off-by: Jacob Pan
> > ---
> > drivers/iommu/ioasid.c | 147
> > +
> > include/linux/ioasid.h | 13 +
> > 2 files changed, 160 insertions(+)
> >
> > diff --git a/drivers/iommu/ioasid.c b/drivers/iommu/ioasid.c
> > index 4026e52855b9..27ee57f7079b 100644
> > --- a/drivers/iommu/ioasid.c
> > +++ b/drivers/iommu/ioasid.c
> > @@ -10,6 +10,25 @@
> > #include
> > #include
> >
> > +static DEFINE_XARRAY_ALLOC(ioasid_sets);
> > +/**
> > + * struct ioasid_set_data - Meta data about ioasid_set
> > + *
> > + * @token: Unique to identify an IOASID set
> > + * @xa:XArray to store subset ID and IOASID
> > mapping
>
> what is a subset? is it a different thing from set?
>
Subset is a set, but a subset ID is an ID only valid within the set.
When we have non-identity Guest-Host PASID mapping, Subset ID is
the Guest PASID but in more general terms. Or call it "Set Private ID"
This can be confusing, perhaps I rephrase it as:
"XArray to store ioasid_set private ID to system-wide IOASID mapping"
> > + * @size: Max number of IOASIDs can be allocated within the
> > set
>
> 'size' reads more like 'current size' instead of 'max size'. maybe
> call it 'max_ioasids' to align with 'nr_ioasids'? or simplify both as
> 'max' and 'nr'?
>
Right, how about max_id and nr_id?
> > + * @nr_ioasids Number of IOASIDs allocated in the set
> > + * @sidID 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;
> > +};
> > +
> > struct ioasid_data {
> > ioasid_t id;
> > struct ioasid_set *set;
> > @@ -388,6 +407,111 @@ void ioasid_free(ioasid_t ioasid)
> > EXPORT_SYMBOL_GPL(ioasid_free);
> >
> > /**
> > + * ioasid_alloc_set - Allocate a set of IOASIDs
>
> 'a set of IOASIDS' sounds like 'many IOASIDs'. Just saying 'allocate
> an IOASID set' is more clear.
>
Make sense
> > + * @token: Unique token of the IOASID set
> > + * @quota: Quota allowed in this set
> > + * @sid: IOASID set ID to be assigned
> > + *
> > + * Return 0 upon success. Token will be stored internally for
> > lookup,
> > + * IOASID allocation within the set and other per set operations
> > will use
> > + * the @sid assigned.
> > + *
> > + */
> > +int ioasid_alloc_set(struct ioasid_set *token, ioasid_t quota, int
> > *sid) +{
> > + struct ioasid_set_data *sdata;
> > + ioasid_t id;
> > + int ret = 0;
> > +
> > + if (quota > ioasid_capacity_avail) {
> > + pr_warn("Out of IOASID capacity! ask %d, avail
> > %d\n",
> > + quota, ioasid_capacity_avail);
> > + return -ENOSPC;
> > + }
> > +
> > + sdata = kzalloc(sizeof(*sdata), GFP_KERNEL);
> > + if (!sdata)
> > + return -ENOMEM;
> > +
> > + spin_lock(_allocator_lock);
> > +
> > + ret = xa_alloc(_sets, , sdata,
> > + XA_LIMIT(0, ioasid_capacity_avail - quota),
> > + GFP_KERNEL);
>
> Interestingly I didn't find the definition of ioasid_sets. and it is
> not in existing file.
>
It is at the beginning of this file
+static DEFINE_XARRAY_ALLOC(ioasid_sets);
> I'm not sure how many sets can be created, but anyway the set
> namespace is different from ioasid name space. Then why do we
> use ioasid capability as the limitation for allocating set id here?
>
I am assuming the worst case scenario which is one IOASID per set, that
is why the number of sets are limited by the number of system IOASIDs.
> > + if (ret) {
> > + kfree(sdata);
> > + goto error;
> > + }
> > +
> > + sdata->token = token;
>
> given token must be unique, a check on any conflict is required here?
>
Right, I will add a check to reject duplicated tokens.
/* Search existing set tokens, reject duplicates */
xa_for_each(_sets, index, sdata) {
if (sdata->token == token) {
pr_warn("Token already exists in the set %lu\n", index);
ret = -EEXIST;
goto error;
}
}
> > +
RE: [PATCH 03/10] iommu/ioasid: Introduce per set allocation APIs
> From: Jacob Pan
> Sent: Thursday, March 26, 2020 1:55 AM
>
> IOASID set defines a group of IDs that share the same token. The
> ioasid_set concept helps to do permission checking among users as in the
> current code.
>
> With guest SVA usage, each VM has its own IOASID set. More
> functionalities are needed:
> 1. Enforce quota, each guest may be assigned limited quota such that one
> guest cannot abuse all the system resource.
> 2. Stores IOASID mapping between guest and host IOASIDs
> 3. Per set operations, e.g. free the entire set
>
> For each ioasid_set token, a unique set ID is assigned. This makes
> reference of the set and data lookup much easier to implement.
>
> Signed-off-by: Liu Yi L
> Signed-off-by: Jacob Pan
> ---
> drivers/iommu/ioasid.c | 147
> +
> include/linux/ioasid.h | 13 +
> 2 files changed, 160 insertions(+)
>
> diff --git a/drivers/iommu/ioasid.c b/drivers/iommu/ioasid.c
> index 4026e52855b9..27ee57f7079b 100644
> --- a/drivers/iommu/ioasid.c
> +++ b/drivers/iommu/ioasid.c
> @@ -10,6 +10,25 @@
> #include
> #include
>
> +static DEFINE_XARRAY_ALLOC(ioasid_sets);
> +/**
> + * struct ioasid_set_data - Meta data about ioasid_set
> + *
> + * @token: Unique to identify an IOASID set
> + * @xa: XArray to store subset ID and IOASID mapping
what is a subset? is it a different thing from set?
> + * @size:Max number of IOASIDs can be allocated within the set
'size' reads more like 'current size' instead of 'max size'. maybe call
it 'max_ioasids' to align with 'nr_ioasids'? or simplify both as
'max' and 'nr'?
> + * @nr_ioasids 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;
> +};
> +
> struct ioasid_data {
> ioasid_t id;
> struct ioasid_set *set;
> @@ -388,6 +407,111 @@ void ioasid_free(ioasid_t ioasid)
> EXPORT_SYMBOL_GPL(ioasid_free);
>
> /**
> + * ioasid_alloc_set - Allocate a set of IOASIDs
'a set of IOASIDS' sounds like 'many IOASIDs'. Just saying 'allocate
an IOASID set' is more clear.
> + * @token: Unique token of the IOASID set
> + * @quota: Quota allowed in this set
> + * @sid: IOASID set ID to be assigned
> + *
> + * Return 0 upon success. Token will be stored internally for lookup,
> + * IOASID allocation within the set and other per set operations will use
> + * the @sid assigned.
> + *
> + */
> +int ioasid_alloc_set(struct ioasid_set *token, ioasid_t quota, int *sid)
> +{
> + struct ioasid_set_data *sdata;
> + ioasid_t id;
> + int ret = 0;
> +
> + if (quota > ioasid_capacity_avail) {
> + pr_warn("Out of IOASID capacity! ask %d, avail %d\n",
> + quota, ioasid_capacity_avail);
> + return -ENOSPC;
> + }
> +
> + sdata = kzalloc(sizeof(*sdata), GFP_KERNEL);
> + if (!sdata)
> + return -ENOMEM;
> +
> + spin_lock(_allocator_lock);
> +
> + ret = xa_alloc(_sets, , sdata,
> +XA_LIMIT(0, ioasid_capacity_avail - quota),
> +GFP_KERNEL);
Interestingly I didn't find the definition of ioasid_sets. and it is not in
existing file.
I'm not sure how many sets can be created, but anyway the set
namespace is different from ioasid name space. Then why do we
use ioasid capability as the limitation for allocating set id here?
> + if (ret) {
> + kfree(sdata);
> + goto error;
> + }
> +
> + sdata->token = token;
given token must be unique, a check on any conflict is required here?
> + sdata->size = quota;
> + sdata->sid = id;
> +
> + /*
> + * Set Xarray is used to store IDs within the set, get ready for
> + * sub-set ID and system-wide IOASID allocation results.
looks 'subset' is the same thing as 'set'. let's make it consistent.
> + */
> + xa_init_flags(>xa, XA_FLAGS_ALLOC);
> +
> + ioasid_capacity_avail -= quota;
> + *sid = id;
> +
> +error:
> + spin_unlock(_allocator_lock);
> +
> + return ret;
> +}
> +EXPORT_SYMBOL_GPL(ioasid_alloc_set);
> +
> +/**
> + * ioasid_free_set - Free all IOASIDs within the set
> + *
> + * @sid: The IOASID set ID to be freed
> + * @destroy_set: Whether to keep the set for further allocation.
> + * If true, the set will be destroyed.
> + *
> + * All IOASIDs allocated within the set will be freed upon return.
> + */
> +void ioasid_free_set(int sid, bool destroy_set)
> +{
what is the actual usage of just freeing ioasid while keeping the
set itself?
> + struct ioasid_set_data *sdata;
> + struct ioasid_data *entry;
> + unsigned long index;
> +
> + spin_lock(_allocator_lock);
> + sdata = xa_load(_sets, sid);
> + if (!sdata) {
> + pr_err("No IOASID set found to free
Re: [PATCH 03/10] iommu/ioasid: Introduce per set allocation APIs
Hi Baolu,
On Thu, 26 Mar 2020 10:12:36 +0800
Lu Baolu wrote:
> On 2020/3/26 1:55, Jacob Pan wrote:
> > IOASID set defines a group of IDs that share the same token. The
> > ioasid_set concept helps to do permission checking among users as
> > in the current code.
> >
> > With guest SVA usage, each VM has its own IOASID set. More
> > functionalities are needed:
> > 1. Enforce quota, each guest may be assigned limited quota such
> > that one guest cannot abuse all the system resource.
> > 2. Stores IOASID mapping between guest and host IOASIDs
> > 3. Per set operations, e.g. free the entire set
> >
> > For each ioasid_set token, a unique set ID is assigned. This makes
> > reference of the set and data lookup much easier to implement.
> >
> > Signed-off-by: Liu Yi L
> > Signed-off-by: Jacob Pan
> > ---
> > drivers/iommu/ioasid.c | 147
> > +
> > include/linux/ioasid.h | 13 + 2 files changed, 160
> > insertions(+)
> >
> > diff --git a/drivers/iommu/ioasid.c b/drivers/iommu/ioasid.c
> > index 4026e52855b9..27ee57f7079b 100644
> > --- a/drivers/iommu/ioasid.c
> > +++ b/drivers/iommu/ioasid.c
> > @@ -10,6 +10,25 @@
> > #include
> > #include
> >
> > +static DEFINE_XARRAY_ALLOC(ioasid_sets);
> > +/**
> > + * struct ioasid_set_data - Meta data about ioasid_set
> > + *
> > + * @token: Unique to identify an IOASID set
> > + * @xa:XArray to store subset ID and IOASID mapping
> > + * @size: Max number of IOASIDs can be allocated within the
> > set
> > + * @nr_ioasids Number of IOASIDs allocated in the set
> > + * @sidID 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;
> > +};
> > +
> > struct ioasid_data {
> > ioasid_t id;
> > struct ioasid_set *set;
> > @@ -388,6 +407,111 @@ void ioasid_free(ioasid_t ioasid)
> > EXPORT_SYMBOL_GPL(ioasid_free);
> >
> > /**
> > + * ioasid_alloc_set - Allocate a set of IOASIDs
> > + * @token: Unique token of the IOASID set
> > + * @quota: Quota allowed in this set
> > + * @sid: IOASID set ID to be assigned
> > + *
> > + * Return 0 upon success. Token will be stored internally for
> > lookup,
> > + * IOASID allocation within the set and other per set operations
> > will use
> > + * the @sid assigned.
> > + *
> > + */
> > +int ioasid_alloc_set(struct ioasid_set *token, ioasid_t quota, int
> > *sid) +{
> > + struct ioasid_set_data *sdata;
> > + ioasid_t id;
> > + int ret = 0;
> > +
> > + if (quota > ioasid_capacity_avail) {
> > + pr_warn("Out of IOASID capacity! ask %d, avail
> > %d\n",
> > + quota, ioasid_capacity_avail);
> > + return -ENOSPC;
> > + }
> > +
> > + sdata = kzalloc(sizeof(*sdata), GFP_KERNEL);
> > + if (!sdata)
> > + return -ENOMEM;
> > +
> > + spin_lock(_allocator_lock);
> > +
> > + ret = xa_alloc(_sets, , sdata,
> > + XA_LIMIT(0, ioasid_capacity_avail - quota),
> > + GFP_KERNEL);
> > + if (ret) {
> > + kfree(sdata);
> > + goto error;
> > + }
> > +
> > + sdata->token = token;
> > + sdata->size = quota;
> > + sdata->sid = id;
> > +
> > + /*
> > +* Set Xarray is used to store IDs within the set, get
> > ready for
> > +* sub-set ID and system-wide IOASID allocation results.
> > +*/
> > + xa_init_flags(>xa, XA_FLAGS_ALLOC);
> > +
> > + ioasid_capacity_avail -= quota;
> > + *sid = id;
> > +
> > +error:
> > + spin_unlock(_allocator_lock);
> > +
> > + return ret;
> > +}
> > +EXPORT_SYMBOL_GPL(ioasid_alloc_set);
> > +
> > +/**
> > + * ioasid_free_set - Free all IOASIDs within the set
> > + *
> > + * @sid: The IOASID set ID to be freed
> > + * @destroy_set: Whether to keep the set for further
> > allocation.
> > + * If true, the set will be destroyed.
> > + *
> > + * All IOASIDs allocated within the set will be freed upon return.
> > + */
> > +void ioasid_free_set(int sid, bool destroy_set)
> > +{
> > + struct ioasid_set_data *sdata;
> > + struct ioasid_data *entry;
> > + unsigned long index;
> > +
> > + spin_lock(_allocator_lock);
> > + sdata = xa_load(_sets, sid);
> > + if (!sdata) {
> > + pr_err("No IOASID set found to free %d\n", sid);
> > + goto done_unlock;
> > + }
> > +
> > + if (xa_empty(>xa)) {
> > + pr_warn("No IOASIDs in the set %d\n", sdata->sid);
> > + goto done_destroy;
> > + }
> > +
> > + /* Just a place holder for now */
> > + xa_for_each(>xa, index, entry) {
> > + /* Free from per sub-set pool */
> > + xa_erase(>xa, index);
> > + }
> > +
> > +done_destroy:
> > + if (destroy_set) {
> > + xa_erase(_sets, sid);
> > +
> > + /* Return the quota back to system pool */
> > + ioasid_capacity_avail += sdata->size;
> > +
Re: [PATCH 03/10] iommu/ioasid: Introduce per set allocation APIs
On 2020/3/26 1:55, Jacob Pan wrote:
IOASID set defines a group of IDs that share the same token. The
ioasid_set concept helps to do permission checking among users as in the
current code.
With guest SVA usage, each VM has its own IOASID set. More
functionalities are needed:
1. Enforce quota, each guest may be assigned limited quota such that one
guest cannot abuse all the system resource.
2. Stores IOASID mapping between guest and host IOASIDs
3. Per set operations, e.g. free the entire set
For each ioasid_set token, a unique set ID is assigned. This makes
reference of the set and data lookup much easier to implement.
Signed-off-by: Liu Yi L
Signed-off-by: Jacob Pan
---
drivers/iommu/ioasid.c | 147 +
include/linux/ioasid.h | 13 +
2 files changed, 160 insertions(+)
diff --git a/drivers/iommu/ioasid.c b/drivers/iommu/ioasid.c
index 4026e52855b9..27ee57f7079b 100644
--- a/drivers/iommu/ioasid.c
+++ b/drivers/iommu/ioasid.c
@@ -10,6 +10,25 @@
#include
#include
+static DEFINE_XARRAY_ALLOC(ioasid_sets);
+/**
+ * struct ioasid_set_data - Meta data about ioasid_set
+ *
+ * @token: Unique to identify an IOASID set
+ * @xa:XArray to store subset ID and IOASID mapping
+ * @size: Max number of IOASIDs can be allocated within the set
+ * @nr_ioasids Number of IOASIDs allocated in the set
+ * @sidID 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;
+};
+
struct ioasid_data {
ioasid_t id;
struct ioasid_set *set;
@@ -388,6 +407,111 @@ void ioasid_free(ioasid_t ioasid)
EXPORT_SYMBOL_GPL(ioasid_free);
/**
+ * ioasid_alloc_set - Allocate a set of IOASIDs
+ * @token: Unique token of the IOASID set
+ * @quota: Quota allowed in this set
+ * @sid: IOASID set ID to be assigned
+ *
+ * Return 0 upon success. Token will be stored internally for lookup,
+ * IOASID allocation within the set and other per set operations will use
+ * the @sid assigned.
+ *
+ */
+int ioasid_alloc_set(struct ioasid_set *token, ioasid_t quota, int *sid)
+{
+ struct ioasid_set_data *sdata;
+ ioasid_t id;
+ int ret = 0;
+
+ if (quota > ioasid_capacity_avail) {
+ pr_warn("Out of IOASID capacity! ask %d, avail %d\n",
+ quota, ioasid_capacity_avail);
+ return -ENOSPC;
+ }
+
+ sdata = kzalloc(sizeof(*sdata), GFP_KERNEL);
+ if (!sdata)
+ return -ENOMEM;
+
+ spin_lock(_allocator_lock);
+
+ ret = xa_alloc(_sets, , sdata,
+ XA_LIMIT(0, ioasid_capacity_avail - quota),
+ GFP_KERNEL);
+ if (ret) {
+ kfree(sdata);
+ goto error;
+ }
+
+ sdata->token = token;
+ sdata->size = quota;
+ sdata->sid = id;
+
+ /*
+* Set Xarray is used to store IDs within the set, get ready for
+* sub-set ID and system-wide IOASID allocation results.
+*/
+ xa_init_flags(>xa, XA_FLAGS_ALLOC);
+
+ ioasid_capacity_avail -= quota;
+ *sid = id;
+
+error:
+ spin_unlock(_allocator_lock);
+
+ return ret;
+}
+EXPORT_SYMBOL_GPL(ioasid_alloc_set);
+
+/**
+ * ioasid_free_set - Free all IOASIDs within the set
+ *
+ * @sid: The IOASID set ID to be freed
+ * @destroy_set: Whether to keep the set for further allocation.
+ * If true, the set will be destroyed.
+ *
+ * All IOASIDs allocated within the set will be freed upon return.
+ */
+void ioasid_free_set(int sid, bool destroy_set)
+{
+ struct ioasid_set_data *sdata;
+ struct ioasid_data *entry;
+ unsigned long index;
+
+ spin_lock(_allocator_lock);
+ sdata = xa_load(_sets, sid);
+ if (!sdata) {
+ pr_err("No IOASID set found to free %d\n", sid);
+ goto done_unlock;
+ }
+
+ if (xa_empty(>xa)) {
+ pr_warn("No IOASIDs in the set %d\n", sdata->sid);
+ goto done_destroy;
+ }
+
+ /* Just a place holder for now */
+ xa_for_each(>xa, index, entry) {
+ /* Free from per sub-set pool */
+ xa_erase(>xa, index);
+ }
+
+done_destroy:
+ if (destroy_set) {
+ xa_erase(_sets, sid);
+
+ /* Return the quota back to system pool */
+ ioasid_capacity_avail += sdata->size;
+ kfree_rcu(sdata, rcu);
+ }
+
+done_unlock:
+ spin_unlock(_allocator_lock);
+}
+EXPORT_SYMBOL_GPL(ioasid_free_set);
+
+
+/**
* ioasid_find - Find IOASID data
* @set: the IOASID set
* @ioasid: the IOASID to find
@@ -431,6 +555,29 @@ void *ioasid_find(struct ioasid_set *set, ioasid_t ioasid,
}
EXPORT_SYMBOL_GPL(ioasid_find);
+/**
+ * ioasid_find_sid - Retrieve IOASID
[PATCH 03/10] iommu/ioasid: Introduce per set allocation APIs
IOASID set defines a group of IDs that share the same token. The
ioasid_set concept helps to do permission checking among users as in the
current code.
With guest SVA usage, each VM has its own IOASID set. More
functionalities are needed:
1. Enforce quota, each guest may be assigned limited quota such that one
guest cannot abuse all the system resource.
2. Stores IOASID mapping between guest and host IOASIDs
3. Per set operations, e.g. free the entire set
For each ioasid_set token, a unique set ID is assigned. This makes
reference of the set and data lookup much easier to implement.
Signed-off-by: Liu Yi L
Signed-off-by: Jacob Pan
---
drivers/iommu/ioasid.c | 147 +
include/linux/ioasid.h | 13 +
2 files changed, 160 insertions(+)
diff --git a/drivers/iommu/ioasid.c b/drivers/iommu/ioasid.c
index 4026e52855b9..27ee57f7079b 100644
--- a/drivers/iommu/ioasid.c
+++ b/drivers/iommu/ioasid.c
@@ -10,6 +10,25 @@
#include
#include
+static DEFINE_XARRAY_ALLOC(ioasid_sets);
+/**
+ * struct ioasid_set_data - Meta data about ioasid_set
+ *
+ * @token: Unique to identify an IOASID set
+ * @xa:XArray to store subset ID and IOASID mapping
+ * @size: Max number of IOASIDs can be allocated within the set
+ * @nr_ioasids Number of IOASIDs allocated in the set
+ * @sidID 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;
+};
+
struct ioasid_data {
ioasid_t id;
struct ioasid_set *set;
@@ -388,6 +407,111 @@ void ioasid_free(ioasid_t ioasid)
EXPORT_SYMBOL_GPL(ioasid_free);
/**
+ * ioasid_alloc_set - Allocate a set of IOASIDs
+ * @token: Unique token of the IOASID set
+ * @quota: Quota allowed in this set
+ * @sid: IOASID set ID to be assigned
+ *
+ * Return 0 upon success. Token will be stored internally for lookup,
+ * IOASID allocation within the set and other per set operations will use
+ * the @sid assigned.
+ *
+ */
+int ioasid_alloc_set(struct ioasid_set *token, ioasid_t quota, int *sid)
+{
+ struct ioasid_set_data *sdata;
+ ioasid_t id;
+ int ret = 0;
+
+ if (quota > ioasid_capacity_avail) {
+ pr_warn("Out of IOASID capacity! ask %d, avail %d\n",
+ quota, ioasid_capacity_avail);
+ return -ENOSPC;
+ }
+
+ sdata = kzalloc(sizeof(*sdata), GFP_KERNEL);
+ if (!sdata)
+ return -ENOMEM;
+
+ spin_lock(_allocator_lock);
+
+ ret = xa_alloc(_sets, , sdata,
+ XA_LIMIT(0, ioasid_capacity_avail - quota),
+ GFP_KERNEL);
+ if (ret) {
+ kfree(sdata);
+ goto error;
+ }
+
+ sdata->token = token;
+ sdata->size = quota;
+ sdata->sid = id;
+
+ /*
+* Set Xarray is used to store IDs within the set, get ready for
+* sub-set ID and system-wide IOASID allocation results.
+*/
+ xa_init_flags(>xa, XA_FLAGS_ALLOC);
+
+ ioasid_capacity_avail -= quota;
+ *sid = id;
+
+error:
+ spin_unlock(_allocator_lock);
+
+ return ret;
+}
+EXPORT_SYMBOL_GPL(ioasid_alloc_set);
+
+/**
+ * ioasid_free_set - Free all IOASIDs within the set
+ *
+ * @sid: The IOASID set ID to be freed
+ * @destroy_set: Whether to keep the set for further allocation.
+ * If true, the set will be destroyed.
+ *
+ * All IOASIDs allocated within the set will be freed upon return.
+ */
+void ioasid_free_set(int sid, bool destroy_set)
+{
+ struct ioasid_set_data *sdata;
+ struct ioasid_data *entry;
+ unsigned long index;
+
+ spin_lock(_allocator_lock);
+ sdata = xa_load(_sets, sid);
+ if (!sdata) {
+ pr_err("No IOASID set found to free %d\n", sid);
+ goto done_unlock;
+ }
+
+ if (xa_empty(>xa)) {
+ pr_warn("No IOASIDs in the set %d\n", sdata->sid);
+ goto done_destroy;
+ }
+
+ /* Just a place holder for now */
+ xa_for_each(>xa, index, entry) {
+ /* Free from per sub-set pool */
+ xa_erase(>xa, index);
+ }
+
+done_destroy:
+ if (destroy_set) {
+ xa_erase(_sets, sid);
+
+ /* Return the quota back to system pool */
+ ioasid_capacity_avail += sdata->size;
+ kfree_rcu(sdata, rcu);
+ }
+
+done_unlock:
+ spin_unlock(_allocator_lock);
+}
+EXPORT_SYMBOL_GPL(ioasid_free_set);
+
+
+/**
* ioasid_find - Find IOASID data
* @set: the IOASID set
* @ioasid: the IOASID to find
@@ -431,6 +555,29 @@ void *ioasid_find(struct ioasid_set *set, ioasid_t ioasid,
}
EXPORT_SYMBOL_GPL(ioasid_find);
+/**
+ * ioasid_find_sid - Retrieve IOASID set ID from an ioasid
+ * Caller must
