Re: [PATCH v2 2/2] xen/arm32: head: Improve logging in head.S

[Michal Orzel <michal.orzel@xxxxxxx>]

On 12/01/2024 11:58, Julien Grall wrote:
> 
> 
> On 12/01/2024 08:49, Michal Orzel wrote:
>> Hi Julien,
> 
> Hi Michal,
> 
>> On 11/01/2024 19:34, Julien Grall wrote:
>>>
>>>
>>> From: Julien Grall <jgrall@xxxxxxxxxx>
>>>
>>> The sequence to enable the MMU on arm32 is quite complex as we may need
>>> to jump to a temporary mapping to map Xen.
>>>
>>> Recently, we had one bug in the logic (see f5a49eb7f8b3 ("xen/arm32:
>>> head: Add mising isb in switch_to_runtime_mapping()") and it was
>>> a pain to debug because there are no logging.
>>>
>>> In order to improve the logging in the MMU switch we need to add
>>> support for early printk while running on the identity mapping
>>> and also on the temporary mapping.
>>>
>>> For the identity mapping, we have only the first page of Xen mapped.
>>> So all the strings should reside in the first page. For that purpose
>>> a new macro PRINT_ID is introduced.
>>>
>>> For the temporary mapping, the fixmap is already linked in the temporary
>>> area (and so does the UART). So we just need to update the register
>>> storing the UART address (i.e. r11) to point to the UART temporary
>>> mapping.
>>>
>>> Take the opportunity to introduce mov_w_on_cond in order to
>>> conditionally execute mov_w and avoid branches.
>>>
>>> Signed-off-by: Julien Grall <jgrall@xxxxxxxxxx>
>> Reviewed-by: Michal Orzel <michal.orzel@xxxxxxx>
> 
> Thanks!
> 
>>>   /*
>>> @@ -29,16 +33,26 @@
>>>
>>>   #ifdef CONFIG_EARLY_PRINTK
>>>   /*
>>> - * Macro to print a string to the UART, if there is one.
>>> + * Macros to print a string to the UART, if there is one.
>>> + *
>>> + * There are multiple flavors:
>>> + *  - PRINT_SECT(section, string): The @string will be located in @section
>>> + *  - PRINT(): The string will be located in .rodata.str.
>>> + *  - PRINT_ID(): When Xen is running on the Identity Mapping, it is
>>> + *    only possible to have a limited amount of Xen. This will create
>>> + *    the string in .rodata.idmap which will always be mapped.
>>>    *
>>>    * Clobbers r0 - r3
>>>    */
>>> -#define PRINT(_s)           \
>>> -        mov   r3, lr       ;\
>>> -        adr_l r0, 98f      ;\
>>> -        bl    asm_puts     ;\
>>> -        mov   lr, r3       ;\
>>> -        RODATA_STR(98, _s)
>>> +#define PRINT_SECT(section, string)         \
>>> +        mov   r3, lr                       ;\
>>> +        adr_l r0, 98f                      ;\
>>> +        bl    asm_puts                     ;\
>>> +        mov   lr, r3                       ;\
>>> +        RODATA_SECT(section, 98, string)
>>> +
>>> +#define PRINT(string) PRINT_SECT(.rodata.str, string)
>>> +#define PRINT_ID(string) PRINT_SECT(.rodata.idmap, string)
>> I know this is just a macro but does it make sense to have something MMU 
>> specific in common header?
>> I don't expect MPU to use it.
> For cache coloring, I would like secondary boot CPUs to start directly
> on the colored Xen. This means that any message used before enabling the
> MMU will need to be part of the .rodata.idmap.
> 
> I know that 32-bit is not in scope for the cache coloring series. But I
> would like to keep 32-bit and 64-bit boot logic fairly similar.
> 
> With that in mind, would you be happy if I keep PRINT_ID() in macros.h?
> Note that I would be ok to move in mmu/head.S and move back in macros.h
> later on. I just wanted to avoid code movement :).
With the above explanation it does not make sense to move it back and forth, so 
let's keep it as is.

~Michal