Re: [PATCH] iommu/dma: Fix IOVA reserve dma ranges

[Srinath Mannam]

On Wed, Sep 9, 2020 at 5:35 PM Robin Murphy <robin.murphy@xxxxxxx> 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@xxxxxxxxxxxx>
> > ---
> >   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;
> >