Message ID | 20220624091146.35716-2-julien@xen.org (mailing list archive) |
---|---|
State | Superseded |
Headers | show |
Series | xen/arm: mm: Bunch of clean-ups | expand |
Hi Julien, On 24.06.2022 11:11, Julien Grall wrote: > From: Julien Grall <jgrall@amazon.com> > > 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, ). > > Also take the opportunity to fix the coding style of the comment touched > in mm.c. > > Signed-off-by: Julien Grall <jgrall@amazon.com> > > ---- > > 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. > > This was originally sent separately (lets call it v0). > > Changes in v1: > - Mention the coding style change. > --- > 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) ) The second condition does not seem to be correct. Instead, this check should like like following: if ( (va >= VMAP_VIRT_START) && (va < (VMAP_VIRT_START + 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); > } > > /* The rest looks good. Cheers, Michal
On 27/06/2022 07:23, Michal Orzel wrote: > Hi Julien, Hi, Thanks for the review. > On 24.06.2022 11:11, Julien Grall wrote: >> From: Julien Grall <jgrall@amazon.com> >> --- >> 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) ) > The second condition does not seem to be correct. Hmm... You are right, it wants to be ((va - VMAP_VIRT_START) < VMAP_VIRT_SIZE) > Instead, this check should like like following: > if ( (va >= VMAP_VIRT_START) && (va < (VMAP_VIRT_START + VMAP_VIRT_SIZE)) ) This check would still be incorrect because if the VMAP is right at the edge of the address (e.g. 2^32 - 1 on arm32), then VMAP_VIRT_START + VMAP_VIRT_SIZE would be 0. Cheers,
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); } /*