xen/arm: Remove most of the *_VIRT_END defines

Commit Message

Julien Grall May 23, 2022, 7:49 p.m. UTC

From: Julien Grall <[e-mail]>

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 <[e-mail]>

----

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(-)

Comments

Wei Chen May 24, 2022, 1:29 a.m. UTC | #1

Hi Julien,

> -----Original Message-----
> From: Xen-devel <[e-mail]> On Behalf Of
> Julien Grall
> Sent: 2022年5月24日 3:50
> To: [e-mail]
> Cc: [e-mail]; Julien Grall <[e-mail]>; Stefano Stabellini
> <[e-mail]>; Bertrand Marquis <[e-mail]>;
> Volodymyr Babchuk <[e-mail]>; Konrad Rzeszutek Wilk
> <[e-mail]>; Ross Lagerwall <[e-mail]>
> Subject: [PATCH] xen/arm: Remove most of the *_VIRT_END defines
> 
> From: Julien Grall <[e-mail]>
> 
> 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 <[e-mail]>
> 
> ----
> 
> 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
>

Wei Chen May 24, 2022, 3:39 a.m. UTC | #2

(resend again, seems the first one is failed)

> -----Original Message-----
> From: Xen-devel <[e-mail]> On Behalf Of
> Julien Grall
> Sent: 2022年5月24日 3:50
> To: [e-mail]
> Cc: [e-mail]; Julien Grall <[e-mail]>; Stefano Stabellini
> <[e-mail]>; Bertrand Marquis <[e-mail]>;
> Volodymyr Babchuk <[e-mail]>; Konrad Rzeszutek Wilk
> <[e-mail]>; Ross Lagerwall <[e-mail]>
> Subject: [PATCH] xen/arm: Remove most of the *_VIRT_END defines
> 
> From: Julien Grall <[e-mail]>
> 
> 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 <[e-mail]>
> 
> ----
> 
> 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
>

Bertrand Marquis May 24, 2022, 8:05 a.m. UTC | #3

Hi Julien,

> On 23 May 2022, at 20:49, Julien Grall <[e-mail]> wrote:
> 
> From: Julien Grall <[e-mail]>
> 
> 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 <[e-mail]>
> 
> ----
> 
> 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

Bertrand Marquis May 24, 2022, 8:06 a.m. UTC | #4

Hi Wei,

> On 24 May 2022, at 02:29, Wei Chen <[e-mail]> wrote:
> 
> Hi Julien,
> 
>> -----Original Message-----
>> From: Xen-devel <[e-mail]> On Behalf Of
>> Julien Grall
>> Sent: 2022年5月24日 3:50
>> To: [e-mail]
>> Cc: [e-mail]; Julien Grall <[e-mail]>; Stefano Stabellini
>> <[e-mail]>; Bertrand Marquis <[e-mail]>;
>> Volodymyr Babchuk <[e-mail]>; Konrad Rzeszutek Wilk
>> <[e-mail]>; Ross Lagerwall <[e-mail]>
>> Subject: [PATCH] xen/arm: Remove most of the *_VIRT_END defines
>> 
>> From: Julien Grall <[e-mail]>
>> 
>> 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 <[e-mail]>
>> 
>> ----
>> 
>> 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

Wei Chen May 24, 2022, 8:16 a.m. UTC | #5

Hi Bertrand,

> -----Original Message-----
> From: Bertrand Marquis <[e-mail]>
> Sent: 2022年5月24日 16:07
> To: Wei Chen <[e-mail]>
> Cc: Julien Grall <[e-mail]>; [e-mail]; Julien
> Grall <[e-mail]>; Stefano Stabellini <[e-mail]>;
> Volodymyr Babchuk <[e-mail]>; Konrad Rzeszutek Wilk
> <[e-mail]>; Ross Lagerwall <[e-mail]>
> Subject: Re: [PATCH] xen/arm: Remove most of the *_VIRT_END defines
> 
> Hi Wei,
> 
> > On 24 May 2022, at 02:29, Wei Chen <[e-mail]> wrote:
> >
> > Hi Julien,
> >
> >> -----Original Message-----
> >> From: Xen-devel <[e-mail]> On Behalf Of
> >> Julien Grall
> >> Sent: 2022年5月24日 3:50
> >> To: [e-mail]
> >> Cc: [e-mail]; Julien Grall <[e-mail]>; Stefano
> Stabellini
> >> <[e-mail]>; Bertrand Marquis <[e-mail]>;
> >> Volodymyr Babchuk <[e-mail]>; Konrad Rzeszutek Wilk
> >> <[e-mail]>; Ross Lagerwall <[e-mail]>
> >> Subject: [PATCH] xen/arm: Remove most of the *_VIRT_END defines
> >>
> >> From: Julien Grall <[e-mail]>
> >>
> >> 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 <[e-mail]>
> >>
> >> ----
> >>
> >> 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
>

Julien Grall May 24, 2022, 9:12 a.m. UTC | #6

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 June 23, 2022, 9:45 p.m. UTC | #7

Hi Bertrand,

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

Bertrand Marquis June 28, 2022, 2:24 p.m. UTC | #8

Hi Julien,

> On 23 Jun 2022, at 22:45, Julien Grall <[e-mail]> wrote:
> 
> Hi Bertrand,
> 
> On 24/05/2022 09:05, Bertrand Marquis wrote:
>>> Signed-off-by: Julien Grall <[e-mail]>
>>> 
>>> ----
>>> 
>>> 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

Patch

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);
 }
 
 /*