Re: [Xen-devel] [PATCH 06/45] xen: arm64: initial build + config changes, start of day code

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

On Thu, 2013-02-07 at 13:12 +0000, Tim Deegan wrote:
> At 15:56 +0000 on 23 Jan (1358956572), Ian Campbell wrote:
> > diff --git a/config/arm64.mk b/config/arm64.mk
> > new file mode 100644
> > index 0000000..b2457eb
> > --- /dev/null
> > +++ b/config/arm64.mk
> > @@ -0,0 +1,12 @@
> > +CONFIG_ARM := y
> > +CONFIG_ARM_64 := y
> > +CONFIG_ARM_$(XEN_OS) := y
> > +
> > +CFLAGS += #-marm -march= -mcpu= etc
> > +
> > +HAS_PL011 := y
> > +
> > +# Use only if calling $(LD) directly.
> > +LDFLAGS_DIRECT += -maarch64elf
> > +
> > +CONFIG_LOAD_ADDRESS ?= 0x80000000
> > diff --git a/xen/arch/arm/Rules.mk b/xen/arch/arm/Rules.mk
> > index f83bfee..2250366 100644
> > --- a/xen/arch/arm/Rules.mk
> > +++ b/xen/arch/arm/Rules.mk
> > @@ -25,6 +25,12 @@ arm32 := y
> >  arm64 := n
> >  endif
> >  
> > +ifeq ($(TARGET_SUBARCH),arm64)
> > +CFLAGS += -mcpu=generic
> 
> Should this go in config/arm64.mk?  I realise the equivalent flags for
> arm32 are in this file, just wondering if we ought to tidy them all into
> config/.

The x86 ones are in xen/arch/x86/Rules.mk too, I'm not averse to fixing
that though (perhaps as a subsequent cleanup though?)

> 
> > --- /dev/null
> > +++ b/xen/arch/arm/arm64/head.S
> > @@ -0,0 +1,405 @@

> > +/* Macro to print a string to the UART, if there is one.
> > + * Clobbers r0-r3. */
> > +#ifdef EARLY_UART_ADDRESS
> > +#define PRINT(_s)       \
> > +   adr   x0, 98f ; \
> > +   bl    puts    ; \
> > +   b     99f     ; \
> > +98:        .asciz _s     ; \
> > +   .align 2      ; \
> 
> Backslashes are misaligned here; I suspect because of hard tabs.

Yes, I'll fix all those up.


> > +/*
> > + * Exception Levels
> > + */
> > +#define EL0t   0x00000000
> > +#define EL1t   0x00000004
> > +#define EL1h   0x00000005
> > +#define EL2t   0x00000008
> > +#define EL2h   0x00000009
> > +#define EL3t   0x0000000c
> > +#define EL3h   0x0000000d
> 
> Do these belong in a header somewhere?  If not maybe they can go at the
> top of this file with the PT_ macros.

When I started working on 64-bit guests I ended up adding the exact same
thing to xen/include/public/arch-arm.h (to complement the existing
PSR_MODE_*). I'll do that here instead too.

[..]
> > +1: PRINT("- Started in EL3 -\r\n- Entering EL2 -\r\n")
> > +   ldr   x1, =enter_el2_mode    /* VA of function */
> > +   add   x1, x1, x20            /* PA of function */
> > +   adr   x30, hyp               /* Set return address for call */
> > +   br    x1                     /* Call function */
> > +
> > +2: PRINT("- Started in Hyp mode -\r\n")
> 
> Some confusion here between 'EL2' and 'Hyp' -- are they the same thing?
> Should we use 'el2' consistently?

Hyp is the AArch32 mode, EL2 is the AArch64 mode, they are sort of the
same thing. Should consistently use EL2 here though IMHO, so I'll change
that.

[...]
> > +.globl enter_el2_mode
> > +enter_el2_mode:
> > +        mov     x0, #0x30                       // RES1
> > +        orr     x0, x0, #(1 << 0)               // Non-secure EL1
> > +        orr     x0, x0, #(1 << 8)               // HVC enable
> > +        orr     x0, x0, #(1 << 10)              // 64-bit EL2
> 
> Is this any better than ldr'ing a magic constant?

This came pretty much directly from the bootwrapper stuff, not sure why
it is this way.

> > +
> > +        ldr     x1, =(GIC_BASE_ADDRESS+GIC_DR_OFFSET) // GICD_CTLR
> > +        mov     w0, #3                          // EnableGrp0 | EnableGrp1
> > +        str     w0, [x1]
> > +
> > +1:      ldr     x1, =(GIC_BASE_ADDRESS+GIC_DR_OFFSET+0x80) // GICD_IGROUPR
> 
> Can we use the gic.h constants here?  I gues the '/4' everywhere makes
> it ugly.

I'll try.

> > +        ldr     w0, [x1]
> > +        mov     w0, #3                          // EnableGrp0 | EnableGrp1
> > +        str     w0, [x1]
> > +
> > +        mov     w0, #1 << 7                     // allow NS access to 
> > GICC_PMR
> > +        str     w0, [x1, #4]                    // GICC_PMR
> > +
> > +        msr     sctlr_el2, xzr
> > +
> > +        /*
> > +         * Prepare the switch to the EL2_SP1 mode from EL3
> > +         */
> > +        msr     elr_el3, x30                    // Return to desired 
> > function
> 
> Are you passing an argument in x30?  Or is x30 the link register?

x30 is lr, yes. In entry.S I define an alias, I think I'll do the same
here.

> > +        mov     x1, #0x3c9                      // EL2_SP1 | D | A | I | F
> > +        msr     spsr_el3, x1
> > +        eret
> 
> Maybe add an emacs block here?

Absolutely.

Thanks for the review.

Ian.



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