Re: [PATCH V6 vfio 05/10] vfio: Introduce the DMA logging feature support

[Leon Romanovsky <leon@kernel.org>]

On Tue, Sep 06, 2022 at 04:41:54PM -0600, Alex Williamson wrote:
> On Mon, 5 Sep 2022 13:58:47 +0300
> Yishai Hadas <yishaih@nvidia.com> wrote:
> > diff --git a/include/linux/vfio.h b/include/linux/vfio.h
> > index e05ddc6fe6a5..b17f2f454389 100644
> > --- a/include/linux/vfio.h
> > +++ b/include/linux/vfio.h
> > @@ -108,6 +110,21 @@ struct vfio_migration_ops {
> >  				   enum vfio_device_mig_state *curr_state);
> >  };
> >  
> > +/**
> > + * @log_start: Optional callback to ask the device start DMA logging.
> > + * @log_stop: Optional callback to ask the device stop DMA logging.
> > + * @log_read_and_clear: Optional callback to ask the device read
> > + *         and clear the dirty DMAs in some given range.
> 
> I don't see anywhere in the core that we track the device state
> relative to the DEVICE_FEATURE_DMA_LOGGING features, nor do we
> explicitly put the responsibility on the driver implementation to
> handle invalid user requests.  The mlx5 driver implementation appears
> to do this, but maybe we should at least include a requirement here, ex.
> 
>    The vfio core implementation of the DEVICE_FEATURE_DMA_LOGGING_ set
>    of features does not track logging state relative to the device,
>    therefore the device implementation of vfio_log_ops must handle
>    arbitrary user requests.  This includes rejecting subsequent calls
>    to log_start without an intervening log_stop, as well as graceful
>    handling of log_stop and log_read_and_clear from invalid states.
> 
> With something like that.
> 
> Acked-by: Alex Williamson <alex.williamson@redhat.com>
> 
> You can also add my Ack on 3, 4, and (fwiw) 6-10 as I assume this would
> be a PR from Leon.  Thanks,

Yes, it is.
I sent PR based on clean 6.0-rc4.

Thanks

> 
> Alex
> 
> > + */
> > +struct vfio_log_ops {
> > +	int (*log_start)(struct vfio_device *device,
> > +		struct rb_root_cached *ranges, u32 nnodes, u64 *page_size);
> > +	int (*log_stop)(struct vfio_device *device);
> > +	int (*log_read_and_clear)(struct vfio_device *device,
> > +		unsigned long iova, unsigned long length,
> > +		struct iova_bitmap *dirty);
> > +};
> > +
> >  /**
> >   * vfio_check_feature - Validate user input for the VFIO_DEVICE_FEATURE ioctl
> >   * @flags: Arg from the device_feature op
>