Re: [PATCH v6 02/15] iommu: Report domain nesting info

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

On Sun, 16 Aug 2020 14:40:57 +0200
Auger Eric <eric.auger@redhat.com> wrote:

> Hi Yi,
> 
> On 8/14/20 9:15 AM, Liu, Yi L wrote:
> > Hi Eric,
> >   
> >> From: Auger Eric <eric.auger@redhat.com>
> >> Sent: Thursday, August 13, 2020 8:53 PM
> >>
> >> Yi,
> >> On 7/28/20 8:27 AM, Liu Yi L wrote:  
> >>> IOMMUs that support nesting translation needs report the
> >>> capability info  
> >> s/needs/need to  
> >>> to userspace. It gives information about requirements the
> >>> userspace needs to implement plus other features characterizing
> >>> the physical implementation.
> >>>
> >>> This patch reports nesting info by DOMAIN_ATTR_NESTING. Caller
> >>> can get nesting info after setting DOMAIN_ATTR_NESTING. For VFIO,
> >>> it is after selecting VFIO_TYPE1_NESTING_IOMMU.  
> >> This is not what this patch does ;-) It introduces a new IOMMU UAPI
> >> struct that gives information about the nesting capabilities and
> >> features. This struct is supposed to be returned by
> >> iommu_domain_get_attr() with DOMAIN_ATTR_NESTING attribute
> >> parameter, one a domain whose type has been set to
> >> DOMAIN_ATTR_NESTING.  
> > 
> > got it. let me apply your suggestion. thanks. :-)
> >   
> >>>
> >>> Cc: Kevin Tian <kevin.tian@intel.com>
> >>> CC: Jacob Pan <jacob.jun.pan@linux.intel.com>
> >>> Cc: Alex Williamson <alex.williamson@redhat.com>
> >>> Cc: Eric Auger <eric.auger@redhat.com>
> >>> Cc: Jean-Philippe Brucker <jean-philippe@linaro.org>
> >>> Cc: Joerg Roedel <joro@8bytes.org>
> >>> Cc: Lu Baolu <baolu.lu@linux.intel.com>
> >>> Signed-off-by: Liu Yi L <yi.l.liu@intel.com>
> >>> Signed-off-by: Jacob Pan <jacob.jun.pan@linux.intel.com>
> >>> ---
> >>> v5 -> v6:
> >>> *) rephrase the feature notes per comments from Eric Auger.
> >>> *) rename @size of struct iommu_nesting_info to @argsz.
> >>>
> >>> v4 -> v5:
> >>> *) address comments from Eric Auger.
> >>>
> >>> v3 -> v4:
> >>> *) split the SMMU driver changes to be a separate patch
> >>> *) move the @addr_width and @pasid_bits from vendor specific
> >>>    part to generic part.
> >>> *) tweak the description for the @features field of struct
> >>>    iommu_nesting_info.
> >>> *) add description on the @data[] field of struct
> >>> iommu_nesting_info
> >>>
> >>> v2 -> v3:
> >>> *) remvoe cap/ecap_mask in iommu_nesting_info.
> >>> *) reuse DOMAIN_ATTR_NESTING to get nesting info.
> >>> *) return an empty iommu_nesting_info for SMMU drivers per Jean'
> >>>    suggestion.
> >>> ---
> >>>  include/uapi/linux/iommu.h | 74  
> >> ++++++++++++++++++++++++++++++++++++++++++++++  
> >>>  1 file changed, 74 insertions(+)
> >>>
> >>> diff --git a/include/uapi/linux/iommu.h
> >>> b/include/uapi/linux/iommu.h index 7c8e075..5e4745a 100644
> >>> --- a/include/uapi/linux/iommu.h
> >>> +++ b/include/uapi/linux/iommu.h
> >>> @@ -332,4 +332,78 @@ struct iommu_gpasid_bind_data {
> >>>  	} vendor;
> >>>  };
> >>>
> >>> +/*
> >>> + * struct iommu_nesting_info - Information for nesting-capable
> >>> IOMMU.
> >>> + *			       userspace should check it
> >>> before using
> >>> + *			       nesting capability.
> >>> + *
> >>> + * @argsz:	size of the whole structure.
> >>> + * @flags:	currently reserved for future extension. must
> >>> set to 0.
> >>> + * @format:	PASID table entry format, the same definition
> >>> as struct
> >>> + *		iommu_gpasid_bind_data @format.
> >>> + * @features:	supported nesting features.
> >>> + * @addr_width:	The output addr width of first
> >>> level/stage translation
> >>> + * @pasid_bits:	Maximum supported PASID bits, 0
> >>> represents no PASID
> >>> + *		support.
> >>> + * @data:	vendor specific cap info. data[] structure type
> >>> can be deduced
> >>> + *		from @format field.
> >>> + *
> >>> + *  
> >> +===============+===================================================
> >> ===+  
> >>> + * | feature       |
> >>> Notes                                               |
> >>> + *  
> >> +===============+===================================================
> >> ===+  
> >>> + * | SYSWIDE_PASID |  IOMMU vendor driver sets it to mandate
> >>> userspace    |
> >>> + * |               |  to allocate PASID from kernel. All PASID
> >>> allocation |
> >>> + * |               |  free must be mediated through the TBD
> >>> API.          |  
> >> s/TBD/IOMMU  
> > 
> > got it.
> >   
> >>> + *
> >>> +---------------+------------------------------------------------------+
> >>> + * | BIND_PGTBL    |  IOMMU vendor driver sets it to mandate
> >>> userspace    |
> >>> + * |               |  bind the first level/stage page table to
> >>> associated |  
> >> s/bind/to bind  
> > 
> > got it.
> >   
> >>> + * |               |  PASID (either the one specified in bind
> >>> request or  |
> >>> + * |               |  the default PASID of iommu domain),
> >>> through IOMMU   |
> >>> + * |               |
> >>> UAPI.                                               |
> >>> + *
> >>> +---------------+------------------------------------------------------+
> >>> + * | CACHE_INVLD   |  IOMMU vendor driver sets it to mandate
> >>> userspace    |  
> >>  
> >>> + * |               |  explicitly invalidate the IOMMU cache
> >>> through IOMMU |  
> >> to explicitly  
> > 
> > I see.
> >   
> >>> + * |               |  U
> >>> API according to vendor-specific requirement when  |
> >>> + * |               |  changing the 1st level/stage page
> >>> table.            |
> >>> + *
> >>> +---------------+------------------------------------------------------+
> >>> + *
> >>> + * @data[] types defined for @format:
> >>> + *  
> >> +================================+==================================
> >> ===+  
> >>> + * | @format                        |
> >>> @data[]                             |
> >>> + *  
> >> +================================+==================================
> >> ===+  
> >>> + * | IOMMU_PASID_FORMAT_INTEL_VTD   | struct
> >>> iommu_nesting_info_vtd       |
> >>> + *
> >>> +--------------------------------+-------------------------------------+
> >>> + *
> >>> + */
> >>> +struct iommu_nesting_info {
> >>> +	__u32	argsz;
> >>> +	__u32	flags;
> >>> +	__u32	format;
> >>> +#define IOMMU_NESTING_FEAT_SYSWIDE_PASID	(1 << 0)
> >>> +#define IOMMU_NESTING_FEAT_BIND_PGTBL		(1 << 1)
> >>> +#define IOMMU_NESTING_FEAT_CACHE_INVLD		(1 << 2)
> >>> +	__u32	features;
> >>> +	__u16	addr_width;
> >>> +	__u16	pasid_bits;
> >>> +	__u32	padding;
> >>> +	__u8	data[];
> >>> +};  
> >> As opposed to other IOMMU UAPI structs there is no union member at
> >> the end.  
> > 
> > nice catch. do you think it would be better to adding a union and
> > put the struct iommu_nesting_info_vtd in it?  
> Yes I think so. At least it would be consistent with the rest of the
> API and with the guidelines.
> >   
> >> Also this struct is not documented in [PATCH v7 1/7] docs: IOMMU
> >> user API. Shouldn't we align.
> >> You may also consider to move this patch in Jacob's series for
> >> consistency, thoughts?  
> > 
> > this was talked one time between Jacob and me. It was put in this
> > series as the major user of nesting_info is in this series. e.g.
> > vfio checks the SYSWIDE_PASID. but I'm open to merge it with Jacob's
> > series if it would make the merge easier.  
> Yep I think it would make sense to move in Jacob's series to have a
> general understanding of the uapi
> 
I a little reluctant to include this in my UAPI set, the reason is that
there are two dimensions IOMMU UAPI are extended:
1. Define the protocols in interaction with VFIO, sanity checking, and
backward compatibility.
2. Adding more UAPI data structures that are parallel to the existing
ones.

My patchset is to address #1, this patch is for #2. My thinking is that
once we have reached consensus on #1, new UAPI structures such as this
patch can just follow the suit.

If that is OK with you, I would like to keep them separate to avoid
diverging conversations.

Thanks,

Jacob

> Thanks
> 
> Eric
> > 
> > Thanks,
> > Yi Liu
> >   
> >>> +
> >>> +/*
> >>> + * struct iommu_nesting_info_vtd - Intel VT-d specific nesting
> >>> info.
> >>> + *
> >>> + * @flags:	VT-d specific flags. Currently reserved for
> >>> future
> >>> + *		extension. must be set to 0.
> >>> + * @cap_reg:	Describe basic capabilities as defined in
> >>> VT-d capability
> >>> + *		register.
> >>> + * @ecap_reg:	Describe the extended capabilities as
> >>> defined in VT-d
> >>> + *		extended capability register.
> >>> + */
> >>> +struct iommu_nesting_info_vtd {
> >>> +	__u32	flags;
> >>> +	__u32	padding;
> >>> +	__u64	cap_reg;
> >>> +	__u64	ecap_reg;
> >>> +};
> >>> +
> >>>  #endif /* _UAPI_IOMMU_H */
> >>>  
> >>
> >> Thanks
> >>
> >> Eric
> >>  
> >   
> 

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