Re: [PATCH v2 4/6] xen/riscv: introduce identity mapping

[Oleksii <oleksii.kurochko@xxxxxxxxx>]

On Thu, 2023-07-06 at 13:35 +0200, Jan Beulich wrote:
> On 19.06.2023 15:34, Oleksii Kurochko wrote:
> > Since it is not easy to keep track where the identity map was
> > mapped,
> > so we will look for the top-level entry exclusive to the identity
> > map and remove it.
> 
> I think you mean "top-most" or some such, as it's not necessarily the
> top-level entry you zap.
Yeah, 'top-most' is more appropriate in this context.
> 
> > --- a/xen/arch/riscv/include/asm/config.h
> > +++ b/xen/arch/riscv/include/asm/config.h
> > @@ -1,3 +1,5 @@
> > +/* SPDX-License-Identifier: GPL-2.0-only */
> > +
> >  #ifndef __RISCV_CONFIG_H__
> >  #define __RISCV_CONFIG_H__
> >  
> 
> Unrelated change?
It  should be part of [PATCH v2 5/6] xen/riscv: introduce identity
mapping.

> 
> > --- a/xen/arch/riscv/mm.c
> > +++ b/xen/arch/riscv/mm.c
> > @@ -25,6 +25,12 @@ unsigned long __ro_after_init phys_offset;
> >  #define LOAD_TO_LINK(addr) ((unsigned long)(addr) - phys_offset)
> >  #define LINK_TO_LOAD(addr) ((unsigned long)(addr) + phys_offset)
> >  
> > +/*
> > + * Should be removed as soon as enough headers will be merged for
> > inclusion of
> > + * <xen/lib.h>.
> > + */
> > +#define ARRAY_SIZE(arr)                (sizeof(arr) /
> > sizeof((arr)[0]))
> > +
> >  /*
> >   * It is expected that Xen won't be more then 2 MB.
> >   * The check in xen.lds.S guarantees that.
> > @@ -35,8 +41,10 @@ unsigned long __ro_after_init phys_offset;
> >   *
> >   * It might be needed one more page table in case when Xen load
> > address
> >   * isn't 2 MB aligned.
> > + *
> > + * (CONFIG_PAGING_LEVELS - 1) page tables are needed for identity
> > mapping.
> >   */
> > -#define PGTBL_INITIAL_COUNT ((CONFIG_PAGING_LEVELS - 1) + 1)
> > +#define PGTBL_INITIAL_COUNT ((CONFIG_PAGING_LEVELS - 1) * 2 + 1)
> 
> How come the extra page (see the comment sentence in context) isn't
> needed for the identity-mapping case?
It is needed to allocate no more than two 'nonroot' page tables (L0 and
L1 in case of Sv39 ) as page 'root' table ( L2 in case of Sv39 ) is
always re-used.

The same ( only 'nonroot' page tables might be needed to allocate )
works for any MMU mode.

> 
> > @@ -255,25 +262,30 @@ void __init noreturn noinline enable_mmu()
> >      csr_write(CSR_SATP,
> >                PFN_DOWN((unsigned long)stage1_pgtbl_root) |
> >                RV_STAGE1_MODE << SATP_MODE_SHIFT);
> > +}
> >  
> > -    asm volatile ( ".p2align 2" );
> > - mmu_is_enabled:
> > -    /*
> > -     * Stack should be re-inited as:
> > -     * 1. Right now an address of the stack is relative to load
> > time
> > -     *    addresses what will cause an issue in case of load start
> > address
> > -     *    isn't equal to linker start address.
> > -     * 2. Addresses in stack are all load time relative which can
> > be an
> > -     *    issue in case when load start address isn't equal to
> > linker
> > -     *    start address.
> > -     *
> > -     * We can't return to the caller because the stack was reseted
> > -     * and it may have stash some variable on the stack.
> > -     * Jump to a brand new function as the stack was reseted
> > -     */
> > +void __init remove_identity_mapping(void)
> > +{
> > +    unsigned int i;
> > +    pte_t *pgtbl;
> > +    unsigned int index, xen_index;
> > +    unsigned long load_addr = LINK_TO_LOAD(_start);
> >  
> > -    switch_stack_and_jump((unsigned long)cpu0_boot_stack +
> > STACK_SIZE,
> > -                          cont_after_mmu_is_enabled);
> > +    for ( pgtbl = stage1_pgtbl_root, i = 0;
> > +          i <= (CONFIG_PAGING_LEVELS - 1);
> 
> i < CONFIG_PAGING_LEVELS ? But maybe it would be easier for i to
> start
> at CONFIG_PAGING_LEVELS and be decremented, simplifying ...
> 
> > +          i++ )
> > +    {
> > +        index = pt_index(CONFIG_PAGING_LEVELS - 1 - i, load_addr);
> > +        xen_index = pt_index(CONFIG_PAGING_LEVELS - 1 - i,
> > XEN_VIRT_START);
> 
> ... these two expressions?
It makes sense. I'll update this part of the code.

> 
> > +        if ( index != xen_index )
> > +        {
> > +            pgtbl[index].pte = 0;
> > +            break;
> > +        }
> 
> Is this enough? When load and link address are pretty close (but not
> overlapping), can't they share a leaf table, in which case you need
> to clear more than just a single entry? The present overlap check
> looks to be 4k-granular, not 2M (in which case this couldn't happen;
> IOW adjusting the overlap check may also be a way out).
At the start of setup_initial_pagetables() there is a code which checks
that load and link address don't overlap:

    if ( (linker_start != load_start) &&
         (linker_start <= load_end) && (load_start <= linker_end) )
    {
        early_printk("(XEN) linker and load address ranges overlap\n");
        die();
    }

So the closest difference between load and link address can be 4kb.
Otherwise load and link address ranges are equal ( as we can't map less
then 4kb).

Let's take concrete examples:
  Load address range is   0x8020_0000 - 0x8020_0FFF
  Linker address range is 0x8020_1000 - 0x8020_1FFF
  MMU mode: Sv39 ( so we have 3 page tables )

  So we have:
    * L2 index = 2, L1 index = 1, L0 index = 0 for load address
    * L2 index = 2, L1 index = 1, L0 index = 1 for linker address
  Thereby we have two different L0 tables for load and linker address 
ranges.
  And it looks like it is pretty safe to remove only one L0 index that
was used for identity mapping.

Is it possible that I missed something?

~ Oleksii