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

[oleksii.kurochko@xxxxxxxxx]

On Tue, 2024-11-05 at 16:20 +0100, Jan Beulich wrote:
> On 01.11.2024 14:16, Oleksii Kurochko wrote:> @@ -423,3 +429,138 @@
> void * __init early_fdt_map(paddr_t fdt_paddr)
> >  
> >      return fdt_virt;
> >  }
> > +
> > +vaddr_t __ro_after_init directmap_virt_start =
> > DIRECTMAP_VIRT_START;
> > +
> > +#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)
> > +{
> > +    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 ( 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)));
> > +
> > +    if ( map_pages_to_xen(FRAMETABLE_VIRT_START, base_mfn,
> > +                          PFN_DOWN(frametable_size),
> > +                          PAGE_HYPERVISOR_RW) )
> > +        panic("frametable mappings failed: %#lx -> %#lx\n",
> > +              FRAMETABLE_VIRT_START, mfn_x(base_mfn));
> > +
> > +    memset(&frame_table[0], -1, frametable_size);
> > +    memset(&frame_table[PFN_DOWN(aligned_ps)],
> > +           0, nr_mfns * sizeof(*frame_table));
> 
> Interesting - now you write out a huge amount of -1s, just to then
> overwrite
> most of them with zeroes. I'm not going to insist that you change
> this yet
> another time, but the performance hit from this is going to bite
> you/us as
> soon as Xen is run on bigger-memory systems.
I agree that validating or invalidating frames in a single pass would
be preferable to nearly two passes. I’m considering whether there’s a
way to ensure that frame_table is set to -1 at compile time. It seems
the best approach (and only one?) might be to initialize frame_table in
one pass as follows:
1) [0, aligned_ps) = -1
2) [aligned_ps, nr_mfns * sizeof(*frame_table)) = 0
3) [nr_mfns * sizeof(*frame_table), frame_table_size) = -1
Does this approach seem optimal?

> 
> Plus, unless I'm mistaken, the function continues to rely on ps == 0
> as
> input. Just that the dependency is now better hidden. Specifically if
> you
> calculate nr_mfns from the difference, and then use that for
> allocation,
> then you need to offset the start of the mapping you create
> accordingly.
I'm not quite understanding why the method of calculating nr_mfns
affects how the frame_table is mapped. Isn’t it only necessary to
calculate the correct size of the frame_table that needs to be
allocated?

I assume by the offset you mean something similar to what was done for
directmap mapping?


>  At
> which point you may need to apply extra care to cover the case where
> sizeof(*frame_table) is not a power of two, and hence e.g. the first
> valid
> page might have a struct instance straddling a page boundary.
The first valid page is aligned_ps ( which is aligned on a page
boundary ) so assuming that sizeof(*frame_table) < PAGE_SIZE we can't
straddle a page boundary, can we?

> 
> > +    /*
> > +     * The base address may not be aligned to the second level
> > +     * size in case of Sv39 (e.g. 1GB when using 4KB pages).
> > +     * This would prevent superpage mappings for all the regions
> > +     * because the virtual address and machine address should
> > +     * both be suitably aligned.
> > +     *
> > +     * Prevent that by offsetting the start of the directmap
> > virtual
> > +     * address.
> > +     */
> > +    if ( map_pages_to_xen(DIRECTMAP_VIRT_START + (base_addr &
> > ~high_bits_mask),
> 
> I'm afraid this is correct only for the first invocation of the
> function.
> For any further invocation you'd likely (attempt to) replace
> previously
> established mappings. I think that here you need to use
> directmap_virt_start
> instead.
Banks are sorted by bank start address ( common/device-
tree/bootfdt.c:623 ):
       /*
        * On Arm64 setup_directmap_mappings() expects to be called with
   the lowest
        * bank in memory first. There is no requirement that the DT will
   provide
        * the banks sorted in ascending order. So sort them through.
        */
       sort(mem->bank, mem->nr_banks, sizeof(struct membank),
            cmp_memory_node, swap_memory_node);
( btw, comment is needed to be updated ... )

Thereby no replacement should happen if I don't miss something.

I am thinking wouldn't it be more correct to use mfn_to_virt(base_mfn):
       if ( map_pages_to_xen(__mfn_to_virt(base_mfn) + (base_addr &
   ~high_bits_mask),
                             _mfn(base_mfn), nr_mfns,
                             PAGE_HYPERVISOR_RW) )

> 
> > +/*
> > + * Setup memory management
> > + *
> > + * RISC-V 64 has a large virtual address space (the minimum
> > supported
> > + * MMU mode is Sv39, which provides GBs of VA space).
> > + *
> > + * The directmap_virt_start is shifted lower in the VA space to
> > + * (DIRECTMAP_VIRT_START - masked_low_bits_of_ram_start_address)
> > to avoid
> > + * wasting a large portion of the directmap space, this also
> > allows for simple
> > + * VA <-> PA translations. Also aligns DIRECTMAP_VIRT_START to a
> > GB boundary
> > + * (for Sv39; for other MMU mode boundaries will be bigger ) by
> > masking the
> > + * higher bits of the RAM start address to enable the use of
> > superpages in
> > + * map_pages_to_xen().
> > + *
> > + * The frametable is mapped starting from physical address 0,
> > minimizing
> > + * wasted VA space and simplifying page_to_mfn() and mfn_to_page()
> > + * translations.
> > + */
> > +void __init setup_mm(void)
> > +{
> > +    const struct membanks *banks = bootinfo_get_mem();
> > +    paddr_t ram_start = INVALID_PADDR;
> > +    paddr_t ram_end = 0;
> > +    paddr_t ram_size = 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();
> > +
> > +    total_pages = 0;
> 
> Nit: Is this actually necessary?
Agree, there is no need for total_pages. It should be dropped.

> 
> > +    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_size += bank_size;
> > +        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));
> > +    }
> 
> You maintain ram_start in the loop, just to then ...
> 
> > +    total_pages = PFN_DOWN(ram_size);
> > +
> > +    setup_frametable_mappings(0, ram_end);
> > +    max_page = PFN_DOWN(ram_end);
> > +}
> 
> ... not use it at all - why?
ram_start was needed for the case when setup_frametable_mappings() used
it as the first argument. Now it isn't true anymore so should be
dropped.

Thanks.

~ Oleksii