On 11.11.2024 19:16, Oleksii Kurochko wrote:
> @@ -25,8 +27,11 @@
>
> static inline void *maddr_to_virt(paddr_t ma)
> {
> - BUG_ON("unimplemented");
> - return NULL;
> + unsigned long va_offset = maddr_to_directmapoff(ma);
> +
> + ASSERT(va_offset < DIRECTMAP_SIZE);
Much like with the consideration towards virt_to_maddr() that was
corrected from v4, I think this one also needs adjusting:
ASSERT(va_offset < DIRECTMAP_SIZE + (DIRECTMAP_VIRT_START -
directmap_virt_start));
This is because ...
> + return (void *)(directmap_virt_start + va_offset);
... you're offsetting the VA here. It may then want accompanying
by
ASSERT(va_offset >= DIRECTMAP_VIRT_START - directmap_virt_start);
(probably to go first).
> @@ -423,3 +429,147 @@ void * __init early_fdt_map(paddr_t fdt_paddr)
>
> return fdt_virt;
> }
> +
> +vaddr_t __ro_after_init directmap_virt_start = DIRECTMAP_VIRT_START;
> +
> +struct page_info *__ro_after_init frametable_virt_start = frame_table;
> +
> +#ifndef CONFIG_RISCV_32
> +
> +/* Map a frame table to cover physical addresses ps through pe */
> +static void __init setup_frametable_mappings(paddr_t ps, paddr_t pe)
> +{
> + static mfn_t __initdata frametable_mfn_start = INVALID_MFN_INITIALIZER;
> +
> + paddr_t aligned_ps = ROUNDUP(ps, PAGE_SIZE);
> + paddr_t aligned_pe = ROUNDDOWN(pe, PAGE_SIZE);
> + unsigned long nr_mfns = PFN_DOWN(aligned_pe - aligned_ps);
> + unsigned long frametable_size = nr_mfns * sizeof(*frame_table);
> + mfn_t base_mfn;
> +
> + if ( mfn_eq(frametable_mfn_start, INVALID_MFN) )
> + {
> + frametable_mfn_start = maddr_to_mfn(aligned_ps);
> +
> + frametable_virt_start -= paddr_to_pfn(aligned_ps);
> + }
> + else
> + panic("%s shouldn't be called twice\n", __func__);
As said on the v4 thread - I don't think this is needed. Aiui Misra would
actually dislike it, as it's unreachable code. Just to re-iterate: My
complaint there wasn't about this missing check, but about the function
partly giving the impression of expecting to be called more than once.
> + if ( frametable_size > FRAMETABLE_SIZE )
> + panic("The frametable cannot cover [%#"PRIpaddr", %#"PRIpaddr")\n",
> + ps, pe);
> +
> + /*
> + * align base_mfn and frametable_size to MB(2) to have superpage mapping
> + * in map_pages_to_xen()
> + */
> + frametable_size = ROUNDUP(frametable_size, MB(2));
> + base_mfn = alloc_boot_pages(frametable_size >> PAGE_SHIFT,
> PFN_DOWN(MB(2)));
As you already use PFN_DOWN() once, why do you open-code it for the other
argument? You also use it ...
> + if ( map_pages_to_xen(FRAMETABLE_VIRT_START, base_mfn,
> + PFN_DOWN(frametable_size),
... here, where the purpose of the argument is exactly the same.
> +void __init setup_mm(void)
> +{
> + const struct membanks *banks = bootinfo_get_mem();
> + paddr_t ram_start = INVALID_PADDR;
> + paddr_t ram_end = 0;
> + unsigned int i;
> +
> + /*
> + * We need some memory to allocate the page-tables used for the directmap
> + * mappings. But some regions may contain memory already allocated
> + * for other uses (e.g. modules, reserved-memory...).
> + *
> + * For simplicity, add all the free regions in the boot allocator.
> + */
> + populate_boot_allocator();
> +
> + for ( i = 0; i < banks->nr_banks; i++ )
> + {
> + const struct membank *bank = &banks->bank[i];
> + paddr_t bank_start = ROUNDUP(bank->start, PAGE_SIZE);
> + paddr_t bank_end = ROUNDDOWN(bank->start + bank->size, PAGE_SIZE);
> + unsigned long bank_size = bank_end - bank_start;
> +
> + ram_start = min(ram_start, bank_start);
> + ram_end = max(ram_end, bank_end);
> +
> + setup_directmap_mappings(PFN_DOWN(bank_start), PFN_DOWN(bank_size));
> + }
> +
> + setup_frametable_mappings(ram_start, ram_end);
Just to double check: There is a guarantee that ->nr_banks isn't going to
be zero? Else the setup_frametable_mappings() invocation here would badly
degenerate.
Jan
