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

[Henry Wang <Henry.Wang@xxxxxxx>]

Hi both,

I was lurking around to see how the discussion would go. Thanks for the
discussions/inputs in this thread :) 

> -----Original Message-----
> From: Stefano Stabellini <sstabellini@xxxxxxxxxx>
> Subject: Re: [PATCH 1/3] xen/arm: Add memory overlap check for
> bootinfo.reserved_mem
> > > It cannot be a single static inline function because the bootinfo
> > > arguments are of three different types, it just happens that all three
> > > have a "start" and "size" struct member so it works great with a macro,
> > > but doesn't for a function.
> >
> > It is not clear to me what are the three types you are referring to. Looking
> > at the definition of bootinfo is:
> >
> > struct bootinfo {
> >     struct meminfo mem;
> >     /* The reserved regions are only used when booting using Device-Tree */
> >     struct meminfo reserved_mem;
> >     struct bootmodules modules;
> >     struct bootcmdlines cmdlines;
> > #ifdef CONFIG_ACPI
> >     struct meminfo acpi;
> > #endif
> >     bool static_heap;
> > };
> >
> > cmdlines is something uninteresting here. So we have two types:
> >   - bootmodules for modules
> >   - meminfo used by reserved_mem, mem and acpi

Exactly, we need to check the given input physical address range is
not overlapping with any of the banks in bootmodules and meminfo used by
reserved_mem & acpi.

> >
> > Looking in details the code, now I understand why you suggested the
> macro.
> > This is far better than the checking what the array type (not very 
> > scalable).
> 
> Thank you :-)

+1, I also thought this would be quite painful to extend in the future (once we
add a new member in bootinfo, for example what Penny did in [1], we need to
extend the overlap check as well), but I didn't think of using macro so thank 
you
Stefano :)

> 
> 
> > Personally, I think trying to share the code between the two types is a bit
> > odd. The logic is the same today, but I envision to merge reserved_mem,
> mem
> > and acpi in a single array (it would look like the E820) as this would make
> > easier to find the caching attributes per regions when mapping the RAM. So
> > sharing the code would not be possible.
> >
> > That said, if you really want to share the code between the two types. Then
> I
> > would prefer one of the following option:
> >    1) Provide a callback that is used to fetch the information from the 
> > array
> >    2) Provide a common structure that could be used by the function.
> >
> > This would match other generic function like sort & co.
> 
> I think option 2) would be the best but I couldn't think of a simple way
> to do it (without using a union and I thought a union would not make
> things nicer in this case).
> 
> Rather than option 1), I think I would rather have 2 different functions
> to check struct bootmodules and struct meminfo, or the macro.

I personally don't have specific taste here. I think the option
is good one as long as we can (1) share most part of the code (2) make the
code easy to extend in the future. So as long as you two reach
a consensus here I will change to the agreed method in v2.

[1] 
https://lore.kernel.org/xen-devel/20221115025235.1378931-2-Penny.Zheng@xxxxxxx/

Kind regards,
Henry