[PATCH vhost 0/2] vdpa/mlx5: Iova mapping related fixes

[PATCH vhost 0/2] vdpa/mlx5: Iova mapping related fixes

[Dragos Tatulea]

Here are 2 fixes from Si-Wei:
- The first one is an important fix that has to be applied as far
  back as possible (hence CC'ing linux-stable).
- The second is more of an improvement. That's why it doesn't have the
  Fixes tag.

I'd like to thank Si-Wei for the effort of finding and fixing these
issues. Especially the first issue which was very well hidden and
was there since day 1.

Si-Wei Liu (2):
  vdpa/mlx5: Fix PA offset with unaligned starting iotlb map
  vdpa/mlx5: Fix suboptimal range on iotlb iteration

 drivers/vdpa/mlx5/core/mr.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

-- 
2.46.1

[PATCH vhost 1/2] vdpa/mlx5: Fix PA offset with unaligned starting iotlb map

[Dragos Tatulea]

From: Si-Wei Liu <si-wei.liu@oracle.com>

When calculating the physical address range based on the iotlb and mr
[start,end) ranges, the offset of mr->start relative to map->start
is not taken into account. This leads to some incorrect and duplicate
mappings.

For the case when mr->start < map->start the code is already correct:
the range in [mr->start, map->start) was handled by a different
iteration.

Fixes: 94abbccdf291 ("vdpa/mlx5: Add shared memory registration code")
Cc: stable@vger.kernel.org
Signed-off-by: Si-Wei Liu <si-wei.liu@oracle.com>
Signed-off-by: Dragos Tatulea <dtatulea@nvidia.com>
---
 drivers/vdpa/mlx5/core/mr.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/drivers/vdpa/mlx5/core/mr.c b/drivers/vdpa/mlx5/core/mr.c
index 2dd21e0b399e..7d0c83b5b071 100644
--- a/drivers/vdpa/mlx5/core/mr.c
+++ b/drivers/vdpa/mlx5/core/mr.c
@@ -373,7 +373,7 @@ static int map_direct_mr(struct mlx5_vdpa_dev *mvdev, struct mlx5_vdpa_direct_mr
 	struct page *pg;
 	unsigned int nsg;
 	int sglen;
-	u64 pa;
+	u64 pa, offset;
 	u64 paend;
 	struct scatterlist *sg;
 	struct device *dma = mvdev->vdev.dma_dev;
@@ -396,8 +396,10 @@ static int map_direct_mr(struct mlx5_vdpa_dev *mvdev, struct mlx5_vdpa_direct_mr
 	sg = mr->sg_head.sgl;
 	for (map = vhost_iotlb_itree_first(iotlb, mr->start, mr->end - 1);
 	     map; map = vhost_iotlb_itree_next(map, mr->start, mr->end - 1)) {
-		paend = map->addr + maplen(map, mr);
-		for (pa = map->addr; pa < paend; pa += sglen) {
+		offset = mr->start > map->start ? mr->start - map->start : 0;
+		pa = map->addr + offset;
+		paend = map->addr + offset + maplen(map, mr);
+		for (; pa < paend; pa += sglen) {
 			pg = pfn_to_page(__phys_to_pfn(pa));
 			if (!sg) {
 				mlx5_vdpa_warn(mvdev, "sg null. start 0x%llx, end 0x%llx\n",
-- 
2.46.1

[PATCH vhost 2/2] vdpa/mlx5: Fix suboptimal range on iotlb iteration

[Dragos Tatulea]

From: Si-Wei Liu <si-wei.liu@oracle.com>

The starting iova address to iterate iotlb map entry within a range
was set to an irrelevant value when passing to the itree_next()
iterator, although luckily it doesn't affect the outcome of finding
out the granule of the smallest iotlb map size. Fix the code to make
it consistent with the following for-loop.

Fixes: 94abbccdf291 ("vdpa/mlx5: Add shared memory registration code")
Signed-off-by: Si-Wei Liu <si-wei.liu@oracle.com>
Signed-off-by: Dragos Tatulea <dtatulea@nvidia.com>
---
 drivers/vdpa/mlx5/core/mr.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/vdpa/mlx5/core/mr.c b/drivers/vdpa/mlx5/core/mr.c
index 7d0c83b5b071..8455f08f5d40 100644
--- a/drivers/vdpa/mlx5/core/mr.c
+++ b/drivers/vdpa/mlx5/core/mr.c
@@ -368,7 +368,6 @@ static int map_direct_mr(struct mlx5_vdpa_dev *mvdev, struct mlx5_vdpa_direct_mr
 	unsigned long lgcd = 0;
 	int log_entity_size;
 	unsigned long size;
-	u64 start = 0;
 	int err;
 	struct page *pg;
 	unsigned int nsg;
@@ -379,10 +378,9 @@ static int map_direct_mr(struct mlx5_vdpa_dev *mvdev, struct mlx5_vdpa_direct_mr
 	struct device *dma = mvdev->vdev.dma_dev;
 
 	for (map = vhost_iotlb_itree_first(iotlb, mr->start, mr->end - 1);
-	     map; map = vhost_iotlb_itree_next(map, start, mr->end - 1)) {
+	     map; map = vhost_iotlb_itree_next(map, mr->start, mr->end - 1)) {
 		size = maplen(map, mr);
 		lgcd = gcd(lgcd, size);
-		start += size;
 	}
 	log_entity_size = ilog2(lgcd);
 
-- 
2.46.1

Re: [PATCH vhost 1/2] vdpa/mlx5: Fix PA offset with unaligned starting iotlb map

[Jason Wang]

On Mon, Oct 21, 2024 at 9:41 PM Dragos Tatulea <dtatulea@nvidia.com> wrote:
>
> From: Si-Wei Liu <si-wei.liu@oracle.com>
>
> When calculating the physical address range based on the iotlb and mr
> [start,end) ranges, the offset of mr->start relative to map->start
> is not taken into account. This leads to some incorrect and duplicate
> mappings.
>
> For the case when mr->start < map->start the code is already correct:
> the range in [mr->start, map->start) was handled by a different
> iteration.
>
> Fixes: 94abbccdf291 ("vdpa/mlx5: Add shared memory registration code")
> Cc: stable@vger.kernel.org
> Signed-off-by: Si-Wei Liu <si-wei.liu@oracle.com>
> Signed-off-by: Dragos Tatulea <dtatulea@nvidia.com>
> ---

Acked-by: Jason Wang <jasowang@redhat.com>

Thanks

Re: [PATCH vhost 2/2] vdpa/mlx5: Fix suboptimal range on iotlb iteration

[Jason Wang]

On Mon, Oct 21, 2024 at 9:41 PM Dragos Tatulea <dtatulea@nvidia.com> wrote:
>
> From: Si-Wei Liu <si-wei.liu@oracle.com>
>
> The starting iova address to iterate iotlb map entry within a range
> was set to an irrelevant value when passing to the itree_next()
> iterator, although luckily it doesn't affect the outcome of finding
> out the granule of the smallest iotlb map size. Fix the code to make
> it consistent with the following for-loop.
>
> Fixes: 94abbccdf291 ("vdpa/mlx5: Add shared memory registration code")
> Signed-off-by: Si-Wei Liu <si-wei.liu@oracle.com>
> Signed-off-by: Dragos Tatulea <dtatulea@nvidia.com>
> ---

Acked-by: Jason Wang <jasowang@redhat.com>

Thanks

Re: [PATCH vhost 0/2] vdpa/mlx5: Iova mapping related fixes

[Dragos Tatulea]

On 21.10.24 15:40, Dragos Tatulea wrote:
> Here are 2 fixes from Si-Wei:
> - The first one is an important fix that has to be applied as far
>   back as possible (hence CC'ing linux-stable).
> - The second is more of an improvement. That's why it doesn't have the
>   Fixes tag.
> 
> I'd like to thank Si-Wei for the effort of finding and fixing these
> issues. Especially the first issue which was very well hidden and
> was there since day 1.
> 
> Si-Wei Liu (2):
>   vdpa/mlx5: Fix PA offset with unaligned starting iotlb map
>   vdpa/mlx5: Fix suboptimal range on iotlb iteration
> 
>  drivers/vdpa/mlx5/core/mr.c | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)
>
Gentle nudge for a review. The bug fixed by the first patch is a very
serious and insidious one.

Thanks,
Dragos
 

Re: [PATCH vhost 0/2] vdpa/mlx5: Iova mapping related fixes

[Jason Wang]

On Mon, Nov 11, 2024 at 4:58 PM Dragos Tatulea <dtatulea@nvidia.com> wrote:
>
>
>
> On 21.10.24 15:40, Dragos Tatulea wrote:
> > Here are 2 fixes from Si-Wei:
> > - The first one is an important fix that has to be applied as far
> >   back as possible (hence CC'ing linux-stable).
> > - The second is more of an improvement. That's why it doesn't have the
> >   Fixes tag.
> >
> > I'd like to thank Si-Wei for the effort of finding and fixing these
> > issues. Especially the first issue which was very well hidden and
> > was there since day 1.
> >
> > Si-Wei Liu (2):
> >   vdpa/mlx5: Fix PA offset with unaligned starting iotlb map
> >   vdpa/mlx5: Fix suboptimal range on iotlb iteration
> >
> >  drivers/vdpa/mlx5/core/mr.c | 12 ++++++------
> >  1 file changed, 6 insertions(+), 6 deletions(-)
> >
> Gentle nudge for a review. The bug fixed by the first patch is a very
> serious and insidious one.

I think I've acked to those patches, have you received that?

Thanks

>
> Thanks,
> Dragos
>
>

Re: [PATCH vhost 2/2] vdpa/mlx5: Fix suboptimal range on iotlb iteration

[Michael S. Tsirkin]

On Mon, Oct 21, 2024 at 04:40:40PM +0300, Dragos Tatulea wrote:
> From: Si-Wei Liu <si-wei.liu@oracle.com>
> 
> The starting iova address to iterate iotlb map entry within a range
> was set to an irrelevant value when passing to the itree_next()
> iterator, although luckily it doesn't affect the outcome of finding
> out the granule of the smallest iotlb map size. Fix the code to make
> it consistent with the following for-loop.
> 
> Fixes: 94abbccdf291 ("vdpa/mlx5: Add shared memory registration code")


But the cover letter says "that's why it does not have a fixes tag".
Confused.

> Signed-off-by: Si-Wei Liu <si-wei.liu@oracle.com>
> Signed-off-by: Dragos Tatulea <dtatulea@nvidia.com>
> ---
>  drivers/vdpa/mlx5/core/mr.c | 4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)
> 
> diff --git a/drivers/vdpa/mlx5/core/mr.c b/drivers/vdpa/mlx5/core/mr.c
> index 7d0c83b5b071..8455f08f5d40 100644
> --- a/drivers/vdpa/mlx5/core/mr.c
> +++ b/drivers/vdpa/mlx5/core/mr.c
> @@ -368,7 +368,6 @@ static int map_direct_mr(struct mlx5_vdpa_dev *mvdev, struct mlx5_vdpa_direct_mr
>  	unsigned long lgcd = 0;
>  	int log_entity_size;
>  	unsigned long size;
> -	u64 start = 0;
>  	int err;
>  	struct page *pg;
>  	unsigned int nsg;
> @@ -379,10 +378,9 @@ static int map_direct_mr(struct mlx5_vdpa_dev *mvdev, struct mlx5_vdpa_direct_mr
>  	struct device *dma = mvdev->vdev.dma_dev;
>  
>  	for (map = vhost_iotlb_itree_first(iotlb, mr->start, mr->end - 1);
> -	     map; map = vhost_iotlb_itree_next(map, start, mr->end - 1)) {
> +	     map; map = vhost_iotlb_itree_next(map, mr->start, mr->end - 1)) {
>  		size = maplen(map, mr);
>  		lgcd = gcd(lgcd, size);
> -		start += size;
>  	}
>  	log_entity_size = ilog2(lgcd);
>  
> -- 
> 2.46.1

Re: [PATCH vhost 0/2] vdpa/mlx5: Iova mapping related fixes

[Michael S. Tsirkin]

On Wed, Nov 13, 2024 at 09:45:22AM +0800, Jason Wang wrote:
> On Mon, Nov 11, 2024 at 4:58 PM Dragos Tatulea <dtatulea@nvidia.com> wrote:
> >
> >
> >
> > On 21.10.24 15:40, Dragos Tatulea wrote:
> > > Here are 2 fixes from Si-Wei:
> > > - The first one is an important fix that has to be applied as far
> > >   back as possible (hence CC'ing linux-stable).
> > > - The second is more of an improvement. That's why it doesn't have the
> > >   Fixes tag.
> > >
> > > I'd like to thank Si-Wei for the effort of finding and fixing these
> > > issues. Especially the first issue which was very well hidden and
> > > was there since day 1.
> > >
> > > Si-Wei Liu (2):
> > >   vdpa/mlx5: Fix PA offset with unaligned starting iotlb map
> > >   vdpa/mlx5: Fix suboptimal range on iotlb iteration
> > >
> > >  drivers/vdpa/mlx5/core/mr.c | 12 ++++++------
> > >  1 file changed, 6 insertions(+), 6 deletions(-)
> > >
> > Gentle nudge for a review. The bug fixed by the first patch is a very
> > serious and insidious one.
> 
> I think I've acked to those patches, have you received that?
> 
> Thanks

I saw your acks Jason, thanks!
Patch 1 is now upstream. Patch 2 is queued but I asked a question about
it.


> >
> > Thanks,
> > Dragos
> >
> >

Re: [PATCH vhost 2/2] vdpa/mlx5: Fix suboptimal range on iotlb iteration

[Dragos Tatulea]

On 13.11.24 07:32, Michael S. Tsirkin wrote:
> On Mon, Oct 21, 2024 at 04:40:40PM +0300, Dragos Tatulea wrote:
>> From: Si-Wei Liu <si-wei.liu@oracle.com>
>>
>> The starting iova address to iterate iotlb map entry within a range
>> was set to an irrelevant value when passing to the itree_next()
>> iterator, although luckily it doesn't affect the outcome of finding
>> out the granule of the smallest iotlb map size. Fix the code to make
>> it consistent with the following for-loop.
>>
>> Fixes: 94abbccdf291 ("vdpa/mlx5: Add shared memory registration code")
> 
> 
> But the cover letter says "that's why it does not have a fixes tag".
> Confused.
Sorry about that. Patch is fine with fixes tag, I forgot to drop that
part of the sentence from the cover letter.

Let me know if I need to resend something.

Thanks,
Dragos

Re: [PATCH vhost 2/2] vdpa/mlx5: Fix suboptimal range on iotlb iteration

[Michael S. Tsirkin]

On Wed, Nov 13, 2024 at 03:33:35PM +0100, Dragos Tatulea wrote:
> 
> 
> On 13.11.24 07:32, Michael S. Tsirkin wrote:
> > On Mon, Oct 21, 2024 at 04:40:40PM +0300, Dragos Tatulea wrote:
> >> From: Si-Wei Liu <si-wei.liu@oracle.com>
> >>
> >> The starting iova address to iterate iotlb map entry within a range
> >> was set to an irrelevant value when passing to the itree_next()
> >> iterator, although luckily it doesn't affect the outcome of finding
> >> out the granule of the smallest iotlb map size. Fix the code to make
> >> it consistent with the following for-loop.
> >>
> >> Fixes: 94abbccdf291 ("vdpa/mlx5: Add shared memory registration code")
> > 
> > 
> > But the cover letter says "that's why it does not have a fixes tag".
> > Confused.
> Sorry about that. Patch is fine with fixes tag, I forgot to drop that
> part of the sentence from the cover letter.
> 
> Let me know if I need to resend something.
> 
> Thanks,
> Dragos

But why does it need the fixes tag? That one means "if you have
that hash, you need this patch". Pls do not abuse it for
optimizations.

Re: [PATCH vhost 2/2] vdpa/mlx5: Fix suboptimal range on iotlb iteration

[Dragos Tatulea]

On 13.11.24 15:49, Michael S. Tsirkin wrote:
> On Wed, Nov 13, 2024 at 03:33:35PM +0100, Dragos Tatulea wrote:
>>
>>
>> On 13.11.24 07:32, Michael S. Tsirkin wrote:
>>> On Mon, Oct 21, 2024 at 04:40:40PM +0300, Dragos Tatulea wrote:
>>>> From: Si-Wei Liu <si-wei.liu@oracle.com>
>>>>
>>>> The starting iova address to iterate iotlb map entry within a range
>>>> was set to an irrelevant value when passing to the itree_next()
>>>> iterator, although luckily it doesn't affect the outcome of finding
>>>> out the granule of the smallest iotlb map size. Fix the code to make
>>>> it consistent with the following for-loop.
>>>>
>>>> Fixes: 94abbccdf291 ("vdpa/mlx5: Add shared memory registration code")
>>>
>>>
>>> But the cover letter says "that's why it does not have a fixes tag".
>>> Confused.
>> Sorry about that. Patch is fine with fixes tag, I forgot to drop that
>> part of the sentence from the cover letter.
>>
>> Let me know if I need to resend something.
>>
>> Thanks,
>> Dragos
> 
> But why does it need the fixes tag? That one means "if you have
> that hash, you need this patch". Pls do not abuse it for
> optimizations.
> 
Well, it is a fix but it happens that the code around still works without
this fix. I figured that it would be better to take it into older stable kernels
just like the other one. But if you consider it an improvement I will send a v2
without the Fixes tag.

Thanks,
Dragos

Re: [PATCH vhost 2/2] vdpa/mlx5: Fix suboptimal range on iotlb iteration

[Michael S. Tsirkin]

On Wed, Nov 13, 2024 at 04:01:05PM +0100, Dragos Tatulea wrote:
> 
> 
> On 13.11.24 15:49, Michael S. Tsirkin wrote:
> > On Wed, Nov 13, 2024 at 03:33:35PM +0100, Dragos Tatulea wrote:
> >>
> >>
> >> On 13.11.24 07:32, Michael S. Tsirkin wrote:
> >>> On Mon, Oct 21, 2024 at 04:40:40PM +0300, Dragos Tatulea wrote:
> >>>> From: Si-Wei Liu <si-wei.liu@oracle.com>
> >>>>
> >>>> The starting iova address to iterate iotlb map entry within a range
> >>>> was set to an irrelevant value when passing to the itree_next()
> >>>> iterator, although luckily it doesn't affect the outcome of finding
> >>>> out the granule of the smallest iotlb map size. Fix the code to make
> >>>> it consistent with the following for-loop.
> >>>>
> >>>> Fixes: 94abbccdf291 ("vdpa/mlx5: Add shared memory registration code")
> >>>
> >>>
> >>> But the cover letter says "that's why it does not have a fixes tag".
> >>> Confused.
> >> Sorry about that. Patch is fine with fixes tag, I forgot to drop that
> >> part of the sentence from the cover letter.
> >>
> >> Let me know if I need to resend something.
> >>
> >> Thanks,
> >> Dragos
> > 
> > But why does it need the fixes tag? That one means "if you have
> > that hash, you need this patch". Pls do not abuse it for
> > optimizations.
> > 
> Well, it is a fix but it happens that the code around still works without
> this fix. I figured that it would be better to take it into older stable kernels
> just like the other one. But if you consider it an improvement I will send a v2
> without the Fixes tag.
> 
> Thanks,
> Dragos

No need.