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

[Bertrand Marquis <Bertrand.Marquis@xxxxxxx>]

Hi Julien,

> On 23 Jun 2022, at 22:45, Julien Grall <julien@xxxxxxx> wrote:
> 
> 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.

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