Re: [PATCH v5 1/3] xen/riscv: introduce setup_mm()

[Jan Beulich <jbeulich@xxxxxxxx>]

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