RE: [PATCH v6 08/11] xen/arm64: move MMU related code from head.S to head_mmu.S

[Wei Chen <Wei.Chen@xxxxxxx>]

Hi Julien,

> -----Original Message-----
> From: Julien Grall <julien@xxxxxxx>
> Sent: 2022年11月7日 4:06
> To: Wei Chen <Wei.Chen@xxxxxxx>; xen-devel@xxxxxxxxxxxxxxxxxxxx
> Cc: nd <nd@xxxxxxx>; Stefano Stabellini <sstabellini@xxxxxxxxxx>; Bertrand
> Marquis <Bertrand.Marquis@xxxxxxx>; Volodymyr Babchuk
> <Volodymyr_Babchuk@xxxxxxxx>; Henry Wang <Henry.Wang@xxxxxxx>
> Subject: Re: [PATCH v6 08/11] xen/arm64: move MMU related code from head.S
> to head_mmu.S
> 
> Hi Wei,
> 
> On 04/11/2022 10:07, Wei Chen wrote:
> > There are lots of MMU specific code in head.S. This code will not
> > be used in MPU systems. If we use #ifdef to gate them, the code
> > will become messy and hard to maintain. So we move MMU related
> > code to head_mmu.S, and keep common code still in head.S.
> 
> I am afraid that you can't simply move the MMU code out of head.S
> because this will break Xen when running using the identity map.
> 
> This is because we only map the first 4KB of Xen with PA == VA. At the
> moment, we guarantee it by having everything that needs to be used in
> the identity mapping before _end_boot and checking at link time if this
> fits in 4KB.
> 
> Now that you moved the MMU code outside of head.S. We need to find a
> different way to guarantee it. One way to do it would be to create a
> section that would be used for everything that needs to be identity mapped.
> 

Quote from next email
"
Looking at the code this morning, I noticed that we already have the 
section ".text.header". For now, that should do the job. So we just need
to check the size of .text.header.

Ideally, checking the size should be done in a separate pre-patch so it 
is easier to review.
"

OK. We will create a patch to check the size, and place it in the
head of the series.

> >
> > As we need to access "fail" and "puts" functions out of assembly
> > file, so we have to export them in this patch. And the assembly
> > macros: adr_l and load_paddr will be used by MMU and MPU later,
> > so we move them to macros.h.
> >
> > Signed-off-by: Henry Wang <Henry.Wang@xxxxxxx>
> > Signed-off-by: Wei Chen <wei.chen@xxxxxxx>
> 
> In general, the first signed-off should match the author. So who is who
> here?
> 

I will adjust this order.

> > ---
> >   xen/arch/arm/arm64/Makefile             |   3 +
> >   xen/arch/arm/arm64/head.S               | 407 +-----------------------
> >   xen/arch/arm/arm64/head_mmu.S           | 364 +++++++++++++++++++++
> >   xen/arch/arm/include/asm/arm64/macros.h |  52 ++-
> >   4 files changed, 432 insertions(+), 394 deletions(-)
> >   create mode 100644 xen/arch/arm/arm64/head_mmu.S
> >
> > diff --git a/xen/arch/arm/arm64/Makefile b/xen/arch/arm/arm64/Makefile
> > index 6d507da0d4..22da2f54b5 100644
> > --- a/xen/arch/arm/arm64/Makefile
> > +++ b/xen/arch/arm/arm64/Makefile
> > @@ -8,6 +8,9 @@ obj-y += domctl.o
> >   obj-y += domain.o
> >   obj-y += entry.o
> >   obj-y += head.o
> > +ifneq ($(CONFIG_HAS_MPU),y) > +obj-y += head_mmu.o
> > +endif
> >   obj-y += insn.o
> >   obj-$(CONFIG_LIVEPATCH) += livepatch.o
> >   obj-y += smc.o
> > diff --git a/xen/arch/arm/arm64/head.S b/xen/arch/arm/arm64/head.S
> > index ccedf20dc7..d9a8da9120 100644
> > --- a/xen/arch/arm/arm64/head.S
> > +++ b/xen/arch/arm/arm64/head.S
> > @@ -25,17 +25,6 @@
> >   #include <efi/efierr.h>
> >   #include <asm/arm64/efibind.h>
> >
> > -#define PT_PT     0xf7f /* nG=1 AF=1 SH=11 AP=01 NS=1 ATTR=111 T=1 P=1
> */
> > -#define PT_MEM    0xf7d /* nG=1 AF=1 SH=11 AP=01 NS=1 ATTR=111 T=0 P=1
> */
> > -#define PT_MEM_L3 0xf7f /* nG=1 AF=1 SH=11 AP=01 NS=1 ATTR=111 T=1 P=1
> */
> > -#define PT_DEV    0xe71 /* nG=1 AF=1 SH=10 AP=01 NS=1 ATTR=100 T=0 P=1
> */
> > -#define PT_DEV_L3 0xe73 /* nG=1 AF=1 SH=10 AP=01 NS=1 ATTR=100 T=1 P=1
> */
> > -
> > -/* Convenience defines to get slot used by Xen mapping. */
> > -#define XEN_ZEROETH_SLOT    zeroeth_table_offset(XEN_VIRT_START)
> > -#define XEN_FIRST_SLOT      first_table_offset(XEN_VIRT_START)
> > -#define XEN_SECOND_SLOT     second_table_offset(XEN_VIRT_START)
> > -
> >   #define __HEAD_FLAG_PAGE_SIZE   ((PAGE_SHIFT - 10) / 2)
> >
> >   #define __HEAD_FLAG_PHYS_BASE   1
> > @@ -82,73 +71,22 @@
> >    *  x30 - lr
> >    */
> >
> > -#ifdef CONFIG_EARLY_PRINTK
> > -/*
> > - * Macro to print a string to the UART, if there is one.
> > - *
> > - * Clobbers x0 - x3
> > - */
> > -#define PRINT(_s)          \
> > -        mov   x3, lr ;     \
> > -        adr   x0, 98f ;    \
> > -        bl    puts    ;    \
> > -        mov   lr, x3 ;     \
> > -        RODATA_STR(98, _s)
> > +.section .text.header, "ax", %progbits
> > +/*.aarch64*/
> 
> The patch is already quite difficult to read. So I would rather prefer
> if the indentation is changed separately.
> 

Ok.

> Furthermore, I think it would be best if the functions moved in the
> header are done separately to help checking (I would be able to diff the
> source with the destination more easily).
> 

Did you mean to create a separate patch for moving the functions in macro.h?
Or in header section?

> >
> >   /*
> > - * Macro to print the value of register \xb
> > + * Kernel startup entry point.
> > + * ---------------------------
> 
> Same here about the indentation. I will not comment everywhere where the
> indentation was changed. So please look at it.
> 

OK. we will use a separate patch to clean them.

> [...]
> 
> > -/*
> > - * Map the UART in the fixmap (when earlyprintk is used) and hook the
> > - * fixmap table in the page tables.
> > - *
> > - * The fixmap cannot be mapped in create_page_tables because it may
> > - * clash with the 1:1 mapping.
> > - *
> > - * Inputs:
> > - *   x20: Physical offset
> > - *   x23: Early UART base physical address
> > - *
> > - * Clobbers x0 - x3
> > - */
> > -setup_fixmap:
> > -#ifdef CONFIG_EARLY_PRINTK
> > -        /* Add UART to the fixmap table */
> > -        ldr   x0, =EARLY_UART_VIRTUAL_ADDRESS
> > -        create_mapping_entry xen_fixmap, x0, x23, x1, x2, x3,
> type=PT_DEV_L3
> > -#endif
> > -        /* Map fixmap into boot_second */
> > -        ldr   x0, =FIXMAP_ADDR(0)
> > -        create_table_entry boot_second, xen_fixmap, x0, 2, x1, x2, x3
> > -        /* Ensure any page table updates made above have occurred. */
> > -        dsb   nshst
> > -
> > -        ret
> > -ENDPROC(setup_fixmap)
> > -
> >   /*
> >    * Setup the initial stack and jump to the C world
> >    *
> > @@ -810,41 +458,14 @@ launch:
> >   ENDPROC(launch)
> >
> >   /* Fail-stop */
> > -fail:   PRINT("- Boot failed -\r\n")
> > +ENTRY(fail)
> 
> This name is a bit too generic to be exposed. But it would be better to
> duplicate it.

Yes, duplicate it might be better. We will do it in next version.

Cheers,
Wei Chen

> 
> Cheers,
> 
> --
> Julien Grall