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

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

Jun 16, 2020, 10:15 PM

Post #1 of 14 (241 views)

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

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

mst at redhat

Aug 5, 2020, 12:15 PM

Post #2 of 14 (241 views)

Permalink

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 1/4] vdpa: introduce config op to get valid iova range [ In reply to ]

jasowang at redhat

Aug 5, 2020, 10:15 PM

Post #3 of 14 (241 views)

Permalink

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 1/4] vdpa: introduce config op to get valid iova range [ In reply to ]

mst at redhat

Aug 6, 2020, 12:15 AM

Post #4 of 14 (241 views)

Permalink

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 1/4] vdpa: introduce config op to get valid iova range [ In reply to ]

eli at mellanox

Aug 6, 2020, 10:15 AM

Post #5 of 14 (240 views)

Permalink

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 [ In reply to ]

eli at mellanox

Aug 6, 2020, 10:15 AM

Post #6 of 14 (240 views)

Permalink

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 [ In reply to ]

eli at mellanox

Aug 6, 2020, 10:15 AM

Post #7 of 14 (240 views)

Permalink

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 [ In reply to ]

mst at redhat

Aug 6, 2020, 10:15 AM

Post #8 of 14 (240 views)

Permalink

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 [ In reply to ]

jasowang at redhat

Aug 6, 2020, 8:15 PM

Post #9 of 14 (239 views)

Permalink

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 [ In reply to ]

jasowang at redhat

Aug 6, 2020, 10:15 PM

Post #10 of 14 (238 views)

Permalink

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 [ In reply to ]

mst at redhat

Aug 10, 2020, 6:15 AM

Post #11 of 14 (236 views)

Permalink

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 [ In reply to ]

jasowang at redhat

Aug 10, 2020, 8:15 PM

Post #12 of 14 (236 views)

Permalink

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 [ In reply to ]

mst at redhat

Aug 11, 2020, 2:15 AM

Post #13 of 14 (234 views)

Permalink

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 [ In reply to ]

jasowang at redhat

Aug 11, 2020, 10:15 PM

Post #14 of 14 (223 views)

Permalink

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

>
>
>