| Message ID | 20220309112048.17377-2-julien@xen.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | xen/arm: Don't switch TTBR while the MMU is on | expand |
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
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,
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
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