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