Hi Julien,
> -----Original Message-----
> From: Julien Grall <[email protected]>
> Subject: Re: [PATCH v3 1/3] xen/arm: Add memory overlap check for
> bootinfo.reserved_mem
>
> Hi Henry,
>
> > +{
> > + paddr_t bank_start = INVALID_PADDR, bank_end = 0;
> > + paddr_t region_end = region_start + region_size;
> > + unsigned int i, bank_num = meminfo->nr_banks;
> > +
> > + for ( i = 0; i < bank_num; i++ )
> > + {
> > + bank_start = meminfo->bank[i].start;
> > + bank_end = bank_start + meminfo->bank[i].size;
> > +
> > + if ( region_end <= bank_start || region_start >= bank_end )
>
> ... it clearly shows how this check would be wrong when either the bank
> or the region is at the end of the address space. You may say it doesn't
> overlap when it could (e.g. when region_end < region_start).
Here do you mean if the region is at the end of the addr space,
"region_start + region_end" will overflow and cause
region_end < region_start? If so...
>
> That said, unless we rework 'bank', we would not properly solve the
> problem. But that's likely a bigger piece of work and not something I
> would request.
>
> So for now, I would suggest to add a comment. Stefano, what do you think?
...I am not really sure if simply adding a comment here would help,
because when the overflow happens, we are already doomed because
of the messed-up device tree.
Would adding a `BUG_ON(region_end < region_start)` make sense to you?
>
> > + continue;
> > + else
> > + {
> > + printk("Region: [%#"PRIpaddr", %#"PRIpaddr"] overlapping with
> bank[%u]: [%#"PRIpaddr", %#"PRIpaddr"]\n",
>
> ']' usually mean inclusive. But here, 'end' is exclusive. So you want '['.
Oh, now I understand the misunderstanding in our communication in v1:
I didn't know '[' means exclusive because I was educated to use ')' [1] so I
thought you meant inclusive. Sorry for this.
To keep consistency, may I use ')' here? Because I think this is the current
way in the code base, for example see:
xen/include/xen/numa.h L99: [*start, *end)
xen/drivers/passthrough/amd/iommu_acpi.c L177: overlap [%lx,%lx)
>
> This could be fixed on commit.
>
> BTW, the same comments applies for the second patch.
I will fix this patch and #2 in v4.
[1]
https://en.wikipedia.org/wiki/Interval_(mathematics)#Including_or_excluding_endpoints
Kind regards,
Henry
>
> Cheers,
>
> --
> Julien Grall
