Re: [PATCH early-RFC 1/5] xen/arm: Clean-up the memory layout
Hi Julien, > On 17 Mar 2022, at 20:32, Julien Grall wrote: > > > > On 17/03/2022 15:23, Bertrand Marquis wrote: >> Hi Julien, > > Hi Bertrand, > >>> On 9 Mar 2022, at 11:20, Julien Grall wrote: >>> >>> From: Julien Grall >>> >>> 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 >> Changes looks ok to me so: >> Reviewed-by: Bertrand Marquis >> 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,0x0020) >>> -#define FIXMAP_ADDR(n)(_AT(vaddr_t,0x0040) + (n) * PAGE_SIZE) >>> +#define COMMON_VIRT_START _AT(vaddr_t, 0) >>> >>> -#define BOOT_FDT_VIRT_START_AT(vaddr_t,0x0060) >>> -#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
Re: [PATCH early-RFC 1/5] xen/arm: Clean-up the memory layout
On 17/03/2022 15:23, Bertrand Marquis wrote: Hi Julien, Hi Bertrand, On 9 Mar 2022, at 11:20, Julien Grall wrote: From: Julien Grall 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 Changes looks ok to me so: Reviewed-by: Bertrand Marquis 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,0x0020) -#define FIXMAP_ADDR(n)(_AT(vaddr_t,0x0040) + (n) * PAGE_SIZE) +#define COMMON_VIRT_START _AT(vaddr_t, 0) -#define BOOT_FDT_VIRT_START_AT(vaddr_t,0x0060) -#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
Hi Julien, > On 9 Mar 2022, at 11:20, Julien Grall wrote: > > From: Julien Grall > > 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 Changes looks ok to me so: Reviewed-by: Bertrand Marquis 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,0x0020) > -#define FIXMAP_ADDR(n)(_AT(vaddr_t,0x0040) + (n) * PAGE_SIZE) > +#define COMMON_VIRT_START _AT(vaddr_t, 0) > > -#define BOOT_FDT_VIRT_START_AT(vaddr_t,0x0060) > -#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
[PATCH early-RFC 1/5] xen/arm: Clean-up the memory layout
From: Julien Grall 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 --- 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,0x0020) -#define FIXMAP_ADDR(n)(_AT(vaddr_t,0x0040) + (n) * PAGE_SIZE) +#define COMMON_VIRT_START _AT(vaddr_t, 0) -#define BOOT_FDT_VIRT_START_AT(vaddr_t,0x0060) -#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_SIZEMB(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,0x00a0) -#define LIVEPATCH_VMAP_END (LIVEPATCH_VMAP_START + MB(2)) +#define LIVEPATCH_VMAP_START BOOT_FDT_VIRT_END +#define LIVEPATCH_SLOT_SIZEMB(2) +#define LIVEPATCH_VMAP_END (LIVEPATCH_VMAP_START + LIVEPATCH_SLOT_SIZE) #endif #define HYPERVISOR_VIRT_START XEN_VIRT_START -- 2.32.0
