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

[Julien Grall <julien.grall@xxxxxxxxxx>]

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

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

>   #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.
> +
> +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.
Cheers,

--
Julien Grall

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