[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Xen-devel] [PATCH for-4.12 v3] xen/arm: Stop relocating Xen



On Tue, 18 Dec 2018, Stefano Stabellini wrote:
> On Tue, 18 Dec 2018, Julien Grall wrote:
> > At the moment, Xen is relocated towards the end of the memory. While
> > this has the advantage to free space in low memory, the code is not
> > compliant with the break-before-make because it requires to switch
> > between two sets of page-table. This is not entirely trivial to fix as
> > it would require us to go through an identity mapping and disabling MMU.
> > 
> > Furthermore, it looks like that some platform (such as the Hikey960)
> > may not be able to bring-up secondary CPUs if the entry is too high.
> > 
> > While Xen should be quite tiny (< 2MB), the current algorigthm to
> > allocate Dom0 memory will allocate memory chunk of at least 128MB.
> > Those memory chunks will always be 128MB. This means that depending on
> > where the modules are loaded, an extra 128MB may disappear.
> > 
> > As there are up to 4 modules (initramfs, XSM, kernel, DTB) loaded in
> > low memory. The problem is not entirely new as you could already waste
> > 512MB of low-memory. The right solution would be to fix the allocation
> > algorightm. But this is independent from this patch.
> > 
> > For user in control of the memory (such as in U-boot), all modules
> > should be loaded as much as possible together or outside low-memory (i.e
> > above 4GB). For other users (i.e Grub/UEFI), I believe the bootloader is
> > already keeping everything together.
> > 
> > Based on the above, it would be fine to stop relocating Xen. This has
> > the advantage to simplify the code and should speed-up the boot as
> > relocation is not necessary anymore.
> > 
> > Note that the break-before-make issue is not fixed by this patch.
> > 
> > Signed-off-by: Julien Grall <julien.grall@xxxxxxx>
> > Reported-by: Matthew Daley <mattd@xxxxxxxxxxx>
> > Tested-by: Matthew Daley <mattd@xxxxxxxxxxx>
> 
> Reviewed and committed

I forgot to add that I fixed a couple of grammar errors in the commit
message as I committed.

 
> > ---
> >     Changes in v3:
> >         - Update the commit message
> > 
> >     Changes in v2:
> >         - Add Matthew's tested-by
> > ---
> >  xen/arch/arm/arm32/head.S | 54 +++------------------------------------
> >  xen/arch/arm/arm64/head.S | 50 +++++-------------------------------
> >  xen/arch/arm/mm.c         | 18 +++----------
> >  xen/arch/arm/setup.c      | 65 
> > +++--------------------------------------------
> >  xen/include/asm-arm/mm.h  |  2 +-
> >  5 files changed, 17 insertions(+), 172 deletions(-)
> > 
> > diff --git a/xen/arch/arm/arm32/head.S b/xen/arch/arm/arm32/head.S
> > index 93b51e9ef2..390a505e05 100644
> > --- a/xen/arch/arm/arm32/head.S
> > +++ b/xen/arch/arm/arm32/head.S
> > @@ -469,58 +469,12 @@ fail:   PRINT("- Boot failed -\r\n")
> >  GLOBAL(_end_boot)
> >  
> >  /*
> > - * Copy Xen to new location and switch TTBR
> > + * Switch TTBR
> >   * r1:r0       ttbr
> > - * r2          source address
> > - * r3          destination address
> > - * [sp]=>r4    length
> >   *
> > - * Source and destination must be word aligned, length is rounded up
> > - * to a 16 byte boundary.
> > - *
> > - * MUST BE VERY CAREFUL when saving things to RAM over the copy
> > + * TODO: This code does not comply with break-before-make.
> >   */
> > -ENTRY(relocate_xen)
> > -        push {r4,r5,r6,r7,r8,r9,r10,r11}
> > -
> > -        ldr   r4, [sp, #8*4]                /* Get 4th argument from stack 
> > */
> > -
> > -        /* Copy 16 bytes at a time using:
> > -         * r5:  counter
> > -         * r6:  data
> > -         * r7:  data
> > -         * r8:  data
> > -         * r9:  data
> > -         * r10: source
> > -         * r11: destination
> > -         */
> > -        mov   r5, r4
> > -        mov   r10, r2
> > -        mov   r11, r3
> > -1:      ldmia r10!, {r6, r7, r8, r9}
> > -        stmia r11!, {r6, r7, r8, r9}
> > -
> > -        subs  r5, r5, #16
> > -        bgt   1b
> > -
> > -        /* Flush destination from dcache using:
> > -         * r5: counter
> > -         * r6: step
> > -         * r7: vaddr
> > -         */
> > -        dsb        /* So the CPU issues all writes to the range */
> > -
> > -        mov   r5, r4
> > -        ldr   r6, =dcache_line_bytes /* r6 := step */
> > -        ldr   r6, [r6]
> > -        mov   r7, r3
> > -
> > -1:      mcr   CP32(r7, DCCMVAC)
> > -
> > -        add   r7, r7, r6
> > -        subs  r5, r5, r6
> > -        bgt   1b
> > -
> > +ENTRY(switch_ttbr)
> >          dsb                            /* Ensure the flushes happen before
> >                                          * continuing */
> >          isb                            /* Ensure synchronization with 
> > previous
> > @@ -543,8 +497,6 @@ ENTRY(relocate_xen)
> >          dsb                            /* Ensure completion of TLB+BP 
> > flush */
> >          isb
> >  
> > -        pop {r4, r5,r6,r7,r8,r9,r10,r11}
> > -
> >          mov pc, lr
> >  
> >  #ifdef CONFIG_EARLY_PRINTK
> > diff --git a/xen/arch/arm/arm64/head.S b/xen/arch/arm/arm64/head.S
> > index ef87b5c254..0b7f6e7f92 100644
> > --- a/xen/arch/arm/arm64/head.S
> > +++ b/xen/arch/arm/arm64/head.S
> > @@ -609,52 +609,14 @@ fail:   PRINT("- Boot failed -\r\n")
> >  
> >  GLOBAL(_end_boot)
> >  
> > -/* Copy Xen to new location and switch TTBR
> > - * x0    ttbr
> > - * x1    source address
> > - * x2    destination address
> > - * x3    length
> > +/*
> > + * Switch TTBR
> >   *
> > - * Source and destination must be word aligned, length is rounded up
> > - * to a 16 byte boundary.
> > + * x0    ttbr
> >   *
> > - * MUST BE VERY CAREFUL when saving things to RAM over the copy */
> > -ENTRY(relocate_xen)
> > -        /* Copy 16 bytes at a time using:
> > -         *   x9: counter
> > -         *   x10: data
> > -         *   x11: data
> > -         *   x12: source
> > -         *   x13: destination
> > -         */
> > -        mov     x9, x3
> > -        mov     x12, x1
> > -        mov     x13, x2
> > -
> > -1:      ldp     x10, x11, [x12], #16
> > -        stp     x10, x11, [x13], #16
> > -
> > -        subs    x9, x9, #16
> > -        bgt     1b
> > -
> > -        /* Flush destination from dcache using:
> > -         * x9: counter
> > -         * x10: step
> > -         * x11: vaddr
> > -         */
> > -        dsb   sy        /* So the CPU issues all writes to the range */
> > -
> > -        mov   x9, x3
> > -        ldr   x10, =dcache_line_bytes /* x10 := step */
> > -        ldr   x10, [x10]
> > -        mov   x11, x2
> > -
> > -1:      dc    cvac, x11
> > -
> > -        add   x11, x11, x10
> > -        subs  x9, x9, x10
> > -        bgt   1b
> > -
> > + * TODO: This code does not comply with break-before-make.
> > + */
> > +ENTRY(switch_ttbr)
> >          dsb   sy                     /* Ensure the flushes happen before
> >                                        * continuing */
> >          isb                          /* Ensure synchronization with 
> > previous
> > diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
> > index 91f3aef93c..d96a6655ee 100644
> > --- a/xen/arch/arm/mm.c
> > +++ b/xen/arch/arm/mm.c
> > @@ -601,7 +601,7 @@ void __init remove_early_mappings(void)
> >      flush_xen_data_tlb_range_va(BOOT_FDT_VIRT_START, BOOT_FDT_SLOT_SIZE);
> >  }
> >  
> > -extern void relocate_xen(uint64_t ttbr, void *src, void *dst, size_t len);
> > +extern void switch_ttbr(uint64_t ttbr);
> >  
> >  /* Clear a translation table and clean & invalidate the cache */
> >  static void clear_table(void *table)
> > @@ -612,15 +612,13 @@ static void clear_table(void *table)
> >  
> >  /* Boot-time pagetable setup.
> >   * Changes here may need matching changes in head.S */
> > -void __init setup_pagetables(unsigned long boot_phys_offset, paddr_t 
> > xen_paddr)
> > +void __init setup_pagetables(unsigned long boot_phys_offset)
> >  {
> >      uint64_t ttbr;
> > -    unsigned long dest_va;
> >      lpae_t pte, *p;
> >      int i;
> >  
> > -    /* Calculate virt-to-phys offset for the new location */
> > -    phys_offset = xen_paddr - (unsigned long) _start;
> > +    phys_offset = boot_phys_offset;
> >  
> >  #ifdef CONFIG_ARM_64
> >      p = (void *) xen_pgtable;
> > @@ -686,21 +684,13 @@ void __init setup_pagetables(unsigned long 
> > boot_phys_offset, paddr_t xen_paddr)
> >      pte = boot_second[second_table_offset(BOOT_FDT_VIRT_START + SZ_2M)];
> >      xen_second[second_table_offset(BOOT_FDT_VIRT_START + SZ_2M)] = pte;
> >  
> > -    /* ... Boot Misc area for xen relocation */
> > -    dest_va = BOOT_RELOC_VIRT_START;
> > -    pte = mfn_to_xen_entry(maddr_to_mfn(xen_paddr), MT_NORMAL);
> > -    /* Map the destination in xen_second. */
> > -    xen_second[second_table_offset(dest_va)] = pte;
> > -    /* Map the destination in boot_second. */
> > -    write_pte(boot_second + second_table_offset(dest_va), pte);
> > -    flush_xen_data_tlb_range_va_local(dest_va, SECOND_SIZE);
> >  #ifdef CONFIG_ARM_64
> >      ttbr = (uintptr_t) xen_pgtable + phys_offset;
> >  #else
> >      ttbr = (uintptr_t) cpu0_pgtable + phys_offset;
> >  #endif
> >  
> > -    relocate_xen(ttbr, _start, (void*)dest_va, _end - _start);
> > +    switch_ttbr(ttbr);
> >  
> >      /* Clear the copy of the boot pagetables. Each secondary CPU
> >       * rebuilds these itself (see head.S) */
> > diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
> > index e83221ab79..fb923cdf67 100644
> > --- a/xen/arch/arm/setup.c
> > +++ b/xen/arch/arm/setup.c
> > @@ -374,6 +374,7 @@ void __init discard_initial_modules(void)
> >      remove_early_mappings();
> >  }
> >  
> > +#ifdef CONFIG_ARM_32
> >  /*
> >   * Returns the end address of the highest region in the range s..e
> >   * with required size and alignment that does not conflict with the
> > @@ -440,6 +441,7 @@ static paddr_t __init consider_modules(paddr_t s, 
> > paddr_t e,
> >      }
> >      return e;
> >  }
> > +#endif
> >  
> >  /*
> >   * Return the end of the non-module region starting at s. In other
> > @@ -475,59 +477,6 @@ static paddr_t __init next_module(paddr_t s, paddr_t 
> > *end)
> >      return lowest;
> >  }
> >  
> > -
> > -/**
> > - * get_xen_paddr - get physical address to relocate Xen to
> > - *
> > - * Xen is relocated to as near to the top of RAM as possible and
> > - * aligned to a XEN_PADDR_ALIGN boundary.
> > - */
> > -static paddr_t __init get_xen_paddr(void)
> > -{
> > -    struct meminfo *mi = &bootinfo.mem;
> > -    paddr_t min_size;
> > -    paddr_t paddr = 0;
> > -    int i;
> > -
> > -    min_size = (_end - _start + (XEN_PADDR_ALIGN-1)) & 
> > ~(XEN_PADDR_ALIGN-1);
> > -
> > -    /* Find the highest bank with enough space. */
> > -    for ( i = 0; i < mi->nr_banks; i++ )
> > -    {
> > -        const struct membank *bank = &mi->bank[i];
> > -        paddr_t s, e;
> > -
> > -        if ( bank->size >= min_size )
> > -        {
> > -            e = consider_modules(bank->start, bank->start + bank->size,
> > -                                 min_size, XEN_PADDR_ALIGN, 0);
> > -            if ( !e )
> > -                continue;
> > -
> > -#ifdef CONFIG_ARM_32
> > -            /* Xen must be under 4GB */
> > -            if ( e > 0x100000000ULL )
> > -                e = 0x100000000ULL;
> > -            if ( e < bank->start )
> > -                continue;
> > -#endif
> > -
> > -            s = e - min_size;
> > -
> > -            if ( s > paddr )
> > -                paddr = s;
> > -        }
> > -    }
> > -
> > -    if ( !paddr )
> > -        panic("Not enough memory to relocate Xen\n");
> > -
> > -    printk("Placing Xen at 0x%"PRIpaddr"-0x%"PRIpaddr"\n",
> > -           paddr, paddr + min_size);
> > -
> > -    return paddr;
> > -}
> > -
> >  static void __init init_pdx(void)
> >  {
> >      paddr_t bank_start, bank_size, bank_end;
> > @@ -783,7 +732,6 @@ void __init start_xen(unsigned long boot_phys_offset,
> >  {
> >      size_t fdt_size;
> >      int cpus, i;
> > -    paddr_t xen_paddr;
> >      const char *cmdline;
> >      struct bootmodule *xen_bootmodule;
> >      struct domain *dom0;
> > @@ -827,14 +775,7 @@ void __init start_xen(unsigned long boot_phys_offset,
> >                               (paddr_t)(uintptr_t)(_end - _start + 1), 
> > false);
> >      BUG_ON(!xen_bootmodule);
> >  
> > -    xen_paddr = get_xen_paddr();
> > -    setup_pagetables(boot_phys_offset, xen_paddr);
> > -
> > -    /* Update Xen's address now that we have relocated. */
> > -    printk("Update BOOTMOD_XEN from %"PRIpaddr"-%"PRIpaddr" => 
> > %"PRIpaddr"-%"PRIpaddr"\n",
> > -           xen_bootmodule->start, xen_bootmodule->start + 
> > xen_bootmodule->size,
> > -           xen_paddr, xen_paddr + xen_bootmodule->size);
> > -    xen_bootmodule->start = xen_paddr;
> > +    setup_pagetables(boot_phys_offset);
> >  
> >      setup_mm(fdt_paddr, fdt_size);
> >  
> > diff --git a/xen/include/asm-arm/mm.h b/xen/include/asm-arm/mm.h
> > index b2f6104a7f..eafa26f56e 100644
> > --- a/xen/include/asm-arm/mm.h
> > +++ b/xen/include/asm-arm/mm.h
> > @@ -169,7 +169,7 @@ extern unsigned long total_pages;
> >  #define PDX_GROUP_SHIFT SECOND_SHIFT
> >  
> >  /* Boot-time pagetable setup */
> > -extern void setup_pagetables(unsigned long boot_phys_offset, paddr_t 
> > xen_paddr);
> > +extern void setup_pagetables(unsigned long boot_phys_offset);
> >  /* Map FDT in boot pagetable */
> >  extern void *early_fdt_map(paddr_t fdt_paddr);
> >  /* Remove early mappings */
> > -- 
> > 2.11.0
> > 
> 

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel

 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.