Re: [PATCH early-RFC 1/5] xen/arm: Clean-up the memory layout

[Bertrand Marquis <Bertrand.Marquis@xxxxxxx>]

Hi Julien,

> On 17 Mar 2022, at 20:32, Julien Grall <julien@xxxxxxx> wrote:
> 
> 
> 
> On 17/03/2022 15:23, Bertrand Marquis wrote:
>> Hi Julien,
> 
> Hi Bertrand,
> 
>>> On 9 Mar 2022, at 11:20, Julien Grall <julien@xxxxxxx> wrote:
>>> 
>>> From: Julien Grall <jgrall@xxxxxxxxxx>
>>> 
>>> In a follow-up patch, the base address for the common mappings will
>>> vary between arm32 and arm64. To avoid any duplication, define
>>> every mapping in the common region from the previous one.
>>> 
>>> Take the opportunity to add mising *_SIZE for some mappings.
>>> 
>>> Signed-off-by: Julien Grall <jgrall@xxxxxxxxxx>
>> Changes looks ok to me so:
>> Reviewed-by: Bertrand Marquis <bertrand.marquis@xxxxxxx>
>> Only one question after.
>>> 
>>> ---
>>> 
>>> After the next patch, the term "common" will sound strange because
>>> the base address is different. Any better suggestion?
>> MAPPING_VIRT_START ?
> 
> For arm32, I am planning to reshuffle the layout so the runtime address is 
> always at the end of the layout.
> 
> So I think MAPPING_VIRT_START may be as confusing. How about 
> SHARED_ARCH_VIRT_MAPPING?

> 
>> Or space maybe..
> 
> I am not sure I understand this suggestion. Can you clarify?

VIRT_SPACE_START was in my mind at that time but that is also not good

How about using BASE instead of start: MAPPING_COMMON_VIRT_BASE ?

Anyway hard to find a nice name, so your solution with SHARED is ok for me 
unless someone has a better suggestion.

> 
>>> ---
>>> xen/arch/arm/include/asm/config.h | 24 +++++++++++++++++-------
>>> 1 file changed, 17 insertions(+), 7 deletions(-)
>>> 
>>> diff --git a/xen/arch/arm/include/asm/config.h 
>>> b/xen/arch/arm/include/asm/config.h
>>> index aedb586c8d27..5db28a8dbd56 100644
>>> --- a/xen/arch/arm/include/asm/config.h
>>> +++ b/xen/arch/arm/include/asm/config.h
>>> @@ -107,16 +107,26 @@
>>>  *  Unused
>>>  */
>>> 
>>> -#define XEN_VIRT_START         _AT(vaddr_t,0x00200000)
>>> -#define FIXMAP_ADDR(n)        (_AT(vaddr_t,0x00400000) + (n) * PAGE_SIZE)
>>> +#define COMMON_VIRT_START       _AT(vaddr_t, 0)
>>> 
>>> -#define BOOT_FDT_VIRT_START    _AT(vaddr_t,0x00600000)
>>> -#define BOOT_FDT_SLOT_SIZE     MB(4)
>>> -#define BOOT_FDT_VIRT_END      (BOOT_FDT_VIRT_START + BOOT_FDT_SLOT_SIZE)
>>> +#define XEN_VIRT_START          (COMMON_VIRT_START + MB(2))
>>> +#define XEN_SLOT_SIZE           MB(2)
>> I know this is not modified by your patch, but any idea why SLOT is used 
>> here ?
>> XEN_VIRT_SIZE would sound a bit more logic.
> 
> No idea. I can add a patch to rename it.

I think it would be a good idea, we already have a lot of terms in here and 
SLOT is just adding to the confusion I find.

Thanks
Bertrand