From mboxrd@z Thu Jan 1 00:00:00 1970 From: Julien Grall Subject: Re: [PATCH v2 2/4] xen: arm: Handle 4K aligned hypervisor load address. Date: Sun, 20 Jul 2014 21:28:47 +0100 Message-ID: <53CC267F.8050103@linaro.org> References: <1405699899.6419.7.camel@kazak.uk.xensource.com> <1405699976-9260-1-git-send-email-ian.campbell@citrix.com> <1405699976-9260-2-git-send-email-ian.campbell@citrix.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <1405699976-9260-2-git-send-email-ian.campbell@citrix.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Ian Campbell , xen-devel@lists.xen.org Cc: tim@xen.org, stefano.stabellini@eu.citrix.com List-Id: xen-devel@lists.xenproject.org Hi Ian, On 18/07/14 17:12, Ian Campbell wrote: > diff --git a/xen/arch/arm/arm32/head.S b/xen/arch/arm/arm32/head.S > index 51501dc..0a76c2e 100644 > --- a/xen/arch/arm/arm32/head.S > +++ b/xen/arch/arm/arm32/head.S > @@ -26,6 +26,7 @@ > > #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 */ > > @@ -258,11 +259,11 @@ cpu_init_done: > /* Setup boot_pgtable: */ > ldr r1, =boot_second > add r1, r1, r10 /* r1 := paddr (boot_second) */ > - mov r3, #0x0 > > /* ... map boot_second in boot_pgtable[0] */ > orr r2, r1, #PT_UPPER(PT) /* r2:r3 := table map of boot_second */ > orr r2, r2, #PT_LOWER(PT) /* (+ rights for linear PT) */ > + mov r3, #0x0 > strd r2, r3, [r4, #0] /* Map it in slot 0 */ > > /* ... map of paddr(start) in boot_pgtable */ > @@ -279,31 +280,61 @@ cpu_init_done: > ldr r4, =boot_second > add r4, r4, r10 /* r4 := paddr (boot_second) */ > > - lsr r2, r9, #SECOND_SHIFT /* Base address for 2MB mapping */ > - lsl r2, r2, #SECOND_SHIFT > + ldr r1, =boot_third > + add r1, r1, r10 /* r1 := paddr (boot_third) */ > + > + /* ... map boot_third in boot_second[1] */ > + orr r2, r1, #PT_UPPER(PT) /* r2:r3 := table map of boot_third */ > + orr r2, r2, #PT_LOWER(PT) /* (+ rights for linear PT) */ > + mov r3, #0x0 > + strd r2, r3, [r4, #8] /* Map it in slot 1 */ > + > + /* ... map of paddr(start) in boot_second */ > + lsr r2, r9, #SECOND_SHIFT /* Offset of base paddr in boot_second */ > + mov r3, #0x0ff /* r2 := LPAE entries mask */ The register in the comment looks wrong to me. > + orr r3, r3, #0x100 I took me several minutes to understand why the orr... I think the 2 previous assembly lines could be replaced by a single one: ldr r3, =LPAE_ENTRY_MASK [..] > diff --git a/xen/arch/arm/arm64/head.S b/xen/arch/arm/arm64/head.S > index d46481b..5d7b2b5 100644 > --- a/xen/arch/arm/arm64/head.S > +++ b/xen/arch/arm/arm64/head.S > @@ -27,6 +27,7 @@ > > #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 */ > > @@ -274,8 +275,9 @@ skip_bss: > lsl x2, x1, #ZEROETH_SHIFT /* Base address for 512GB mapping */ > mov x3, #PT_MEM /* x2 := Section mapping */ > orr x2, x2, x3 > - lsl x1, x1, #3 /* x1 := Slot offset */ > - str x2, [x4, x1] /* Mapping of paddr(start)*/ > + and x1, x1, #0x1ff /* x1 := Slot offset */ The #0x1ff could be replaced by #LPAE_ENTRY_MASK > + lsl x1, x1, #3 > + str x2, [x4, x1] /* Mapping of paddr(start) */ > > 1: /* Setup boot_first: */ > ldr x4, =boot_first /* Next level into boot_first */ > @@ -290,7 +292,7 @@ skip_bss: > > /* ... map of paddr(start) in boot_first */ > lsr x2, x19, #FIRST_SHIFT /* x2 := Offset of base paddr in boot_first */ > - and x1, x2, 0x1ff /* x1 := Slot to use */ > + and x1, x2, #0x1ff /* x1 := Slot to use */ Same here. OOI, you only added the #. Was there any issue before? > cbz x1, 1f /* It's in slot 0, map in boot_second */ > > lsl x2, x2, #FIRST_SHIFT /* Base address for 1GB mapping */ > @@ -303,31 +305,55 @@ skip_bss: > ldr x4, =boot_second /* Next level into boot_second */ > add x4, x4, x20 /* x4 := paddr(boot_second) */ > > - lsr x2, x19, #SECOND_SHIFT /* Base address for 2MB mapping */ > - lsl x2, x2, #SECOND_SHIFT > + /* ... map boot_third in boot_second[1] */ > + ldr x1, =boot_third > + add x1, x1, x20 /* x1 := paddr(boot_third) */ > + mov x3, #PT_PT /* x2 := table map of boot_third */ > + orr x2, x1, x3 /* + rights for linear PT */ > + str x2, [x4, #8] /* Map it in slot 1 */ > + > + /* ... map of paddr(start) in boot_second */ > + lsr x2, x19, #SECOND_SHIFT /* x2 := Offset of base paddr in boot_second */ > + and x1, x2, 0x1ff /* x1 := Slot to use */ A bit strange that few lines above you changed a similar lines to add #, and here you forgot it. I would also replace the 0x1ff by LPAE_ENTRY_MASK. Regards, -- Julien Grall