Re: [PATCH v3 15/18] xen/arm64: mm: Introduce helpers to prepare/enable/disable the identity mapping

[Julien Grall <julien@xxxxxxx>]

Hi,

On 13/12/2022 01:41, Stefano Stabellini wrote:
> > diff --git a/xen/arch/arm/include/asm/setup.h b/xen/arch/arm/include/asm/setup.h
> > index fdbf68aadcaa..e7a80fecec14 100644
> > --- a/xen/arch/arm/include/asm/setup.h
> > +++ b/xen/arch/arm/include/asm/setup.h
> > @@ -168,6 +168,17 @@ int map_range_to_domain(const struct dt_device_node *dev,
> >   
> >   extern const char __ro_after_init_start[], __ro_after_init_end[];
> >   
> > +extern DEFINE_BOOT_PAGE_TABLE(boot_pgtable);
> > +
> > +#ifdef CONFIG_ARM_64
> > +extern DEFINE_BOOT_PAGE_TABLE(boot_first_id);
> > +#endif
> > +extern DEFINE_BOOT_PAGE_TABLE(boot_second_id);
> > +extern DEFINE_BOOT_PAGE_TABLE(boot_third_id);
> This is more a matter of taste but I would either:
> - define extern all BOOT_PAGE_TABLEs here both ARM64 and ARM32 with
>    #ifdefs
A grep of BOOT_PAGE_TABLE shows that they are all defined in setup.h.

> - or define all the ARM64 only BOOT_PAGE_TABLE in arm64/mm.h and all the
>    ARM32 only BOOT_PAGE_TABLE in arm32/mm.h >
> Right now we have a mix, as we have boot_first_id with a #ifdef here
> and we have xen_pgtable in arm64/mm.h
We are talking about two distinct set of page-tables. One is used at 
runtime (i.e. xen_pgtable) and the other are for boot/smp-bring up.
So adding the boot_* in setup.h is correct. As I wrote earlier, setup.h 
would need a split. But this is not something I really want to handle 
here...
> Also we are missing boot_second and boot_third. We might as well be
> consistent and declare them all?
My plan is really to kill boot_second and boot_third. So I don't really 
want to export them right now (even temporarily).
In any case, I don't think such change belongs in this patch (it is 
already complex enough).
> > +/* Find where Xen will be residing at runtime and return an PT entry */
> > +lpae_t pte_of_xenaddr(vaddr_t);
> > +
> >   #endif
> >   /*
> >    * Local variables:
> > diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
> > index 0cf7ad4f0e8c..39e0d9e03c9c 100644
> > --- a/xen/arch/arm/mm.c
> > +++ b/xen/arch/arm/mm.c
> > @@ -93,7 +93,7 @@ DEFINE_BOOT_PAGE_TABLE(boot_third);
> >   
> >   #ifdef CONFIG_ARM_64
> >   #define HYP_PT_ROOT_LEVEL 0
> > -static DEFINE_PAGE_TABLE(xen_pgtable);
> > +DEFINE_PAGE_TABLE(xen_pgtable);
> >   static DEFINE_PAGE_TABLE(xen_first);
> >   #define THIS_CPU_PGTABLE xen_pgtable
> >   #else
> > @@ -388,7 +388,7 @@ void flush_page_to_ram(unsigned long mfn, bool sync_icache)
> >           invalidate_icache();
> >   }
> >   
> > -static inline lpae_t pte_of_xenaddr(vaddr_t va)
> > +lpae_t pte_of_xenaddr(vaddr_t va)
> >   {
> >       paddr_t ma = va + phys_offset;
> >   
> > @@ -495,6 +495,8 @@ void __init setup_pagetables(unsigned long boot_phys_offset)
> >   
> >       phys_offset = boot_phys_offset;
> >   
> > +    arch_setup_page_tables();
> > +
> >   #ifdef CONFIG_ARM_64
> >       pte = pte_of_xenaddr((uintptr_t)xen_first);
> >       pte.pt.table = 1;
> > --
> > 2.38.1
Cheers,

--
Julien Grall