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

[Jason Gunthorpe <jgg@nvidia.com>]

On Thu, Aug 25, 2022 at 04:46:51PM -0600, Alex Williamson wrote:
> On Thu, 25 Aug 2022 23:26:04 +0100
> Joao Martins <joao.m.martins@oracle.com> wrote:
> 
> > On 8/25/22 21:49, Alex Williamson wrote:
> > > On Mon, 15 Aug 2022 18:11:04 +0300
> > > Yishai Hadas <yishaih@nvidia.com> wrote:  
> > >> +static int
> > >> +vfio_ioctl_device_feature_logging_report(struct vfio_device *device,
> > >> +					 u32 flags, void __user *arg,
> > >> +					 size_t argsz)
> > >> +{
> > >> +	size_t minsz =
> > >> +		offsetofend(struct vfio_device_feature_dma_logging_report,
> > >> +			    bitmap);
> > >> +	struct vfio_device_feature_dma_logging_report report;
> > >> +	struct iova_bitmap_iter iter;
> > >> +	int ret;
> > >> +
> > >> +	if (!device->log_ops)
> > >> +		return -ENOTTY;
> > >> +
> > >> +	ret = vfio_check_feature(flags, argsz,
> > >> +				 VFIO_DEVICE_FEATURE_GET,
> > >> +				 sizeof(report));
> > >> +	if (ret != 1)
> > >> +		return ret;
> > >> +
> > >> +	if (copy_from_user(&report, arg, minsz))
> > >> +		return -EFAULT;
> > >> +
> > >> +	if (report.page_size < PAGE_SIZE || !is_power_of_2(report.page_size))  
> > > 
> > > Why is PAGE_SIZE a factor here?  I'm under the impression that
> > > iova_bitmap is intended to handle arbitrary page sizes.  Thanks,  
> > 
> > Arbritary page sizes ... which are powers of 2. We use page shift in iova bitmap.
> > While it's not hard to lose this restriction (trading a shift over a slower mul)
> > ... I am not sure it is worth supporting said use considering that there aren't
> > non-powers of 2 page sizes right now?
> > 
> > The PAGE_SIZE restriction might be that it's supposed to be the lowest possible page_size.
> 
> Sorry, I was unclear.  Size relative to PAGE_SIZE was my only question,
> not that we shouldn't require power of 2 sizes.  We're adding device
> level dirty tracking, where the device page size granularity might be
> 4K on a host with a CPU 64K page size.  Maybe there's a use case for
> that.  Given the flexibility claimed by the iova_bitmap support,
> requiring reported page size less than system PAGE_SIZE seems
> unjustified.  Thanks,

+1

Jason