Virtualization - Aug 2020 - [PATCH 1/4] vdpa: introduce config op to get valid iova range

Hi All:

This series introduces API for reporing IOVA range. This is a must for
userspace to work correclty:

- for the process that uses vhost-vDPA directly to properly allocate
  IOVA
- for VM(qemu), when vIOMMU is not enabled, fail early if GPA is out
  of range
- for VM(qemu), when vIOMMU is enabled, determine a valid guest
  address width

Please review.

Thanks

Jason Wang (4):
  vdpa: introduce config op to get valid iova range
  vdpa_sim: implement get_iova_range bus operation
  vdpa: get_iova_range() is mandatory for device specific DMA
    translation
  vhost: vdpa: report iova range

 drivers/vdpa/vdpa.c              |  4 ++++
 drivers/vdpa/vdpa_sim/vdpa_sim.c | 11 +++++++++++
 drivers/vhost/vdpa.c             | 27 +++++++++++++++++++++++++++
 include/linux/vdpa.h             | 14 ++++++++++++++
 include/uapi/linux/vhost.h       |  4 ++++
 include/uapi/linux/vhost_types.h |  5 +++++
 6 files changed, 65 insertions(+)

-- 
2.20.1

This patch introduce a config op to get valid iova range from the vDPA
device.

Signed-off-by: Jason Wang <jasowang at redhat.com>
---
 include/linux/vdpa.h | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/include/linux/vdpa.h b/include/linux/vdpa.h
index 239db794357c..b7633ed2500c 100644
--- a/include/linux/vdpa.h
+++ b/include/linux/vdpa.h
@@ -41,6 +41,16 @@ struct vdpa_device {
 	unsigned int index;
 };
 
+/**
+ * vDPA IOVA range - the IOVA range support by the device
+ * @start: start of the IOVA range
+ * @end: end of the IOVA range
+ */
+struct vdpa_iova_range {
+	u64 start;
+	u64 end;
+};
+
 /**
  * vDPA_config_ops - operations for configuring a vDPA device.
  * Note: vDPA device drivers are required to implement all of the
@@ -134,6 +144,9 @@ struct vdpa_device {
  * @get_generation:		Get device config generation (optional)
  *				@vdev: vdpa device
  *				Returns u32: device generation
+ * @get_iova_range:		Get supported iova range (on-chip IOMMU)
+ *				@vdev: vdpa device
+ *				Returns the iova range supported by the device
  * @set_map:			Set device memory mapping (optional)
  *				Needed for device that using device
  *				specific DMA translation (on-chip IOMMU)
@@ -195,6 +208,7 @@ struct vdpa_config_ops {
 	void (*set_config)(struct vdpa_device *vdev, unsigned int offset,
 			   const void *buf, unsigned int len);
 	u32 (*get_generation)(struct vdpa_device *vdev);
+	struct vdpa_iova_range (*get_iova_range)(struct vdpa_device *vdev);
 
 	/* DMA ops */
 	int (*set_map)(struct vdpa_device *vdev, struct vhost_iotlb *iotlb);
-- 
2.20.1

This patch implements get_iova_range method for vdpa_sim. Since
vdpa_sim is a software device, simply advertise a [0ULL, ~0ULL] range.

Signed-off-by: Jason Wang <jasowang at redhat.com>
---
 drivers/vdpa/vdpa_sim/vdpa_sim.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/drivers/vdpa/vdpa_sim/vdpa_sim.c b/drivers/vdpa/vdpa_sim/vdpa_sim.c
index c7334cc65bb2..b3a6dc5b9984 100644
--- a/drivers/vdpa/vdpa_sim/vdpa_sim.c
+++ b/drivers/vdpa/vdpa_sim/vdpa_sim.c
@@ -524,6 +524,16 @@ static u32 vdpasim_get_generation(struct vdpa_device *vdpa)
 	return vdpasim->generation;
 }
 
+static struct vdpa_iova_range vdpasim_get_iova_range(struct vdpa_device *vdpa)
+{
+	struct vdpa_iova_range range;
+
+	range.start = 0ULL;
+	range.end = ~0ULL;
+
+	return range;
+}
+
 static int vdpasim_set_map(struct vdpa_device *vdpa,
 			   struct vhost_iotlb *iotlb)
 {
@@ -597,6 +607,7 @@ static const struct vdpa_config_ops vdpasim_net_config_ops =
{
 	.get_config             = vdpasim_get_config,
 	.set_config             = vdpasim_set_config,
 	.get_generation         = vdpasim_get_generation,
+	.get_iova_range         = vdpasim_get_iova_range,
 	.set_map                = vdpasim_set_map,
 	.dma_map                = vdpasim_dma_map,
 	.dma_unmap              = vdpasim_dma_unmap,
-- 
2.20.1

In order to let userspace work correctly, get_iova_range() is a must
for the device that has its own DMA translation logic.

Signed-off-by: Jason Wang <jasowang at redhat.com>
---
 drivers/vdpa/vdpa.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/vdpa/vdpa.c b/drivers/vdpa/vdpa.c
index de211ef3738c..ab7af978ef70 100644
--- a/drivers/vdpa/vdpa.c
+++ b/drivers/vdpa/vdpa.c
@@ -82,6 +82,10 @@ struct vdpa_device *__vdpa_alloc_device(struct device
*parent,
 	if (!!config->dma_map != !!config->dma_unmap)
 		goto err;
 
+	if ((config->dma_map || config->set_map) &&
+	    !config->get_iova_range)
+		goto err;
+
 	err = -ENOMEM;
 	vdev = kzalloc(size, GFP_KERNEL);
 	if (!vdev)
-- 
2.20.1

This patch introduces a new ioctl for vhost-vdpa device that can
report the iova range by the device. For device that depends on
platform IOMMU, we fetch the iova range via DOMAIN_ATTR_GEOMETRY. For
devices that has its own DMA translation unit, we fetch it directly
from vDPA bus operation.

Signed-off-by: Jason Wang <jasowang at redhat.com>
---
 drivers/vhost/vdpa.c             | 27 +++++++++++++++++++++++++++
 include/uapi/linux/vhost.h       |  4 ++++
 include/uapi/linux/vhost_types.h |  5 +++++
 3 files changed, 36 insertions(+)

diff --git a/drivers/vhost/vdpa.c b/drivers/vhost/vdpa.c
index 77a0c9fb6cc3..ad23e66cbf57 100644
--- a/drivers/vhost/vdpa.c
+++ b/drivers/vhost/vdpa.c
@@ -332,6 +332,30 @@ static long vhost_vdpa_set_config_call(struct vhost_vdpa
*v, u32 __user *argp)
 
 	return 0;
 }
+
+static long vhost_vdpa_get_iova_range(struct vhost_vdpa *v, u32 __user *argp)
+{
+	struct iommu_domain_geometry geo;
+	struct vdpa_device *vdpa = v->vdpa;
+	const struct vdpa_config_ops *ops = vdpa->config;
+	struct vhost_vdpa_iova_range range;
+	struct vdpa_iova_range vdpa_range;
+
+	if (!ops->set_map && !ops->dma_map) {
+		iommu_domain_get_attr(v->domain,
+				      DOMAIN_ATTR_GEOMETRY, &geo);
+		range.start = geo.aperture_start;
+		range.end = geo.aperture_end;
+	} else {
+		vdpa_range = ops->get_iova_range(vdpa);
+		range.start = vdpa_range.start;
+		range.end = vdpa_range.end;
+	}
+
+	return copy_to_user(argp, &range, sizeof(range));
+
+}
+
 static long vhost_vdpa_vring_ioctl(struct vhost_vdpa *v, unsigned int cmd,
 				   void __user *argp)
 {
@@ -442,6 +466,9 @@ static long vhost_vdpa_unlocked_ioctl(struct file *filep,
 	case VHOST_VDPA_SET_CONFIG_CALL:
 		r = vhost_vdpa_set_config_call(v, argp);
 		break;
+	case VHOST_VDPA_GET_IOVA_RANGE:
+		r = vhost_vdpa_get_iova_range(v, argp);
+		break;
 	default:
 		r = vhost_dev_ioctl(&v->vdev, cmd, argp);
 		if (r == -ENOIOCTLCMD)
diff --git a/include/uapi/linux/vhost.h b/include/uapi/linux/vhost.h
index 0c2349612e77..850956980e27 100644
--- a/include/uapi/linux/vhost.h
+++ b/include/uapi/linux/vhost.h
@@ -144,4 +144,8 @@
 
 /* Set event fd for config interrupt*/
 #define VHOST_VDPA_SET_CONFIG_CALL	_IOW(VHOST_VIRTIO, 0x77, int)
+
+/* Get the valid iova range */
+#define VHOST_VDPA_GET_IOVA_RANGE	_IOW(VHOST_VIRTIO, 0x78, \
+					     struct vhost_vdpa_iova_range)
 #endif
diff --git a/include/uapi/linux/vhost_types.h b/include/uapi/linux/vhost_types.h
index 669457ce5c48..4025b5a36177 100644
--- a/include/uapi/linux/vhost_types.h
+++ b/include/uapi/linux/vhost_types.h
@@ -127,6 +127,11 @@ struct vhost_vdpa_config {
 	__u8 buf[0];
 };
 
+struct vhost_vdpa_iova_range {
+	__u64 start;
+	__u64 end;
+};
+
 /* Feature bits */
 /* Log all write descriptors. Can be changed while device is active. */
 #define VHOST_F_LOG_ALL 26
-- 
2.20.1

On Wed, Jun 17, 2020 at 11:29:44AM +0800, Jason Wang
wrote:
> This patch introduce a config op to get valid iova range from the vDPA
> device.
> 
> Signed-off-by: Jason Wang <jasowang at redhat.com>
> ---
>  include/linux/vdpa.h | 14 ++++++++++++++
>  1 file changed, 14 insertions(+)
> 
> diff --git a/include/linux/vdpa.h b/include/linux/vdpa.h
> index 239db794357c..b7633ed2500c 100644
> --- a/include/linux/vdpa.h
> +++ b/include/linux/vdpa.h
> @@ -41,6 +41,16 @@ struct vdpa_device {
>  	unsigned int index;
>  };
>  
> +/**
> + * vDPA IOVA range - the IOVA range support by the device
> + * @start: start of the IOVA range
> + * @end: end of the IOVA range
> + */
> +struct vdpa_iova_range {
> +	u64 start;
> +	u64 end;
> +};
> +

This is ambiguous. Is end in the range or just behind it?
How about first/last?


>  /**
>   * vDPA_config_ops - operations for configuring a vDPA device.
>   * Note: vDPA device drivers are required to implement all of the
> @@ -134,6 +144,9 @@ struct vdpa_device {
>   * @get_generation:		Get device config generation (optional)
>   *				@vdev: vdpa device
>   *				Returns u32: device generation
> + * @get_iova_range:		Get supported iova range (on-chip IOMMU)
> + *				@vdev: vdpa device
> + *				Returns the iova range supported by the device
>   * @set_map:			Set device memory mapping (optional)
>   *				Needed for device that using device
>   *				specific DMA translation (on-chip IOMMU)
> @@ -195,6 +208,7 @@ struct vdpa_config_ops {
>  	void (*set_config)(struct vdpa_device *vdev, unsigned int offset,
>  			   const void *buf, unsigned int len);
>  	u32 (*get_generation)(struct vdpa_device *vdev);
> +	struct vdpa_iova_range (*get_iova_range)(struct vdpa_device *vdev);
>  
>  	/* DMA ops */
>  	int (*set_map)(struct vdpa_device *vdev, struct vhost_iotlb *iotlb);
> -- 
> 2.20.1

On Wed, Jun 17, 2020 at 11:29:46AM +0800, Jason Wang
wrote:
> In order to let userspace work correctly, get_iova_range() is a must
> for the device that has its own DMA translation logic.
I guess you mean for a device.

However in absence of ths op, I don't see what is wrong with just
assuming device can access any address.
> 
> Signed-off-by: Jason Wang <jasowang at redhat.com>
> ---
>  drivers/vdpa/vdpa.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/vdpa/vdpa.c b/drivers/vdpa/vdpa.c
> index de211ef3738c..ab7af978ef70 100644
> --- a/drivers/vdpa/vdpa.c
> +++ b/drivers/vdpa/vdpa.c
> @@ -82,6 +82,10 @@ struct vdpa_device *__vdpa_alloc_device(struct device
*parent,
>  	if (!!config->dma_map != !!config->dma_unmap)
>  		goto err;
>  
> +	if ((config->dma_map || config->set_map) &&
> +	    !config->get_iova_range)
> +		goto err;
> +
>  	err = -ENOMEM;
>  	vdev = kzalloc(size, GFP_KERNEL);
>  	if (!vdev)
What about devices using an IOMMU for translation?
IOMMUs generally have a limited IOVA range too, right?


> -- 
> 2.20.1

On Wed, Jun 17, 2020 at 11:29:47AM +0800, Jason Wang
wrote:
> This patch introduces a new ioctl for vhost-vdpa device that can
> report the iova range by the device. For device that depends on
> platform IOMMU, we fetch the iova range via DOMAIN_ATTR_GEOMETRY. For
> devices that has its own DMA translation unit, we fetch it directly
> from vDPA bus operation.
> 
> Signed-off-by: Jason Wang <jasowang at redhat.com>
> ---
>  drivers/vhost/vdpa.c             | 27 +++++++++++++++++++++++++++
>  include/uapi/linux/vhost.h       |  4 ++++
>  include/uapi/linux/vhost_types.h |  5 +++++
>  3 files changed, 36 insertions(+)
> 
> diff --git a/drivers/vhost/vdpa.c b/drivers/vhost/vdpa.c
> index 77a0c9fb6cc3..ad23e66cbf57 100644
> --- a/drivers/vhost/vdpa.c
> +++ b/drivers/vhost/vdpa.c
> @@ -332,6 +332,30 @@ static long vhost_vdpa_set_config_call(struct
vhost_vdpa *v, u32 __user *argp)
>  
>  	return 0;
>  }
> +
> +static long vhost_vdpa_get_iova_range(struct vhost_vdpa *v, u32 __user
*argp)
> +{
> +	struct iommu_domain_geometry geo;
> +	struct vdpa_device *vdpa = v->vdpa;
> +	const struct vdpa_config_ops *ops = vdpa->config;
> +	struct vhost_vdpa_iova_range range;
> +	struct vdpa_iova_range vdpa_range;
> +
> +	if (!ops->set_map && !ops->dma_map) {
Why not just check if (ops->get_iova_range) directly?



> +		iommu_domain_get_attr(v->domain,
> +				      DOMAIN_ATTR_GEOMETRY, &geo);
> +		range.start = geo.aperture_start;
> +		range.end = geo.aperture_end;
> +	} else {
> +		vdpa_range = ops->get_iova_range(vdpa);
> +		range.start = vdpa_range.start;
> +		range.end = vdpa_range.end;
> +	}
> +
> +	return copy_to_user(argp, &range, sizeof(range));
> +
> +}
> +
>  static long vhost_vdpa_vring_ioctl(struct vhost_vdpa *v, unsigned int cmd,
>  				   void __user *argp)
>  {
> @@ -442,6 +466,9 @@ static long vhost_vdpa_unlocked_ioctl(struct file
*filep,
>  	case VHOST_VDPA_SET_CONFIG_CALL:
>  		r = vhost_vdpa_set_config_call(v, argp);
>  		break;
> +	case VHOST_VDPA_GET_IOVA_RANGE:
> +		r = vhost_vdpa_get_iova_range(v, argp);
> +		break;
>  	default:
>  		r = vhost_dev_ioctl(&v->vdev, cmd, argp);
>  		if (r == -ENOIOCTLCMD)
> diff --git a/include/uapi/linux/vhost.h b/include/uapi/linux/vhost.h
> index 0c2349612e77..850956980e27 100644
> --- a/include/uapi/linux/vhost.h
> +++ b/include/uapi/linux/vhost.h
> @@ -144,4 +144,8 @@
>  
>  /* Set event fd for config interrupt*/
>  #define VHOST_VDPA_SET_CONFIG_CALL	_IOW(VHOST_VIRTIO, 0x77, int)
> +
> +/* Get the valid iova range */
> +#define VHOST_VDPA_GET_IOVA_RANGE	_IOW(VHOST_VIRTIO, 0x78, \
> +					     struct vhost_vdpa_iova_range)
>  #endif
> diff --git a/include/uapi/linux/vhost_types.h
b/include/uapi/linux/vhost_types.h
> index 669457ce5c48..4025b5a36177 100644
> --- a/include/uapi/linux/vhost_types.h
> +++ b/include/uapi/linux/vhost_types.h
> @@ -127,6 +127,11 @@ struct vhost_vdpa_config {
>  	__u8 buf[0];
>  };
>  
> +struct vhost_vdpa_iova_range {
> +	__u64 start;
> +	__u64 end;
> +};
> +

Pls document fields. And I think first/last is a better API ...
>  /* Feature bits */
>  /* Log all write descriptors. Can be changed while device is active. */
>  #define VHOST_F_LOG_ALL 26
> -- 
> 2.20.1

On Thu, Aug 06, 2020 at 03:03:55PM +0300, Eli Cohen
wrote:
> On Wed, Aug 05, 2020 at 08:51:56AM -0400, Michael S. Tsirkin wrote:
> > On Wed, Jun 17, 2020 at 11:29:44AM +0800, Jason Wang wrote:
> > > This patch introduce a config op to get valid iova range from the
vDPA
> > > device.
> > > 
> > > Signed-off-by: Jason Wang <jasowang at redhat.com>
> > > ---
> > >  include/linux/vdpa.h | 14 ++++++++++++++
> > >  1 file changed, 14 insertions(+)
> > > 
> > > diff --git a/include/linux/vdpa.h b/include/linux/vdpa.h
> > > index 239db794357c..b7633ed2500c 100644
> > > --- a/include/linux/vdpa.h
> > > +++ b/include/linux/vdpa.h
> > > @@ -41,6 +41,16 @@ struct vdpa_device {
> > >  	unsigned int index;
> > >  };
> > >  
> > > +/**
> > > + * vDPA IOVA range - the IOVA range support by the device
> > > + * @start: start of the IOVA range
> > > + * @end: end of the IOVA range
> > > + */
> > > +struct vdpa_iova_range {
> > > +	u64 start;
> > > +	u64 end;
> > > +};
> > > +
> > 
> > 
> > This is ambiguous. Is end in the range or just behind it?
> > How about first/last?
> 
> It is customary in the kernel to use start-end where end corresponds to
> the byte following the last in the range. See struct vm_area_struct
> vm_start and vm_end fields
Exactly my point:

include/linux/mm_types.h:       unsigned long vm_end;           /* The first
byte after our end address

in this case Jason wants it to be the last byte, not one behind.

> > 
> > 
> > 
> > >  /**
> > >   * vDPA_config_ops - operations for configuring a vDPA device.
> > >   * Note: vDPA device drivers are required to implement all of
the
> > > @@ -134,6 +144,9 @@ struct vdpa_device {
> > >   * @get_generation:		Get device config generation (optional)
> > >   *				@vdev: vdpa device
> > >   *				Returns u32: device generation
> > > + * @get_iova_range:		Get supported iova range (on-chip IOMMU)
> > > + *				@vdev: vdpa device
> > > + *				Returns the iova range supported by the device
> > >   * @set_map:			Set device memory mapping (optional)
> > >   *				Needed for device that using device
> > >   *				specific DMA translation (on-chip IOMMU)
> > > @@ -195,6 +208,7 @@ struct vdpa_config_ops {
> > >  	void (*set_config)(struct vdpa_device *vdev, unsigned int
offset,
> > >  			   const void *buf, unsigned int len);
> > >  	u32 (*get_generation)(struct vdpa_device *vdev);
> > > +	struct vdpa_iova_range (*get_iova_range)(struct vdpa_device
*vdev);
> > >  
> > >  	/* DMA ops */
> > >  	int (*set_map)(struct vdpa_device *vdev, struct vhost_iotlb
*iotlb);
> > > -- 
> > > 2.20.1
> >

On 2020/8/6 ??8:10, Eli Cohen wrote:
> On Wed, Jun 17, 2020 at 06:29:44AM +0300, Jason Wang wrote:
>> This patch introduce a config op to get valid iova range from the vDPA
>> device.
>>
>> Signed-off-by: Jason Wang<jasowang at redhat.com>
>> ---
>>   include/linux/vdpa.h | 14 ++++++++++++++
>>   1 file changed, 14 insertions(+)
>>
>> diff --git a/include/linux/vdpa.h b/include/linux/vdpa.h
>> index 239db794357c..b7633ed2500c 100644
>> --- a/include/linux/vdpa.h
>> +++ b/include/linux/vdpa.h
>> @@ -41,6 +41,16 @@ struct vdpa_device {
>>   	unsigned int index;
>>   };
>>   
>> +/**
>> + * vDPA IOVA range - the IOVA range support by the device
>> + * @start: start of the IOVA range
>> + * @end: end of the IOVA range
>> + */
>> +struct vdpa_iova_range {
>> +	u64 start;
>> +	u64 end;
>> +};
>> +
> What do you do with this information? Suppose some device tells you it
> supports some limited range, say, from 0x40000000 to 0x80000000. What
> does qemu do with this information?

For qemu, when qemu will fail the vDPA device creation when:

1) vIOMMU is not enabled and GPA is out of this range
2) vIOMMU is enabled but it can't report such range to guest

For other userspace application, it will know it can only use this range 
as its IOVA.

Thanks

On 2020/8/6 ??8:29, Michael S. Tsirkin wrote:
> On Thu, Aug 06, 2020 at 03:03:55PM +0300, Eli Cohen wrote:
>> On Wed, Aug 05, 2020 at 08:51:56AM -0400, Michael S. Tsirkin wrote:
>>> On Wed, Jun 17, 2020 at 11:29:44AM +0800, Jason Wang wrote:
>>>> This patch introduce a config op to get valid iova range from
the vDPA
>>>> device.
>>>>
>>>> Signed-off-by: Jason Wang<jasowang at redhat.com>
>>>> ---
>>>>   include/linux/vdpa.h | 14 ++++++++++++++
>>>>   1 file changed, 14 insertions(+)
>>>>
>>>> diff --git a/include/linux/vdpa.h b/include/linux/vdpa.h
>>>> index 239db794357c..b7633ed2500c 100644
>>>> --- a/include/linux/vdpa.h
>>>> +++ b/include/linux/vdpa.h
>>>> @@ -41,6 +41,16 @@ struct vdpa_device {
>>>>   	unsigned int index;
>>>>   };
>>>>   
>>>> +/**
>>>> + * vDPA IOVA range - the IOVA range support by the device
>>>> + * @start: start of the IOVA range
>>>> + * @end: end of the IOVA range
>>>> + */
>>>> +struct vdpa_iova_range {
>>>> +	u64 start;
>>>> +	u64 end;
>>>> +};
>>>> +
>>> This is ambiguous. Is end in the range or just behind it?
>>> How about first/last?
>> It is customary in the kernel to use start-end where end corresponds to
>> the byte following the last in the range. See struct vm_area_struct
>> vm_start and vm_end fields
> Exactly my point:
>
> include/linux/mm_types.h:       unsigned long vm_end;           /* The
first byte after our end address
>
> in this case Jason wants it to be the last byte, not one behind.

Ok, I somehow recall the reason :)

See:

struct iommu_domain_geometry {
 ??? dma_addr_t aperture_start; /* First address that can be mapped??? */
 ??? dma_addr_t aperture_end;?? /* Last address that can be mapped???? */
 ??? bool force_aperture;?????? /* DMA only allowed in mappable range? */
};


So what I proposed here is to be consistent with it.

Thanks

>
>

On Thu, Aug 06, 2020 at 03:43:54PM +0300, Eli Cohen
wrote:
> On Thu, Aug 06, 2020 at 08:29:22AM -0400, Michael S. Tsirkin wrote:
> > On Thu, Aug 06, 2020 at 03:03:55PM +0300, Eli Cohen wrote:
> > > On Wed, Aug 05, 2020 at 08:51:56AM -0400, Michael S. Tsirkin
wrote:
> > > > On Wed, Jun 17, 2020 at 11:29:44AM +0800, Jason Wang wrote:
> > > > > This patch introduce a config op to get valid iova
range from the vDPA
> > > > > device.
> > > > > 
> > > > > Signed-off-by: Jason Wang <jasowang at
redhat.com>
> > > > > ---
> > > > >  include/linux/vdpa.h | 14 ++++++++++++++
> > > > >  1 file changed, 14 insertions(+)
> > > > > 
> > > > > diff --git a/include/linux/vdpa.h
b/include/linux/vdpa.h
> > > > > index 239db794357c..b7633ed2500c 100644
> > > > > --- a/include/linux/vdpa.h
> > > > > +++ b/include/linux/vdpa.h
> > > > > @@ -41,6 +41,16 @@ struct vdpa_device {
> > > > >  	unsigned int index;
> > > > >  };
> > > > >  
> > > > > +/**
> > > > > + * vDPA IOVA range - the IOVA range support by the
device
> > > > > + * @start: start of the IOVA range
> > > > > + * @end: end of the IOVA range
> > > > > + */
> > > > > +struct vdpa_iova_range {
> > > > > +	u64 start;
> > > > > +	u64 end;
> > > > > +};
> > > > > +
> > > > 
> > > > 
> > > > This is ambiguous. Is end in the range or just behind it?
> > > > How about first/last?
> > > 
> > > It is customary in the kernel to use start-end where end
corresponds to
> > > the byte following the last in the range. See struct
vm_area_struct
> > > vm_start and vm_end fields
> > 
> > Exactly my point:
> > 
> > include/linux/mm_types.h:       unsigned long vm_end;           /* The
first byte after our end address
> > 
> > in this case Jason wants it to be the last byte, not one behind.
> > 
> > 
> Maybe start, size? Not ambiguous, and you don't need to do annoying
> calculations like size = last - start + 1
Size has a bunch of issues: can overlap, can not cover the entire 64 bit
range. The requisite checks are arguably easier to get wrong than
getting the size if you need it.

On 2020/8/10 ??8:05, Michael S. Tsirkin wrote:
> On Thu, Aug 06, 2020 at 03:43:54PM +0300, Eli Cohen wrote:
>> On Thu, Aug 06, 2020 at 08:29:22AM -0400, Michael S. Tsirkin wrote:
>>> On Thu, Aug 06, 2020 at 03:03:55PM +0300, Eli Cohen wrote:
>>>> On Wed, Aug 05, 2020 at 08:51:56AM -0400, Michael S. Tsirkin
wrote:
>>>>> On Wed, Jun 17, 2020 at 11:29:44AM +0800, Jason Wang wrote:
>>>>>> This patch introduce a config op to get valid iova
range from the vDPA
>>>>>> device.
>>>>>>
>>>>>> Signed-off-by: Jason Wang <jasowang at
redhat.com>
>>>>>> ---
>>>>>>   include/linux/vdpa.h | 14 ++++++++++++++
>>>>>>   1 file changed, 14 insertions(+)
>>>>>>
>>>>>> diff --git a/include/linux/vdpa.h
b/include/linux/vdpa.h
>>>>>> index 239db794357c..b7633ed2500c 100644
>>>>>> --- a/include/linux/vdpa.h
>>>>>> +++ b/include/linux/vdpa.h
>>>>>> @@ -41,6 +41,16 @@ struct vdpa_device {
>>>>>>   	unsigned int index;
>>>>>>   };
>>>>>>   
>>>>>> +/**
>>>>>> + * vDPA IOVA range - the IOVA range support by the
device
>>>>>> + * @start: start of the IOVA range
>>>>>> + * @end: end of the IOVA range
>>>>>> + */
>>>>>> +struct vdpa_iova_range {
>>>>>> +	u64 start;
>>>>>> +	u64 end;
>>>>>> +};
>>>>>> +
>>>>>
>>>>> This is ambiguous. Is end in the range or just behind it?
>>>>> How about first/last?
>>>> It is customary in the kernel to use start-end where end
corresponds to
>>>> the byte following the last in the range. See struct
vm_area_struct
>>>> vm_start and vm_end fields
>>> Exactly my point:
>>>
>>> include/linux/mm_types.h:       unsigned long vm_end;           /*
The first byte after our end address
>>>
>>> in this case Jason wants it to be the last byte, not one behind.
>>>
>>>
>> Maybe start, size? Not ambiguous, and you don't need to do annoying
>> calculations like size = last - start + 1
> Size has a bunch of issues: can overlap, can not cover the entire 64 bit
> range. The requisite checks are arguably easier to get wrong than
> getting the size if you need it.

Yes, so do you still prefer first/last or just begin/end which is 
consistent with iommu_domain_geometry?

Thanks

>