Re: [PATCH v1 4/5] xen/riscv: introduce device tree maping function

[Oleksii <oleksii.kurochko@xxxxxxxxx>]

On Wed, 2024-07-10 at 12:38 +0200, Jan Beulich wrote:
> On 03.07.2024 12:42, Oleksii Kurochko wrote:
> > Signed-off-by: Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>
> > ---
> >  xen/arch/riscv/include/asm/config.h |  6 +++++
> >  xen/arch/riscv/include/asm/mm.h     |  2 ++
> >  xen/arch/riscv/mm.c                 | 37
> > +++++++++++++++++++++++++----
> >  3 files changed, 40 insertions(+), 5 deletions(-)
> 
> I don't think a change like this can come without any description.
> 
> > --- a/xen/arch/riscv/include/asm/config.h
> > +++ b/xen/arch/riscv/include/asm/config.h
> > @@ -74,6 +74,9 @@
> >  #error "unsupported RV_STAGE1_MODE"
> >  #endif
> >  
> > +#define XEN_SIZE                MB(2)
> > +#define XEN_VIRT_END            (XEN_VIRT_START + XEN_SIZE)
> 
> Probably wants accompanying by an assertion in the linker script. Or
> else
> how would one notice when Xen grows bigger than this?
I use XEN_SIZE in the linker script here:
 ASSERT(_end - _start <= MB(2), "Xen too large for early-boot
assumptions")

> 
> > @@ -99,6 +102,9 @@
> >  #define VMAP_VIRT_START         SLOTN(VMAP_SLOT_START)
> >  #define VMAP_VIRT_SIZE          GB(1)
> >  
> > +#define BOOT_FDT_VIRT_START     XEN_VIRT_END
> > +#define BOOT_FDT_VIRT_SIZE      MB(4)
> 
> Is the 4 selected arbitrarily, or derived from something?
Yes, it was chosen arbitrarily. I just checked that I don't have any
DTBs larger than 2 MB, but decided to add a little extra space and
doubled it to an additional 2 MB.

> 
> Also maybe better to keep these #define-s sorted by address? (As to
> "keep":
> I didn't check whether they currently are.)
> 
> > --- a/xen/arch/riscv/include/asm/mm.h
> > +++ b/xen/arch/riscv/include/asm/mm.h
> > @@ -255,4 +255,6 @@ static inline unsigned int
> > arch_get_dma_bitsize(void)
> >      return 32; /* TODO */
> >  }
> >  
> > +void* early_fdt_map(paddr_t fdt_paddr);
> 
> Nit: * and blank want to change places.
> 
> > --- a/xen/arch/riscv/mm.c
> > +++ b/xen/arch/riscv/mm.c
> > @@ -1,5 +1,6 @@
> >  /* SPDX-License-Identifier: GPL-2.0-only */
> >  
> > +#include <xen/bootfdt.h>
> >  #include <xen/bug.h>
> >  #include <xen/compiler.h>
> >  #include <xen/init.h>
> > @@ -7,7 +8,9 @@
> >  #include <xen/macros.h>
> >  #include <xen/mm.h>
> >  #include <xen/pfn.h>
> > +#include <xen/libfdt/libfdt.h>
> 
> This wants to move up, to retain sorting.
> 
> >  #include <xen/sections.h>
> > +#include <xen/sizes.h>
> >  
> >  #include <asm/early_printk.h>
> >  #include <asm/csr.h>
> > @@ -20,7 +23,7 @@ struct mmu_desc {
> >      unsigned int pgtbl_count;
> >      pte_t *next_pgtbl;
> >      pte_t *pgtbl_base;
> > -};
> > +} mmu_desc = { CONFIG_PAGING_LEVELS, 0, NULL, 0 };
> 
> __initdata and static?
> 
> > @@ -39,9 +42,11 @@ static unsigned long __ro_after_init
> > phys_offset;
> >   * isn't 2 MB aligned.
> >   *
> >   * CONFIG_PAGING_LEVELS page tables are needed for the identity
> > mapping,
> > - * except that the root page table is shared with the initial
> > mapping
> > + * except that the root page table is shared with the initial
> > mapping.
> > + *
> > + * CONFIG_PAGING_LEVELS page tables are needed for device tree
> > mapping.
> >   */
> > -#define PGTBL_INITIAL_COUNT ((CONFIG_PAGING_LEVELS - 1) * 2 + 1)
> > +#define PGTBL_INITIAL_COUNT ((CONFIG_PAGING_LEVELS - 1) * 3 + 1 +
> > 1)
> 
> Considering what would happen if two or three more of such
> requirements
> were added, maybe better
> 
> #define PGTBL_INITIAL_COUNT (CONFIG_PAGING_LEVELS * 3 - 1)
> 
> ? However, why is it CONFIG_PAGING_LEVELS that's needed, and not
> CONFIG_PAGING_LEVELS - 1? The top level table is the same as the
> identity map's, isn't it?
The top level table is the same, but I just wanted to be sure that if
DTB size is bigger then 2Mb then we need 2xL0 page tables.

Am I missing something?

~ Oleksii
> 
> > @@ -296,6 +299,30 @@ unsigned long __init calc_phys_offset(void)
> >      return phys_offset;
> >  }
> >  
> > +void* __init early_fdt_map(paddr_t fdt_paddr)
> 
> See earlier remark regarding * placement.
> 
> Jan