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

[Joao Martins <joao.m.martins@oracle.com>]

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.

> 
> Alex
> 
>> +		return -EINVAL;
>> +
>> +	ret = iova_bitmap_iter_init(&iter, report.iova, report.length,
>> +				    report.page_size,
>> +				    u64_to_user_ptr(report.bitmap));
>> +	if (ret)
>> +		return ret;
>> +
>> +	for (; !iova_bitmap_iter_done(&iter) && !ret;
>> +	     ret = iova_bitmap_iter_advance(&iter)) {
>> +		ret = device->log_ops->log_read_and_clear(device,
>> +			iova_bitmap_iova(&iter),
>> +			iova_bitmap_length(&iter), &iter.dirty);
>> +		if (ret)
>> +			break;
>> +	}
>> +
>> +	iova_bitmap_iter_free(&iter);
>> +	return ret;
>> +}
>