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

[Oleksii <oleksii.kurochko@xxxxxxxxx>]

On Fri, 2023-07-07 at 12:51 +0200, Jan Beulich wrote:
> On 07.07.2023 12:37, Oleksii wrote:
> > On Thu, 2023-07-06 at 13:35 +0200, Jan Beulich wrote:
> > > On 19.06.2023 15:34, Oleksii Kurochko wrote:
> > > > --- 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.
> 
> Hmm, here we're discussing "[PATCH v2 4/6] xen/riscv: introduce
> identity
> mapping". I'm confused, I guess.
Sorry for confusion. i meant the patch: [PATCH v2 5/6] xen/riscv: add
SPDX tags.

> 
> > > > --- 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.
> 
> Of course, but if you cross a 2Mb boundary you'll need 2 L0 tables.
Yes, in the case of crossing a 2Mb boundary, it will require 2 L0
tables.

Then, the number of required page tables is needed depending on Xen
size and load address alignment. Because for each 2Mb, we need a new L0
table.

Sure, this is not needed now ( as in xen.lds.S, we have a Xen size
check ), but if someone increases Xen size binary to 4Mb, then the
amount of page tables should be updated too.
Should we take into account that?

> 
> > > > @@ -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?
> 
> Looks as if you are thinking of only a Xen which fits in 4k. The code
> here wants to cope with Xen getting quite a bit bigger.

Yeah, I missed that when I tried to come up with an example.

So it will probably be more universal if we recursively go through the
whole identity mapping and unmap each pte individually.