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

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Sat, 10 Dec 2022, Henry Wang wrote:
> 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.

I think Julien and I already agree on having 2 separate functions to
check for struct bootmodules and struct meminfo. Julien, I take you
prefer the two separate functions to a MACRO, right?