Re: [PATCH v2 8/8] xen/riscv: introduce early_fdt_map()

[Oleksii <oleksii.kurochko@xxxxxxxxx>]

On Mon, 2024-07-15 at 10:52 +0200, Jan Beulich wrote:
> On 12.07.2024 18:22, Oleksii Kurochko wrote:
> > --- a/xen/arch/riscv/include/asm/mm.h
> > +++ b/xen/arch/riscv/include/asm/mm.h
> > @@ -266,4 +266,6 @@ static inline unsigned int
> > arch_get_dma_bitsize(void)
> >  
> >  void setup_fixmap_mappings(void);
> >  
> > +void* early_fdt_map(paddr_t fdt_paddr);
> 
> Please can you take care to address comments on earlier versions
> before
> submitting a new one?
Sorry, missed that Nit comment where you suggested to switch
space/block and *.

> 
> > @@ -435,3 +438,55 @@ inline pte_t mfn_to_xen_entry(mfn_t mfn,
> > unsigned int attr)
> >  
> >      return mfn_to_pte(mfn);
> >  }
> > +
> > +void * __init early_fdt_map(paddr_t fdt_paddr)
> > +{
> > +    /* We are using 2MB superpage for mapping the FDT */
> > +    paddr_t base_paddr = fdt_paddr & XEN_PT_LEVEL_MAP_MASK(1);
> > +    paddr_t offset;
> > +    void *fdt_virt;
> > +    uint32_t size;
> > +    int rc;
> > +
> > +    /*
> > +     * Check whether the physical FDT address is set and meets the
> > minimum
> > +     * alignment requirement. Since we are relying on
> > MIN_FDT_ALIGN to be at
> > +     * least 8 bytes so that we always access the magic and size
> > fields
> > +     * of the FDT header after mapping the first chunk, double
> > check if
> > +     * that is indeed the case.
> > +     */
> > +    BUILD_BUG_ON(MIN_FDT_ALIGN < 8);
> > +    if ( !fdt_paddr || fdt_paddr % MIN_FDT_ALIGN )
> > +        return NULL;
> > +
> > +    /* The FDT is mapped using 2MB superpage */
> > +    BUILD_BUG_ON(BOOT_FDT_VIRT_START % SZ_2M);
> 
> May I suggest that you use MB(2) instead of SZ_2M (not just here)? I
> think
> I had voiced opposition to the introduction of xen/sizes.h about 10
> years
> back, yet sadly it still landed in the tree. I for one think that our
> KB(),
> MB(), and GB() constructs are superior, and (I hope) free of Misra
> issues
> (unlike SZ_2G).
> 
> > +    rc = map_pages_to_xen(BOOT_FDT_VIRT_START,
> > maddr_to_mfn(base_paddr),
> > +                          SZ_2M >> PAGE_SHIFT,
> > +                          PAGE_HYPERVISOR_RO | _PAGE_BLOCK);
> > +    if ( rc )
> > +        panic("Unable to map the device-tree.\n");
> > +
> > +    offset = fdt_paddr % XEN_PT_LEVEL_SIZE(1);
> > +    fdt_virt = (void *)BOOT_FDT_VIRT_START + offset;
> > +
> > +    if ( fdt_magic(fdt_virt) != FDT_MAGIC )
> > +        return NULL;
> > +
> > +    size = fdt_totalsize(fdt_virt);
> > +    if ( size > BOOT_FDT_VIRT_SIZE )
> > +        return NULL;
> > +
> > +    if ( (offset + size) > SZ_2M )
> > +    {
> > +        rc = map_pages_to_xen(BOOT_FDT_VIRT_START + SZ_2M,
> > +                              maddr_to_mfn(base_paddr + SZ_2M),
> > +                              SZ_2M >> PAGE_SHIFT,
> > +                              PAGE_HYPERVISOR_RO | _PAGE_BLOCK);
> > +        if ( rc )
> > +            panic("Unable to map the device-tree\n");
> > +    }
> 
> Why this two part mapping? And why are you mapping perhaps much more
> than "size"?
I wasn't able to find if RISC-V has a requirement for alignment of FDT
address so I decided to follow Arm where FDT is required to be placed
on a 8-byte boundary, so FDT can cross a 2MB boundary, so the second
2MB page should be mapped if the FDT is crossing the 2MB boundary.

> 
> > @@ -48,6 +49,14 @@ void __init noreturn start_xen(unsigned long
> > bootcpu_id,
> >  
> >      setup_fixmap_mappings();
> >  
> > +    device_tree_flattened = early_fdt_map(dtb_addr);
> > +    if ( device_tree_flattened )
> 
> Is this condition perhaps inverted?
Yes, you are right. It should be inverted.

Thanks.

~ Oleksii
> 
> > +        panic("Invalid device tree blob at physical address
> > %#lx.\n"
> > +              "The DTB must be 8-byte aligned and must not exceed
> > %lld "
> > +              "bytes in size.\n\n"
> > +              "Please check your bootloader.\n",
> > +              dtb_addr, BOOT_FDT_VIRT_SIZE);
> > +
> >      printk("All set up\n");
> >  
> >      for ( ;; )
>