RE: [PATCH V2 vfio 03/11] vfio: Introduce DMA logging uAPIs

From: "Tian, Kevin" <kevin.tian@intel.com>
To: Yishai Hadas <yishaih@nvidia.com>,
	"alex.williamson@redhat.com" <alex.williamson@redhat.com>,
	"jgg@nvidia.com" <jgg@nvidia.com>
Cc: "saeedm@nvidia.com" <saeedm@nvidia.com>,
	"kvm@vger.kernel.org" <kvm@vger.kernel.org>,
	"netdev@vger.kernel.org" <netdev@vger.kernel.org>,
	"kuba@kernel.org" <kuba@kernel.org>,
	"Martins, Joao" <joao.m.martins@oracle.com>,
	"leonro@nvidia.com" <leonro@nvidia.com>,
	"maorg@nvidia.com" <maorg@nvidia.com>,
	"cohuck@redhat.com" <cohuck@redhat.com>
Subject: RE: [PATCH V2 vfio 03/11] vfio: Introduce DMA logging uAPIs
Date: Mon, 25 Jul 2022 07:30:46 +0000	[thread overview]
Message-ID: <BN9PR11MB5276BEDFBBD53A44C1525A118C959@BN9PR11MB5276.namprd11.prod.outlook.com> (raw)
In-Reply-To: <56bd06d3-944c-18da-86ed-ae14ce5940b7@nvidia.com>

<please use plain-text next time>

> From: Yishai Hadas <yishaih@nvidia.com> 
> Sent: Thursday, July 21, 2022 7:06 PM
> > > +/*
> > > + * Upon VFIO_DEVICE_FEATURE_SET start device DMA logging.
> >
> > both 'start'/'stop' are via VFIO_DEVICE_FEATURE_SET
>
> Right, we have a note for that near VFIO_DEVICE_FEATURE_DMA_LOGGING_STOP.
> Here it refers to the start option.

let's make it accurate here.

> > > + * page_size is an input that hints what tracking granularity the device
> > > + * should try to achieve. If the device cannot do the hinted page size then it
> > > + * should pick the next closest page size it supports. On output the device
> >
> > next closest 'smaller' page size?
>
> Not only, it depends on the device capabilities/support and should be a driver choice.

'should pick next closest" is a guideline to the driver. If user requests 
8KB while the device supports 4KB and 16KB, which one is closest?

It's probably safer to just say that it's a driver choice when the hinted page
size cannot be set?

> > > +struct vfio_device_feature_dma_logging_control {
> > > +	__aligned_u64 page_size;
> > > +	__u32 num_ranges;
> > > +	__u32 __reserved;
> > > +	__aligned_u64 ranges;
> > > +};
> >
> > should we move the definition of LOG_MAX_RANGES to be here
> > so the user can know the max limits of tracked ranges?
> This was raised as an option as part of this mail thread.
> However, for now it seems redundant as we may not expect user space to hit this limit and it mainly comes to protect kernel from memory exploding by a malicious user.

No matter how realistic an user might hit an limitation, it doesn't
sound good to not expose it if existing.

> > > +
> > > +struct vfio_device_feature_dma_logging_range {
> > > +	__aligned_u64 iova;
> > > +	__aligned_u64 length;
> > > +};
> > > +
> > > +#define VFIO_DEVICE_FEATURE_DMA_LOGGING_START 3
> >
> > Can the user update the range list by doing another START?
>
> No, single start to ask the device what to track and a matching single stop should follow at the end.

let's document it then.

Thanks
Kevin