Re: [PATCH v2 05/40] xen/arm64: prepare for moving MMU related code from head.S

[Julien Grall <julien@xen.org>]

On 18/01/2023 03:09, Wei Chen wrote:
> Hi Julien,
> 
>> -----Original Message-----
>> From: Julien Grall <julien@xen.org>
>> Sent: 2023年1月18日 7:37
>> To: Penny Zheng <Penny.Zheng@arm.com>; xen-devel@lists.xenproject.org
>> Cc: Wei Chen <Wei.Chen@arm.com>; Stefano Stabellini
>> <sstabellini@kernel.org>; Bertrand Marquis <Bertrand.Marquis@arm.com>;
>> Volodymyr Babchuk <Volodymyr_Babchuk@epam.com>
>> Subject: Re: [PATCH v2 05/40] xen/arm64: prepare for moving MMU related
>> code from head.S
>>
>> Hi Penny,
>>
>> On 13/01/2023 05:28, Penny Zheng wrote:
>>> From: Wei Chen <wei.chen@arm.com>
>>>
>>> We want to reuse head.S for MPU systems, but there are some
>>> code implemented for MMU systems only. We will move such
>>> code to another MMU specific file. But before that, we will
>>> do some preparations in this patch to make them easier
>>> for reviewing:
>>
>> Well, I agree that...
>>
>>> 1. Fix the indentations of code comments.
>>
>> ... changing the indentation is better here. But...
>>
>>> 2. Export some symbols that will be accessed out of file
>>>      scope.
>>
>> ... I have no idea which functions are going to be used in a separate
>> file. So I think they should belong to the patch moving the code.
>>
> 
> Ok, I will move these changes to the moving code patches.
> 
>>>
>>> Signed-off-by: Wei Chen <wei.chen@arm.com>
>>
>> Your signed-off-by is missing.
>>
>>> ---
>>> v1 -> v2:
>>> 1. New patch.
>>> ---
>>>    xen/arch/arm/arm64/head.S | 40 +++++++++++++++++++--------------------
>>>    1 file changed, 20 insertions(+), 20 deletions(-)
>>>
>>> diff --git a/xen/arch/arm/arm64/head.S b/xen/arch/arm/arm64/head.S
>>> index 93f9b0b9d5..b2214bc5e3 100644
>>> --- a/xen/arch/arm/arm64/head.S
>>> +++ b/xen/arch/arm/arm64/head.S
>>> @@ -136,22 +136,22 @@
>>>            add \xb, \xb, x20
>>>    .endm
>>>
>>> -        .section .text.header, "ax", %progbits
>>> -        /*.aarch64*/
>>> +.section .text.header, "ax", %progbits
>>> +/*.aarch64*/
>>
>> This change is not mentioned.
>>
> 
> I will add the description in commit message.
> 
>>>
>>> -        /*
>>> -         * Kernel startup entry point.
>>> -         * ---------------------------
>>> -         *
>>> -         * The requirements are:
>>> -         *   MMU = off, D-cache = off, I-cache = on or off,
>>> -         *   x0 = physical address to the FDT blob.
>>> -         *
>>> -         * This must be the very first address in the loaded image.
>>> -         * It should be linked at XEN_VIRT_START, and loaded at any
>>> -         * 4K-aligned address.  All of text+data+bss must fit in 2MB,
>>> -         * or the initial pagetable code below will need adjustment.
>>> -         */
>>> +/*
>>> + * Kernel startup entry point.
>>> + * ---------------------------
>>> + *
>>> + * The requirements are:
>>> + *   MMU = off, D-cache = off, I-cache = on or off,
>>> + *   x0 = physical address to the FDT blob.
>>> + *
>>> + * This must be the very first address in the loaded image.
>>> + * It should be linked at XEN_VIRT_START, and loaded at any
>>> + * 4K-aligned address.  All of text+data+bss must fit in 2MB,
>>> + * or the initial pagetable code below will need adjustment.
>>> + */
>>>
>>>    GLOBAL(start)
>>>            /*
>>> @@ -586,7 +586,7 @@ ENDPROC(cpu_init)
>>>     *
>>>     * Clobbers x0 - x4
>>>     */
>>> -create_page_tables:
>>> +ENTRY(create_page_tables)
>>
>> I am not sure about keeping this name. Now we have create_page_tables()
>> and arch_setup_page_tables().
>>
>> I would conside to name it create_boot_page_tables().
>>
> 
> Do you need me to rename it in this patch?

So looking at the rest of the series, I see you are already renaming the 
helper in patch #11. I think it would be better if the naming is done 
earlier.

That said, I am not convinced that create_page_tables() should actually 
be called externally.

In fact, you have something like:

    bl create_page_tables
    bl enable_mmu

Both will need a MMU/MPU specific implementation. So it would be better 
if we provide a wrapper to limit the number of external functions.

Cheers,

-- 
Julien Grall