| Message ID | 20200510075510.987823-3-hch@lst.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [01/31] arm: fix the flush_icache_range arguments in set_fiq_handler | expand |
[+James and Catalin] On Sun, May 10, 2020 at 09:54:41AM +0200, Christoph Hellwig wrote: > The second argument is the end "pointer", not the length. > > Signed-off-by: Christoph Hellwig <hch@lst.de> > --- > arch/arm64/kernel/machine_kexec.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/arch/arm64/kernel/machine_kexec.c b/arch/arm64/kernel/machine_kexec.c > index 8e9c924423b4e..a0b144cfaea71 100644 > --- a/arch/arm64/kernel/machine_kexec.c > +++ b/arch/arm64/kernel/machine_kexec.c > @@ -177,6 +177,7 @@ void machine_kexec(struct kimage *kimage) > * the offline CPUs. Therefore, we must use the __* variant here. > */ > __flush_icache_range((uintptr_t)reboot_code_buffer, > + (uintptr_t)reboot_code_buffer + > arm64_relocate_new_kernel_size); Urgh, well spotted. It's annoyingly different from __flush_dcache_area(). But now I'm wondering what this code actually does... the loop condition in invalidate_icache_by_line works with 64-bit arithmetic, so we could spend a /very/ long time here afaict. It's also a bit annoying that we do a bunch of redundant D-cache maintenance too. Should we use invalidate_icache_range() here instead? (and why does that thing need to toggle uaccess)? Argh, too many questions! Will
On Mon, May 11, 2020 at 08:51:15AM +0100, Will Deacon wrote: > On Sun, May 10, 2020 at 09:54:41AM +0200, Christoph Hellwig wrote: > > The second argument is the end "pointer", not the length. > > > > Signed-off-by: Christoph Hellwig <hch@lst.de> > > --- > > arch/arm64/kernel/machine_kexec.c | 1 + > > 1 file changed, 1 insertion(+) > > > > diff --git a/arch/arm64/kernel/machine_kexec.c b/arch/arm64/kernel/machine_kexec.c > > index 8e9c924423b4e..a0b144cfaea71 100644 > > --- a/arch/arm64/kernel/machine_kexec.c > > +++ b/arch/arm64/kernel/machine_kexec.c > > @@ -177,6 +177,7 @@ void machine_kexec(struct kimage *kimage) > > * the offline CPUs. Therefore, we must use the __* variant here. > > */ > > __flush_icache_range((uintptr_t)reboot_code_buffer, > > + (uintptr_t)reboot_code_buffer + > > arm64_relocate_new_kernel_size); > > Urgh, well spotted. It's annoyingly different from __flush_dcache_area(). > > But now I'm wondering what this code actually does... the loop condition > in invalidate_icache_by_line works with 64-bit arithmetic, so we could > spend a /very/ long time here afaict. I think it goes through the loop only once. The 'b.lo' saves us here. OTOH, there is no I-cache maintenance done. > It's also a bit annoying that we do a bunch of redundant D-cache > maintenance too. Should we use invalidate_icache_range() here instead? Since we have the __flush_dcache_area() above it for cleaning to PoC, we could use invalidate_icache_range() here. We probably didn't have this function at the time, it was added for KVM (commit 4fee94736603cd6). > (and why does that thing need to toggle uaccess)? invalidate_icache_range() doesn't need to, it works on the kernel linear map. __flush_icache_range() doesn't need to either, that's a side-effect of the fall-through implementation. Anyway, I think Christoph's patch needs to go in with a fixes tag: Fixes: d28f6df1305a ("arm64/kexec: Add core kexec support") Cc: <stable@vger.kernel.org> # 4.8.x- and we'll change these functions/helpers going forward for arm64. Happy to pick this up via the arm64 for-next/fixes branch.
On Mon, May 11, 2020 at 12:00:14PM +0100, Catalin Marinas wrote: > Anyway, I think Christoph's patch needs to go in with a fixes tag: > > Fixes: d28f6df1305a ("arm64/kexec: Add core kexec support") > Cc: <stable@vger.kernel.org> # 4.8.x- > > and we'll change these functions/helpers going forward for arm64. > > Happy to pick this up via the arm64 for-next/fixes branch. Please do, there are no dependencies on it in this series (I originally planned to switch flush_icache_range to pass a kernel pointer + len instead of the strange unsigned long start and end. That still looks very useful, but the series already is way too large, so I'm going to defer that change for another merge window).
diff --git a/arch/arm64/kernel/machine_kexec.c b/arch/arm64/kernel/machine_kexec.c index 8e9c924423b4e..a0b144cfaea71 100644 --- a/arch/arm64/kernel/machine_kexec.c +++ b/arch/arm64/kernel/machine_kexec.c @@ -177,6 +177,7 @@ void machine_kexec(struct kimage *kimage) * the offline CPUs. Therefore, we must use the __* variant here. */ __flush_icache_range((uintptr_t)reboot_code_buffer, + (uintptr_t)reboot_code_buffer + arm64_relocate_new_kernel_size); /* Flush the kimage list and its buffers. */
The second argument is the end "pointer", not the length. Signed-off-by: Christoph Hellwig <hch@lst.de> --- arch/arm64/kernel/machine_kexec.c | 1 + 1 file changed, 1 insertion(+)