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

[Julien Grall <julien@xxxxxxx>]

On 27/06/2022 07:23, Michal Orzel wrote:
> Hi Julien,
Hi,

Thanks for the review.

> On 24.06.2022 11:11, Julien Grall wrote:
> > From: Julien Grall <jgrall@xxxxxxxxxx>
> > ---
> >   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) )
> The second condition does not seem to be correct.
Hmm... You are right, it wants to be

((va - VMAP_VIRT_START) < VMAP_VIRT_SIZE)

> Instead, this check should like like following:
> if ( (va >= VMAP_VIRT_START) && (va < (VMAP_VIRT_START + VMAP_VIRT_SIZE)) )
This check would still be incorrect because if the VMAP is right at the 
edge of the address (e.g. 2^32 - 1 on arm32), then VMAP_VIRT_START + 
VMAP_VIRT_SIZE would be 0.
Cheers,

--
Julien Grall