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@xxxxxxxxxx> wrote:
>
> > On 8/25/22 21:49, Alex Williamson wrote:
> > > On Mon, 15 Aug 2022 18:11:04 +0300
> > > Yishai Hadas <yishaih@xxxxxxxxxx> 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
