Re: [PATCH v9 3/7] iommu/uapi: Introduce enum type for PASID data format

[Jacob Pan <jacob.jun.pan@linux.intel.com>]

Hi Joerg,

On Fri, 18 Sep 2020 11:44:50 +0200, Joerg Roedel <joro@8bytes.org> wrote:

> On Fri, Sep 11, 2020 at 02:57:52PM -0700, Jacob Pan wrote:
> > There can be multiple vendor-specific PASID data formats used in UAPI
> > structures. This patch adds enum type with a last entry which makes
> > range checking much easier.  
> 
> But it also makes it much easier to screw up the numbers (which are ABI)
> by inserting a new value into the middle. I prefer defines here, or
> alternativly BUILD_BUG_ON() checks for the numbers.
> 
I am not following, the purpose of IOMMU_PASID_FORMAT_LAST *is* for
preparing the future insertion of new value into the middle.
The checking against IOMMU_PASID_FORMAT_LAST is to protect ABI
compatibility by making sure that out of range format are rejected in all
versions of the ABI.
For example, in v5.10, ABI has IOMMU_PASID_FORMAT_LAST = 2, then user data
with format = 2 will be rejected. So this user app will not work or
released.

Now say in v5.11, we add one more format in the middle and set
IOMMU_PASID_FORMAT_LAST = 3. Then user data with the new format = 2 can be supported.

Without the checking for IOMMU_PASID_FORMAT_LAST, at v5.10 time the user
binary may succeed and become legacy binary that we cannot break in v5.11.
This renders format = 2 unusable for v5.11.

I thought enum makes it less susceptible to programming errors than defines
by making sure the ascending order. I might have missed your point, could
you elaborate?

> Regards,
> 
> 	Joerg
> 
> > 
> > Suggested-by: Alex Williamson <alex.williamson@redhat.com>
> > Reviewed-by: Eric Auger <eric.auger@redhat.com>
> > Signed-off-by: Jacob Pan <jacob.jun.pan@linux.intel.com>
> > ---
> >  include/uapi/linux/iommu.h | 8 ++++++--
> >  1 file changed, 6 insertions(+), 2 deletions(-)
> > 
> > diff --git a/include/uapi/linux/iommu.h b/include/uapi/linux/iommu.h
> > index b42acc8fe007..7cc6ee6c41f7 100644
> > --- a/include/uapi/linux/iommu.h
> > +++ b/include/uapi/linux/iommu.h
> > @@ -298,11 +298,16 @@ struct iommu_gpasid_bind_data_vtd {
> >  					 IOMMU_SVA_VTD_GPASID_PCD |  \
> >  					 IOMMU_SVA_VTD_GPASID_PWT)
> >  
> > +enum iommu_pasid_data_format {
> > +	IOMMU_PASID_FORMAT_INTEL_VTD = 1,
> > +	IOMMU_PASID_FORMAT_LAST,
> > +};
> > +
> >  /**
> >   * struct iommu_gpasid_bind_data - Information about device and guest
> > PASID binding
> >   * @argsz:	User filled size of this data
> >   * @version:	Version of this data structure
> > - * @format:	PASID table entry format
> > + * @format:	PASID table entry format of enum
> > iommu_pasid_data_format type
> >   * @flags:	Additional information on guest bind request
> >   * @gpgd:	Guest page directory base of the guest mm to bind
> >   * @hpasid:	Process address space ID used for the guest mm in
> > host IOMMU @@ -321,7 +326,6 @@ struct iommu_gpasid_bind_data {
> >  	__u32 argsz;
> >  #define IOMMU_GPASID_BIND_VERSION_1	1
> >  	__u32 version;
> > -#define IOMMU_PASID_FORMAT_INTEL_VTD	1
> >  	__u32 format;
> >  	__u32 addr_width;
> >  #define IOMMU_SVA_GPASID_VAL	(1 << 0) /* guest PASID valid */
> > -- 
> > 2.7.4  


Thanks,

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