* RE: [PATCH] xen/arm: Remove most of the *_VIRT_END defines
2022-05-23 19:49 [PATCH] xen/arm: Remove most of the *_VIRT_END defines Julien Grall
@ 2022-05-24 1:29 ` Wei Chen
2022-05-24 8:06 ` Bertrand Marquis
2022-05-24 3:39 ` Wei Chen
2022-05-24 8:05 ` Bertrand Marquis
2 siblings, 1 reply; 9+ messages in thread
From: Wei Chen @ 2022-05-24 1:29 UTC (permalink / raw)
To: Julien Grall, xen-devel
Cc: Julien Grall, Stefano Stabellini, Bertrand Marquis,
Volodymyr Babchuk, Konrad Rzeszutek Wilk, Ross Lagerwall
Hi Julien,
> -----Original Message-----
> From: Xen-devel <xen-devel-bounces@lists.xenproject.org> On Behalf Of
> Julien Grall
> Sent: 2022年5月24日 3:50
> To: xen-devel@lists.xenproject.org
> Cc: julien@xen.org; Julien Grall <jgrall@amazon.com>; Stefano Stabellini
> <sstabellini@kernel.org>; Bertrand Marquis <Bertrand.Marquis@arm.com>;
> Volodymyr Babchuk <Volodymyr_Babchuk@epam.com>; Konrad Rzeszutek Wilk
> <konrad.wilk@oracle.com>; Ross Lagerwall <ross.lagerwall@citrix.com>
> Subject: [PATCH] xen/arm: Remove most of the *_VIRT_END defines
>
> 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, ).
>
> 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.
> ---
> 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.
Cheers,
Wei Chen
> }
>
> /*
> --
> 2.32.0
>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] xen/arm: Remove most of the *_VIRT_END defines
2022-05-24 1:29 ` Wei Chen
@ 2022-05-24 8:06 ` Bertrand Marquis
2022-05-24 8:16 ` Wei Chen
0 siblings, 1 reply; 9+ messages in thread
From: Bertrand Marquis @ 2022-05-24 8:06 UTC (permalink / raw)
To: Wei Chen
Cc: Julien Grall, xen-devel, Julien Grall, Stefano Stabellini,
Volodymyr Babchuk, Konrad Rzeszutek Wilk, Ross Lagerwall
Hi Wei,
> On 24 May 2022, at 02:29, Wei Chen <Wei.Chen@arm.com> wrote:
>
> Hi Julien,
>
>> -----Original Message-----
>> From: Xen-devel <xen-devel-bounces@lists.xenproject.org> On Behalf Of
>> Julien Grall
>> Sent: 2022年5月24日 3:50
>> To: xen-devel@lists.xenproject.org
>> Cc: julien@xen.org; Julien Grall <jgrall@amazon.com>; Stefano Stabellini
>> <sstabellini@kernel.org>; Bertrand Marquis <Bertrand.Marquis@arm.com>;
>> Volodymyr Babchuk <Volodymyr_Babchuk@epam.com>; Konrad Rzeszutek Wilk
>> <konrad.wilk@oracle.com>; Ross Lagerwall <ross.lagerwall@citrix.com>
>> Subject: [PATCH] xen/arm: Remove most of the *_VIRT_END defines
>>
>> 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, ).
>>
>> 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.
>> ---
>> 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
^ permalink raw reply [flat|nested] 9+ messages in thread
* RE: [PATCH] xen/arm: Remove most of the *_VIRT_END defines
2022-05-24 8:06 ` Bertrand Marquis
@ 2022-05-24 8:16 ` Wei Chen
2022-05-24 9:12 ` Julien Grall
0 siblings, 1 reply; 9+ messages in thread
From: Wei Chen @ 2022-05-24 8:16 UTC (permalink / raw)
To: Bertrand Marquis
Cc: Julien Grall, xen-devel, Julien Grall, Stefano Stabellini,
Volodymyr Babchuk, Konrad Rzeszutek Wilk, Ross Lagerwall
Hi Bertrand,
> -----Original Message-----
> From: Bertrand Marquis <Bertrand.Marquis@arm.com>
> Sent: 2022年5月24日 16:07
> To: Wei Chen <Wei.Chen@arm.com>
> Cc: Julien Grall <julien@xen.org>; xen-devel@lists.xenproject.org; Julien
> Grall <jgrall@amazon.com>; Stefano Stabellini <sstabellini@kernel.org>;
> Volodymyr Babchuk <Volodymyr_Babchuk@epam.com>; Konrad Rzeszutek Wilk
> <konrad.wilk@oracle.com>; Ross Lagerwall <ross.lagerwall@citrix.com>
> Subject: Re: [PATCH] xen/arm: Remove most of the *_VIRT_END defines
>
> Hi Wei,
>
> > On 24 May 2022, at 02:29, Wei Chen <Wei.Chen@arm.com> wrote:
> >
> > Hi Julien,
> >
> >> -----Original Message-----
> >> From: Xen-devel <xen-devel-bounces@lists.xenproject.org> On Behalf Of
> >> Julien Grall
> >> Sent: 2022年5月24日 3:50
> >> To: xen-devel@lists.xenproject.org
> >> Cc: julien@xen.org; Julien Grall <jgrall@amazon.com>; Stefano
> Stabellini
> >> <sstabellini@kernel.org>; Bertrand Marquis <Bertrand.Marquis@arm.com>;
> >> Volodymyr Babchuk <Volodymyr_Babchuk@epam.com>; Konrad Rzeszutek Wilk
> >> <konrad.wilk@oracle.com>; Ross Lagerwall <ross.lagerwall@citrix.com>
> >> Subject: [PATCH] xen/arm: Remove most of the *_VIRT_END defines
> >>
> >> 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, ).
> >>
> >> 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.
> >> ---
> >> 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.
>
Maybe I didn't describe it clearly : )
I agree with the SIZE that Julien has done. My concern is that, should we
need a guide line for how to use the SIZE to calculate END?
For example, in this patch, Julien is using END=START+SIZE, then END is
pointing to the address after the end. But for me, I intend to use
END=START+SIZE-1, because I want the END point to the last address.
Over time, when there are a lot of get_xxx_end functions in the code,
their actual meanings will be different, just as confusing as the previous
macro definitions
Cheers,
Wei Chen
> Cheers
> Bertrand
>
> >
> > Cheers,
> > Wei Chen
> >
> >> }
> >>
> >> /*
> >> --
> >> 2.32.0
>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] xen/arm: Remove most of the *_VIRT_END defines
2022-05-24 8:16 ` Wei Chen
@ 2022-05-24 9:12 ` Julien Grall
0 siblings, 0 replies; 9+ messages in thread
From: Julien Grall @ 2022-05-24 9:12 UTC (permalink / raw)
To: Wei Chen, Bertrand Marquis
Cc: xen-devel, Julien Grall, Stefano Stabellini, Volodymyr Babchuk,
Konrad Rzeszutek Wilk, Ross Lagerwall
Hi Wei,
On 24/05/2022 09:16, Wei Chen wrote:
>>> 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.
>>
>
> Maybe I didn't describe it clearly : )
> I agree with the SIZE that Julien has done. My concern is that, should we
> need a guide line for how to use the SIZE to calculate END
It is not possible to have a guideline at the moment because we are not
consistent how "END" is defined in Xen.
This is also why I want to get rid of "END" completely. It is more
difficult (to not say impossible) to interpret (start, size) differently.
As I wrote in the commit message, I haven't yet addressed the common
part (there work is a lot more consequent). But I wanted at least to
start to get rid of some and push the use of "end" as far as possible.
> For example, in this patch, Julien is using END=START+SIZE, then END is
> pointing to the address after the end. But for me, I intend to use
> END=START+SIZE-1, because I want the END point to the last address.
Same here.
>
> Over time, when there are a lot of get_xxx_end functions in the code,
> their actual meanings will be different, just as confusing as the previous
> macro definitions
My plan is to get rid of get_XXX_end completely and instead return
start, size.
This is only the first part of the work. I need this one start
reshuffling the memory layout because I want to be able to use (start,
size) consistently in the layout.
Cheers,
--
Julien Grall
^ permalink raw reply [flat|nested] 9+ messages in thread
* RE: [PATCH] xen/arm: Remove most of the *_VIRT_END defines
2022-05-23 19:49 [PATCH] xen/arm: Remove most of the *_VIRT_END defines Julien Grall
2022-05-24 1:29 ` Wei Chen
@ 2022-05-24 3:39 ` Wei Chen
2022-05-24 8:05 ` Bertrand Marquis
2 siblings, 0 replies; 9+ messages in thread
From: Wei Chen @ 2022-05-24 3:39 UTC (permalink / raw)
To: Julien Grall, xen-devel
Cc: Julien Grall, Stefano Stabellini, Bertrand Marquis,
Volodymyr Babchuk, Konrad Rzeszutek Wilk, Ross Lagerwall
(resend again, seems the first one is failed)
> -----Original Message-----
> From: Xen-devel <xen-devel-bounces@lists.xenproject.org> On Behalf Of
> Julien Grall
> Sent: 2022年5月24日 3:50
> To: xen-devel@lists.xenproject.org
> Cc: julien@xen.org; Julien Grall <jgrall@amazon.com>; Stefano Stabellini
> <sstabellini@kernel.org>; Bertrand Marquis <Bertrand.Marquis@arm.com>;
> Volodymyr Babchuk <Volodymyr_Babchuk@epam.com>; Konrad Rzeszutek Wilk
> <konrad.wilk@oracle.com>; Ross Lagerwall <ross.lagerwall@citrix.com>
> Subject: [PATCH] xen/arm: Remove most of the *_VIRT_END defines
>
> 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, ).
>
> 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.
> ---
> 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.
Cheers,
Wei Chen
> }
>
> /*
> --
> 2.32.0
>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] xen/arm: Remove most of the *_VIRT_END defines
2022-05-23 19:49 [PATCH] xen/arm: Remove most of the *_VIRT_END defines Julien Grall
2022-05-24 1:29 ` Wei Chen
2022-05-24 3:39 ` Wei Chen
@ 2022-05-24 8:05 ` Bertrand Marquis
2022-06-23 21:45 ` Julien Grall
2 siblings, 1 reply; 9+ messages in thread
From: Bertrand Marquis @ 2022-05-24 8:05 UTC (permalink / raw)
To: Julien Grall
Cc: xen-devel, Julien Grall, Stefano Stabellini, Volodymyr Babchuk,
Konrad Rzeszutek Wilk, Ross Lagerwall
Hi Julien,
> On 23 May 2022, at 20:49, Julien Grall <julien@xen.org> 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, ).
Thanks to have remembered this one :-)
>
> 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.
> ---
> 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) ?
If not then there might be something worse explaining with a comment here.
>
> #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.
> + */
Please just mention that you also fixed a comment coding style in the commit message.
> 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);
> }
>
> /*
> --
> 2.32.0
>
Cheers
Bertrand
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] xen/arm: Remove most of the *_VIRT_END defines
2022-05-24 8:05 ` Bertrand Marquis
@ 2022-06-23 21:45 ` Julien Grall
2022-06-28 14:24 ` Bertrand Marquis
0 siblings, 1 reply; 9+ messages in thread
From: Julien Grall @ 2022-06-23 21:45 UTC (permalink / raw)
To: Bertrand Marquis
Cc: xen-devel, Julien Grall, Stefano Stabellini, Volodymyr Babchuk,
Konrad Rzeszutek Wilk, Ross Lagerwall
Hi Bertrand,
On 24/05/2022 09:05, Bertrand Marquis wrote:
>> 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.
>> ---
>> 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
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] xen/arm: Remove most of the *_VIRT_END defines
2022-06-23 21:45 ` Julien Grall
@ 2022-06-28 14:24 ` Bertrand Marquis
0 siblings, 0 replies; 9+ messages in thread
From: Bertrand Marquis @ 2022-06-28 14:24 UTC (permalink / raw)
To: Julien Grall
Cc: xen-devel, Julien Grall, Stefano Stabellini, Volodymyr Babchuk,
Konrad Rzeszutek Wilk, Ross Lagerwall
Hi Julien,
> On 23 Jun 2022, at 22:45, Julien Grall <julien@xen.org> wrote:
>
> Hi Bertrand,
>
> On 24/05/2022 09:05, Bertrand Marquis wrote:
>>> 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.
>>> ---
>>> 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
^ permalink raw reply [flat|nested] 9+ messages in thread