Re: [PATCH] xen/arm: Remove most of the *_VIRT_END defines

[Bertrand Marquis <Bertrand.Marquis@xxxxxxx>]

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