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

[Jan Beulich <jbeulich@xxxxxxxx>]

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?

> @@ -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"?

> @@ -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?

Jan

> +        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 ( ;; )