Hi Bertrand,
On 24/05/2022 09:05, Bertrand Marquis wrote:
Signed-off-by: Julien Grall <[email protected]>
----
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.
[...]
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,
--
Julien Grall
