iommu.lists.linux-foundation.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] iommu/dma: Fix IOVA reserve dma ranges
@ 2020-09-09  5:32 Srinath Mannam via iommu
  2020-09-09 12:05 ` Robin Murphy
  0 siblings, 1 reply; 3+ messages in thread
From: Srinath Mannam via iommu @ 2020-09-09  5:32 UTC (permalink / raw)
  To: Robin Murphy, Joerg Roedel, Lorenzo Pieralisi, Bjorn Helgaas, poza
  Cc: Srinath Mannam, iommu, bcm-kernel-feedback-list, linux-kernel

Fix IOVA reserve failure for memory regions listed in dma-ranges in the
following cases.

- start address of memory region is 0x0.
- end address of a memory region is equal to start address of next memory
  region.

Fixes: aadad097cd46f ("iommu/dma: Reserve IOVA for PCIe inaccessible DMA address")
Signed-off-by: Srinath Mannam <srinath.mannam@broadcom.com>
---
 drivers/iommu/dma-iommu.c | 15 +++++++++++----
 1 file changed, 11 insertions(+), 4 deletions(-)

diff --git a/drivers/iommu/dma-iommu.c b/drivers/iommu/dma-iommu.c
index 5141d49a046b..0a3f67a4f9ae 100644
--- a/drivers/iommu/dma-iommu.c
+++ b/drivers/iommu/dma-iommu.c
@@ -213,14 +213,21 @@ static int iova_reserve_pci_windows(struct pci_dev *dev,
 	resource_list_for_each_entry(window, &bridge->dma_ranges) {
 		end = window->res->start - window->offset;
 resv_iova:
+		if (end < start) {
+			/* dma_ranges list should be sorted */
+			dev_err(&dev->dev, "Failed to reserve IOVA\n");
+			return -EINVAL;
+		}
+		/*
+		 * Skip the cases when start address of first memory region is
+		 * 0x0 and end address of one memory region and start address
+		 * of next memory region are equal. Reserve IOVA for rest of
+		 * addresses fall in between given memory ranges.
+		 */
 		if (end > start) {
 			lo = iova_pfn(iovad, start);
 			hi = iova_pfn(iovad, end);
 			reserve_iova(iovad, lo, hi);
-		} else {
-			/* dma_ranges list should be sorted */
-			dev_err(&dev->dev, "Failed to reserve IOVA\n");
-			return -EINVAL;
 		}
 
 		start = window->res->end - window->offset + 1;
-- 
2.17.1

_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [PATCH] iommu/dma: Fix IOVA reserve dma ranges
  2020-09-09  5:32 [PATCH] iommu/dma: Fix IOVA reserve dma ranges Srinath Mannam via iommu
@ 2020-09-09 12:05 ` Robin Murphy
  2020-09-10  8:26   ` Srinath Mannam via iommu
  0 siblings, 1 reply; 3+ messages in thread
From: Robin Murphy @ 2020-09-09 12:05 UTC (permalink / raw)
  To: Srinath Mannam, Joerg Roedel, Lorenzo Pieralisi, Bjorn Helgaas, poza
  Cc: iommu, bcm-kernel-feedback-list, linux-kernel

On 2020-09-09 06:32, Srinath Mannam wrote:
> Fix IOVA reserve failure for memory regions listed in dma-ranges in the
> following cases.
> 
> - start address of memory region is 0x0.

That's fair enough, and in fact generalises to the case of zero-sized 
gaps between regions, which is indeed an oversight.

> - end address of a memory region is equal to start address of next memory
>    region.

This part doesn't make much sense, however - if the regions described in 
bridge->dma_ranges overlap, that's a bug in whoever created a malformed 
list to begin with. Possibly it's just poor wording, and you're using 
"memory regions" to refer to any or all of the dma_ranges, the reserved 
IOVA ranges, and what "start" and "end" in this function represent which 
isn't quite either of those.

> Fixes: aadad097cd46f ("iommu/dma: Reserve IOVA for PCIe inaccessible DMA address")
> Signed-off-by: Srinath Mannam <srinath.mannam@broadcom.com>
> ---
>   drivers/iommu/dma-iommu.c | 15 +++++++++++----
>   1 file changed, 11 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/iommu/dma-iommu.c b/drivers/iommu/dma-iommu.c
> index 5141d49a046b..0a3f67a4f9ae 100644
> --- a/drivers/iommu/dma-iommu.c
> +++ b/drivers/iommu/dma-iommu.c
> @@ -213,14 +213,21 @@ static int iova_reserve_pci_windows(struct pci_dev *dev,
>   	resource_list_for_each_entry(window, &bridge->dma_ranges) {
>   		end = window->res->start - window->offset;
>   resv_iova:
> +		if (end < start) {
> +			/* dma_ranges list should be sorted */
> +			dev_err(&dev->dev, "Failed to reserve IOVA\n");
> +			return -EINVAL;
> +		}
> +		/*
> +		 * Skip the cases when start address of first memory region is
> +		 * 0x0 and end address of one memory region and start address
> +		 * of next memory region are equal. Reserve IOVA for rest of
> +		 * addresses fall in between given memory ranges.
> +		 */
>   		if (end > start) {
>   			lo = iova_pfn(iovad, start);
>   			hi = iova_pfn(iovad, end);
>   			reserve_iova(iovad, lo, hi);
> -		} else {

Surely this only needs to be a one-liner?

-		} else {
+		} else if (end < start) {

(or possibly "end != start"; I can't quite decide which expresses the 
semantic intent better)

The rest just looks like unnecessary churn - I don't think it needs 
commenting that a sorted list may simply not have gaps between entries, 
and as above I think the wording of that comment is actively misleading.

Robin.

> -			/* dma_ranges list should be sorted */
> -			dev_err(&dev->dev, "Failed to reserve IOVA\n");
> -			return -EINVAL;
>   		}
>   
>   		start = window->res->end - window->offset + 1;
> 
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [PATCH] iommu/dma: Fix IOVA reserve dma ranges
  2020-09-09 12:05 ` Robin Murphy
@ 2020-09-10  8:26   ` Srinath Mannam via iommu
  0 siblings, 0 replies; 3+ messages in thread
From: Srinath Mannam via iommu @ 2020-09-10  8:26 UTC (permalink / raw)
  To: Robin Murphy
  Cc: poza, Linux Kernel Mailing List, iommu, BCM Kernel Feedback,
	Bjorn Helgaas

On Wed, Sep 9, 2020 at 5:35 PM Robin Murphy <robin.murphy@arm.com> wrote:
>
Hi Robin,
Thanks for review
> On 2020-09-09 06:32, Srinath Mannam wrote:
> > Fix IOVA reserve failure for memory regions listed in dma-ranges in the
> > following cases.
> >
> > - start address of memory region is 0x0.
>
> That's fair enough, and in fact generalises to the case of zero-sized
> gaps between regions, which is indeed an oversight.
Yes this is the main reason for the requirement of this fix.
>
> > - end address of a memory region is equal to start address of next memory
> >    region.
>
> This part doesn't make much sense, however - if the regions described in
> bridge->dma_ranges overlap, that's a bug in whoever created a malformed
> list to begin with. Possibly it's just poor wording, and you're using
> "memory regions" to refer to any or all of the dma_ranges, the reserved
> IOVA ranges, and what "start" and "end" in this function represent which
> isn't quite either of those.
You are right, this case is very unlikely that nobody lists regions with zero
gap, in such a case they will combine both the regions. Reason for highlighting
this point is, the same fix will handle this case also. Here I used memory
regions to refer entries of dma-ranges(allowed IOVA addresses range) not
reserved IOVA ranges. start and end variables in this function refers to
start and end addresses of reserved IOVA ranges which are derived from
dma ranges resources start and end values.
>
> > Fixes: aadad097cd46f ("iommu/dma: Reserve IOVA for PCIe inaccessible DMA address")
> > Signed-off-by: Srinath Mannam <srinath.mannam@broadcom.com>
> > ---
> >   drivers/iommu/dma-iommu.c | 15 +++++++++++----
> >   1 file changed, 11 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/iommu/dma-iommu.c b/drivers/iommu/dma-iommu.c
> > index 5141d49a046b..0a3f67a4f9ae 100644
> > --- a/drivers/iommu/dma-iommu.c
> > +++ b/drivers/iommu/dma-iommu.c
> > @@ -213,14 +213,21 @@ static int iova_reserve_pci_windows(struct pci_dev *dev,
> >       resource_list_for_each_entry(window, &bridge->dma_ranges) {
> >               end = window->res->start - window->offset;
> >   resv_iova:
> > +             if (end < start) {
> > +                     /* dma_ranges list should be sorted */
> > +                     dev_err(&dev->dev, "Failed to reserve IOVA\n");
> > +                     return -EINVAL;
> > +             }
> > +             /*
> > +              * Skip the cases when start address of first memory region is
> > +              * 0x0 and end address of one memory region and start address
> > +              * of next memory region are equal. Reserve IOVA for rest of
> > +              * addresses fall in between given memory ranges.
> > +              */
> >               if (end > start) {
> >                       lo = iova_pfn(iovad, start);
> >                       hi = iova_pfn(iovad, end);
> >                       reserve_iova(iovad, lo, hi);
> > -             } else {
>
> Surely this only needs to be a one-liner?
Yes I agree with you this one line is sufficient.
>
> -               } else {
> +               } else if (end < start) {
>
> (or possibly "end != start"; I can't quite decide which expresses the
> semantic intent better)
I think "end < start" is better choice because it tells list is not sorted
and "!=" contradicts previous condition "end > start".
>
> The rest just looks like unnecessary churn - I don't think it needs
> commenting that a sorted list may simply not have gaps between entries,
> and as above I think the wording of that comment is actively misleading.
I agree with you, these lines were added to explain the issue and fix with
more details.
I will send a new patch with a single line change as you said.
" } else if (end < start) {"

Thanks & Regards,
Srinath.
>
> Robin.
>
> > -                     /* dma_ranges list should be sorted */
> > -                     dev_err(&dev->dev, "Failed to reserve IOVA\n");
> > -                     return -EINVAL;
> >               }
> >
> >               start = window->res->end - window->offset + 1;
> >
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

^ permalink raw reply	[flat|nested] 3+ messages in thread

end of thread, other threads:[~2020-09-10  8:26 UTC | newest]

Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-09-09  5:32 [PATCH] iommu/dma: Fix IOVA reserve dma ranges Srinath Mannam via iommu
2020-09-09 12:05 ` Robin Murphy
2020-09-10  8:26   ` Srinath Mannam via iommu

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).