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

[Volodymyr Babchuk <vlad.babchuk@xxxxxxxxx>]

Hi Juergen,

On 3 November 2017 at 17:09, Juergen Gross <lists@xxxxxxxxx> wrote:
> On 03/11/17 16:04, Volodymyr Babchuk wrote:
>> Hi Huang,
>>
>> On 3 November 2017 at 05:11, Huang Shijie <shijie.huang@xxxxxxx> wrote:
>>> The e820 is x86 specific. This patch adds a new
>>> function arch_check_mem_block() and a 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.
>>>
>>> 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>
>>>  #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;
>>> +
>>> +unsigned mem_blocks = 1;
>>> +
>>> +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;
>>>
>>>  static char *e820_types[E820_TYPES] = {
>>>      [E820_RAM]      = "RAM",
>>> @@ -191,6 +192,18 @@ void arch_print_memmap(void)
>>>  }
>>>  #endif
>>>
>>> +int arch_check_mem_block(int index, unsigned long *r_min, unsigned long 
>>> *r_max)
>>> +{
>>> +    if (e820_map[index].type != E820_RAM)
>>> +        return 1;
>>> +    if (e820_map[index].addr + e820_map[index].size >= ULONG_MAX)
>> Hmm, does this check makes sense? You need to cast left part to 128-bit 
>> integer.
>> I see that you just move this code from another place. But for me it
>> looks wrong.
>
> No, this is correct. It should catch 32 bit overflow on X86_32, where
> ULONG_MAX is 0xffffffff while e820_map[index].addr is a 64 bit quantity.
I'm not so good with x86 architecture. But, as I know, it support
LPAE, so, theoretically,
it is possible to map RAM in that way, so part of it will be under 4GB
and another part - above 4GB.
This check will break such setup, wouldn't it?

-- 
WBR Volodymyr Babchuk aka lorc [+380976646013]
mailto: vlad.babchuk@xxxxxxxxx

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