RE: [PATCH v3 1/3] xen/arm: Add memory overlap check for bootinfo.reserved_mem

[Henry Wang <Henry.Wang@xxxxxxx>]

Hi Julien,

> -----Original Message-----
> From: Julien Grall <julien@xxxxxxx>
> 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