[PATCH 0/4] vDPA: API for reporting IOVA range

[PATCH 0/4] vDPA: API for reporting IOVA range

[Jason Wang]

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

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

[Jason Wang]

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

Signed-off-by: Jason Wang <jasowang@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

[PATCH 2/4] vdpa_sim: implement get_iova_range bus operation

[Jason Wang]

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@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

[PATCH 3/4] vdpa: get_iova_range() is mandatory for device specific DMA translation

[Jason Wang]

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@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

[PATCH 4/4] vhost: vdpa: report iova range

[Jason Wang]

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@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

Re: [PATCH 1/4] vdpa: introduce config op to get valid iova range

[Michael S. Tsirkin]

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@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

Re: [PATCH 3/4] vdpa: get_iova_range() is mandatory for device specific DMA translation

[Michael S. Tsirkin]

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@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

Re: [PATCH 4/4] vhost: vdpa: report iova range

[Michael S. Tsirkin]

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@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

Re: [PATCH 1/4] vdpa: introduce config op to get valid iova range

[Jason Wang]

On 2020/8/5 下午8:51, 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@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?


In the range.


> How about first/last?


Sure.

Thanks


>
>
>

Re: [PATCH 3/4] vdpa: get_iova_range() is mandatory for device specific DMA translation

[Jason Wang]

On 2020/8/5 下午8:55, Michael S. Tsirkin wrote:
> 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.


It's just for safe, if you want, we can assume any address without this op.


>
>> Signed-off-by: Jason Wang <jasowang@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?


See patch 4 which query the IOMMU geometry in this case:

+        iommu_domain_get_attr(v->domain,
+                      DOMAIN_ATTR_GEOMETRY, &geo);
+        range.start = geo.aperture_start;
+        range.end = geo.aperture_end;

Thanks


>
>
>
>> -- 
>> 2.20.1

Re: [PATCH 4/4] vhost: vdpa: report iova range

[Jason Wang]

On 2020/8/5 下午8:58, Michael S. Tsirkin wrote:
> 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@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?


Because set_map || dma_ops is a hint that the device has its own DMA 
translation logic.

Device without get_iova_range does not necessarily meant it use IOMMU 
driver.

Thanks


>
>
>
>
>> +		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

Re: [PATCH 1/4] vdpa: introduce config op to get valid iova range

[Michael S. Tsirkin]

On Thu, Aug 06, 2020 at 11:25:11AM +0800, Jason Wang wrote:
> 
> On 2020/8/5 下午8:51, 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@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?
> 
> 
> In the range.

OK I guess we can treat it as a bugfix and merge after rc1,
but pls add a bit more in the commit log about what's
currently broken.

> 
> > How about first/last?
> 
> 
> Sure.
> 
> Thanks
> 
> 
> > 
> > 
> > 

Re: [PATCH 4/4] vhost: vdpa: report iova range

[Michael S. Tsirkin]

On Thu, Aug 06, 2020 at 11:29:16AM +0800, Jason Wang wrote:
> 
> On 2020/8/5 下午8:58, Michael S. Tsirkin wrote:
> > 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@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?
> 
> 
> Because set_map || dma_ops is a hint that the device has its own DMA
> translation logic.
> 
> Device without get_iova_range does not necessarily meant it use IOMMU
> driver.
> 
> Thanks

OK let's add some code comments please, and check get_iova_range
is actually there before calling.

> 
> > 
> > 
> > 
> > 
> > > +		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

Re: [PATCH 1/4] vdpa: introduce config op to get valid iova range

[Eli Cohen]

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@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
> 
> 
> 
> >  /**
> >   * 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
> 

Re: [PATCH 1/4] vdpa: introduce config op to get valid iova range

[Eli Cohen]

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@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?

>  /**
>   * 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
> 

Re: [PATCH 1/4] vdpa: introduce config op to get valid iova range

[Michael S. Tsirkin]

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@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
> > 

Re: [PATCH 1/4] vdpa: introduce config op to get valid iova range

[Eli Cohen]

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@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

Re: [PATCH 1/4] vdpa: introduce config op to get valid iova range

[Jason Wang]

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@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

Re: [PATCH 1/4] vdpa: introduce config op to get valid iova range

[Jason Wang]

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@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


>
>

Re: [PATCH 1/4] vdpa: introduce config op to get valid iova range

[Michael S. Tsirkin]

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@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.

Re: [PATCH 1/4] vdpa: introduce config op to get valid iova range

[Jason Wang]

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@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


>

Re: [PATCH 1/4] vdpa: introduce config op to get valid iova range

[Michael S. Tsirkin]

On Tue, Aug 11, 2020 at 10:53:09AM +0800, Jason Wang wrote:
> 
> 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@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

I prefer first/last I think, these are unambiguous.
E.g.

        dma_addr_t aperture_start; /* First address that can be mapped    */
        dma_addr_t aperture_end;   /* Last address that can be mapped     */

instead of addressing ambiguity with a comment, let's just name the field well.



> 
> > 

Re: [PATCH 1/4] vdpa: introduce config op to get valid iova range

[Jason Wang]

On 2020/8/11 下午4:29, Michael S. Tsirkin wrote:
> On Tue, Aug 11, 2020 at 10:53:09AM +0800, Jason Wang wrote:
>> 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@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
> I prefer first/last I think, these are unambiguous.
> E.g.
>
>          dma_addr_t aperture_start; /* First address that can be mapped    */
>          dma_addr_t aperture_end;   /* Last address that can be mapped     */
>
> instead of addressing ambiguity with a comment, let's just name the field well.


Ok, will do.

Thanks



>
>
>

Re: [PATCH 0/4] vDPA: API for reporting IOVA range

[Michael S. Tsirkin]

On Wed, Jun 17, 2020 at 11:29:43AM +0800, Jason Wang wrote:
> 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

OK so what is the plan here? Change begin-end->first-last and repost?

> 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

Re: [PATCH 0/4] vDPA: API for reporting IOVA range

[Jason Wang]

On 2020/10/21 下午10:45, Michael S. Tsirkin wrote:
> On Wed, Jun 17, 2020 at 11:29:43AM +0800, Jason Wang wrote:
>> 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
> OK so what is the plan here? Change begin-end->first-last and repost?


I've posted V2 with this change, but it get some warning for buildbot.

Will post a V3.

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