Re: [Minios-devel] [PATCH 13/40] mini-os: remove the e820 from common code

[Julien Grall <julien.grall@xxxxxxxxxx>]

Hi Shijie,

On 07/11/17 08:47, Huang Shijie wrote:
> On Mon, Nov 06, 2017 at 12:22:43PM +0000, Julien Grall wrote:
> > > Change-Id: I6cfa5bcb12128f55b910f72f592e5b43ebd31dd4
> > > Jira: ENTOS-247
> > > Signed-off-by: Huang Shijie <shijie.huang@xxxxxxx>
> > > ---
> > >    arch/arm/mm.c | 18 ++++++++++--------
> > >    arch/x86/mm.c | 17 +++++++++++++++--
> > >    include/mm.h  |  3 +++
> > >    mm.c          |  8 ++------
> > >    4 files changed, 30 insertions(+), 16 deletions(-)
> > >
> > > diff --git a/arch/arm/mm.c b/arch/arm/mm.c
> > > index 3d88d3b..f600672 100644
> > > --- a/arch/arm/mm.c
> > > +++ b/arch/arm/mm.c
> > > @@ -3,18 +3,20 @@
> > >    #include <arch_mm.h>
> > >    #include <mini-os/errno.h>
> > >    #include <mini-os/hypervisor.h>
> > > +#include <mini-os/posix/limits.h>
> > Why do you need to include that?
> The ULONG_MAX needs this header.
ULONG_MAX was used in this file before. So why suddenly this is needed?

If it is because a compilation error, then it should really go in a 
separate patch...
> > >    #include <libfdt.h>
> > >    #include <lib.h>
> > >    paddr_t physical_address_offset;
> > > -struct e820entry e820_map[1] = {
> > > -    {
> > > -        .addr = 0,
> > > -        .size = ULONG_MAX - 1,
> > > -        .type = E820_RAM
> > > -    }
> > > -};
> > > -unsigned e820_entries = 1;
> > I see you remove e820_entries but I am a bit surprised
> >
> > > +
> > > +unsigned mem_blocks = 1;
> > I am not sure where to comment it. But I am still a bit surprised that
> > mem_blocks is 1 for Arm in general.
> >
> > There may be multiple banks in different place of the memory layout. So you
> > would end up with hole considered as real memory by Mini-OS.
> >
> > Note th at the moment the first bank only have 3GB of memory. But that may
> > in the future as this is not part of the ABI.
> Keep it as 1, and fix it if we need more memory banks for arm.
Ok.

> > > +
> > > +int arch_check_mem_block(int index, unsigned long *r_min, unsigned long *r_max)
> > > +{
> > > +    *r_min = 0;
> > > +    *r_max = ULONG_MAX - 1;
> > > +    return 0;
> > > +}
> > >    unsigned long allocate_ondemand(unsigned long n, unsigned long alignment)
> > >    {
> > > diff --git a/arch/x86/mm.c b/arch/x86/mm.c
> > > index 05ad029..ba1bfc5 100644
> > > --- a/arch/x86/mm.c
> > > +++ b/arch/x86/mm.c
> > > @@ -71,7 +71,7 @@ struct e820entry e820_map[1] = {
> > >            .type = E820_RAM
> > >        }
> > >    };
> > > -unsigned e820_entries = 1;
> > > +unsigned mem_blocks = 1;
> > >    void arch_mm_preinit(void *p)
> > >    {
> > > @@ -113,7 +113,8 @@ desc_ptr idt_ptr =
> > >    };
> > >    struct e820entry e820_map[E820_MAX];
> > > -unsigned e820_entries;
> > > +unsigned mem_blocks;
> > > +#define e820_entries mem_blocks;
> > e820_entries is only used in 3 places. Please replace them all by mem_blocks
> > and drop that define.
> I think it will make the x86 guys unhappy :)
> So I suggest keep the e820 for x86 files.
... Did you ask them? I really can't see how you will make them unhappy 
to rename 3 occurrences...
Juergen, Wei, Samuel, any opinion?

Cheers,

--
Julien Grall

_______________________________________________
Minios-devel mailing list
Minios-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/cgi-bin/mailman/listinfo/minios-devel