[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [PATCH] xen/arm: Remove most of the *_VIRT_END defines
Hi Wei, > On 24 May 2022, at 02:29, Wei Chen <Wei.Chen@xxxxxxx> wrote: > > Hi Julien, > >> -----Original Message----- >> From: Xen-devel <xen-devel-bounces@xxxxxxxxxxxxxxxxxxxx> On Behalf Of >> Julien Grall >> Sent: 2022年5月24日 3:50 >> To: xen-devel@xxxxxxxxxxxxxxxxxxxx >> Cc: julien@xxxxxxx; Julien Grall <jgrall@xxxxxxxxxx>; Stefano Stabellini >> <sstabellini@xxxxxxxxxx>; Bertrand Marquis <Bertrand.Marquis@xxxxxxx>; >> Volodymyr Babchuk <Volodymyr_Babchuk@xxxxxxxx>; Konrad Rzeszutek Wilk >> <konrad.wilk@xxxxxxxxxx>; Ross Lagerwall <ross.lagerwall@xxxxxxxxxx> >> Subject: [PATCH] xen/arm: Remove most of the *_VIRT_END defines >> >> From: Julien Grall <jgrall@xxxxxxxxxx> >> >> At the moment, *_VIRT_END may either point to the address after the end >> or the last address of the region. >> >> The lack of consistency make quite difficult to reason with them. >> >> Furthermore, there is a risk of overflow in the case where the address >> points past to the end. I am not aware of any cases, so this is only a >> latent bug. >> >> Start to solve the problem by removing all the *_VIRT_END exclusively used >> by the Arm code and add *_VIRT_SIZE when it is not present. >> >> Take the opportunity to rename BOOT_FDT_SLOT_SIZE to BOOT_FDT_VIRT_SIZE >> for better consistency and use _AT(vaddr_t, ). >> >> Signed-off-by: Julien Grall <jgrall@xxxxxxxxxx> >> >> ---- >> >> I noticed that a few functions in Xen expect [start, end[. This is risky >> as we may end up with end < start if the region is defined right at the >> top of the address space. >> >> I haven't yet tackle this issue. But I would at least like to get rid >> of *_VIRT_END. >> --- >> xen/arch/arm/include/asm/config.h | 18 ++++++++---------- >> xen/arch/arm/livepatch.c | 2 +- >> xen/arch/arm/mm.c | 13 ++++++++----- >> 3 files changed, 17 insertions(+), 16 deletions(-) >> >> diff --git a/xen/arch/arm/include/asm/config.h >> b/xen/arch/arm/include/asm/config.h >> index 3e2a55a91058..66db618b34e7 100644 >> --- a/xen/arch/arm/include/asm/config.h >> +++ b/xen/arch/arm/include/asm/config.h >> @@ -111,12 +111,11 @@ >> #define FIXMAP_ADDR(n) (_AT(vaddr_t,0x00400000) + (n) * PAGE_SIZE) >> >> #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 BOOT_FDT_VIRT_SIZE _AT(vaddr_t, MB(4)) >> >> #ifdef CONFIG_LIVEPATCH >> #define LIVEPATCH_VMAP_START _AT(vaddr_t,0x00a00000) >> -#define LIVEPATCH_VMAP_END (LIVEPATCH_VMAP_START + MB(2)) >> +#define LIVEPATCH_VMAP_SIZE _AT(vaddr_t, MB(2)) >> #endif >> >> #define HYPERVISOR_VIRT_START XEN_VIRT_START >> @@ -132,18 +131,18 @@ >> #define FRAMETABLE_VIRT_END (FRAMETABLE_VIRT_START + FRAMETABLE_SIZE - >> 1) >> >> #define VMAP_VIRT_START _AT(vaddr_t,0x10000000) >> +#define VMAP_VIRT_SIZE _AT(vaddr_t, GB(1) - MB(256)) >> >> #define XENHEAP_VIRT_START _AT(vaddr_t,0x40000000) >> -#define XENHEAP_VIRT_END _AT(vaddr_t,0x7fffffff) >> -#define DOMHEAP_VIRT_START _AT(vaddr_t,0x80000000) >> -#define DOMHEAP_VIRT_END _AT(vaddr_t,0xffffffff) >> +#define XENHEAP_VIRT_SIZE _AT(vaddr_t, GB(1)) >> >> -#define VMAP_VIRT_END XENHEAP_VIRT_START >> +#define DOMHEAP_VIRT_START _AT(vaddr_t,0x80000000) >> +#define DOMHEAP_VIRT_SIZE _AT(vaddr_t, GB(2)) >> >> #define DOMHEAP_ENTRIES 1024 /* 1024 2MB mapping slots */ >> >> /* Number of domheap pagetable pages required at the second level (2MB >> mappings) */ >> -#define DOMHEAP_SECOND_PAGES ((DOMHEAP_VIRT_END - DOMHEAP_VIRT_START + >> 1) >> FIRST_SHIFT) >> +#define DOMHEAP_SECOND_PAGES (DOMHEAP_VIRT_SIZE >> FIRST_SHIFT) >> >> #else /* ARM_64 */ >> >> @@ -152,12 +151,11 @@ >> #define SLOT0_ENTRY_SIZE SLOT0(1) >> >> #define VMAP_VIRT_START GB(1) >> -#define VMAP_VIRT_END (VMAP_VIRT_START + GB(1)) >> +#define VMAP_VIRT_SIZE GB(1) >> >> #define FRAMETABLE_VIRT_START GB(32) >> #define FRAMETABLE_SIZE GB(32) >> #define FRAMETABLE_NR (FRAMETABLE_SIZE / sizeof(*frame_table)) >> -#define FRAMETABLE_VIRT_END (FRAMETABLE_VIRT_START + FRAMETABLE_SIZE - >> 1) >> >> #define DIRECTMAP_VIRT_START SLOT0(256) >> #define DIRECTMAP_SIZE (SLOT0_ENTRY_SIZE * (265-256)) >> diff --git a/xen/arch/arm/livepatch.c b/xen/arch/arm/livepatch.c >> index 75e8adcfd6a1..57abc746e60b 100644 >> --- a/xen/arch/arm/livepatch.c >> +++ b/xen/arch/arm/livepatch.c >> @@ -175,7 +175,7 @@ void __init arch_livepatch_init(void) >> void *start, *end; >> >> start = (void *)LIVEPATCH_VMAP_START; >> - end = (void *)LIVEPATCH_VMAP_END; >> + end = start + LIVEPATCH_VMAP_SIZE; >> >> vm_init_type(VMAP_XEN, start, end); >> >> diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c >> index be37176a4725..0607c65f95cd 100644 >> --- a/xen/arch/arm/mm.c >> +++ b/xen/arch/arm/mm.c >> @@ -128,9 +128,11 @@ static DEFINE_PAGE_TABLE(xen_first); >> /* xen_pgtable == root of the trie (zeroeth level on 64-bit, first on 32- >> bit) */ >> static DEFINE_PER_CPU(lpae_t *, xen_pgtable); >> #define THIS_CPU_PGTABLE this_cpu(xen_pgtable) >> -/* xen_dommap == pages used by map_domain_page, these pages contain >> +/* >> + * xen_dommap == pages used by map_domain_page, these pages contain >> * the second level pagetables which map the domheap region >> - * DOMHEAP_VIRT_START...DOMHEAP_VIRT_END in 2MB chunks. */ >> + * starting at DOMHEAP_VIRT_START in 2MB chunks. >> + */ >> static DEFINE_PER_CPU(lpae_t *, xen_dommap); >> /* Root of the trie for cpu0, other CPU's PTs are dynamically allocated >> */ >> static DEFINE_PAGE_TABLE(cpu0_pgtable); >> @@ -476,7 +478,7 @@ mfn_t domain_page_map_to_mfn(const void *ptr) >> int slot = (va - DOMHEAP_VIRT_START) >> SECOND_SHIFT; >> unsigned long offset = (va>>THIRD_SHIFT) & XEN_PT_LPAE_ENTRY_MASK; >> >> - if ( va >= VMAP_VIRT_START && va < VMAP_VIRT_END ) >> + if ( (va >= VMAP_VIRT_START) && ((VMAP_VIRT_START - va) < >> VMAP_VIRT_SIZE) ) >> return virt_to_mfn(va); >> >> ASSERT(slot >= 0 && slot < DOMHEAP_ENTRIES); >> @@ -570,7 +572,8 @@ void __init remove_early_mappings(void) >> int rc; >> >> /* destroy the _PAGE_BLOCK mapping */ >> - rc = modify_xen_mappings(BOOT_FDT_VIRT_START, BOOT_FDT_VIRT_END, >> + rc = modify_xen_mappings(BOOT_FDT_VIRT_START, >> + BOOT_FDT_VIRT_START + BOOT_FDT_VIRT_SIZE, >> _PAGE_BLOCK); >> BUG_ON(rc); >> } >> @@ -850,7 +853,7 @@ void __init setup_frametable_mappings(paddr_t ps, >> paddr_t pe) >> >> void *__init arch_vmap_virt_end(void) >> { >> - return (void *)VMAP_VIRT_END; >> + return (void *)(VMAP_VIRT_START + VMAP_VIRT_SIZE); > > It seems you prefer to point _end to the address after the end. Even > though we got rid of the macro definition of _END. But we didn't agree > on how to use it. For me, when I first saw > "end = start + LIVEPATCH_VMAP_SIZE" I subconsciously think the -1 is > missing here. I even added a comment, but removed it when I reached > to this line : ) > May be it's better to place some code guide for END in code comment > in the SIZE definition, otherwise, we may have different point addresses > of _end functions. I think it is quite common to have size being the actual size and not size -1. END is clearly the last address of the area but SIZE should be the number of bytes in the area so I think Julien here is right. Cheers Bertrand > > Cheers, > Wei Chen > >> } >> >> /* >> -- >> 2.32.0
|
![]() |
Lists.xenproject.org is hosted with RackSpace, monitoring our |