[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [PATCH] xen/arm: Remove most of the *_VIRT_END defines
Hi Julien, > On 23 Jun 2022, at 22:45, Julien Grall <julien@xxxxxxx> wrote: > > Hi Bertrand, > > On 24/05/2022 09:05, Bertrand Marquis wrote: >>> 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)) >> This looks a bit odd, any reason not to use MB(768) ? > > This is to match how we define FRAMETABLE_SIZE. It is a lot easier to figure > out how the size was found and match the comment: > > 256M - 1G VMAP: ioremap and early_ioremap use this virtual address > space > >> If not then there might be something worse explaining with a comment here. > > This is really a matter of taste here. I don't think it has to be explained > in the commit message. You are right make sense with the comment at the beginning of the section in config.h > > [...] > >>> 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. >>> + */ >> Please just mention that you also fixed a comment coding style in the commit >> message. > > Sure. > >>> 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); >>> } >>> >>> /* Cheers Bertrand
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |