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

[Juergen Gross <lists@xxxxxxxxx>]

On 03/11/17 18:05, Volodymyr Babchuk wrote:
> 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?

Mini-OS supports only one virtual address space and it wants to address
all its memory. So memory for a 32 bit system has to be smaller than
4GB. If you want more memory use X86_64. The hypervisor requires a
64 bit processor on X86 anyway.


Juergen

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