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

Julien Grall posted 5 patches 3 years, 11 months ago
There is a newer version of this series
[PATCH early-RFC 1/5] xen/arm: Clean-up the memory layout
Posted by Julien Grall 3 years, 11 months ago
From: Julien Grall <jgrall@amazon.com>

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@amazon.com>

---

After the next patch, the term "common" will sound strange because
the base address is different. Any 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)
+#define XEN_VIRT_END            (XEN_VIRT_START + XEN_SLOT_SIZE)
+
+#define FIXMAP_VIRT_START       XEN_VIRT_END
+#define FIXMAP_SLOT_SIZE        MB(2)
+#define FIXMAP_VIRT_END         (FIXMAP_VIRT_START + FIXMAP_SLOT_SIZE)
+
+#define FIXMAP_ADDR(n)          (FIXMAP_VIRT_START + (n) * PAGE_SIZE)
+
+#define BOOT_FDT_VIRT_START     FIXMAP_VIRT_END
+#define BOOT_FDT_SLOT_SIZE      MB(4)
+#define BOOT_FDT_VIRT_END       (BOOT_FDT_VIRT_START + BOOT_FDT_SLOT_SIZE)
 
 #ifdef CONFIG_LIVEPATCH
-#define LIVEPATCH_VMAP_START   _AT(vaddr_t,0x00a00000)
-#define LIVEPATCH_VMAP_END     (LIVEPATCH_VMAP_START + MB(2))
+#define LIVEPATCH_VMAP_START   BOOT_FDT_VIRT_END
+#define LIVEPATCH_SLOT_SIZE    MB(2)
+#define LIVEPATCH_VMAP_END     (LIVEPATCH_VMAP_START + LIVEPATCH_SLOT_SIZE)
 #endif
 
 #define HYPERVISOR_VIRT_START  XEN_VIRT_START
-- 
2.32.0
Re: [PATCH early-RFC 1/5] xen/arm: Clean-up the memory layout
Posted by Bertrand Marquis 3 years, 10 months ago
Hi Julien,

> On 9 Mar 2022, at 11:20, Julien Grall <julien@xen.org> wrote:
> 
> From: Julien Grall <jgrall@amazon.com>
> 
> 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@amazon.com>

Changes looks ok to me so:
Reviewed-by: Bertrand Marquis <bertrand.marquis@arm.com>

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 ?

Or space maybe..

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

Cheers
Bertrand
Re: [PATCH early-RFC 1/5] xen/arm: Clean-up the memory layout
Posted by Julien Grall 3 years, 10 months ago

On 17/03/2022 15:23, Bertrand Marquis wrote:
> Hi Julien,

Hi Bertrand,

>> On 9 Mar 2022, at 11:20, Julien Grall <julien@xen.org> wrote:
>>
>> From: Julien Grall <jgrall@amazon.com>
>>
>> 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@amazon.com>
> 
> Changes looks ok to me so:
> Reviewed-by: Bertrand Marquis <bertrand.marquis@arm.com>
> 
> 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?

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

Cheers,

-- 
Julien Grall
Re: [PATCH early-RFC 1/5] xen/arm: Clean-up the memory layout
Posted by Bertrand Marquis 3 years, 10 months ago
Hi Julien,

> On 17 Mar 2022, at 20:32, Julien Grall <julien@xen.org> wrote:
> 
> 
> 
> On 17/03/2022 15:23, Bertrand Marquis wrote:
>> Hi Julien,
> 
> Hi Bertrand,
> 
>>> On 9 Mar 2022, at 11:20, Julien Grall <julien@xen.org> wrote:
>>> 
>>> From: Julien Grall <jgrall@amazon.com>
>>> 
>>> 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@amazon.com>
>> Changes looks ok to me so:
>> Reviewed-by: Bertrand Marquis <bertrand.marquis@arm.com>
>> 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