Hi Julien, On 24.06.2022 11:11, Julien Grall wrote: > From: Julien Grall <jgr...@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 <jgr...@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