Re: [PATCH v17 Kernel 6/7] vfio iommu: Adds flag to indicate dirty pages tracking capability support

[Alex Williamson <alex.williamson@redhat.com>]

On Wed, 1 Apr 2020 22:55:57 +0530
Kirti Wankhede <kwankhede@nvidia.com> wrote:

> On 4/1/2020 12:45 AM, Alex Williamson wrote:
> > On Wed, 1 Apr 2020 00:38:49 +0530
> > Kirti Wankhede <kwankhede@nvidia.com> wrote:
> >   
> >> On 3/31/2020 2:28 AM, Alex Williamson wrote:  
> >>> On Mon, 30 Mar 2020 22:20:43 +0530
> >>> Kirti Wankhede <kwankhede@nvidia.com> wrote:
> >>>      
> >>>> Flag VFIO_IOMMU_INFO_DIRTY_PGS in VFIO_IOMMU_GET_INFO indicates that driver
> >>>> support dirty pages tracking.
> >>>>
> >>>> Signed-off-by: Kirti Wankhede <kwankhede@nvidia.com>
> >>>> Reviewed-by: Neo Jia <cjia@nvidia.com>
> >>>> ---
> >>>>    drivers/vfio/vfio_iommu_type1.c | 3 ++-
> >>>>    include/uapi/linux/vfio.h       | 5 +++--
> >>>>    2 files changed, 5 insertions(+), 3 deletions(-)
> >>>>
> >>>> diff --git a/drivers/vfio/vfio_iommu_type1.c b/drivers/vfio/vfio_iommu_type1.c
> >>>> index 266550bd7307..9fe12b425976 100644
> >>>> --- a/drivers/vfio/vfio_iommu_type1.c
> >>>> +++ b/drivers/vfio/vfio_iommu_type1.c
> >>>> @@ -2390,7 +2390,8 @@ static long vfio_iommu_type1_ioctl(void *iommu_data,
> >>>>    			info.cap_offset = 0; /* output, no-recopy necessary */
> >>>>    		}
> >>>>    
> >>>> -		info.flags = VFIO_IOMMU_INFO_PGSIZES;
> >>>> +		info.flags = VFIO_IOMMU_INFO_PGSIZES |
> >>>> +			     VFIO_IOMMU_INFO_DIRTY_PGS;
> >>>>    
> >>>>    		info.iova_pgsizes = vfio_pgsize_bitmap(iommu);
> >>>>    
> >>>> diff --git a/include/uapi/linux/vfio.h b/include/uapi/linux/vfio.h
> >>>> index e3cbf8b78623..0fe7c9a6f211 100644
> >>>> --- a/include/uapi/linux/vfio.h
> >>>> +++ b/include/uapi/linux/vfio.h
> >>>> @@ -985,8 +985,9 @@ struct vfio_device_feature {
> >>>>    struct vfio_iommu_type1_info {
> >>>>    	__u32	argsz;
> >>>>    	__u32	flags;
> >>>> -#define VFIO_IOMMU_INFO_PGSIZES (1 << 0)	/* supported page sizes info */
> >>>> -#define VFIO_IOMMU_INFO_CAPS	(1 << 1)	/* Info supports caps */
> >>>> +#define VFIO_IOMMU_INFO_PGSIZES   (1 << 0) /* supported page sizes info */
> >>>> +#define VFIO_IOMMU_INFO_CAPS      (1 << 1) /* Info supports caps */
> >>>> +#define VFIO_IOMMU_INFO_DIRTY_PGS (1 << 2) /* supports dirty page tracking */
> >>>>    	__u64	iova_pgsizes;	/* Bitmap of supported page sizes */
> >>>>    	__u32   cap_offset;	/* Offset within info struct of first cap */
> >>>>    };  
> >>>
> >>>
> >>> As I just mentioned in my reply to Yan, I'm wondering if
> >>> VFIO_CHECK_EXTENSION would be a better way to expose this.  The
> >>> difference is relatively trivial, but currently the only flag
> >>> set by VFIO_IOMMU_GET_INFO is to indicate the presence of a field in
> >>> the returned structure.  I think this is largely true of other INFO
> >>> ioctls within vfio as well and we're already using the
> >>> VFIO_CHECK_EXTENSION ioctl to check supported IOMMU models, and IOMMU
> >>> cache coherency.  We'd simply need to define a VFIO_DIRTY_PGS_IOMMU
> >>> value (9) and return 1 for that case.  Then when we enable support for
> >>> dirt pages that can span multiple mappings, we can add a v2 extensions,
> >>> or "MULTI" variant of this extension, since it should be backwards
> >>> compatible.
> >>>
> >>> The v2/multi version will again require that the user provide a zero'd
> >>> bitmap, but I don't think that should be a problem as part of the
> >>> definition of that version (we won't know if the user is using v1 or
> >>> v2, but a v1 user should only retrieve bitmaps that exactly match
> >>> existing mappings, where all bits will be written).  Thanks,
> >>>
> >>> Alex
> >>>      
> >>
> >> I look at these two ioctls as : VFIO_CHECK_EXTENSION is used to get
> >> IOMMU type, while VFIO_IOMMU_GET_INFO is used to get properties of a
> >> particular IOMMU type, right?  
> > 
> > Not exclusively, see for example VFIO_DMA_CC_IOMMU,
> >   
> >> Then I think VFIO_IOMMU_INFO_DIRTY_PGS should be part of
> >> VFIO_IOMMU_GET_INFO and when we add code for v2/multi, a flag should be
> >> added to VFIO_IOMMU_GET_INFO.  
> > 
> > Which burns through flags, which is a far more limited resource than
> > our 32bit extension address space, especially when we're already
> > planning for one or more extensions to this support.  Thanks,
> >   
> 
> To use flag from VFIO_IOMMU_GET_INFO was your original suggestion, only 
> 3 bits are used here as of now.

Sorry, I'm not infallible.  Perhaps I was short sighted and thought we
might only need one flag, perhaps I forgot about the check-extension
ioctl.  Are there any technical reasons to keep it on the get-info
ioctl?  As I'm trying to look ahead for how we're going to fill the
gaps of this initial implementation, it seems to me that what we're
exposing here is in line with what we've used check-extension for in
the past, and it offers us essentially unlimited extensions to burn
through, while we're clearly limited on the get-info flags.  We do have
the precedent of the reset flag on the device_get_info ioctl, but I'm
largely under the impression that was a mistake and queuing up multiple
missing features in addition to the base flags feels like compounding
another mistake.  Thanks,

Alex