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

[oleksii.kurochko@xxxxxxxxx]

On Mon, 2024-11-11 at 11:29 +0100, Jan Beulich wrote:
> On 08.11.2024 13:51, Oleksii Kurochko wrote:
> > @@ -37,9 +42,9 @@ static inline void *maddr_to_virt(paddr_t ma)
> >   */
> >  static inline unsigned long virt_to_maddr(unsigned long va)
> >  {
> > -    if ((va >= DIRECTMAP_VIRT_START) &&
> > +    if ((va >= directmap_virt_start) &&
> 
> Is this a valid / necessary change to make?
You are right, this not valid change, va value is DIRECTMAP_VIRT_START-
relative.

>  Right now there looks to be
> nothing immediately below the directmap, yet that would need
> guaranteeing
> (e.g. by some BUILD_BIG_ON() or whatever else) if code builds upon
> that.
It is not really clear how to check that nothing below the directmap is
present/used. But IIUC there is no need for this check if properly
correct the condition above.

> 
> >          (va < (DIRECTMAP_VIRT_START + DIRECTMAP_SIZE)))
> > -        return directmapoff_to_maddr(va - DIRECTMAP_VIRT_START);
> > +        return directmapoff_to_maddr(va - directmap_virt_start);
> 
> FTAOD - no question about this part of the change.
> 
> > @@ -423,3 +429,140 @@ 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;
> 
> As for directmap_virt_start - perhaps better with initializer?
Do you mean to initialized by NULL or frame_table?

If to initialize by frame_table then the if-condition won't work
properly in setup_frametable_mappings() ( but I think that this
condition could be dropped as setup_frametable_mappings() is supposed
to be called only once ?! ). And you mentioned about this condition
here ...

> 
> > +#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_virt_start )
> > +        frametable_virt_start = frame_table -
> > paddr_to_pfn(aligned_ps);
> 
> If you make this conditional, then you need an "else" (or something
> that's
> effectively one) just like you have in setup_directmap_mappings().
> Like
> for the earlier assumption on ps being zero: Assumptions you make on
> how
> a function is used want to at least be self-consistent. I.e. here
> either
> you assume the function may be called more than once, or you don't.
...

Do we have in Xen something to be sure that the function is called only
once or I have to come up with static variable inside the function?

> 
> > +static void __init setup_directmap_mappings(unsigned long
> > base_mfn,
> > +                                            unsigned long nr_mfns)
> > +{
> > +    static mfn_t __initdata directmap_mfn_start =
> > INVALID_MFN_INITIALIZER;
> > +
> > +    unsigned long base_addr = mfn_to_maddr(_mfn(base_mfn));
> 
> Seeing this and ...
> 
> > +    unsigned long high_bits_mask =
> > XEN_PT_LEVEL_MAP_MASK(HYP_PT_ROOT_LEVEL);
> > +
> > +    /* First call sets the directmap physical and virtual offset.
> > */
> > +    if ( mfn_eq(directmap_mfn_start, INVALID_MFN) )
> > +    {
> > +        directmap_mfn_start = _mfn(base_mfn);
> 
> ... this (and more further down) - perhaps better to have the
> function take
> mfn_t right away?
Agree, it makes sense. I'll update correspondingly.

> 
> > +       /*
> > +        * 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.
> > +        */
> > +        directmap_virt_start -=
> > +            (base_addr & high_bits_mask) + (base_addr &
> > ~high_bits_mask);
> 
> Isn't this the same as
> 
>         directmap_virt_start -= base_addr;
> 
> i.e. no different from what you had a few revisions back? I continue
> to
> think that only the low bits matter for the offsetting.
IIUYC you mean that "(base_addr &
~high_bits_mask)" should be dropped then I agree. Thanks for noticing
that.

> 
> > +    }
> > +
> > +    if ( base_mfn < mfn_x(directmap_mfn_start) )
> > +        panic("can't add directmap mapping at %#lx below directmap
> > start %#lx\n",
> > +              base_mfn, mfn_x(directmap_mfn_start));
> > +
> > +    if ( map_pages_to_xen((vaddr_t)mfn_to_virt(base_mfn),
> > +                          _mfn(base_mfn), nr_mfns,
> > +                          PAGE_HYPERVISOR_RW) )
> > +        panic("Directmap mappings for [%#"PRIpaddr", %#"PRIpaddr")
> > failed\n",
> > +              mfn_to_maddr(_mfn(base_mfn)),
> > +              mfn_to_maddr(_mfn(base_mfn + nr_mfns)));
> 
> Maybe worth also logging the error code?
I am not really understand why do we need that as the use will see what
is the issue in the message inside panic().

> 
> > +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();
> > +
> > +    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;
> 
> As before - you maintain ram_size here, ...
> 
> > +        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);
> > +    max_page = PFN_DOWN(ram_end);
> > +}
> 
> ... without ever using the value. Why?
I started to use bank_end and bank_size in v2 and ram_size isn't really
needed anymore.

Thanks.

~ Oleksii