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

[Julien Grall <julien@xxxxxxx>]

Hi Bertrand,

On 24/05/2022 09:05, Bertrand Marquis wrote:
> > Signed-off-by: Julien Grall <jgrall@xxxxxxxxxx>
> >
> > ----
> >
> > I noticed that a few functions in Xen expect [start, end[. This is risky
> > as we may end up with end < start if the region is defined right at the
> > top of the address space.
> >
> > I haven't yet tackle this issue. But I would at least like to get rid
> > of *_VIRT_END.
> > ---
> > xen/arch/arm/include/asm/config.h | 18 ++++++++----------
> > xen/arch/arm/livepatch.c          |  2 +-
> > xen/arch/arm/mm.c                 | 13 ++++++++-----
> > 3 files changed, 17 insertions(+), 16 deletions(-)
> >
> > diff --git a/xen/arch/arm/include/asm/config.h 
> > b/xen/arch/arm/include/asm/config.h
> > index 3e2a55a91058..66db618b34e7 100644
> > --- a/xen/arch/arm/include/asm/config.h
> > +++ b/xen/arch/arm/include/asm/config.h
> > @@ -111,12 +111,11 @@
> > #define FIXMAP_ADDR(n)        (_AT(vaddr_t,0x00400000) + (n) * PAGE_SIZE)
> >
> > #define BOOT_FDT_VIRT_START    _AT(vaddr_t,0x00600000)
> > -#define BOOT_FDT_SLOT_SIZE     MB(4)
> > -#define BOOT_FDT_VIRT_END      (BOOT_FDT_VIRT_START + BOOT_FDT_SLOT_SIZE)
> > +#define BOOT_FDT_VIRT_SIZE     _AT(vaddr_t, MB(4))
> >
> > #ifdef CONFIG_LIVEPATCH
> > #define LIVEPATCH_VMAP_START   _AT(vaddr_t,0x00a00000)
> > -#define LIVEPATCH_VMAP_END     (LIVEPATCH_VMAP_START + MB(2))
> > +#define LIVEPATCH_VMAP_SIZE    _AT(vaddr_t, MB(2))
> > #endif
> >
> > #define HYPERVISOR_VIRT_START  XEN_VIRT_START
> > @@ -132,18 +131,18 @@
> > #define FRAMETABLE_VIRT_END    (FRAMETABLE_VIRT_START + FRAMETABLE_SIZE - 1)
> >
> > #define VMAP_VIRT_START        _AT(vaddr_t,0x10000000)
> > +#define VMAP_VIRT_SIZE         _AT(vaddr_t, GB(1) - MB(256))
> 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