Re: [Xen-devel] [PATCH for-4.9 2/4] xen/arm: Move the code to map FDT in the boot tables from assembly to C

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Wed, 19 Apr 2017, Julien Grall wrote:
> Hi Stefano,
> 
> On 19/04/2017 22:01, Stefano Stabellini wrote:
> > On Wed, 19 Apr 2017, Julien Grall wrote:
> > > The FDT will not be accessed before start_xen (begining of C code) is
> > > called and it will be easier to maintain as the code could be common
> > > between AArch32 and AArch64.
> > > 
> > > A new function early_fdt_map is introduced to map the FDT in the boot
> > > page table.
> > > 
> > > Note that create_mapping will flush the TLBs for us so no need to add an
> > > extra on in case the entry was previously used by the 1:1 mapping.
> > > 
> > > Signed-off-by: Julien Grall <julien.grall@xxxxxxx>
> > > 
> > > ---
> > > 
> > >     I can move the function create_mappings at the beginning of the
> > >     function instead of adding a static declaration. But I thought it
> > >     was not Xen 4.9 material. Any opinions?
> > > ---
> > >  xen/arch/arm/arm32/head.S | 14 --------------
> > >  xen/arch/arm/arm64/head.S | 13 -------------
> > >  xen/arch/arm/mm.c         | 20 ++++++++++++++++++++
> > >  xen/arch/arm/setup.c      |  4 +---
> > >  xen/include/asm-arm/mm.h  |  2 ++
> > >  5 files changed, 23 insertions(+), 30 deletions(-)
> > > 
> > > diff --git a/xen/arch/arm/arm32/head.S b/xen/arch/arm/arm32/head.S
> > > index ec63ba4c04..4090f4a744 100644
> > > --- a/xen/arch/arm/arm32/head.S
> > > +++ b/xen/arch/arm/arm32/head.S
> > > @@ -389,20 +389,6 @@ paging:
> > >          /* Use a virtual address to access the UART. */
> > >          ldr   r11, =EARLY_UART_VIRTUAL_ADDRESS
> > >  #endif
> > > -        /* Map the DTB in the boot misc slot */
> > > -        teq   r12, #0                /* Only on boot CPU */
> > > -        bne   1f
> > > -
> > > -        ldr   r1, =boot_second
> > > -        mov   r3, #0x0
> > > -        lsr   r2, r8, #SECOND_SHIFT
> > > -        lsl   r2, r2, #SECOND_SHIFT  /* r2: 2MB-aligned paddr of DTB */
> > > -        orr   r2, r2, #PT_UPPER(MEM)
> > > -        orr   r2, r2, #PT_LOWER(MEM) /* r2:r3 := 2MB RAM incl. DTB */
> > > -        ldr   r4, =BOOT_FDT_VIRT_START
> > > -        mov   r4, r4, lsr #(SECOND_SHIFT - 3)   /* Slot for
> > > BOOT_FDT_VIRT_START */
> > > -        strd  r2, r3, [r1, r4]       /* Map it in the early fdt slot */
> > > -1:
> > > 
> > >          /*
> > >           * Flush the TLB in case the 1:1 mapping happens to clash with
> > > diff --git a/xen/arch/arm/arm64/head.S b/xen/arch/arm/arm64/head.S
> > > index 72ea4e0233..78292f4396 100644
> > > --- a/xen/arch/arm/arm64/head.S
> > > +++ b/xen/arch/arm/arm64/head.S
> > > @@ -550,19 +550,6 @@ paging:
> > >          ldr   x23, =EARLY_UART_VIRTUAL_ADDRESS
> > >  #endif
> > > 
> > > -        /* Map the DTB in the boot misc slot */
> > > -        cbnz  x22, 1f                /* Only on boot CPU */
> > > -
> > > -        ldr   x4, =boot_second       /* x4 := vaddr (boot_second) */
> > > -        lsr   x2, x21, #SECOND_SHIFT
> > > -        lsl   x2, x2, #SECOND_SHIFT  /* x2 := 2MB-aligned paddr of DTB */
> > > -        mov   x3, #PT_MEM            /* x2 := 2MB RAM incl. DTB */
> > > -        orr   x2, x2, x3
> > > -        ldr   x1, =BOOT_FDT_VIRT_START
> > > -        lsr   x1, x1, #(SECOND_SHIFT - 3)   /* x4 := Slot for
> > > BOOT_FDT_VIRT_START */
> > > -        str   x2, [x4, x1]           /* Map it in the early fdt slot */
> > > -1:
> > > -
> > >          /*
> > >           * Flush the TLB in case the 1:1 mapping happens to clash with
> > >           * the virtual addresses used by the fixmap or DTB.
> > > diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
> > > index f0a2eddaaf..0d076489c6 100644
> > > --- a/xen/arch/arm/mm.c
> > > +++ b/xen/arch/arm/mm.c
> > > @@ -38,6 +38,13 @@
> > >  #include <xen/vmap.h>
> > >  #include <xsm/xsm.h>
> > >  #include <xen/pfn.h>
> > > +#include <xen/sizes.h>
> > > +
> > > +static void __init create_mappings(lpae_t *second,
> > > +                                   unsigned long virt_offset,
> > > +                                   unsigned long base_mfn,
> > > +                                   unsigned long nr_mfns,
> > > +                                   unsigned int mapping_size);
> > 
> > I would have added early_fdt_map to this file in a way to avoid the need
> > for duplicating the declaration of create_mappings (because this version
> > doesn't have the useful comment on top).
> 
> I wanted to keep the function close to the counterpart remove_early_mappings
> rather than adding somewhere that make less sense.
> 
> Hence why I suggested to move the create_mappings function. Would you be fine
> with code motion for Xen 4.9?

Sure. But please keep the code motion in its own separate patch.

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel