| Message ID | 4e4f94817fb3bba5f52336ea0b4491f7bc867d49.1720799926.git.oleksii.kurochko@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | RISCV device tree mapping | expand |
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 ( ;; )
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 ( ;; ) >
On 15.07.2024 15:58, Oleksii wrote: > On Mon, 2024-07-15 at 10:52 +0200, Jan Beulich wrote: >> On 12.07.2024 18:22, Oleksii Kurochko wrote: >>> + 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. This explains why you may need to map more than 2Mb (which wasn't the question), but it doesn't explain why you need to do it in two steps. Jan
On 15.07.2024 16:58, Jan Beulich wrote: > On 15.07.2024 15:58, Oleksii wrote: >> On Mon, 2024-07-15 at 10:52 +0200, Jan Beulich wrote: >>> On 12.07.2024 18:22, Oleksii Kurochko wrote: >>>> + 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. > > This explains why you may need to map more than 2Mb (which wasn't the > question), but it doesn't explain why you need to do it in two steps. Oh, wait - you know the full size only after having mapped the initial part. I'm sorry, I didn't spot that early enough. Jan
diff --git a/xen/arch/riscv/include/asm/mm.h b/xen/arch/riscv/include/asm/mm.h index c54546c275..a12ef5bb63 100644 --- 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); + #endif /* _ASM_RISCV_MM_H */ diff --git a/xen/arch/riscv/mm.c b/xen/arch/riscv/mm.c index 44f36359c8..428c26b636 100644 --- a/xen/arch/riscv/mm.c +++ b/xen/arch/riscv/mm.c @@ -1,13 +1,16 @@ /* SPDX-License-Identifier: GPL-2.0-only */ +#include <xen/bootfdt.h> #include <xen/bug.h> #include <xen/compiler.h> #include <xen/init.h> #include <xen/kernel.h> +#include <xen/libfdt/libfdt.h> #include <xen/macros.h> #include <xen/mm.h> #include <xen/pfn.h> #include <xen/sections.h> +#include <xen/sizes.h> #include <asm/early_printk.h> #include <asm/csr.h> @@ -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); + + 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"); + } + + return fdt_virt; +} diff --git a/xen/arch/riscv/setup.c b/xen/arch/riscv/setup.c index 13f0e8c77d..21628b7300 100644 --- a/xen/arch/riscv/setup.c +++ b/xen/arch/riscv/setup.c @@ -2,6 +2,7 @@ #include <xen/bug.h> #include <xen/compile.h> +#include <xen/device_tree.h> #include <xen/init.h> #include <xen/mm.h> @@ -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 ) + 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 ( ;; )
Introduce function which allows to map FDT to Xen. Also, initialization of device_tree_flattened happens using early_fdt_map. Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com> --- Changes in V2: - rework early_fdt_map to use map_pages_to_xen() - move call early_fdt_map() to C code after MMU is enabled. --- xen/arch/riscv/include/asm/mm.h | 2 ++ xen/arch/riscv/mm.c | 55 +++++++++++++++++++++++++++++++++ xen/arch/riscv/setup.c | 9 ++++++ 3 files changed, 66 insertions(+)