| Message ID | 1414440752-9411-4-git-send-email-lauraa@codeaurora.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 27 October 2014 21:12, Laura Abbott <lauraa@codeaurora.org> wrote: > The function cpu_resume currently lives in the .data > section. There's no reason for it to be there since > we can use relative instructions without a problem. > Move it to the text section where it belongs. > > Reviewed-by: Kees Cook <keescook@chromium.org> > Signed-off-by: Laura Abbott <lauraa@codeaurora.org> > --- > arch/arm64/kernel/sleep.S | 12 +++++++----- > 1 file changed, 7 insertions(+), 5 deletions(-) > Hi Laura, Apologies for waiting until v4 to bring this up, but I have some minor comments. > diff --git a/arch/arm64/kernel/sleep.S b/arch/arm64/kernel/sleep.S > index a564b44..5762b16 100644 > --- a/arch/arm64/kernel/sleep.S > +++ b/arch/arm64/kernel/sleep.S > @@ -147,12 +147,12 @@ cpu_resume_after_mmu: > ret > ENDPROC(cpu_resume_after_mmu) > > - .data > ENTRY(cpu_resume) > bl el2_setup // if in EL2 drop to EL1 cleanly > #ifdef CONFIG_SMP > mrs x1, mpidr_el1 > - adr x4, mpidr_hash_ptr > + adrp x4, mpidr_hash_ptr > + add x4, x4, #:lo12:mpidr_hash_ptr Instead of this change, you could put mpidr_hash_ptr itself in .text as well. > ldr x5, [x4] > add x8, x4, x5 // x8 = struct mpidr_hash phys address > /* retrieve mpidr_hash members to compute the hash */ > @@ -164,14 +164,15 @@ ENTRY(cpu_resume) > #else > mov x7, xzr > #endif > - adr x0, sleep_save_sp > + adrp x0, sleep_save_sp > + add x0, x0, #:lo12:sleep_save_sp > ldr x0, [x0, #SLEEP_SAVE_SP_PHYS] Nit: ldr x0, [x0, #:lo12:sleep_save_sp + SLEEP_SAVE_SP_PHYS] > ldr x0, [x0, x7, lsl #3] > /* load sp from context */ > ldr x2, [x0, #CPU_CTX_SP] > - adr x1, sleep_idmap_phys > + adrp x1, sleep_idmap_phys > /* load physical address of identity map page table in x1 */ > - ldr x1, [x1] > + ldr x1, [x1, #:lo12:sleep_idmap_phys] > mov sp, x2 > /* > * cpu_do_resume expects x0 to contain context physical address > @@ -181,6 +182,7 @@ ENTRY(cpu_resume) > b cpu_resume_mmu // Resume MMU, never returns > ENDPROC(cpu_resume) > > + .data > .align 3 > mpidr_hash_ptr: > /* Acked-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
On 28 October 2014 09:10, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote: > On 27 October 2014 21:12, Laura Abbott <lauraa@codeaurora.org> wrote: >> The function cpu_resume currently lives in the .data >> section. There's no reason for it to be there since >> we can use relative instructions without a problem. >> Move it to the text section where it belongs. >> >> Reviewed-by: Kees Cook <keescook@chromium.org> >> Signed-off-by: Laura Abbott <lauraa@codeaurora.org> >> --- >> arch/arm64/kernel/sleep.S | 12 +++++++----- >> 1 file changed, 7 insertions(+), 5 deletions(-) >> > > Hi Laura, > > Apologies for waiting until v4 to bring this up, but I have some minor comments. > >> diff --git a/arch/arm64/kernel/sleep.S b/arch/arm64/kernel/sleep.S >> index a564b44..5762b16 100644 >> --- a/arch/arm64/kernel/sleep.S >> +++ b/arch/arm64/kernel/sleep.S >> @@ -147,12 +147,12 @@ cpu_resume_after_mmu: >> ret >> ENDPROC(cpu_resume_after_mmu) >> >> - .data >> ENTRY(cpu_resume) >> bl el2_setup // if in EL2 drop to EL1 cleanly >> #ifdef CONFIG_SMP >> mrs x1, mpidr_el1 >> - adr x4, mpidr_hash_ptr >> + adrp x4, mpidr_hash_ptr >> + add x4, x4, #:lo12:mpidr_hash_ptr > > Instead of this change, you could put mpidr_hash_ptr itself in .text as well. > Actually, looking more closely, it appears mpidr_hash_ptr can be dropped completely: the address obtained by adrp/add is PC relative, so the whole sequence could just be adrp x8, mpidr_hash add x8, x8, #:lo12:mpidr_hash // x8 = struct mpidr_hash phys address (and the ldr and add below can be dropped, and so can mpidr_hash_ptr at the end) >> ldr x5, [x4] >> add x8, x4, x5 // x8 = struct mpidr_hash phys address >> /* retrieve mpidr_hash members to compute the hash */ >> @@ -164,14 +164,15 @@ ENTRY(cpu_resume) >> #else >> mov x7, xzr >> #endif >> - adr x0, sleep_save_sp >> + adrp x0, sleep_save_sp >> + add x0, x0, #:lo12:sleep_save_sp >> ldr x0, [x0, #SLEEP_SAVE_SP_PHYS] > > Nit: > ldr x0, [x0, #:lo12:sleep_save_sp + SLEEP_SAVE_SP_PHYS] > >> ldr x0, [x0, x7, lsl #3] >> /* load sp from context */ >> ldr x2, [x0, #CPU_CTX_SP] >> - adr x1, sleep_idmap_phys >> + adrp x1, sleep_idmap_phys >> /* load physical address of identity map page table in x1 */ >> - ldr x1, [x1] >> + ldr x1, [x1, #:lo12:sleep_idmap_phys] >> mov sp, x2 >> /* >> * cpu_do_resume expects x0 to contain context physical address >> @@ -181,6 +182,7 @@ ENTRY(cpu_resume) >> b cpu_resume_mmu // Resume MMU, never returns >> ENDPROC(cpu_resume) >> >> + .data >> .align 3 >> mpidr_hash_ptr: >> /* > > Acked-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
On Tue, Oct 28, 2014 at 08:22:00AM +0000, Ard Biesheuvel wrote: > On 28 October 2014 09:10, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote: > > On 27 October 2014 21:12, Laura Abbott <lauraa@codeaurora.org> wrote: > >> The function cpu_resume currently lives in the .data > >> section. There's no reason for it to be there since > >> we can use relative instructions without a problem. > >> Move it to the text section where it belongs. > >> > >> Reviewed-by: Kees Cook <keescook@chromium.org> > >> Signed-off-by: Laura Abbott <lauraa@codeaurora.org> > >> --- > >> arch/arm64/kernel/sleep.S | 12 +++++++----- > >> 1 file changed, 7 insertions(+), 5 deletions(-) > >> > > > > Hi Laura, > > > > Apologies for waiting until v4 to bring this up, but I have some minor comments. > > > >> diff --git a/arch/arm64/kernel/sleep.S b/arch/arm64/kernel/sleep.S > >> index a564b44..5762b16 100644 > >> --- a/arch/arm64/kernel/sleep.S > >> +++ b/arch/arm64/kernel/sleep.S > >> @@ -147,12 +147,12 @@ cpu_resume_after_mmu: > >> ret > >> ENDPROC(cpu_resume_after_mmu) > >> > >> - .data > >> ENTRY(cpu_resume) > >> bl el2_setup // if in EL2 drop to EL1 cleanly > >> #ifdef CONFIG_SMP > >> mrs x1, mpidr_el1 > >> - adr x4, mpidr_hash_ptr > >> + adrp x4, mpidr_hash_ptr > >> + add x4, x4, #:lo12:mpidr_hash_ptr > > > > Instead of this change, you could put mpidr_hash_ptr itself in .text as well. > > > > Actually, looking more closely, it appears mpidr_hash_ptr can be > dropped completely: > the address obtained by adrp/add is PC relative, so the whole sequence > could just be > > adrp x8, mpidr_hash > add x8, x8, #:lo12:mpidr_hash // x8 = struct mpidr_hash phys address > > (and the ldr and add below can be dropped, and so can mpidr_hash_ptr at the end) Also, if you update arch/arm64/kernel/suspend.c to drop the extern from sleep_save_sp and sleep_idmap_phys, you can drop all of the .data from sleep.S I gave all of that a go locally and my Juno happily booted to userspace with cpuidle running. I had to apply Marc's timer fix to get cpuidle to work at all with v3.18-rc2, but that should be fixed for -rc3. So with those changes: Reviewed-by: Mark Rutland <mark.rutland@arm.com> Tested-by: Mark Rutland <mark.rutland@arm.com> Thanks, Mark.
On Tue, Oct 28, 2014 at 12:43:15PM +0000, Mark Rutland wrote: > On Tue, Oct 28, 2014 at 08:22:00AM +0000, Ard Biesheuvel wrote: > > On 28 October 2014 09:10, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote: > > > On 27 October 2014 21:12, Laura Abbott <lauraa@codeaurora.org> wrote: > > >> The function cpu_resume currently lives in the .data > > >> section. There's no reason for it to be there since > > >> we can use relative instructions without a problem. > > >> Move it to the text section where it belongs. > > >> > > >> Reviewed-by: Kees Cook <keescook@chromium.org> > > >> Signed-off-by: Laura Abbott <lauraa@codeaurora.org> > > >> --- > > >> arch/arm64/kernel/sleep.S | 12 +++++++----- > > >> 1 file changed, 7 insertions(+), 5 deletions(-) > > >> > > > > > > Hi Laura, > > > > > > Apologies for waiting until v4 to bring this up, but I have some minor comments. > > > > > >> diff --git a/arch/arm64/kernel/sleep.S b/arch/arm64/kernel/sleep.S > > >> index a564b44..5762b16 100644 > > >> --- a/arch/arm64/kernel/sleep.S > > >> +++ b/arch/arm64/kernel/sleep.S > > >> @@ -147,12 +147,12 @@ cpu_resume_after_mmu: > > >> ret > > >> ENDPROC(cpu_resume_after_mmu) > > >> > > >> - .data > > >> ENTRY(cpu_resume) > > >> bl el2_setup // if in EL2 drop to EL1 cleanly > > >> #ifdef CONFIG_SMP > > >> mrs x1, mpidr_el1 > > >> - adr x4, mpidr_hash_ptr > > >> + adrp x4, mpidr_hash_ptr > > >> + add x4, x4, #:lo12:mpidr_hash_ptr > > > > > > Instead of this change, you could put mpidr_hash_ptr itself in .text as well. > > > > > > > Actually, looking more closely, it appears mpidr_hash_ptr can be > > dropped completely: > > the address obtained by adrp/add is PC relative, so the whole sequence > > could just be > > > > adrp x8, mpidr_hash > > add x8, x8, #:lo12:mpidr_hash // x8 = struct mpidr_hash phys address > > > > (and the ldr and add below can be dropped, and so can mpidr_hash_ptr at the end) > > Also, if you update arch/arm64/kernel/suspend.c to drop the extern from > sleep_save_sp and sleep_idmap_phys, you can drop all of the .data from > sleep.S If with that you also mean that all adr references to those data structures in the resume path should become adrp I agree otherwise we might have an offset issue. Please respin the patch with the above changes, I tested by applying local changes myself, I will add my tags to the final version. Thanks, Lorenzo > I gave all of that a go locally and my Juno happily booted to userspace > with cpuidle running. I had to apply Marc's timer fix to get cpuidle to > work at all with v3.18-rc2, but that should be fixed for -rc3. > > So with those changes: > > Reviewed-by: Mark Rutland <mark.rutland@arm.com> > Tested-by: Mark Rutland <mark.rutland@arm.com>
On Tue, Oct 28, 2014 at 03:26:56PM +0000, Lorenzo Pieralisi wrote: > On Tue, Oct 28, 2014 at 12:43:15PM +0000, Mark Rutland wrote: > > On Tue, Oct 28, 2014 at 08:22:00AM +0000, Ard Biesheuvel wrote: > > > On 28 October 2014 09:10, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote: > > > > On 27 October 2014 21:12, Laura Abbott <lauraa@codeaurora.org> wrote: > > > >> The function cpu_resume currently lives in the .data > > > >> section. There's no reason for it to be there since > > > >> we can use relative instructions without a problem. > > > >> Move it to the text section where it belongs. > > > >> > > > >> Reviewed-by: Kees Cook <keescook@chromium.org> > > > >> Signed-off-by: Laura Abbott <lauraa@codeaurora.org> > > > >> --- > > > >> arch/arm64/kernel/sleep.S | 12 +++++++----- > > > >> 1 file changed, 7 insertions(+), 5 deletions(-) > > > >> > > > > > > > > Hi Laura, > > > > > > > > Apologies for waiting until v4 to bring this up, but I have some minor comments. > > > > > > > >> diff --git a/arch/arm64/kernel/sleep.S b/arch/arm64/kernel/sleep.S > > > >> index a564b44..5762b16 100644 > > > >> --- a/arch/arm64/kernel/sleep.S > > > >> +++ b/arch/arm64/kernel/sleep.S > > > >> @@ -147,12 +147,12 @@ cpu_resume_after_mmu: > > > >> ret > > > >> ENDPROC(cpu_resume_after_mmu) > > > >> > > > >> - .data > > > >> ENTRY(cpu_resume) > > > >> bl el2_setup // if in EL2 drop to EL1 cleanly > > > >> #ifdef CONFIG_SMP > > > >> mrs x1, mpidr_el1 > > > >> - adr x4, mpidr_hash_ptr > > > >> + adrp x4, mpidr_hash_ptr > > > >> + add x4, x4, #:lo12:mpidr_hash_ptr > > > > > > > > Instead of this change, you could put mpidr_hash_ptr itself in .text as well. > > > > > > > > > > Actually, looking more closely, it appears mpidr_hash_ptr can be > > > dropped completely: > > > the address obtained by adrp/add is PC relative, so the whole sequence > > > could just be > > > > > > adrp x8, mpidr_hash > > > add x8, x8, #:lo12:mpidr_hash // x8 = struct mpidr_hash phys address > > > > > > (and the ldr and add below can be dropped, and so can mpidr_hash_ptr at the end) > > > > Also, if you update arch/arm64/kernel/suspend.c to drop the extern from > > sleep_save_sp and sleep_idmap_phys, you can drop all of the .data from > > sleep.S > > If with that you also mean that all adr references to those data structures > in the resume path should become adrp I agree otherwise we might have an > offset issue. Yes. The original patch had those modifications already, so I assumed they would remain. Mark.
On 10/28/2014 8:31 AM, Mark Rutland wrote: > On Tue, Oct 28, 2014 at 03:26:56PM +0000, Lorenzo Pieralisi wrote: >> On Tue, Oct 28, 2014 at 12:43:15PM +0000, Mark Rutland wrote: >>> On Tue, Oct 28, 2014 at 08:22:00AM +0000, Ard Biesheuvel wrote: >>>> On 28 October 2014 09:10, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote: >>>>> On 27 October 2014 21:12, Laura Abbott <lauraa@codeaurora.org> wrote: >>>>>> The function cpu_resume currently lives in the .data >>>>>> section. There's no reason for it to be there since >>>>>> we can use relative instructions without a problem. >>>>>> Move it to the text section where it belongs. >>>>>> >>>>>> Reviewed-by: Kees Cook <keescook@chromium.org> >>>>>> Signed-off-by: Laura Abbott <lauraa@codeaurora.org> >>>>>> --- >>>>>> arch/arm64/kernel/sleep.S | 12 +++++++----- >>>>>> 1 file changed, 7 insertions(+), 5 deletions(-) >>>>>> >>>>> >>>>> Hi Laura, >>>>> >>>>> Apologies for waiting until v4 to bring this up, but I have some minor comments. >>>>> >>>>>> diff --git a/arch/arm64/kernel/sleep.S b/arch/arm64/kernel/sleep.S >>>>>> index a564b44..5762b16 100644 >>>>>> --- a/arch/arm64/kernel/sleep.S >>>>>> +++ b/arch/arm64/kernel/sleep.S >>>>>> @@ -147,12 +147,12 @@ cpu_resume_after_mmu: >>>>>> ret >>>>>> ENDPROC(cpu_resume_after_mmu) >>>>>> >>>>>> - .data >>>>>> ENTRY(cpu_resume) >>>>>> bl el2_setup // if in EL2 drop to EL1 cleanly >>>>>> #ifdef CONFIG_SMP >>>>>> mrs x1, mpidr_el1 >>>>>> - adr x4, mpidr_hash_ptr >>>>>> + adrp x4, mpidr_hash_ptr >>>>>> + add x4, x4, #:lo12:mpidr_hash_ptr >>>>> >>>>> Instead of this change, you could put mpidr_hash_ptr itself in .text as well. >>>>> >>>> >>>> Actually, looking more closely, it appears mpidr_hash_ptr can be >>>> dropped completely: >>>> the address obtained by adrp/add is PC relative, so the whole sequence >>>> could just be >>>> >>>> adrp x8, mpidr_hash >>>> add x8, x8, #:lo12:mpidr_hash // x8 = struct mpidr_hash phys address >>>> >>>> (and the ldr and add below can be dropped, and so can mpidr_hash_ptr at the end) >>> >>> Also, if you update arch/arm64/kernel/suspend.c to drop the extern from >>> sleep_save_sp and sleep_idmap_phys, you can drop all of the .data from >>> sleep.S >> >> If with that you also mean that all adr references to those data structures >> in the resume path should become adrp I agree otherwise we might have an >> offset issue. > > Yes. The original patch had those modifications already, so I assumed > they would remain. > Thanks all. I'll incorporate the suggestions in the next version. Laura
diff --git a/arch/arm64/kernel/sleep.S b/arch/arm64/kernel/sleep.S index a564b44..5762b16 100644 --- a/arch/arm64/kernel/sleep.S +++ b/arch/arm64/kernel/sleep.S @@ -147,12 +147,12 @@ cpu_resume_after_mmu: ret ENDPROC(cpu_resume_after_mmu) - .data ENTRY(cpu_resume) bl el2_setup // if in EL2 drop to EL1 cleanly #ifdef CONFIG_SMP mrs x1, mpidr_el1 - adr x4, mpidr_hash_ptr + adrp x4, mpidr_hash_ptr + add x4, x4, #:lo12:mpidr_hash_ptr ldr x5, [x4] add x8, x4, x5 // x8 = struct mpidr_hash phys address /* retrieve mpidr_hash members to compute the hash */ @@ -164,14 +164,15 @@ ENTRY(cpu_resume) #else mov x7, xzr #endif - adr x0, sleep_save_sp + adrp x0, sleep_save_sp + add x0, x0, #:lo12:sleep_save_sp ldr x0, [x0, #SLEEP_SAVE_SP_PHYS] ldr x0, [x0, x7, lsl #3] /* load sp from context */ ldr x2, [x0, #CPU_CTX_SP] - adr x1, sleep_idmap_phys + adrp x1, sleep_idmap_phys /* load physical address of identity map page table in x1 */ - ldr x1, [x1] + ldr x1, [x1, #:lo12:sleep_idmap_phys] mov sp, x2 /* * cpu_do_resume expects x0 to contain context physical address @@ -181,6 +182,7 @@ ENTRY(cpu_resume) b cpu_resume_mmu // Resume MMU, never returns ENDPROC(cpu_resume) + .data .align 3 mpidr_hash_ptr: /*