Re: [PATCH v5 2/5] iommu/uapi: Add argsz for user filled data

2020-07-20 Thread Jacob Pan
On Fri, 17 Jul 2020 15:44:23 +0200
Auger Eric  wrote:

> Hi Jacob,
> 
> On 7/16/20 8:45 PM, Jacob Pan wrote:
> > As IOMMU UAPI gets extended, user data size may increase. To support
> > backward compatibiliy, this patch introduces a size field to each
> > UAPI data structures. It is *always* the responsibility for the
> > user to fill in the correct size. Padding fields are adjusted to
> > ensure 8 byte alignment.
> > 
> > Specific scenarios for user data handling are documented in:
> > Documentation/userspace-api/iommu.rst
> > 
> > Signed-off-by: Liu Yi L 
> > Signed-off-by: Jacob Pan 
> > ---
> >  include/uapi/linux/iommu.h | 12 +---
> >  1 file changed, 9 insertions(+), 3 deletions(-)
> > 
> > diff --git a/include/uapi/linux/iommu.h b/include/uapi/linux/iommu.h
> > index e907b7091a46..d5e9014f690e 100644
> > --- a/include/uapi/linux/iommu.h
> > +++ b/include/uapi/linux/iommu.h
> > @@ -135,6 +135,7 @@ enum iommu_page_response_code {
> >  
> >  /**
> >   * struct iommu_page_response - Generic page response information
> > + * @argsz: User filled size of this data
> >   * @version: API version of this structure
> >   * @flags: encodes whether the corresponding fields are valid
> >   * (IOMMU_FAULT_PAGE_RESPONSE_* values)
> > @@ -143,6 +144,7 @@ enum iommu_page_response_code {
> >   * @code: response code from  iommu_page_response_code
> >   */
> >  struct iommu_page_response {
> > +   __u32   argsz;
> >  #define IOMMU_PAGE_RESP_VERSION_1  1  
> Don't you need to incr the version for all the modified structs?
not literal "flags" but @cache and @granularity are flags in reality. I
think that is OK. I also updated document to say "flags or equivalent".

> > __u32   version;
> >  #define IOMMU_PAGE_RESP_PASID_VALID(1 << 0)
> > @@ -218,6 +220,7 @@ struct iommu_inv_pasid_info {
> >  /**
> >   * struct iommu_cache_invalidate_info - First level/stage
> > invalidation
> >   * information
> > + * @argsz: User filled size of this data
> >   * @version: API version of this structure
> >   * @cache: bitfield that allows to select which caches to
> > invalidate
> >   * @granularity: defines the lowest granularity used for the
> > invalidation: @@ -246,6 +249,7 @@ struct iommu_inv_pasid_info {
> >   * must support the used granularity.
> >   */
> >  struct iommu_cache_invalidate_info {
> > +   __u32   argsz;
> >  #define IOMMU_CACHE_INVALIDATE_INFO_VERSION_1 1
> > __u32   version;  
> so there is no "flags" field in this struct. Is it OK?
> >  /* IOMMU paging structure cache */
> > @@ -255,7 +259,7 @@ struct iommu_cache_invalidate_info {
> >  #define IOMMU_CACHE_INV_TYPE_NR(3)
> > __u8cache;
> > __u8granularity;
> > -   __u8padding[2];
> > +   __u8padding[6];
> > union {
> > struct iommu_inv_pasid_info pasid_info;
> > struct iommu_inv_addr_info addr_info;
> > @@ -292,6 +296,7 @@ struct iommu_gpasid_bind_data_vtd {
> >  
> >  /**
> >   * 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
> >   * @flags: Additional information on guest bind request
> > @@ -309,17 +314,18 @@ struct iommu_gpasid_bind_data_vtd {
> >   * PASID to host PASID based on this bind data.
> >   */
> >  struct iommu_gpasid_bind_data {
> > +   __u32 argsz;
> >  #define IOMMU_GPASID_BIND_VERSION_11
> > __u32 version;
> >  #define IOMMU_PASID_FORMAT_INTEL_VTD   1
> > __u32 format;
> > +   __u32 addr_width;
> >  #define IOMMU_SVA_GPASID_VAL   (1 << 0) /* guest PASID valid
> > */ __u64 flags;
> > __u64 gpgd;
> > __u64 hpasid;
> > __u64 gpasid;
> > -   __u32 addr_width;
> > -   __u8  padding[12];
> > +   __u8  padding[8];
> > /* Vendor specific data */
> > union {
> > struct iommu_gpasid_bind_data_vtd vtd;
> >   
> Thanks
> 
> Eric
> 

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


Re: [PATCH v5 2/5] iommu/uapi: Add argsz for user filled data

2020-07-17 Thread Auger Eric
Hi Jacob,

On 7/16/20 8:45 PM, Jacob Pan wrote:
> As IOMMU UAPI gets extended, user data size may increase. To support
> backward compatibiliy, this patch introduces a size field to each UAPI
> data structures. It is *always* the responsibility for the user to fill in
> the correct size. Padding fields are adjusted to ensure 8 byte alignment.
> 
> Specific scenarios for user data handling are documented in:
> Documentation/userspace-api/iommu.rst
> 
> Signed-off-by: Liu Yi L 
> Signed-off-by: Jacob Pan 
> ---
>  include/uapi/linux/iommu.h | 12 +---
>  1 file changed, 9 insertions(+), 3 deletions(-)
> 
> diff --git a/include/uapi/linux/iommu.h b/include/uapi/linux/iommu.h
> index e907b7091a46..d5e9014f690e 100644
> --- a/include/uapi/linux/iommu.h
> +++ b/include/uapi/linux/iommu.h
> @@ -135,6 +135,7 @@ enum iommu_page_response_code {
>  
>  /**
>   * struct iommu_page_response - Generic page response information
> + * @argsz: User filled size of this data
>   * @version: API version of this structure
>   * @flags: encodes whether the corresponding fields are valid
>   * (IOMMU_FAULT_PAGE_RESPONSE_* values)
> @@ -143,6 +144,7 @@ enum iommu_page_response_code {
>   * @code: response code from  iommu_page_response_code
>   */
>  struct iommu_page_response {
> + __u32   argsz;
>  #define IOMMU_PAGE_RESP_VERSION_11
Don't you need to incr the version for all the modified structs?
>   __u32   version;
>  #define IOMMU_PAGE_RESP_PASID_VALID  (1 << 0)
> @@ -218,6 +220,7 @@ struct iommu_inv_pasid_info {
>  /**
>   * struct iommu_cache_invalidate_info - First level/stage invalidation
>   * information
> + * @argsz: User filled size of this data
>   * @version: API version of this structure
>   * @cache: bitfield that allows to select which caches to invalidate
>   * @granularity: defines the lowest granularity used for the invalidation:
> @@ -246,6 +249,7 @@ struct iommu_inv_pasid_info {
>   * must support the used granularity.
>   */
>  struct iommu_cache_invalidate_info {
> + __u32   argsz;
>  #define IOMMU_CACHE_INVALIDATE_INFO_VERSION_1 1
>   __u32   version;
so there is no "flags" field in this struct. Is it OK?
>  /* IOMMU paging structure cache */
> @@ -255,7 +259,7 @@ struct iommu_cache_invalidate_info {
>  #define IOMMU_CACHE_INV_TYPE_NR  (3)
>   __u8cache;
>   __u8granularity;
> - __u8padding[2];
> + __u8padding[6];
>   union {
>   struct iommu_inv_pasid_info pasid_info;
>   struct iommu_inv_addr_info addr_info;
> @@ -292,6 +296,7 @@ struct iommu_gpasid_bind_data_vtd {
>  
>  /**
>   * 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
>   * @flags:   Additional information on guest bind request
> @@ -309,17 +314,18 @@ struct iommu_gpasid_bind_data_vtd {
>   * PASID to host PASID based on this bind data.
>   */
>  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 */
>   __u64 flags;
>   __u64 gpgd;
>   __u64 hpasid;
>   __u64 gpasid;
> - __u32 addr_width;
> - __u8  padding[12];
> + __u8  padding[8];
>   /* Vendor specific data */
>   union {
>   struct iommu_gpasid_bind_data_vtd vtd;
> 
Thanks

Eric

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