Re: [PATCH v3 12/52] xen/mmu: extract early uart mapping from setup_fixmap

[Penny Zheng <penny.zheng@xxxxxxx>]

Hi Julien

On 2023/7/5 18:35, Julien Grall wrote:
> Hi Penny,
>
> On 05/07/2023 10:03, Penny Zheng wrote:
> > On 2023/7/5 06:25, Julien Grall wrote:
> > > Hi Penny,
> > >
> > > Title: You want to clarify that this change is arm64 only. So:
> > >
> > > xen/arm64: mmu: ...
> > >
> > > On 26/06/2023 04:34, Penny Zheng wrote:
> > > > Original setup_fixmap is actually doing two seperate tasks, one is 
> > > > enabling
> > > > the early UART when earlyprintk on, and the other is to set up the 
> > > > fixmap
> > > > even when earlyprintk is not configured.
> > > >
> > > > To be more dedicated and precise, the old function shall be split 
> > > > into two
> > > > functions, setup_early_uart and new setup_fixmap.
> > > While some of the split before would be warrant even without the MPU 
> > > support. This one is not because there is limited point to have 3 
> > > lines function. So I think you want to justify based on what you plan 
> > > to do with the MPU code.
> > > That said, I don't think we need to introduce setup_fixmap. See below.
> > >
> > >
> > > > Signed-off-by: Penny Zheng <penny.zheng@xxxxxxx>
> > > > Signed-off-by: Wei Chen <wei.chen@xxxxxxx>
> > > > ---
> > > > v3:
> > > > - new patch
> > > > ---
> > > >   xen/arch/arm/arm64/head.S     |  3 +++
> > > >   xen/arch/arm/arm64/mmu/head.S | 24 +++++++++++++++++-------
> > > >   2 files changed, 20 insertions(+), 7 deletions(-)
> > > >
> > > > diff --git a/xen/arch/arm/arm64/head.S b/xen/arch/arm/arm64/head.S
> > > > index e63886b037..55a4cfe69e 100644
> > > > --- a/xen/arch/arm/arm64/head.S
> > > > +++ b/xen/arch/arm/arm64/head.S
> > > > @@ -258,7 +258,10 @@ real_start_efi:
> > > >           b     enable_boot_mm
> > > >   primary_switched:
> > > > +        bl    setup_early_uart
> > > > +#ifdef CONFIG_HAS_FIXMAP
> > > >           bl    setup_fixmap
> > > > +#endif
> > > >   #ifdef CONFIG_EARLY_PRINTK
> > > >           /* Use a virtual address to access the UART. */
> > > >           ldr   x23, =EARLY_UART_VIRTUAL_ADDRESS
> > > > diff --git a/xen/arch/arm/arm64/mmu/head.S 
> > > > b/xen/arch/arm/arm64/mmu/head.S
> > > > index 2b209fc3ce..295596aca1 100644
> > > > --- a/xen/arch/arm/arm64/mmu/head.S
> > > > +++ b/xen/arch/arm/arm64/mmu/head.S
> > > > @@ -367,24 +367,34 @@ identity_mapping_removed:
> > > >   ENDPROC(remove_identity_mapping)
> > > >   /*
> > > > - * Map the UART in the fixmap (when earlyprintk is used) and hook the
> > > > - * fixmap table in the page tables.
> > > > - *
> > > > - * The fixmap cannot be mapped in create_page_tables because it may
> > > > - * clash with the 1:1 mapping.
> > > Since commit 9d267c049d92 ("xen/arm64: Rework the memory layout"), 
> > > there is no chance that the fixmap will clash with the 1:1 mapping. 
> > > So rather than introducing a new function, I would move the creation 
> > > of the fixmap in create_pagetables().
> > Understood. I'll move the creation of the fixmap in create_pagetables().
> >
> > > This would avoid the #ifdef CONFIG_HAS_FIXMAP in head.S.
> > >
> > > > + * Map the UART in the fixmap (when earlyprintk is used)
> > > >    *
> > > >    * Inputs:
> > > > - *   x20: Physical offset
> > > >    *   x23: Early UART base physical address
> > > >    *
> > > >    * Clobbers x0 - x3
> > > >    */
> > > > -ENTRY(setup_fixmap)
> > > > +ENTRY(setup_early_uart)
> > > >   #ifdef CONFIG_EARLY_PRINTK
> > > >           /* Add UART to the fixmap table */
> > > >           ldr   x0, =EARLY_UART_VIRTUAL_ADDRESS
> > > >           create_mapping_entry xen_fixmap, x0, x23, x1, x2, x3, 
> > > > type=PT_DEV_L3
> > > > +        /* Ensure any page table updates made above have occurred. */
> > > > +        dsb   nshst
> > > > +
> > > > +        ret
> > > The 'ret' needs to be outside of the '#ifdef' block. But, in this 
> > > case, I would prefer if we don't call setup_early_uart() when 
> > > !CONFIG_EARLY_PRINTK in head.S
> > okay. I'll move the #ifdef to the caller in head.S.
> Thinking about this again. I think you can actually move the mapping of 
> the UART in create_pagetables() because it will also not clash with the 
> 1:1.
> For the MPU, the mapping could then be moved in prepare_early_mappings().
>
> This would reduce the number of functions exposed.
Understood! will do.

> Cheers,