Re: [PATCH 5/9] xen/arm: Rework the code mapping Xen to avoid relying on the size of Xen

[Michal Orzel <michal.orzel@xxxxxxx>]

On 28/06/2023 22:02, Julien Grall wrote:
> 
> 
> Hi,
> 
> On 26/06/2023 12:43, Michal Orzel wrote:
>>
>>
>> On 25/06/2023 22:49, Julien Grall wrote:
>>>
>>>
>>> From: Julien Grall <jgrall@xxxxxxxxxx>
>>>
>>> At the moment, the maximum size of Xen binary we can support is 2MB.
>>> This is what we reserved in the virtual address but also what all
>>> the code in Xen relies on as we only allocate one L3 page-table.
>>>
>>> When feature like UBSAN (will be enabled in a follow-up patch) and GCOV
>>> are enabled, the binary will be way over 2MB.
>>>
>>> The code is now reworked so it doesn't realy on a specific size but
>> s/realy/rely
>>
>>> will instead look at the reversed size and compute the number of
>>> page-table to allocate/map.
>>>
>>> While at it, replace any reference to 4KB mappings with a more
>>> generic word because the page-size may change in the future.
>>>
>>> Also fix the typo s/tlb/tbl/ in code move in arm32/head.S
>>>
>>> Signed-off-by: Julien Grall <jgrall@xxxxxxxxxx>
>>> ---
>>>   xen/arch/arm/arm32/head.S         | 61 ++++++++++++++++++++++++-------
>>>   xen/arch/arm/arm64/head.S         | 51 +++++++++++++++++++++-----
>>>   xen/arch/arm/include/asm/config.h |  1 +
>>>   xen/arch/arm/include/asm/lpae.h   |  8 ++--
>>>   xen/arch/arm/mm.c                 | 24 +++++++-----
>>>   5 files changed, 108 insertions(+), 37 deletions(-)
>>>
>>> diff --git a/xen/arch/arm/arm32/head.S b/xen/arch/arm/arm32/head.S
>>> index 5e3692eb3abf..5448671de897 100644
>>> --- a/xen/arch/arm/arm32/head.S
>>> +++ b/xen/arch/arm/arm32/head.S
>>> @@ -373,35 +373,55 @@ ENDPROC(cpu_init)
>>>   .endm
>>>
>>>   /*
>>> - * Macro to create a page table entry in \ptbl to \tbl
>>> + * Macro to create a page table entry in \ptbl to \tbl (physical
>>> + * address)
>>>    *
>>>    * ptbl:    table symbol where the entry will be created
>>> - * tbl:     table symbol to point to
>>> + * tbl:     physical address of the table to point to
>>>    * virt:    virtual address
>>>    * lvl:     page-table level
>>>    *
>>>    * Preserves \virt
>>> - * Clobbers r1 - r4
>>> + * Clobbers \tbl, r1 - r3
>>>    *
>>>    * Also use r10 for the phys offset.
>> This no longer applies.
> 
> Good point. I will remove it.
> 
>>
>>>    *
>>> - * Note that \virt should be in a register other than r1 - r4
>>> + * Note that \tbl and \virt should be in a register other than r1 - r3
>>>    */
>>> -.macro create_table_entry, ptbl, tbl, virt, lvl
>>> -        get_table_slot r1, \virt, \lvl  /* r1 := slot in \tlb */
>>> -        lsl   r1, r1, #3                /* r1 := slot offset in \tlb */
>>> -
>>> -        load_paddr r4, \tbl
>>> +.macro create_table_entry_from_paddr, ptbl, tbl, virt, lvl
>>> +        get_table_slot r1, \virt, \lvl  /* r1 := slot in \tbl */
>>> +        lsl   r1, r1, #3                /* r1 := slot offset in \tbl */
>>>
>>>           movw  r2, #PT_PT             /* r2:r3 := right for linear PT */
>>> -        orr   r2, r2, r4             /*           + \tlb paddr */
>>> +        orr   r2, r2, \tbl           /*           + \tbl paddr */
>>>           mov   r3, #0
>>>
>>> -        adr_l r4, \ptbl
>>> +        adr_l \tbl, \ptbl            /* \tbl := (v,p)addr of \ptbl */
>>>
>>> -        strd  r2, r3, [r4, r1]
>>> +        strd  r2, r3, [\tbl, r1]
>>>   .endm
>>>
>>> +
>>> +/*
>>> + * Macro to create a page table entry in \ptbl to \tbl (symbol)
>>> + *
>>> + * ptbl:    table symbol where the entry will be created
>>> + * tbl:     table symbol to point to
>>> + * virt:    virtual address
>>> + * lvl:     page-table level
>>> + *
>>> + * Preserves \virt
>>> + * Clobbers r1 - r4
>>> + *
>>> + * Also use r10 for the phys offset.
>>> + *
>>> + * Note that \virt should be in a register other than r1 - r4
>>> + */
>>> +.macro create_table_entry, ptbl, tbl, virt, lvl
>>> +        load_paddr r4, \tbl
>>> +        create_table_entry_from_paddr \ptbl, r4, \virt, \lvl
>>> + .endm
>>> +
>>>   /*
>>>    * Macro to create a mapping entry in \tbl to \paddr. Only mapping in 3rd
>>>    * level table (i.e page granularity) is supported.
>>> @@ -451,13 +471,26 @@ ENDPROC(cpu_init)
>>>    * Output:
>>>    *   r12: Was a temporary mapping created?
>>>    *
>>> - * Clobbers r0 - r4
>>> + * Clobbers r0 - r5
>>>    */
>>>   create_page_tables:
>>>           /* Prepare the page-tables for mapping Xen */
>>>           mov_w r0, XEN_VIRT_START
>>>           create_table_entry boot_pgtable, boot_second, r0, 1
>>> -        create_table_entry boot_second, boot_third, r0, 2
>>> +
>>> +        /*
>>> +         * We need to use a stash register because
>>> +         * create_table_entry_paddr() will clobber the register storing
>>> +         * the physical address of the table to point to.
>>> +         */
>>> +        load_paddr r5, boot_third
>>> +        mov_w r4, XEN_VIRT_START
>> Can you just reuse r0 that is already set to XEN_VIRT_START not to repeat 
>> this operation over and over again?
> I decided against this approach for a few reasons:
>   * I wanted the register to be ordered when
> create_table_entry_from_paddr is called.
>   * There is a necessary largish comment on top of XEN_VIRT_START. So it
> makes more "difficult" to find the content of the registers.
> 
> I am actually a bit puzzled with your comment given you were recently
> the one that was pushing for adding extra ISB in the code (I still need
> to send a patch for that) to make the code clearer. ISBs are way more
> expensive than executing two instructions. So is this request just down
> to a matter of taste?
I believe you are right. The resulting code seems to be more clear and easy
to understand and this is what we should care about (which stays consistent 
with ISB case).
Also, this was more like a comment to check if you just missed it or did it 
deliberately,
not something crucial (hence I already provided my tag).

~Michal