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

[Juergen Gross <jgross@xxxxxxxx>]

On 07/11/17 11:14, Julien Grall wrote:
> 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?

I'm fine with renaming.


Juergen

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