IOASID set token

IOASID set token

[Jacob Pan]

Hi Jean,

Have a question for you on whether we can have a fixed token type for
ioasid_set.

Currently, ioasid_set has an arbitrary token. For VT-d vSVA usage, we
choose mm as ioasid_set token to identify PASIDs within a guest. We have
multiple in-kernel users of PASIDs such as VFIO, KVM, and VDCM. When an
IOASID set is created, there is not a good way to communicate about the
token choices. So we have to let VDCM and KVM *assume* mm is used as
token, then retrieve ioasid_set based on the token.

This assumption of "mm as token" is not a reliable SW architecture. So
we are thinking if we can have an explicit ioasid_set token type where
mm is used. After all, PASID and mm are closely related.

The code change might be the following:
1. add a flag to indicate token type when ioasid_set is allocated, e.g.
IOASID_SET_TYPE_MM
IOASID_SET_TYPE_ANY
2. other users of the ioasid_set can query if an mm token exists based
on the flag IOASID_SET_TYPE_MM, then retrieve the ioasid_set.

Existing ioasid_set user can still use arbitrary token under the flag
IOASID_SET_TYPE_ANY

Would this be an issue for ARM usage?

Thanks,

Jacob
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: IOASID set token

[Jacob Pan]

Hi Jean,

Just realized I should send this to your Linaro account instead of ARM.
So Hi again :)

On Wed, 1 Jul 2020 23:29:16 -0700
Jacob Pan <jacob.jun.pan@linux.intel.com> wrote:

> Hi Jean,
> 
> Have a question for you on whether we can have a fixed token type for
> ioasid_set.
> 
> Currently, ioasid_set has an arbitrary token. For VT-d vSVA usage, we
> choose mm as ioasid_set token to identify PASIDs within a guest. We
> have multiple in-kernel users of PASIDs such as VFIO, KVM, and VDCM.
> When an IOASID set is created, there is not a good way to communicate
> about the token choices. So we have to let VDCM and KVM *assume* mm
> is used as token, then retrieve ioasid_set based on the token.
> 
> This assumption of "mm as token" is not a reliable SW architecture. So
> we are thinking if we can have an explicit ioasid_set token type where
> mm is used. After all, PASID and mm are closely related.
> 
> The code change might be the following:
> 1. add a flag to indicate token type when ioasid_set is allocated,
> e.g. IOASID_SET_TYPE_MM
> IOASID_SET_TYPE_ANY
> 2. other users of the ioasid_set can query if an mm token exists based
> on the flag IOASID_SET_TYPE_MM, then retrieve the ioasid_set.
> 
> Existing ioasid_set user can still use arbitrary token under the flag
> IOASID_SET_TYPE_ANY
> 
> Would this be an issue for ARM usage?
> 
> Thanks,
> 
> Jacob

[Jacob Pan]
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: IOASID set token

[Jean-Philippe Brucker]

Hi Jacob,

On Thu, Jul 02, 2020 at 06:48:25AM -0700, Jacob Pan wrote:
> Hi Jean,
> 
> Just realized I should send this to your Linaro account instead of ARM.
> So Hi again :)
> 
> On Wed, 1 Jul 2020 23:29:16 -0700
> Jacob Pan <jacob.jun.pan@linux.intel.com> wrote:
> 
> > Hi Jean,
> > 
> > Have a question for you on whether we can have a fixed token type for
> > ioasid_set.
> > 
> > Currently, ioasid_set has an arbitrary token. For VT-d vSVA usage, we
> > choose mm as ioasid_set token to identify PASIDs within a guest. We
> > have multiple in-kernel users of PASIDs such as VFIO, KVM, and VDCM.
> > When an IOASID set is created, there is not a good way to communicate
> > about the token choices. So we have to let VDCM and KVM *assume* mm
> > is used as token, then retrieve ioasid_set based on the token.
> > 
> > This assumption of "mm as token" is not a reliable SW architecture.

I don't see this as a problem. The token type is tied to the IOASID set,
so users that pass those IOASID sets to ioasid_find() can safely assume
that the returned pointer is an mm_struct. That said I'm not opposed to
consolidating the API with explicit types, it could definitely be more
elegant.

> > So
> > we are thinking if we can have an explicit ioasid_set token type where
> > mm is used. After all, PASID and mm are closely related.
> > 
> > The code change might be the following:
> > 1. add a flag to indicate token type when ioasid_set is allocated,
> > e.g. IOASID_SET_TYPE_MM
> > IOASID_SET_TYPE_ANY
> > 2. other users of the ioasid_set can query if an mm token exists based
> > on the flag IOASID_SET_TYPE_MM, then retrieve the ioasid_set.
> > 
> > Existing ioasid_set user can still use arbitrary token under the flag
> > IOASID_SET_TYPE_ANY
> > 
> > Would this be an issue for ARM usage?

In my current implementation of auxiliary domains for Arm SMMU (which
might never be useful enough to go upstream) I don't even use a token for
the private IOASID set. However I still think we should leave the option
to use a type different than mm_struct as token for some IOASID sets
because device drivers (e.g. AMD kfd) may also want to dip into the IOASID
space and use their own token type.

For the moment, though, we could actually specialize the IOASID API to
only take an mm_struct as token. For example the functions exported by the
IOASID lib would be:

  ioasid_t ioasid_alloc_mm(set, min, max, struct mm_struct *mm)
  struct mm_struct *ioasid_find_mm(set, ioasid)
  ...

And ioasid_alloc(), ioasid_find(), etc would be internal to ioasid.c and
deal with IOASID_SET_TYPE_MM (or even be removed entirely for now). New
users that need different token types could then introduce their own
IOASID_SET_TYPE_* and use the lower-level functions.

Thanks,
Jean
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: IOASID set token

[Jacob Pan]

Hi Jean,
Thanks for the feedback, please see replies inline.

On Mon, 6 Jul 2020 12:30:41 +0200
Jean-Philippe Brucker <jean-philippe@linaro.org> wrote:

> Hi Jacob,
> 
> On Thu, Jul 02, 2020 at 06:48:25AM -0700, Jacob Pan wrote:
> > Hi Jean,
> > 
> > Just realized I should send this to your Linaro account instead of
> > ARM. So Hi again :)
> > 
> > On Wed, 1 Jul 2020 23:29:16 -0700
> > Jacob Pan <jacob.jun.pan@linux.intel.com> wrote:
> >   
> > > Hi Jean,
> > > 
> > > Have a question for you on whether we can have a fixed token type
> > > for ioasid_set.
> > > 
> > > Currently, ioasid_set has an arbitrary token. For VT-d vSVA
> > > usage, we choose mm as ioasid_set token to identify PASIDs within
> > > a guest. We have multiple in-kernel users of PASIDs such as VFIO,
> > > KVM, and VDCM. When an IOASID set is created, there is not a good
> > > way to communicate about the token choices. So we have to let
> > > VDCM and KVM *assume* mm is used as token, then retrieve
> > > ioasid_set based on the token.
> > > 
> > > This assumption of "mm as token" is not a reliable SW
> > > architecture.  
> 
> I don't see this as a problem. The token type is tied to the IOASID
> set, so users that pass those IOASID sets to ioasid_find() can safely
> assume that the returned pointer is an mm_struct. That said I'm not
> opposed to consolidating the API with explicit types, it could
> definitely be more elegant.
> 
In our use case, we need token to ioasid_set lookup. e.g. VFIO creates
an ioasid_set with a token. KVM instance needs to know the
ioasid_set based on its mm. So ioasid_find() does not know the set
until it finds it based on the token and token type.
> > > So
> > > we are thinking if we can have an explicit ioasid_set token type
> > > where mm is used. After all, PASID and mm are closely related.
> > > 
> > > The code change might be the following:
> > > 1. add a flag to indicate token type when ioasid_set is allocated,
> > > e.g. IOASID_SET_TYPE_MM
> > > IOASID_SET_TYPE_ANY
> > > 2. other users of the ioasid_set can query if an mm token exists
> > > based on the flag IOASID_SET_TYPE_MM, then retrieve the
> > > ioasid_set.
> > > 
> > > Existing ioasid_set user can still use arbitrary token under the
> > > flag IOASID_SET_TYPE_ANY
> > > 
> > > Would this be an issue for ARM usage?  
> 
> In my current implementation of auxiliary domains for Arm SMMU (which
> might never be useful enough to go upstream) I don't even use a token
> for the private IOASID set. However I still think we should leave the
> option to use a type different than mm_struct as token for some
> IOASID sets because device drivers (e.g. AMD kfd) may also want to
> dip into the IOASID space and use their own token type.
> 
> For the moment, though, we could actually specialize the IOASID API to
> only take an mm_struct as token.
That would be fine with VT-d. We can use init_mm for host PASID set,
process mm for VM set.

> For example the functions exported
> by the IOASID lib would be:
> 
>   ioasid_t ioasid_alloc_mm(set, min, max, struct mm_struct *mm)
what is the set argument used for? In my view, ioasid_set and mm are
1:1 mapped.

struct ioasid_set *ioasid_alloc_set(void *token, ioasid_t quota)


I was thinking we still have APIs for IOASID set alloc/free since we
want to embed ioasid_set info in driver data structures for ownership
check.

>   struct mm_struct *ioasid_find_mm(set, ioasid)
I don't get why we need ioasid to find the set token. If we put
mm_struct* inside ioasid_set, then we can get the token from the set
directly.

>   ...
> 
> And ioasid_alloc(), ioasid_find(), etc would be internal to ioasid.c
> and deal with IOASID_SET_TYPE_MM (or even be removed entirely for
> now). New users that need different token types could then introduce
> their own IOASID_SET_TYPE_* and use the lower-level functions.
> 
I will keep that in mind in my next set. I think it would be much
easier to explain with code.

My takeaway is that we have a high-level agreement to have explicit mm
as token in IOASID APIs. I think we can work out the details with
patches.

Jacob


> Thanks,
> Jean

[Jacob Pan]
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: IOASID set token

[Jean-Philippe Brucker]

On Mon, Jul 06, 2020 at 01:51:37PM -0700, Jacob Pan wrote:
> Hi Jean,
> Thanks for the feedback, please see replies inline.
> 
> On Mon, 6 Jul 2020 12:30:41 +0200
> Jean-Philippe Brucker <jean-philippe@linaro.org> wrote:
> 
> > Hi Jacob,
> > 
> > On Thu, Jul 02, 2020 at 06:48:25AM -0700, Jacob Pan wrote:
> > > Hi Jean,
> > > 
> > > Just realized I should send this to your Linaro account instead of
> > > ARM. So Hi again :)
> > > 
> > > On Wed, 1 Jul 2020 23:29:16 -0700
> > > Jacob Pan <jacob.jun.pan@linux.intel.com> wrote:
> > >   
> > > > Hi Jean,
> > > > 
> > > > Have a question for you on whether we can have a fixed token type
> > > > for ioasid_set.
> > > > 
> > > > Currently, ioasid_set has an arbitrary token. For VT-d vSVA
> > > > usage, we choose mm as ioasid_set token to identify PASIDs within
> > > > a guest. We have multiple in-kernel users of PASIDs such as VFIO,
> > > > KVM, and VDCM. When an IOASID set is created, there is not a good
> > > > way to communicate about the token choices. So we have to let
> > > > VDCM and KVM *assume* mm is used as token, then retrieve
> > > > ioasid_set based on the token.
> > > > 
> > > > This assumption of "mm as token" is not a reliable SW
> > > > architecture.  
> > 
> > I don't see this as a problem. The token type is tied to the IOASID
> > set, so users that pass those IOASID sets to ioasid_find() can safely
> > assume that the returned pointer is an mm_struct. That said I'm not
> > opposed to consolidating the API with explicit types, it could
> > definitely be more elegant.
> > 
> In our use case, we need token to ioasid_set lookup. e.g. VFIO creates
> an ioasid_set with a token. KVM instance needs to know the
> ioasid_set based on its mm. So ioasid_find() does not know the set
> until it finds it based on the token and token type.

Right, I mixed up "token" with the private data associated with each
IOASID. I had mostly forgotten about the principles you introduced in the
"IOASID extensions for guest SVA" series, sorry for the confusion.

> > > > So
> > > > we are thinking if we can have an explicit ioasid_set token type
> > > > where mm is used. After all, PASID and mm are closely related.
> > > > 
> > > > The code change might be the following:
> > > > 1. add a flag to indicate token type when ioasid_set is allocated,
> > > > e.g. IOASID_SET_TYPE_MM
> > > > IOASID_SET_TYPE_ANY
> > > > 2. other users of the ioasid_set can query if an mm token exists
> > > > based on the flag IOASID_SET_TYPE_MM, then retrieve the
> > > > ioasid_set.
> > > > 
> > > > Existing ioasid_set user can still use arbitrary token under the
> > > > flag IOASID_SET_TYPE_ANY
> > > > 
> > > > Would this be an issue for ARM usage?  
> > 
> > In my current implementation of auxiliary domains for Arm SMMU (which
> > might never be useful enough to go upstream) I don't even use a token
> > for the private IOASID set. However I still think we should leave the
> > option to use a type different than mm_struct as token for some
> > IOASID sets because device drivers (e.g. AMD kfd) may also want to
> > dip into the IOASID space and use their own token type.
> > 
> > For the moment, though, we could actually specialize the IOASID API to
> > only take an mm_struct as token.
> That would be fine with VT-d. We can use init_mm for host PASID set,
> process mm for VM set.

I'm not fond of using init_mm for the host PASID set. Does it need a token
at all?

> 
> > For example the functions exported
> > by the IOASID lib would be:
> > 
> >   ioasid_t ioasid_alloc_mm(set, min, max, struct mm_struct *mm)
> what is the set argument used for? In my view, ioasid_set and mm are
> 1:1 mapped.

Please disregard this, it was replacing the void* argument of
ioasid_alloc() with an mm_struct.

> struct ioasid_set *ioasid_alloc_set(void *token, ioasid_t quota)
> 
> 
> I was thinking we still have APIs for IOASID set alloc/free since we
> want to embed ioasid_set info in driver data structures for ownership
> check.
> 
> >   struct mm_struct *ioasid_find_mm(set, ioasid)
> I don't get why we need ioasid to find the set token. If we put
> mm_struct* inside ioasid_set, then we can get the token from the set
> directly.

Same here, this was replacing the void* returned by ioasid_find() with an
mm_struct.

> 
> >   ...
> > 
> > And ioasid_alloc(), ioasid_find(), etc would be internal to ioasid.c
> > and deal with IOASID_SET_TYPE_MM (or even be removed entirely for
> > now). New users that need different token types could then introduce
> > their own IOASID_SET_TYPE_* and use the lower-level functions.
> > 
> I will keep that in mind in my next set. I think it would be much
> easier to explain with code.
> 
> My takeaway is that we have a high-level agreement to have explicit mm
> as token in IOASID APIs. I think we can work out the details with
> patches.

Yes I think it would be easier to discuss with code.

Thanks,
Jean

_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: IOASID set token

[Jacob Pan]

Hi Jean,

All other points agreed.

On Tue, 7 Jul 2020 12:07:18 +0200
Jean-Philippe Brucker <jean-philippe@linaro.org> wrote:

> > > For the moment, though, we could actually specialize the IOASID
> > > API to only take an mm_struct as token.  
> > That would be fine with VT-d. We can use init_mm for host PASID set,
> > process mm for VM set.  
> 
> I'm not fond of using init_mm for the host PASID set. Does it need a
> token at all?
No need to use init_mm at all, probably can do without a token as well.
Just need to allocate a set for the native usage. I will give it a try.

Thanks,

Jacob
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu