Re: [PATCH 1/2] xen/arm32: head: Rework how the fixmap and early UART mapping are prepared

From: Henry Wang <Henry.Wang@arm.com>
To: Julien Grall <julien@xen.org>
Cc: Xen-devel <xen-devel@lists.xenproject.org>,
	Michal Orzel <michal.orzel@amd.com>,
	Ayan Kumar Halder <ayan.kumar.halder@amd.com>,
	Julien Grall <jgrall@amazon.com>,
	Stefano Stabellini <sstabellini@kernel.org>,
	Bertrand Marquis <Bertrand.Marquis@arm.com>,
	Volodymyr Babchuk <Volodymyr_Babchuk@epam.com>
Subject: Re: [PATCH 1/2] xen/arm32: head: Rework how the fixmap and early UART mapping are prepared
Date: Thu, 2 Nov 2023 02:29:57 +0000	[thread overview]
Message-ID: <FAB1A89E-0F0A-4410-80DC-F8DD1EC8ADE6@arm.com> (raw)
In-Reply-To: <20231101233011.83098-2-julien@xen.org>

Hi Julien,

> On Nov 2, 2023, at 07:30, Julien Grall <julien@xen.org> wrote:
> 
> From: Julien Grall <jgrall@amazon.com>
> 
> Since commit 5e213f0f4d2c ("xen/arm32: head: Widen the use of the
> temporary mapping"), boot_second (used to cover regions like Xen and
> the fixmap) will not be mapped if the identity mapping overlap.
> 
> So it is ok to prepare the fixmap table and link it in boot_second
> earlier. With that, the fixmap can also be used earlier via the
> temporary mapping.
> 
> Therefore split setup_fixmap() in two:
>    * The table is now linked in create_page_tables() because
>      the boot page tables needs to be recreated for every CPU.
>    * The early UART mapping is only added for the boot CPU0 as the
>      fixmap table is not cleared when secondary CPUs boot.
> 
> Signed-off-by: Julien Grall <jgrall@amazon.com>
> ---
> xen/arch/arm/arm32/head.S | 48 ++++++++-------------------------------
> 1 file changed, 9 insertions(+), 39 deletions(-)
> 
> diff --git a/xen/arch/arm/arm32/head.S b/xen/arch/arm/arm32/head.S
> index 33b038e7e0ca..fec2433e568f 100644
> --- a/xen/arch/arm/arm32/head.S
> +++ b/xen/arch/arm/arm32/head.S
> @@ -183,12 +183,16 @@ past_zImage:
>         bl    check_cpu_mode
>         bl    cpu_init
>         bl    create_page_tables
> +        /* Add the UART mapping if requested */
> +#ifdef CONFIG_EARLY_PRINTK
> +        mov_w r0, EARLY_UART_VIRTUAL_ADDRESS
> +        create_mapping_entry xen_fixmap, r0, r11, type=PT_DEV_L3

I tried to test this series today and found a corner case in this patch:

If we build Xen only with this patch, and CONFIG_EARLY_PRINTK=y, we
will get below from compiler:

```
arch/arm/arm32/head.S: Assembler messages:
arch/arm/arm32/head.S:189: Error: bad instruction `create_mapping_entry xen_fixmap,r0,r11,type=0xe73'
make[3]: *** [Rules.mk:253: arch/arm/arm32/head.o] Error 1
```

With the second patch applied, the above error will disappear and I confirm
arm32 will work fine with both patch applied + either CONFIG_EARLY_PRINTK={y,n}.

Kind regards,
Henry