RE: [PATCH v2 1/3] iommu/uapi: Define uapi version and capabilities
> From: Jacob Pan > Sent: Wednesday, April 15, 2020 11:39 PM > > On Tue, 14 Apr 2020 23:47:40 + > "Tian, Kevin" wrote: > > > > From: Jacob Pan > > > Sent: Wednesday, April 15, 2020 6:32 AM > > > > > > On Tue, 14 Apr 2020 10:13:04 -0700 > > > Jacob Pan wrote: > > > > > > > > > > In any of the proposed solutions, the > > > > > > > IOMMU driver is ultimately responsible for validating the > > > > > > > user data, so do we want vfio performing the > > > > > > > copy_from_user() to an object that could later be assumed > > > > > > > to be sanitized, or should vfio just pass a user pointer to > > > > > > > make it obvious that the consumer is responsible for all > > > > > > > the user protections? Seems like the latter. > > > > > > I like the latter as well. > > > > > > > > > On a second thought, I think the former is better. Two reasons: > > > > > > 1. IOMMU API such as page_response is also used in baremetal. So it > > > is not suitable to pass a __user *. > > > https://www.spinics.net/lists/arm-kernel/msg798677.html > > > > You can have a wrapped version accepting a __user* and an internal > > version for kernel pointers. > > > I have thought about that also but the problem is that some of the > flags are processed in the vendor IOMMU ops so it is hard to do that in > a generic wrapper. All vendor IOMMU ops are defined in the same header file, so they can be verified in one common IOMMU wrapper, just like how you dealt with it in VFIO originally... > > > > > > > 2. Some data are in the mandatory (fixed offset, never removed or > > > extended) portion of the uAPI structure. It is simpler for VFIO to > > > extract that and pass it to IOMMU API. For example, the PASID value > > > used for unbind_gpasid(). VFIO also need to sanitize the PASID > > > value to make sure it belongs to the same VM that did the > > > allocation. > > > > I don't think this makes much difference. If anyway you still plan to > > let IOMMU driver parse some user pointers, why not making a clear > > split to have it sparse all IOMMU specific fields? > > > The plan is not to have IOMMU driver parse user pointers. This is the > "former" case in Alex's comment. I.e. vfio performing the > copy_from_user based on argsz in IOMMU uAPI. > I'm confused. I thought Alex proposed the latter one: ---[quote] > So, __user * will be passed to IOMMU driver if VFIO checks minsz > include flags and they are valid. > IOMMU driver can copy the rest based on the mandatory version/minsz and > flags in the IOMMU uAPI structs. > Does it sound right? This is really choice #2. Sounds like each IOMMU UAPI struct just needs to have an embedded size and flags field, but yes. Thanks Kevin ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v2 1/3] iommu/uapi: Define uapi version and capabilities
On Tue, 14 Apr 2020 23:47:40 +
"Tian, Kevin" wrote:
> > From: Jacob Pan
> > Sent: Wednesday, April 15, 2020 6:32 AM
> >
> > On Tue, 14 Apr 2020 10:13:04 -0700
> > Jacob Pan wrote:
> >
> > > > > > In any of the proposed solutions, the
> > > > > > IOMMU driver is ultimately responsible for validating the
> > > > > > user data, so do we want vfio performing the
> > > > > > copy_from_user() to an object that could later be assumed
> > > > > > to be sanitized, or should vfio just pass a user pointer to
> > > > > > make it obvious that the consumer is responsible for all
> > > > > > the user protections? Seems like the latter.
> > > > > I like the latter as well.
> > > > >
> > On a second thought, I think the former is better. Two reasons:
> >
> > 1. IOMMU API such as page_response is also used in baremetal. So it
> > is not suitable to pass a __user *.
> > https://www.spinics.net/lists/arm-kernel/msg798677.html
>
> You can have a wrapped version accepting a __user* and an internal
> version for kernel pointers.
>
I have thought about that also but the problem is that some of the
flags are processed in the vendor IOMMU ops so it is hard to do that in
a generic wrapper.
> >
> > 2. Some data are in the mandatory (fixed offset, never removed or
> > extended) portion of the uAPI structure. It is simpler for VFIO to
> > extract that and pass it to IOMMU API. For example, the PASID value
> > used for unbind_gpasid(). VFIO also need to sanitize the PASID
> > value to make sure it belongs to the same VM that did the
> > allocation.
>
> I don't think this makes much difference. If anyway you still plan to
> let IOMMU driver parse some user pointers, why not making a clear
> split to have it sparse all IOMMU specific fields?
>
The plan is not to have IOMMU driver parse user pointers. This is the
"former" case in Alex's comment. I.e. vfio performing the
copy_from_user based on argsz in IOMMU uAPI.
> Thanks
> Kevin
>
> >
> >
> > > > > > That still really
> > > > > > doesn't address what's in that user data blob yet, but the
> > > > > > vfio interface could be:
> > > > > >
> > > > > > struct {
> > > > > > __u32 argsz;
> > > > > > __u32 flags;
> > > > > > __u8 data[];
> > > > > > }
> > > > > >
> > > > > > Where flags might be partitioned like we do for
> > > > > > DEVICE_FEATURE to indicate the format of data and what vfio
> > > > > > should do with it, and data might simply be defined as a
> > > > > > (__u64 __user *).
> > > > > So, __user * will be passed to IOMMU driver if VFIO checks
> > > > > minsz include flags and they are valid.
> > > > > IOMMU driver can copy the rest based on the mandatory
> > > > > version/minsz and flags in the IOMMU uAPI structs.
> > > > > Does it sound right? This is really choice #2.
> > > >
> > > > Sounds like each IOMMU UAPI struct just needs to have an
> > > > embedded size and flags field, but yes.
> > > >
> > > Yes, an argsz field can be added to each UAPI. There are already
> > > flags or the equivalent. IOMMU driver can process the __user *
> > > based on the argsz, flags, check argsz against
> > > offsetofend(iommu_uapi_struct, last_element), etc.;
[Jacob Pan]
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
RE: [PATCH v2 1/3] iommu/uapi: Define uapi version and capabilities
> From: Jacob Pan
> Sent: Wednesday, April 15, 2020 6:32 AM
>
> On Tue, 14 Apr 2020 10:13:04 -0700
> Jacob Pan wrote:
>
> > > > > In any of the proposed solutions, the
> > > > > IOMMU driver is ultimately responsible for validating the user
> > > > > data, so do we want vfio performing the copy_from_user() to an
> > > > > object that could later be assumed to be sanitized, or should
> > > > > vfio just pass a user pointer to make it obvious that the
> > > > > consumer is responsible for all the user protections? Seems
> > > > > like the latter.
> > > > I like the latter as well.
> > > >
> On a second thought, I think the former is better. Two reasons:
>
> 1. IOMMU API such as page_response is also used in baremetal. So it is
> not suitable to pass a __user *.
> https://www.spinics.net/lists/arm-kernel/msg798677.html
You can have a wrapped version accepting a __user* and an internal
version for kernel pointers.
>
> 2. Some data are in the mandatory (fixed offset, never removed or
> extended) portion of the uAPI structure. It is simpler for VFIO to
> extract that and pass it to IOMMU API. For example, the PASID value used
> for unbind_gpasid(). VFIO also need to sanitize the PASID value to make
> sure it belongs to the same VM that did the allocation.
I don't think this makes much difference. If anyway you still plan to
let IOMMU driver parse some user pointers, why not making a clear
split to have it sparse all IOMMU specific fields?
Thanks
Kevin
>
>
> > > > > That still really
> > > > > doesn't address what's in that user data blob yet, but the vfio
> > > > > interface could be:
> > > > >
> > > > > struct {
> > > > > __u32 argsz;
> > > > > __u32 flags;
> > > > > __u8 data[];
> > > > > }
> > > > >
> > > > > Where flags might be partitioned like we do for DEVICE_FEATURE
> > > > > to indicate the format of data and what vfio should do with it,
> > > > > and data might simply be defined as a (__u64 __user *).
> > > > >
> > > > So, __user * will be passed to IOMMU driver if VFIO checks minsz
> > > > include flags and they are valid.
> > > > IOMMU driver can copy the rest based on the mandatory
> > > > version/minsz and flags in the IOMMU uAPI structs.
> > > > Does it sound right? This is really choice #2.
> > >
> > > Sounds like each IOMMU UAPI struct just needs to have an embedded
> > > size and flags field, but yes.
> > >
> > Yes, an argsz field can be added to each UAPI. There are already flags
> > or the equivalent. IOMMU driver can process the __user * based on the
> > argsz, flags, check argsz against offsetofend(iommu_uapi_struct,
> > last_element), etc.;
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v2 1/3] iommu/uapi: Define uapi version and capabilities
On Tue, 14 Apr 2020 10:13:04 -0700
Jacob Pan wrote:
> > > > In any of the proposed solutions, the
> > > > IOMMU driver is ultimately responsible for validating the user
> > > > data, so do we want vfio performing the copy_from_user() to an
> > > > object that could later be assumed to be sanitized, or should
> > > > vfio just pass a user pointer to make it obvious that the
> > > > consumer is responsible for all the user protections? Seems
> > > > like the latter.
> > > I like the latter as well.
> > >
On a second thought, I think the former is better. Two reasons:
1. IOMMU API such as page_response is also used in baremetal. So it is
not suitable to pass a __user *.
https://www.spinics.net/lists/arm-kernel/msg798677.html
2. Some data are in the mandatory (fixed offset, never removed or
extended) portion of the uAPI structure. It is simpler for VFIO to
extract that and pass it to IOMMU API. For example, the PASID value used
for unbind_gpasid(). VFIO also need to sanitize the PASID value to make
sure it belongs to the same VM that did the allocation.
> > > > That still really
> > > > doesn't address what's in that user data blob yet, but the vfio
> > > > interface could be:
> > > >
> > > > struct {
> > > > __u32 argsz;
> > > > __u32 flags;
> > > > __u8 data[];
> > > > }
> > > >
> > > > Where flags might be partitioned like we do for DEVICE_FEATURE
> > > > to indicate the format of data and what vfio should do with it,
> > > > and data might simply be defined as a (__u64 __user *).
> > > >
> > > So, __user * will be passed to IOMMU driver if VFIO checks minsz
> > > include flags and they are valid.
> > > IOMMU driver can copy the rest based on the mandatory
> > > version/minsz and flags in the IOMMU uAPI structs.
> > > Does it sound right? This is really choice #2.
> >
> > Sounds like each IOMMU UAPI struct just needs to have an embedded
> > size and flags field, but yes.
> >
> Yes, an argsz field can be added to each UAPI. There are already flags
> or the equivalent. IOMMU driver can process the __user * based on the
> argsz, flags, check argsz against offsetofend(iommu_uapi_struct,
> last_element), etc.;
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v2 1/3] iommu/uapi: Define uapi version and capabilities
On Tue, 14 Apr 2020 10:13:58 -0600
Alex Williamson wrote:
> On Mon, 13 Apr 2020 22:05:15 -0700
> Jacob Pan wrote:
>
> > Hi Alex,
> > Thanks a lot for the feedback, my comments inline.
> >
> > On Mon, 13 Apr 2020 16:21:29 -0600
> > Alex Williamson wrote:
> >
> > > On Mon, 13 Apr 2020 13:41:57 -0700
> > > Jacob Pan wrote:
> > >
> > > > Hi All,
> > > >
> > > > Just a gentle reminder, any feedback on the options I listed
> > > > below? New ideas will be even better.
> > > >
> > > > Christoph, does the explanation make sense to you? We do have
> > > > the capability/flag based scheme for IOMMU API extension, the
> > > > version is mainly used for size lookup. Compatibility checking
> > > > is another use of the version, it makes checking easy when a
> > > > vIOMMU is launched.
> > > >
> > > > Thanks,
> > > >
> > > > Jacob
> > > >
> > > > On Thu, 2 Apr 2020 11:36:04 -0700
> > > > Jacob Pan wrote:
> > > >
> > > > > On Wed, 1 Apr 2020 05:32:21 +
> > > > > "Tian, Kevin" wrote:
> > > > >
> > > > > > > From: Jacob Pan
> > > > > > > Sent: Tuesday, March 31, 2020 11:55 PM
> > > > > > >
> > > > > > > On Tue, 31 Mar 2020 06:06:38 +
> > > > > > > "Tian, Kevin" wrote:
> > > > > > >
> > > > > > > > > From: Jacob Pan
> > > > > > > > > Sent: Tuesday, March 31, 2020 12:08 AM
> > > > > > > > >
> > > > > > > > > On Mon, 30 Mar 2020 05:40:40 +
> > > > > > > > > "Tian, Kevin" wrote:
> > > > > > > > >
> > > > > > > > > > > From: Jacob Pan
> > > > > > > > > > > Sent: Saturday, March 28, 2020 7:54 AM
> > > > > > > > > > >
> > > > > > > > > > > On Fri, 27 Mar 2020 00:47:02 -0700
> > > > > > > > > > > Christoph Hellwig wrote:
> > > > > > > > > > >
> > > > > > > > > > > > On Fri, Mar 27, 2020 at 02:49:55AM +, Tian,
> > > > > > > > > > > > Kevin wrote:
> > > > > > > > > > > > > If those API calls are inter-dependent for
> > > > > > > > > > > > > composing a feature (e.g. SVA), shouldn't we
> > > > > > > > > > > > > need a way to check them together before
> > > > > > > > > > > > > exposing the feature to the guest, e.g.
> > > > > > > > > > > > > through a iommu_get_uapi_capabilities
> > > > > > > > > > > > > interface?
> > > > > > > > > > > >
> > > > > > > > > > > > Yes, that makes sense. The important bit is to
> > > > > > > > > > > > have a capability flags and not version
> > > > > > > > > > > > numbers.
> > > > > > > > > > >
> > > > > > > > > > > The challenge is that there are two consumers in
> > > > > > > > > > > the kernel for this. 1. VFIO only look for
> > > > > > > > > > > compatibility, and size of each data struct such
> > > > > > > > > > > that it can copy_from_user.
> > > > > > > > > > >
> > > > > > > > > > > 2. IOMMU driver, the "real consumer" of the
> > > > > > > > > > > content.
> > > > > > > > > > >
> > > > > > > > > > > For 2, I agree and we do plan to use the
> > > > > > > > > > > capability flags to check content and maintain
> > > > > > > > > > > backward compatibility etc.
> > > > > > > > > > >
> > > > > > > > > > > For VFIO, it is difficult to do size look up
> > > > > > > > > > > based on capability flags.
> > > > > > > > > >
> > > > > > > > > > Can you elaborate the difficulty in VFIO? if, as
> > > > > > > > > > Christoph Hellwig pointed out, version number is
> > > > > > > > > > already avoided everywhere, it is interesting to
> > > > > > > > > > know whether this work becomes a real exception or
> > > > > > > > > > just requires a different mindset.
> > > > > > > > > From VFIO p.o.v. the IOMMU UAPI data is opaque, it
> > > > > > > > > only needs to do two things:
> > > > > > > > > 1. is the UAPI compatible?
> > > > > > > > > 2. what is the size to copy?
> > > > > > > > >
> > > > > > > > > If you look at the version number, this is really a
> > > > > > > > > "version as size" lookup, as provided by the helper
> > > > > > > > > function in this patch. An example can be the newly
> > > > > > > > > introduced clone3 syscall.
> > > > > > > > > https://lwn.net/Articles/792628/ In clone3, new
> > > > > > > > > version must have new size. The slight difference
> > > > > > > > > here is that, unlike clone3, we have multiple data
> > > > > > > > > structures instead of a single struct clone_args {}.
> > > > > > > > > And each struct has flags to enumerate its contents
> > > > > > > > > besides size.
> > > > > > > >
> > > > > > > > Thanks for providing that link. However clone3 doesn't
> > > > > > > > include a version field to do "version as size" lookup.
> > > > > > > > Instead, as you said, it includes a size parameter which
> > > > > > > > sounds like the option 3 (argsz) listed below.
> > > > > > > >
> > > > > > > Right, there is no version in clone3. size = version. I
> > > > > > > view this as a 1:1 lookup.
> > > > > > >
> > > > > > > > >
> > > > > > > > > Besides breaching data abstraction, if VFIO has to
> > > > > > > >
Re: [PATCH v2 1/3] iommu/uapi: Define uapi version and capabilities
On Mon, 13 Apr 2020 22:05:15 -0700
Jacob Pan wrote:
> Hi Alex,
> Thanks a lot for the feedback, my comments inline.
>
> On Mon, 13 Apr 2020 16:21:29 -0600
> Alex Williamson wrote:
>
> > On Mon, 13 Apr 2020 13:41:57 -0700
> > Jacob Pan wrote:
> >
> > > Hi All,
> > >
> > > Just a gentle reminder, any feedback on the options I listed below?
> > > New ideas will be even better.
> > >
> > > Christoph, does the explanation make sense to you? We do have the
> > > capability/flag based scheme for IOMMU API extension, the version is
> > > mainly used for size lookup. Compatibility checking is another use
> > > of the version, it makes checking easy when a vIOMMU is launched.
> > >
> > > Thanks,
> > >
> > > Jacob
> > >
> > > On Thu, 2 Apr 2020 11:36:04 -0700
> > > Jacob Pan wrote:
> > >
> > > > On Wed, 1 Apr 2020 05:32:21 +
> > > > "Tian, Kevin" wrote:
> > > >
> > > > > > From: Jacob Pan
> > > > > > Sent: Tuesday, March 31, 2020 11:55 PM
> > > > > >
> > > > > > On Tue, 31 Mar 2020 06:06:38 +
> > > > > > "Tian, Kevin" wrote:
> > > > > >
> > > > > > > > From: Jacob Pan
> > > > > > > > Sent: Tuesday, March 31, 2020 12:08 AM
> > > > > > > >
> > > > > > > > On Mon, 30 Mar 2020 05:40:40 +
> > > > > > > > "Tian, Kevin" wrote:
> > > > > > > >
> > > > > > > > > > From: Jacob Pan
> > > > > > > > > > Sent: Saturday, March 28, 2020 7:54 AM
> > > > > > > > > >
> > > > > > > > > > On Fri, 27 Mar 2020 00:47:02 -0700
> > > > > > > > > > Christoph Hellwig wrote:
> > > > > > > > > >
> > > > > > > > > > > On Fri, Mar 27, 2020 at 02:49:55AM +, Tian,
> > > > > > > > > > > Kevin wrote:
> > > > > > > > > > > > If those API calls are inter-dependent for
> > > > > > > > > > > > composing a feature (e.g. SVA), shouldn't we need
> > > > > > > > > > > > a way to check them together before exposing the
> > > > > > > > > > > > feature to the guest, e.g. through a
> > > > > > > > > > > > iommu_get_uapi_capabilities interface?
> > > > > > > > > > >
> > > > > > > > > > > Yes, that makes sense. The important bit is to
> > > > > > > > > > > have a capability flags and not version
> > > > > > > > > > > numbers.
> > > > > > > > > >
> > > > > > > > > > The challenge is that there are two consumers in the
> > > > > > > > > > kernel for this. 1. VFIO only look for compatibility,
> > > > > > > > > > and size of each data struct such that it can
> > > > > > > > > > copy_from_user.
> > > > > > > > > >
> > > > > > > > > > 2. IOMMU driver, the "real consumer" of the content.
> > > > > > > > > >
> > > > > > > > > > For 2, I agree and we do plan to use the capability
> > > > > > > > > > flags to check content and maintain backward
> > > > > > > > > > compatibility etc.
> > > > > > > > > >
> > > > > > > > > > For VFIO, it is difficult to do size look up based on
> > > > > > > > > > capability flags.
> > > > > > > > >
> > > > > > > > > Can you elaborate the difficulty in VFIO? if, as
> > > > > > > > > Christoph Hellwig pointed out, version number is
> > > > > > > > > already avoided everywhere, it is interesting to know
> > > > > > > > > whether this work becomes a real exception or just
> > > > > > > > > requires a different mindset.
> > > > > > > > From VFIO p.o.v. the IOMMU UAPI data is opaque, it only
> > > > > > > > needs to do two things:
> > > > > > > > 1. is the UAPI compatible?
> > > > > > > > 2. what is the size to copy?
> > > > > > > >
> > > > > > > > If you look at the version number, this is really a
> > > > > > > > "version as size" lookup, as provided by the helper
> > > > > > > > function in this patch. An example can be the newly
> > > > > > > > introduced clone3 syscall.
> > > > > > > > https://lwn.net/Articles/792628/ In clone3, new version
> > > > > > > > must have new size. The slight difference here is that,
> > > > > > > > unlike clone3, we have multiple data structures instead
> > > > > > > > of a single struct clone_args {}. And each struct has
> > > > > > > > flags to enumerate its contents besides size.
> > > > > > >
> > > > > > > Thanks for providing that link. However clone3 doesn't
> > > > > > > include a version field to do "version as size" lookup.
> > > > > > > Instead, as you said, it includes a size parameter which
> > > > > > > sounds like the option 3 (argsz) listed below.
> > > > > > >
> > > > > > Right, there is no version in clone3. size = version. I view
> > > > > > this as a 1:1 lookup.
> > > > > >
> > > > > > > >
> > > > > > > > Besides breaching data abstraction, if VFIO has to check
> > > > > > > > IOMMU flags to determine the sizes, it has many
> > > > > > > > combinations.
> > > > > > > >
> > > > > > > > We also separate the responsibilities into two parts
> > > > > > > > 1. compatibility - version, size by VFIO
> > > > > > > > 2. sanity check - capability flags - by IOMMU
> > > > > > >
> > > > > > > I feel argsz+flags approach can perfectly meet
Re: [PATCH v2 1/3] iommu/uapi: Define uapi version and capabilities
On Tue, 14 Apr 2020 01:11:07 -0700 Christoph Hellwig wrote: > On Mon, Apr 13, 2020 at 01:41:57PM -0700, Jacob Pan wrote: > > Hi All, > > > > Just a gentle reminder, any feedback on the options I listed below? > > New ideas will be even better. > > > > Christoph, does the explanation make sense to you? We do have the > > capability/flag based scheme for IOMMU API extension, the version is > > mainly used for size lookup. Compatibility checking is another use > > of the version, it makes checking easy when a vIOMMU is launched. > > No. If you truely need different versions use different ioctl > identifiers. OK. I will drop the global version and keep the current per API/IOCTL struct. > If it really is just the size pass the size and not a > version. OK, I think we have a path forward. I will remove the size-version lookup. My concern was that since we cannot trust the size provided by userspace. We must sanity check the argsz based on knowledge of the struct within the kernel. AFAIK, VFIO does this check by looking at offsetofend(user_struct, last_element). But in this case, VFIO is not the end consumer, and last_element can be a variable size union. So we'd better let IOMMU driver deal with it. ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v2 1/3] iommu/uapi: Define uapi version and capabilities
On Mon, Apr 13, 2020 at 04:21:29PM -0600, Alex Williamson wrote: > Is the objection to a global version or to any version fields? I don't > really understand the global version, I'd think a mechanism to check > extensions plus a per structure flags/version would be preferred. The > former should resolve how userspace can test support for features > requiring multiple interfaces. A global version also implies that > we're only ever adding features and never removing. For example, > feature Foo is added in version 4, but it's replaced by feature Bar in > version 5, now userspace can't simply test version >= 4 must include > feature Foo. The objection is to versions vs the much more sensible struct size + capability flags. Making it global just increases the problems with a version for all of the above reasons. > It seems to me that version and flags can also be complimentary, for > example a field might be defined by a version but a flag could indicate > if it's implemented. With only the flag, we'd infer the field from the > flag, with only the version we'd need to assume the field is always > implemented. So I have a hard time making a blanket statement that all > versions fields should be avoided. s/version/struct size/, but otherwise agreed. ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v2 1/3] iommu/uapi: Define uapi version and capabilities
On Mon, Apr 13, 2020 at 01:41:57PM -0700, Jacob Pan wrote: > Hi All, > > Just a gentle reminder, any feedback on the options I listed below? New > ideas will be even better. > > Christoph, does the explanation make sense to you? We do have the > capability/flag based scheme for IOMMU API extension, the version is > mainly used for size lookup. Compatibility checking is another use of > the version, it makes checking easy when a vIOMMU is launched. No. If you truely need different versions use different ioctl identifiers. If it really is just the size pass the size and not a version. ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v2 1/3] iommu/uapi: Define uapi version and capabilities
Hi Alex,
Thanks a lot for the feedback, my comments inline.
On Mon, 13 Apr 2020 16:21:29 -0600
Alex Williamson wrote:
> On Mon, 13 Apr 2020 13:41:57 -0700
> Jacob Pan wrote:
>
> > Hi All,
> >
> > Just a gentle reminder, any feedback on the options I listed below?
> > New ideas will be even better.
> >
> > Christoph, does the explanation make sense to you? We do have the
> > capability/flag based scheme for IOMMU API extension, the version is
> > mainly used for size lookup. Compatibility checking is another use
> > of the version, it makes checking easy when a vIOMMU is launched.
> >
> > Thanks,
> >
> > Jacob
> >
> > On Thu, 2 Apr 2020 11:36:04 -0700
> > Jacob Pan wrote:
> >
> > > On Wed, 1 Apr 2020 05:32:21 +
> > > "Tian, Kevin" wrote:
> > >
> > > > > From: Jacob Pan
> > > > > Sent: Tuesday, March 31, 2020 11:55 PM
> > > > >
> > > > > On Tue, 31 Mar 2020 06:06:38 +
> > > > > "Tian, Kevin" wrote:
> > > > >
> > > > > > > From: Jacob Pan
> > > > > > > Sent: Tuesday, March 31, 2020 12:08 AM
> > > > > > >
> > > > > > > On Mon, 30 Mar 2020 05:40:40 +
> > > > > > > "Tian, Kevin" wrote:
> > > > > > >
> > > > > > > > > From: Jacob Pan
> > > > > > > > > Sent: Saturday, March 28, 2020 7:54 AM
> > > > > > > > >
> > > > > > > > > On Fri, 27 Mar 2020 00:47:02 -0700
> > > > > > > > > Christoph Hellwig wrote:
> > > > > > > > >
> > > > > > > > > > On Fri, Mar 27, 2020 at 02:49:55AM +, Tian,
> > > > > > > > > > Kevin wrote:
> > > > > > > > > > > If those API calls are inter-dependent for
> > > > > > > > > > > composing a feature (e.g. SVA), shouldn't we need
> > > > > > > > > > > a way to check them together before exposing the
> > > > > > > > > > > feature to the guest, e.g. through a
> > > > > > > > > > > iommu_get_uapi_capabilities interface?
> > > > > > > > > >
> > > > > > > > > > Yes, that makes sense. The important bit is to
> > > > > > > > > > have a capability flags and not version
> > > > > > > > > > numbers.
> > > > > > > > >
> > > > > > > > > The challenge is that there are two consumers in the
> > > > > > > > > kernel for this. 1. VFIO only look for compatibility,
> > > > > > > > > and size of each data struct such that it can
> > > > > > > > > copy_from_user.
> > > > > > > > >
> > > > > > > > > 2. IOMMU driver, the "real consumer" of the content.
> > > > > > > > >
> > > > > > > > > For 2, I agree and we do plan to use the capability
> > > > > > > > > flags to check content and maintain backward
> > > > > > > > > compatibility etc.
> > > > > > > > >
> > > > > > > > > For VFIO, it is difficult to do size look up based on
> > > > > > > > > capability flags.
> > > > > > > >
> > > > > > > > Can you elaborate the difficulty in VFIO? if, as
> > > > > > > > Christoph Hellwig pointed out, version number is
> > > > > > > > already avoided everywhere, it is interesting to know
> > > > > > > > whether this work becomes a real exception or just
> > > > > > > > requires a different mindset.
> > > > > > > From VFIO p.o.v. the IOMMU UAPI data is opaque, it only
> > > > > > > needs to do two things:
> > > > > > > 1. is the UAPI compatible?
> > > > > > > 2. what is the size to copy?
> > > > > > >
> > > > > > > If you look at the version number, this is really a
> > > > > > > "version as size" lookup, as provided by the helper
> > > > > > > function in this patch. An example can be the newly
> > > > > > > introduced clone3 syscall.
> > > > > > > https://lwn.net/Articles/792628/ In clone3, new version
> > > > > > > must have new size. The slight difference here is that,
> > > > > > > unlike clone3, we have multiple data structures instead
> > > > > > > of a single struct clone_args {}. And each struct has
> > > > > > > flags to enumerate its contents besides size.
> > > > > >
> > > > > > Thanks for providing that link. However clone3 doesn't
> > > > > > include a version field to do "version as size" lookup.
> > > > > > Instead, as you said, it includes a size parameter which
> > > > > > sounds like the option 3 (argsz) listed below.
> > > > > >
> > > > > Right, there is no version in clone3. size = version. I view
> > > > > this as a 1:1 lookup.
> > > > >
> > > > > > >
> > > > > > > Besides breaching data abstraction, if VFIO has to check
> > > > > > > IOMMU flags to determine the sizes, it has many
> > > > > > > combinations.
> > > > > > >
> > > > > > > We also separate the responsibilities into two parts
> > > > > > > 1. compatibility - version, size by VFIO
> > > > > > > 2. sanity check - capability flags - by IOMMU
> > > > > >
> > > > > > I feel argsz+flags approach can perfectly meet above
> > > > > > requirement. The userspace set the size and flags for
> > > > > > whatever capabilities it uses, and VFIO simply copies the
> > > > > > parameters by size and pass to IOMMU for further sanity
> > > > > > check. Of course the assumption is that we do provide an
> > > > > > interface for
Re: [PATCH v2 1/3] iommu/uapi: Define uapi version and capabilities
On Mon, 13 Apr 2020 13:41:57 -0700
Jacob Pan wrote:
> Hi All,
>
> Just a gentle reminder, any feedback on the options I listed below? New
> ideas will be even better.
>
> Christoph, does the explanation make sense to you? We do have the
> capability/flag based scheme for IOMMU API extension, the version is
> mainly used for size lookup. Compatibility checking is another use of
> the version, it makes checking easy when a vIOMMU is launched.
>
> Thanks,
>
> Jacob
>
> On Thu, 2 Apr 2020 11:36:04 -0700
> Jacob Pan wrote:
>
> > On Wed, 1 Apr 2020 05:32:21 +
> > "Tian, Kevin" wrote:
> >
> > > > From: Jacob Pan
> > > > Sent: Tuesday, March 31, 2020 11:55 PM
> > > >
> > > > On Tue, 31 Mar 2020 06:06:38 +
> > > > "Tian, Kevin" wrote:
> > > >
> > > > > > From: Jacob Pan
> > > > > > Sent: Tuesday, March 31, 2020 12:08 AM
> > > > > >
> > > > > > On Mon, 30 Mar 2020 05:40:40 +
> > > > > > "Tian, Kevin" wrote:
> > > > > >
> > > > > > > > From: Jacob Pan
> > > > > > > > Sent: Saturday, March 28, 2020 7:54 AM
> > > > > > > >
> > > > > > > > On Fri, 27 Mar 2020 00:47:02 -0700
> > > > > > > > Christoph Hellwig wrote:
> > > > > > > >
> > > > > > > > > On Fri, Mar 27, 2020 at 02:49:55AM +, Tian, Kevin
> > > > > > > > > wrote:
> > > > > > > > > > If those API calls are inter-dependent for composing a
> > > > > > > > > > feature (e.g. SVA), shouldn't we need a way to check
> > > > > > > > > > them together before exposing the feature to the
> > > > > > > > > > guest, e.g. through a iommu_get_uapi_capabilities
> > > > > > > > > > interface?
> > > > > > > > >
> > > > > > > > > Yes, that makes sense. The important bit is to have a
> > > > > > > > > capability flags and not version numbers.
> > > > > > > >
> > > > > > > > The challenge is that there are two consumers in the
> > > > > > > > kernel for this. 1. VFIO only look for compatibility, and
> > > > > > > > size of each data struct such that it can copy_from_user.
> > > > > > > >
> > > > > > > > 2. IOMMU driver, the "real consumer" of the content.
> > > > > > > >
> > > > > > > > For 2, I agree and we do plan to use the capability flags
> > > > > > > > to check content and maintain backward compatibility etc.
> > > > > > > >
> > > > > > > > For VFIO, it is difficult to do size look up based on
> > > > > > > > capability flags.
> > > > > > >
> > > > > > > Can you elaborate the difficulty in VFIO? if, as Christoph
> > > > > > > Hellwig pointed out, version number is already avoided
> > > > > > > everywhere, it is interesting to know whether this work
> > > > > > > becomes a real exception or just requires a different
> > > > > > > mindset.
> > > > > > From VFIO p.o.v. the IOMMU UAPI data is opaque, it only needs
> > > > > > to do two things:
> > > > > > 1. is the UAPI compatible?
> > > > > > 2. what is the size to copy?
> > > > > >
> > > > > > If you look at the version number, this is really a "version
> > > > > > as size" lookup, as provided by the helper function in this
> > > > > > patch. An example can be the newly introduced clone3 syscall.
> > > > > > https://lwn.net/Articles/792628/
> > > > > > In clone3, new version must have new size. The slight
> > > > > > difference here is that, unlike clone3, we have multiple data
> > > > > > structures instead of a single struct clone_args {}. And each
> > > > > > struct has flags to enumerate its contents besides size.
> > > > >
> > > > > Thanks for providing that link. However clone3 doesn't include a
> > > > > version field to do "version as size" lookup. Instead, as you
> > > > > said, it includes a size parameter which sounds like the option
> > > > > 3 (argsz) listed below.
> > > > >
> > > > Right, there is no version in clone3. size = version. I view this
> > > > as a 1:1 lookup.
> > > >
> > > > > >
> > > > > > Besides breaching data abstraction, if VFIO has to check IOMMU
> > > > > > flags to determine the sizes, it has many combinations.
> > > > > >
> > > > > > We also separate the responsibilities into two parts
> > > > > > 1. compatibility - version, size by VFIO
> > > > > > 2. sanity check - capability flags - by IOMMU
> > > > >
> > > > > I feel argsz+flags approach can perfectly meet above
> > > > > requirement. The userspace set the size and flags for whatever
> > > > > capabilities it uses, and VFIO simply copies the parameters by
> > > > > size and pass to IOMMU for further sanity check. Of course the
> > > > > assumption is that we do provide an interface for userspace to
> > > > > enumerate all supported capabilities.
> > > > You cannot trust user for argsz. the size to be copied from user
> > > > must be based on knowledge in kernel. That is why we have this
> > > > version to size lookup.
> > > >
> > > > In VFIO, the size to copy is based on knowledge of each VFIO UAPI
> > > > structures and VFIO flags. But here the flags are IOMMU UAPI
> > > > flags. As you pointed out in another thread, VFIO is
Re: [PATCH v2 1/3] iommu/uapi: Define uapi version and capabilities
Hi All,
Just a gentle reminder, any feedback on the options I listed below? New
ideas will be even better.
Christoph, does the explanation make sense to you? We do have the
capability/flag based scheme for IOMMU API extension, the version is
mainly used for size lookup. Compatibility checking is another use of
the version, it makes checking easy when a vIOMMU is launched.
Thanks,
Jacob
On Thu, 2 Apr 2020 11:36:04 -0700
Jacob Pan wrote:
> On Wed, 1 Apr 2020 05:32:21 +
> "Tian, Kevin" wrote:
>
> > > From: Jacob Pan
> > > Sent: Tuesday, March 31, 2020 11:55 PM
> > >
> > > On Tue, 31 Mar 2020 06:06:38 +
> > > "Tian, Kevin" wrote:
> > >
> > > > > From: Jacob Pan
> > > > > Sent: Tuesday, March 31, 2020 12:08 AM
> > > > >
> > > > > On Mon, 30 Mar 2020 05:40:40 +
> > > > > "Tian, Kevin" wrote:
> > > > >
> > > > > > > From: Jacob Pan
> > > > > > > Sent: Saturday, March 28, 2020 7:54 AM
> > > > > > >
> > > > > > > On Fri, 27 Mar 2020 00:47:02 -0700
> > > > > > > Christoph Hellwig wrote:
> > > > > > >
> > > > > > > > On Fri, Mar 27, 2020 at 02:49:55AM +, Tian, Kevin
> > > > > > > > wrote:
> > > > > > > > > If those API calls are inter-dependent for composing a
> > > > > > > > > feature (e.g. SVA), shouldn't we need a way to check
> > > > > > > > > them together before exposing the feature to the
> > > > > > > > > guest, e.g. through a iommu_get_uapi_capabilities
> > > > > > > > > interface?
> > > > > > > >
> > > > > > > > Yes, that makes sense. The important bit is to have a
> > > > > > > > capability flags and not version numbers.
> > > > > > >
> > > > > > > The challenge is that there are two consumers in the
> > > > > > > kernel for this. 1. VFIO only look for compatibility, and
> > > > > > > size of each data struct such that it can copy_from_user.
> > > > > > >
> > > > > > > 2. IOMMU driver, the "real consumer" of the content.
> > > > > > >
> > > > > > > For 2, I agree and we do plan to use the capability flags
> > > > > > > to check content and maintain backward compatibility etc.
> > > > > > >
> > > > > > > For VFIO, it is difficult to do size look up based on
> > > > > > > capability flags.
> > > > > >
> > > > > > Can you elaborate the difficulty in VFIO? if, as Christoph
> > > > > > Hellwig pointed out, version number is already avoided
> > > > > > everywhere, it is interesting to know whether this work
> > > > > > becomes a real exception or just requires a different
> > > > > > mindset.
> > > > > From VFIO p.o.v. the IOMMU UAPI data is opaque, it only needs
> > > > > to do two things:
> > > > > 1. is the UAPI compatible?
> > > > > 2. what is the size to copy?
> > > > >
> > > > > If you look at the version number, this is really a "version
> > > > > as size" lookup, as provided by the helper function in this
> > > > > patch. An example can be the newly introduced clone3 syscall.
> > > > > https://lwn.net/Articles/792628/
> > > > > In clone3, new version must have new size. The slight
> > > > > difference here is that, unlike clone3, we have multiple data
> > > > > structures instead of a single struct clone_args {}. And each
> > > > > struct has flags to enumerate its contents besides size.
> > > >
> > > > Thanks for providing that link. However clone3 doesn't include a
> > > > version field to do "version as size" lookup. Instead, as you
> > > > said, it includes a size parameter which sounds like the option
> > > > 3 (argsz) listed below.
> > > >
> > > Right, there is no version in clone3. size = version. I view this
> > > as a 1:1 lookup.
> > >
> > > > >
> > > > > Besides breaching data abstraction, if VFIO has to check IOMMU
> > > > > flags to determine the sizes, it has many combinations.
> > > > >
> > > > > We also separate the responsibilities into two parts
> > > > > 1. compatibility - version, size by VFIO
> > > > > 2. sanity check - capability flags - by IOMMU
> > > >
> > > > I feel argsz+flags approach can perfectly meet above
> > > > requirement. The userspace set the size and flags for whatever
> > > > capabilities it uses, and VFIO simply copies the parameters by
> > > > size and pass to IOMMU for further sanity check. Of course the
> > > > assumption is that we do provide an interface for userspace to
> > > > enumerate all supported capabilities.
> > > You cannot trust user for argsz. the size to be copied from user
> > > must be based on knowledge in kernel. That is why we have this
> > > version to size lookup.
> > >
> > > In VFIO, the size to copy is based on knowledge of each VFIO UAPI
> > > structures and VFIO flags. But here the flags are IOMMU UAPI
> > > flags. As you pointed out in another thread, VFIO is one user.
> >
> > If that is the case, can we let VFIO only copy its own UAPI fields
> > while simply passing the user pointer of IOMMU UAPI structure to
> > IOMMU driver for further size check and copy? Otherwise we are
> > entering a dead end that VFIO doesn't want to parse a structure
> >
Re: [PATCH v2 1/3] iommu/uapi: Define uapi version and capabilities
On Wed, 1 Apr 2020 05:32:21 +
"Tian, Kevin" wrote:
> > From: Jacob Pan
> > Sent: Tuesday, March 31, 2020 11:55 PM
> >
> > On Tue, 31 Mar 2020 06:06:38 +
> > "Tian, Kevin" wrote:
> >
> > > > From: Jacob Pan
> > > > Sent: Tuesday, March 31, 2020 12:08 AM
> > > >
> > > > On Mon, 30 Mar 2020 05:40:40 +
> > > > "Tian, Kevin" wrote:
> > > >
> > > > > > From: Jacob Pan
> > > > > > Sent: Saturday, March 28, 2020 7:54 AM
> > > > > >
> > > > > > On Fri, 27 Mar 2020 00:47:02 -0700
> > > > > > Christoph Hellwig wrote:
> > > > > >
> > > > > > > On Fri, Mar 27, 2020 at 02:49:55AM +, Tian, Kevin
> > > > > > > wrote:
> > > > > > > > If those API calls are inter-dependent for composing a
> > > > > > > > feature (e.g. SVA), shouldn't we need a way to check
> > > > > > > > them together before exposing the feature to the guest,
> > > > > > > > e.g. through a iommu_get_uapi_capabilities interface?
> > > > > > >
> > > > > > > Yes, that makes sense. The important bit is to have a
> > > > > > > capability flags and not version numbers.
> > > > > >
> > > > > > The challenge is that there are two consumers in the kernel
> > > > > > for this. 1. VFIO only look for compatibility, and size of
> > > > > > each data struct such that it can copy_from_user.
> > > > > >
> > > > > > 2. IOMMU driver, the "real consumer" of the content.
> > > > > >
> > > > > > For 2, I agree and we do plan to use the capability flags to
> > > > > > check content and maintain backward compatibility etc.
> > > > > >
> > > > > > For VFIO, it is difficult to do size look up based on
> > > > > > capability flags.
> > > > >
> > > > > Can you elaborate the difficulty in VFIO? if, as Christoph
> > > > > Hellwig pointed out, version number is already avoided
> > > > > everywhere, it is interesting to know whether this work
> > > > > becomes a real exception or just requires a different mindset.
> > > > >
> > > > From VFIO p.o.v. the IOMMU UAPI data is opaque, it only needs
> > > > to do two things:
> > > > 1. is the UAPI compatible?
> > > > 2. what is the size to copy?
> > > >
> > > > If you look at the version number, this is really a "version as
> > > > size" lookup, as provided by the helper function in this patch.
> > > > An example can be the newly introduced clone3 syscall.
> > > > https://lwn.net/Articles/792628/
> > > > In clone3, new version must have new size. The slight difference
> > > > here is that, unlike clone3, we have multiple data structures
> > > > instead of a single struct clone_args {}. And each struct has
> > > > flags to enumerate its contents besides size.
> > >
> > > Thanks for providing that link. However clone3 doesn't include a
> > > version field to do "version as size" lookup. Instead, as you
> > > said, it includes a size parameter which sounds like the option 3
> > > (argsz) listed below.
> > >
> > Right, there is no version in clone3. size = version. I view this as
> > a 1:1 lookup.
> >
> > > >
> > > > Besides breaching data abstraction, if VFIO has to check IOMMU
> > > > flags to determine the sizes, it has many combinations.
> > > >
> > > > We also separate the responsibilities into two parts
> > > > 1. compatibility - version, size by VFIO
> > > > 2. sanity check - capability flags - by IOMMU
> > >
> > > I feel argsz+flags approach can perfectly meet above requirement.
> > > The userspace set the size and flags for whatever capabilities it
> > > uses, and VFIO simply copies the parameters by size and pass to
> > > IOMMU for further sanity check. Of course the assumption is that
> > > we do provide an interface for userspace to enumerate all
> > > supported capabilities.
> > You cannot trust user for argsz. the size to be copied from user
> > must be based on knowledge in kernel. That is why we have this
> > version to size lookup.
> >
> > In VFIO, the size to copy is based on knowledge of each VFIO UAPI
> > structures and VFIO flags. But here the flags are IOMMU UAPI flags.
> > As you pointed out in another thread, VFIO is one user.
>
> If that is the case, can we let VFIO only copy its own UAPI fields
> while simply passing the user pointer of IOMMU UAPI structure to IOMMU
> driver for further size check and copy? Otherwise we are entering a
> dead end that VFIO doesn't want to parse a structure which is not
> defined by him while using version to represent the black box size
> is considered as a discarded scheme and doesn't scale well...
>
I think this could be an other viable option. Let me try to summarize
since this has been a long discussion since the original version.
Problem statements:
1. When launching vIOMMU in the guest, how can we ensure the host has
compatible support upfront? as compared to fail later.
2. As UAPI data gets extended (both in size and flags), how can we know
the size to copy
3. Maintain backward compatibility while allowing extensions?
I think we all agreed that using flags (capability or types) is the way
to address #3. As Christoph
RE: [PATCH v2 1/3] iommu/uapi: Define uapi version and capabilities
> From: Jacob Pan
> Sent: Tuesday, March 31, 2020 11:55 PM
>
> On Tue, 31 Mar 2020 06:06:38 +
> "Tian, Kevin" wrote:
>
> > > From: Jacob Pan
> > > Sent: Tuesday, March 31, 2020 12:08 AM
> > >
> > > On Mon, 30 Mar 2020 05:40:40 +
> > > "Tian, Kevin" wrote:
> > >
> > > > > From: Jacob Pan
> > > > > Sent: Saturday, March 28, 2020 7:54 AM
> > > > >
> > > > > On Fri, 27 Mar 2020 00:47:02 -0700
> > > > > Christoph Hellwig wrote:
> > > > >
> > > > > > On Fri, Mar 27, 2020 at 02:49:55AM +, Tian, Kevin wrote:
> > > > > > > If those API calls are inter-dependent for composing a
> > > > > > > feature (e.g. SVA), shouldn't we need a way to check them
> > > > > > > together before exposing the feature to the guest, e.g.
> > > > > > > through a iommu_get_uapi_capabilities interface?
> > > > > >
> > > > > > Yes, that makes sense. The important bit is to have a
> > > > > > capability flags and not version numbers.
> > > > >
> > > > > The challenge is that there are two consumers in the kernel for
> > > > > this. 1. VFIO only look for compatibility, and size of each data
> > > > > struct such that it can copy_from_user.
> > > > >
> > > > > 2. IOMMU driver, the "real consumer" of the content.
> > > > >
> > > > > For 2, I agree and we do plan to use the capability flags to
> > > > > check content and maintain backward compatibility etc.
> > > > >
> > > > > For VFIO, it is difficult to do size look up based on capability
> > > > > flags.
> > > >
> > > > Can you elaborate the difficulty in VFIO? if, as Christoph Hellwig
> > > > pointed out, version number is already avoided everywhere, it is
> > > > interesting to know whether this work becomes a real exception
> > > > or just requires a different mindset.
> > > >
> > > From VFIO p.o.v. the IOMMU UAPI data is opaque, it only needs to do
> > > two things:
> > > 1. is the UAPI compatible?
> > > 2. what is the size to copy?
> > >
> > > If you look at the version number, this is really a "version as
> > > size" lookup, as provided by the helper function in this patch. An
> > > example can be the newly introduced clone3 syscall.
> > > https://lwn.net/Articles/792628/
> > > In clone3, new version must have new size. The slight difference
> > > here is that, unlike clone3, we have multiple data structures
> > > instead of a single struct clone_args {}. And each struct has flags
> > > to enumerate its contents besides size.
> >
> > Thanks for providing that link. However clone3 doesn't include a
> > version field to do "version as size" lookup. Instead, as you said,
> > it includes a size parameter which sounds like the option 3 (argsz)
> > listed below.
> >
> Right, there is no version in clone3. size = version. I view this as
> a 1:1 lookup.
>
> > >
> > > Besides breaching data abstraction, if VFIO has to check IOMMU
> > > flags to determine the sizes, it has many combinations.
> > >
> > > We also separate the responsibilities into two parts
> > > 1. compatibility - version, size by VFIO
> > > 2. sanity check - capability flags - by IOMMU
> >
> > I feel argsz+flags approach can perfectly meet above requirement. The
> > userspace set the size and flags for whatever capabilities it uses,
> > and VFIO simply copies the parameters by size and pass to IOMMU for
> > further sanity check. Of course the assumption is that we do provide
> > an interface for userspace to enumerate all supported capabilities.
> >
> You cannot trust user for argsz. the size to be copied from user must
> be based on knowledge in kernel. That is why we have this version to
> size lookup.
>
> In VFIO, the size to copy is based on knowledge of each VFIO UAPI
> structures and VFIO flags. But here the flags are IOMMU UAPI flags. As
> you pointed out in another thread, VFIO is one user.
If that is the case, can we let VFIO only copy its own UAPI fields while
simply passing the user pointer of IOMMU UAPI structure to IOMMU
driver for further size check and copy? Otherwise we are entering a
dead end that VFIO doesn't want to parse a structure which is not
defined by him while using version to represent the black box size
is considered as a discarded scheme and doesn't scale well...
>
> > Is there anything that I overlooked here? I suppose there might be
> > some difficulties that block you from going the argsz way...
> >
> > Thanks
> > Kevin
> >
> > >
> > > I think the latter matches what Christoph's comments. So we are in
> > > agreement at the IOMMU level :)
> > >
> > > For example:
> > > During guest PASID bind, IOMMU driver operates on the data passed
> > > from VFIO and check format & flags to take different code path.
> > >
> > > #define IOMMU_PASID_FORMAT_INTEL_VTD 1
> > > __u32 format;
> > > #define IOMMU_SVA_GPASID_VAL (1 << 0) /* guest PASID valid */
> > > __u64 flags;
> > >
> > > Jacob
> > >
> > > > btw the most relevant discussion which I can find out now is here:
> > > > https://lkml.org/lkml/2020/2/3/1126
> > > >
> > > > It mentioned 3 options for handling
Re: [PATCH v2 1/3] iommu/uapi: Define uapi version and capabilities
On Tue, 31 Mar 2020 06:06:38 +
"Tian, Kevin" wrote:
> > From: Jacob Pan
> > Sent: Tuesday, March 31, 2020 12:08 AM
> >
> > On Mon, 30 Mar 2020 05:40:40 +
> > "Tian, Kevin" wrote:
> >
> > > > From: Jacob Pan
> > > > Sent: Saturday, March 28, 2020 7:54 AM
> > > >
> > > > On Fri, 27 Mar 2020 00:47:02 -0700
> > > > Christoph Hellwig wrote:
> > > >
> > > > > On Fri, Mar 27, 2020 at 02:49:55AM +, Tian, Kevin wrote:
> > > > > > If those API calls are inter-dependent for composing a
> > > > > > feature (e.g. SVA), shouldn't we need a way to check them
> > > > > > together before exposing the feature to the guest, e.g.
> > > > > > through a iommu_get_uapi_capabilities interface?
> > > > >
> > > > > Yes, that makes sense. The important bit is to have a
> > > > > capability flags and not version numbers.
> > > >
> > > > The challenge is that there are two consumers in the kernel for
> > > > this. 1. VFIO only look for compatibility, and size of each data
> > > > struct such that it can copy_from_user.
> > > >
> > > > 2. IOMMU driver, the "real consumer" of the content.
> > > >
> > > > For 2, I agree and we do plan to use the capability flags to
> > > > check content and maintain backward compatibility etc.
> > > >
> > > > For VFIO, it is difficult to do size look up based on capability
> > > > flags.
> > >
> > > Can you elaborate the difficulty in VFIO? if, as Christoph Hellwig
> > > pointed out, version number is already avoided everywhere, it is
> > > interesting to know whether this work becomes a real exception
> > > or just requires a different mindset.
> > >
> > From VFIO p.o.v. the IOMMU UAPI data is opaque, it only needs to do
> > two things:
> > 1. is the UAPI compatible?
> > 2. what is the size to copy?
> >
> > If you look at the version number, this is really a "version as
> > size" lookup, as provided by the helper function in this patch. An
> > example can be the newly introduced clone3 syscall.
> > https://lwn.net/Articles/792628/
> > In clone3, new version must have new size. The slight difference
> > here is that, unlike clone3, we have multiple data structures
> > instead of a single struct clone_args {}. And each struct has flags
> > to enumerate its contents besides size.
>
> Thanks for providing that link. However clone3 doesn't include a
> version field to do "version as size" lookup. Instead, as you said,
> it includes a size parameter which sounds like the option 3 (argsz)
> listed below.
>
Right, there is no version in clone3. size = version. I view this as
a 1:1 lookup.
> >
> > Besides breaching data abstraction, if VFIO has to check IOMMU
> > flags to determine the sizes, it has many combinations.
> >
> > We also separate the responsibilities into two parts
> > 1. compatibility - version, size by VFIO
> > 2. sanity check - capability flags - by IOMMU
>
> I feel argsz+flags approach can perfectly meet above requirement. The
> userspace set the size and flags for whatever capabilities it uses,
> and VFIO simply copies the parameters by size and pass to IOMMU for
> further sanity check. Of course the assumption is that we do provide
> an interface for userspace to enumerate all supported capabilities.
>
You cannot trust user for argsz. the size to be copied from user must
be based on knowledge in kernel. That is why we have this version to
size lookup.
In VFIO, the size to copy is based on knowledge of each VFIO UAPI
structures and VFIO flags. But here the flags are IOMMU UAPI flags. As
you pointed out in another thread, VFIO is one user.
> Is there anything that I overlooked here? I suppose there might be
> some difficulties that block you from going the argsz way...
>
> Thanks
> Kevin
>
> >
> > I think the latter matches what Christoph's comments. So we are in
> > agreement at the IOMMU level :)
> >
> > For example:
> > During guest PASID bind, IOMMU driver operates on the data passed
> > from VFIO and check format & flags to take different code path.
> >
> > #define IOMMU_PASID_FORMAT_INTEL_VTD1
> > __u32 format;
> > #define IOMMU_SVA_GPASID_VAL(1 << 0) /* guest PASID valid */
> > __u64 flags;
> >
> > Jacob
> >
> > > btw the most relevant discussion which I can find out now is here:
> > > https://lkml.org/lkml/2020/2/3/1126
> > >
> > > It mentioned 3 options for handling extension:
> > > --
> > > 1. Disallow adding new members to each structure other than reuse
> > > padding bits or adding union members at the end.
> > > 2. Allow extension of the structures beyond union, but union size
> > > has to be fixed with reserved spaces
> > > 3. Adopt VFIO argsz scheme, I don't think we need version for each
> > > struct anymore. argsz implies the version that user is using
> > > assuming UAPI data is extension only.
> > > --
> > >
> > > the first two are both version-based. Looks most guys agreed with
> > > option-1 (in this v2), but Alex didn't give his opinion at the
> > > moment. The last response from him was
RE: [PATCH v2 1/3] iommu/uapi: Define uapi version and capabilities
> From: Jacob Pan
> Sent: Tuesday, March 31, 2020 12:08 AM
>
> On Mon, 30 Mar 2020 05:40:40 +
> "Tian, Kevin" wrote:
>
> > > From: Jacob Pan
> > > Sent: Saturday, March 28, 2020 7:54 AM
> > >
> > > On Fri, 27 Mar 2020 00:47:02 -0700
> > > Christoph Hellwig wrote:
> > >
> > > > On Fri, Mar 27, 2020 at 02:49:55AM +, Tian, Kevin wrote:
> > > > > If those API calls are inter-dependent for composing a feature
> > > > > (e.g. SVA), shouldn't we need a way to check them together
> > > > > before exposing the feature to the guest, e.g. through a
> > > > > iommu_get_uapi_capabilities interface?
> > > >
> > > > Yes, that makes sense. The important bit is to have a capability
> > > > flags and not version numbers.
> > >
> > > The challenge is that there are two consumers in the kernel for
> > > this. 1. VFIO only look for compatibility, and size of each data
> > > struct such that it can copy_from_user.
> > >
> > > 2. IOMMU driver, the "real consumer" of the content.
> > >
> > > For 2, I agree and we do plan to use the capability flags to check
> > > content and maintain backward compatibility etc.
> > >
> > > For VFIO, it is difficult to do size look up based on capability
> > > flags.
> >
> > Can you elaborate the difficulty in VFIO? if, as Christoph Hellwig
> > pointed out, version number is already avoided everywhere, it is
> > interesting to know whether this work becomes a real exception
> > or just requires a different mindset.
> >
> From VFIO p.o.v. the IOMMU UAPI data is opaque, it only needs to do two
> things:
> 1. is the UAPI compatible?
> 2. what is the size to copy?
>
> If you look at the version number, this is really a "version as size"
> lookup, as provided by the helper function in this patch. An example
> can be the newly introduced clone3 syscall.
> https://lwn.net/Articles/792628/
> In clone3, new version must have new size. The slight difference here
> is that, unlike clone3, we have multiple data structures instead of a
> single struct clone_args {}. And each struct has flags to enumerate its
> contents besides size.
Thanks for providing that link. However clone3 doesn't include a version
field to do "version as size" lookup. Instead, as you said, it includes a
size parameter which sounds like the option 3 (argsz) listed below.
>
> Besides breaching data abstraction, if VFIO has to check IOMMU flags to
> determine the sizes, it has many combinations.
>
> We also separate the responsibilities into two parts
> 1. compatibility - version, size by VFIO
> 2. sanity check - capability flags - by IOMMU
I feel argsz+flags approach can perfectly meet above requirement. The
userspace set the size and flags for whatever capabilities it uses, and
VFIO simply copies the parameters by size and pass to IOMMU for
further sanity check. Of course the assumption is that we do provide
an interface for userspace to enumerate all supported capabilities.
Is there anything that I overlooked here? I suppose there might be
some difficulties that block you from going the argsz way...
Thanks
Kevin
>
> I think the latter matches what Christoph's comments. So we are in
> agreement at the IOMMU level :)
>
> For example:
> During guest PASID bind, IOMMU driver operates on the data passed from
> VFIO and check format & flags to take different code path.
>
> #define IOMMU_PASID_FORMAT_INTEL_VTD 1
> __u32 format;
> #define IOMMU_SVA_GPASID_VAL (1 << 0) /* guest PASID valid */
> __u64 flags;
>
> Jacob
>
> > btw the most relevant discussion which I can find out now is here:
> > https://lkml.org/lkml/2020/2/3/1126
> >
> > It mentioned 3 options for handling extension:
> > --
> > 1. Disallow adding new members to each structure other than reuse
> > padding bits or adding union members at the end.
> > 2. Allow extension of the structures beyond union, but union size has
> > to be fixed with reserved spaces
> > 3. Adopt VFIO argsz scheme, I don't think we need version for each
> > struct anymore. argsz implies the version that user is using assuming
> > UAPI data is extension only.
> > --
> >
> > the first two are both version-based. Looks most guys agreed with
> > option-1 (in this v2), but Alex didn't give his opinion at the
> > moment. The last response from him was the raise of option-3 using
> > argsz to avoid version. So, we also need hear from him. Alex?
> >
> > Thanks
> > Kevin
>
> [Jacob Pan]
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v2 1/3] iommu/uapi: Define uapi version and capabilities
On Mon, 30 Mar 2020 05:40:40 +
"Tian, Kevin" wrote:
> > From: Jacob Pan
> > Sent: Saturday, March 28, 2020 7:54 AM
> >
> > On Fri, 27 Mar 2020 00:47:02 -0700
> > Christoph Hellwig wrote:
> >
> > > On Fri, Mar 27, 2020 at 02:49:55AM +, Tian, Kevin wrote:
> > > > If those API calls are inter-dependent for composing a feature
> > > > (e.g. SVA), shouldn't we need a way to check them together
> > > > before exposing the feature to the guest, e.g. through a
> > > > iommu_get_uapi_capabilities interface?
> > >
> > > Yes, that makes sense. The important bit is to have a capability
> > > flags and not version numbers.
> >
> > The challenge is that there are two consumers in the kernel for
> > this. 1. VFIO only look for compatibility, and size of each data
> > struct such that it can copy_from_user.
> >
> > 2. IOMMU driver, the "real consumer" of the content.
> >
> > For 2, I agree and we do plan to use the capability flags to check
> > content and maintain backward compatibility etc.
> >
> > For VFIO, it is difficult to do size look up based on capability
> > flags.
>
> Can you elaborate the difficulty in VFIO? if, as Christoph Hellwig
> pointed out, version number is already avoided everywhere, it is
> interesting to know whether this work becomes a real exception
> or just requires a different mindset.
>
>From VFIO p.o.v. the IOMMU UAPI data is opaque, it only needs to do two
things:
1. is the UAPI compatible?
2. what is the size to copy?
If you look at the version number, this is really a "version as size"
lookup, as provided by the helper function in this patch. An example
can be the newly introduced clone3 syscall.
https://lwn.net/Articles/792628/
In clone3, new version must have new size. The slight difference here
is that, unlike clone3, we have multiple data structures instead of a
single struct clone_args {}. And each struct has flags to enumerate its
contents besides size.
Besides breaching data abstraction, if VFIO has to check IOMMU flags to
determine the sizes, it has many combinations.
We also separate the responsibilities into two parts
1. compatibility - version, size by VFIO
2. sanity check - capability flags - by IOMMU
I think the latter matches what Christoph's comments. So we are in
agreement at the IOMMU level :)
For example:
During guest PASID bind, IOMMU driver operates on the data passed from
VFIO and check format & flags to take different code path.
#define IOMMU_PASID_FORMAT_INTEL_VTD1
__u32 format;
#define IOMMU_SVA_GPASID_VAL(1 << 0) /* guest PASID valid */
__u64 flags;
Jacob
> btw the most relevant discussion which I can find out now is here:
> https://lkml.org/lkml/2020/2/3/1126
>
> It mentioned 3 options for handling extension:
> --
> 1. Disallow adding new members to each structure other than reuse
> padding bits or adding union members at the end.
> 2. Allow extension of the structures beyond union, but union size has
> to be fixed with reserved spaces
> 3. Adopt VFIO argsz scheme, I don't think we need version for each
> struct anymore. argsz implies the version that user is using assuming
> UAPI data is extension only.
> --
>
> the first two are both version-based. Looks most guys agreed with
> option-1 (in this v2), but Alex didn't give his opinion at the
> moment. The last response from him was the raise of option-3 using
> argsz to avoid version. So, we also need hear from him. Alex?
>
> Thanks
> Kevin
[Jacob Pan]
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
RE: [PATCH v2 1/3] iommu/uapi: Define uapi version and capabilities
> From: Jacob Pan > Sent: Saturday, March 28, 2020 7:54 AM > > On Fri, 27 Mar 2020 00:47:02 -0700 > Christoph Hellwig wrote: > > > On Fri, Mar 27, 2020 at 02:49:55AM +, Tian, Kevin wrote: > > > If those API calls are inter-dependent for composing a feature > > > (e.g. SVA), shouldn't we need a way to check them together before > > > exposing the feature to the guest, e.g. through a > > > iommu_get_uapi_capabilities interface? > > > > Yes, that makes sense. The important bit is to have a capability > > flags and not version numbers. > > The challenge is that there are two consumers in the kernel for this. > 1. VFIO only look for compatibility, and size of each data struct such > that it can copy_from_user. > > 2. IOMMU driver, the "real consumer" of the content. > > For 2, I agree and we do plan to use the capability flags to check > content and maintain backward compatibility etc. > > For VFIO, it is difficult to do size look up based on capability flags. > Can you elaborate the difficulty in VFIO? if, as Christoph Hellwig pointed out, version number is already avoided everywhere, it is interesting to know whether this work becomes a real exception or just requires a different mindset. btw the most relevant discussion which I can find out now is here: https://lkml.org/lkml/2020/2/3/1126 It mentioned 3 options for handling extension: -- 1. Disallow adding new members to each structure other than reuse padding bits or adding union members at the end. 2. Allow extension of the structures beyond union, but union size has to be fixed with reserved spaces 3. Adopt VFIO argsz scheme, I don't think we need version for each struct anymore. argsz implies the version that user is using assuming UAPI data is extension only. -- the first two are both version-based. Looks most guys agreed with option-1 (in this v2), but Alex didn't give his opinion at the moment. The last response from him was the raise of option-3 using argsz to avoid version. So, we also need hear from him. Alex? Thanks Kevin ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v2 1/3] iommu/uapi: Define uapi version and capabilities
On Fri, 27 Mar 2020 00:47:02 -0700 Christoph Hellwig wrote: > On Fri, Mar 27, 2020 at 02:49:55AM +, Tian, Kevin wrote: > > If those API calls are inter-dependent for composing a feature > > (e.g. SVA), shouldn't we need a way to check them together before > > exposing the feature to the guest, e.g. through a > > iommu_get_uapi_capabilities interface? > > Yes, that makes sense. The important bit is to have a capability > flags and not version numbers. The challenge is that there are two consumers in the kernel for this. 1. VFIO only look for compatibility, and size of each data struct such that it can copy_from_user. 2. IOMMU driver, the "real consumer" of the content. For 2, I agree and we do plan to use the capability flags to check content and maintain backward compatibility etc. For VFIO, it is difficult to do size look up based on capability flags. Jacob ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v2 1/3] iommu/uapi: Define uapi version and capabilities
On Fri, Mar 27, 2020 at 02:49:55AM +, Tian, Kevin wrote: > If those API calls are inter-dependent for composing a feature (e.g. SVA), > shouldn't we need a way to check them together before exposing the > feature to the guest, e.g. through a iommu_get_uapi_capabilities interface? Yes, that makes sense. The important bit is to have a capability flags and not version numbers. ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
RE: [PATCH v2 1/3] iommu/uapi: Define uapi version and capabilities
> From: Jacob Pan
> Sent: Friday, March 27, 2020 12:45 AM
>
> Hi Christoph,
>
> Thanks for the review. Please see my comments inline.
>
> On Thu, 26 Mar 2020 02:23:16 -0700
> Christoph Hellwig wrote:
>
> > On Wed, Mar 25, 2020 at 04:17:05PM -0700, Jacob Pan wrote:
> > > Having a single UAPI version to govern the user-kernel data
> > > structures makes compatibility check straightforward. On the
> > > contrary, supporting combinations of multiple versions of the data
> > > can be a nightmare to maintain.
> > >
> > > This patch defines a unified UAPI version to be used for
> > > compatibility checks between user and kernel.
> >
> > This is bullshit. Version numbers don't scale and we've avoided them
> > everywhere. You need need flags specifying specific behavior.
> >
> We have flags or the equivalent in each UAPI structures. The flags
> are used for checking validity of extensions as well. And you are right
> we can use them for checking specific behavior.
>
> So we have two options here:
> 1. Have a overall version for a quick compatibility check while
> starting a VM. If not compatible, we will stop guest SVA entirely.
>
> 2. Let each API calls check its own capabilities/flags at runtime. It
> may fail later on and lead to more complex error handling.
> For example, guest starts with SVA support, allocate a PASID, bind
> guest PASID works great. Then when IO page fault happens, it try to do
> page request service and found out certain flags are not compatible.
> This case is more complex to handle.
If those API calls are inter-dependent for composing a feature (e.g. SVA),
shouldn't we need a way to check them together before exposing the
feature to the guest, e.g. through a iommu_get_uapi_capabilities interface?
>
> I am guessing your proposal is #2. right?
>
> Overall, we don;t expect much change to the UAPI other than adding some
> vendor specific part of the union. Is the scalability concern based on
> frequent changes?
>
> > > +#define IOMMU_UAPI_VERSION 1
> > > +static inline int iommu_get_uapi_version(void)
> > > +{
> > > + return IOMMU_UAPI_VERSION;
> > > +}
> >
> > Also inline functions like this in UAPI headers that actually get
> > included by userspace programs simply don't work.
>
> I will remove that, user can just use IOMMU_UAPI_VERSION directly.
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v2 1/3] iommu/uapi: Define uapi version and capabilities
Hi Christoph,
Thanks for the review. Please see my comments inline.
On Thu, 26 Mar 2020 02:23:16 -0700
Christoph Hellwig wrote:
> On Wed, Mar 25, 2020 at 04:17:05PM -0700, Jacob Pan wrote:
> > Having a single UAPI version to govern the user-kernel data
> > structures makes compatibility check straightforward. On the
> > contrary, supporting combinations of multiple versions of the data
> > can be a nightmare to maintain.
> >
> > This patch defines a unified UAPI version to be used for
> > compatibility checks between user and kernel.
>
> This is bullshit. Version numbers don't scale and we've avoided them
> everywhere. You need need flags specifying specific behavior.
>
We have flags or the equivalent in each UAPI structures. The flags
are used for checking validity of extensions as well. And you are right
we can use them for checking specific behavior.
So we have two options here:
1. Have a overall version for a quick compatibility check while
starting a VM. If not compatible, we will stop guest SVA entirely.
2. Let each API calls check its own capabilities/flags at runtime. It
may fail later on and lead to more complex error handling.
For example, guest starts with SVA support, allocate a PASID, bind
guest PASID works great. Then when IO page fault happens, it try to do
page request service and found out certain flags are not compatible.
This case is more complex to handle.
I am guessing your proposal is #2. right?
Overall, we don;t expect much change to the UAPI other than adding some
vendor specific part of the union. Is the scalability concern based on
frequent changes?
> > +#define IOMMU_UAPI_VERSION 1
> > +static inline int iommu_get_uapi_version(void)
> > +{
> > + return IOMMU_UAPI_VERSION;
> > +}
>
> Also inline functions like this in UAPI headers that actually get
> included by userspace programs simply don't work.
I will remove that, user can just use IOMMU_UAPI_VERSION directly.
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v2 1/3] iommu/uapi: Define uapi version and capabilities
On Wed, Mar 25, 2020 at 04:17:05PM -0700, Jacob Pan wrote:
> Having a single UAPI version to govern the user-kernel data structures
> makes compatibility check straightforward. On the contrary, supporting
> combinations of multiple versions of the data can be a nightmare to
> maintain.
>
> This patch defines a unified UAPI version to be used for compatibility
> checks between user and kernel.
This is bullshit. Version numbers don't scale and we've avoided them
everywhere. You need need flags specifying specific behavior.
> +#define IOMMU_UAPI_VERSION 1
> +static inline int iommu_get_uapi_version(void)
> +{
> + return IOMMU_UAPI_VERSION;
> +}
Also inline functions like this in UAPI headers that actually get
included by userspace programs simply don't work.
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
