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

[Huang Shijie <shijie.huang@xxxxxxx>]

On Mon, Nov 06, 2017 at 12:22:43PM +0000, Julien Grall wrote:
> Hi Shijie,
> 
> On 03/11/17 03:11, Huang Shijie wrote:
> > The e820 is x86 specific. This patch adds a new
> > function arch_check_mem_block() and a mem_blocks.
> 
> s/a mem_blocks/a variable mem_blocks/. But basically what you do is renaming
> e820_entries to mem_blocks. So it might be better to say:
> 
> "and rename e820_entries to mem_blocks".
okay.

> 
> > 
> > Different archs implements the mem_blocks and arch_check_mem_block.
> > By this way, we remove the e820 code from the common code.
> 
> Not really, the e820 header is still in common code and used (see mm.c).
> This should be moved in x86 to remove completely.

I will fix it in next version.
> 
> > 
> > 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.

> 
> >   #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.

> 
> > +
> > +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.

Thanks
Huang Shijie

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