| Message ID | 20191004185234.31471-15-pasha.tatashin@soleen.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | arm64: MMU enabled kexec relocation | expand |
Hi Pavel, On 04/10/2019 19:52, Pavel Tatashin wrote: > Currently, kernel relocation function is configured in machine_kexec() > at the time of kexec reboot by using control_code_page. > > This operation, however, is more logical to be done during kexec_load, > and thus remove from reboot time. Move, setup of this function to > newly added machine_kexec_post_load(). > > In addition, do some cleanup: add infor about reloction function to infor ? reloction? > kexec_image_info(), and remove extra messages from machine_kexec(). > Make dtb_mem, always available, if CONFIG_KEXEC_FILE is not configured > dtb_mem is set to zero anyway. This is unrelated cleanup, please do it as an earlier patch to make it clearer what you are changing here. (I'm not convinced you need to cache va<->pa) > diff --git a/arch/arm64/include/asm/kexec.h b/arch/arm64/include/asm/kexec.h > index 12a561a54128..d15ca1ca1e83 100644 > --- a/arch/arm64/include/asm/kexec.h > +++ b/arch/arm64/include/asm/kexec.h > @@ -90,14 +90,15 @@ static inline void crash_prepare_suspend(void) {} > static inline void crash_post_resume(void) {} > #endif > > -#ifdef CONFIG_KEXEC_FILE > #define ARCH_HAS_KIMAGE_ARCH > > struct kimage_arch { > void *dtb; > unsigned long dtb_mem; > + unsigned long kern_reloc; This is cache-ing the physical address of an all-architectures value from struct kimage, in the arch specific part of struct kiamge. Why? (You must have the struct kimage on hand to access this thing at all!) If its supposed to be a physical address, please use phys_addr_t. > }; > diff --git a/arch/arm64/kernel/machine_kexec.c b/arch/arm64/kernel/machine_kexec.c > index 0df8493624e0..9b41da50e6f7 100644 > --- a/arch/arm64/kernel/machine_kexec.c > +++ b/arch/arm64/kernel/machine_kexec.c > @@ -42,6 +42,7 @@ static void _kexec_image_info(const char *func, int line, > pr_debug(" start: %lx\n", kimage->start); > pr_debug(" head: %lx\n", kimage->head); > pr_debug(" nr_segments: %lu\n", kimage->nr_segments); > + pr_debug(" kern_reloc: %pa\n", &kimage->arch.kern_reloc); > > for (i = 0; i < kimage->nr_segments; i++) { > pr_debug(" segment[%lu]: %016lx - %016lx, 0x%lx bytes, %lu pages\n", > @@ -58,6 +59,19 @@ void machine_kexec_cleanup(struct kimage *kimage) > /* Empty routine needed to avoid build errors. */ > } > > +int machine_kexec_post_load(struct kimage *kimage) > +{ > + unsigned long kern_reloc; > + > + kern_reloc = page_to_phys(kimage->control_code_page); kern_reloc should be phys_addr_t. > + memcpy(__va(kern_reloc), arm64_relocate_new_kernel, > + arm64_relocate_new_kernel_size); > + kimage->arch.kern_reloc = kern_reloc; Please move the cache maintenance in here too. This will save us doing it late during kdump. This will also group the mmu-on changes together. > +} Thanks, James
> > In addition, do some cleanup: add infor about reloction function to > > infor ? reloction? Typo. I have fixed commit log. It meant to be "info about arm64_relocate_new_kernel function" > > > > kexec_image_info(), and remove extra messages from machine_kexec(). > > > > Make dtb_mem, always available, if CONFIG_KEXEC_FILE is not configured > > dtb_mem is set to zero anyway. > > This is unrelated cleanup, please do it as an earlier patch to make it clearer what you > are changing here. Ok. > > (I'm not convinced you need to cache va<->pa) > > > > diff --git a/arch/arm64/include/asm/kexec.h b/arch/arm64/include/asm/kexec.h > > index 12a561a54128..d15ca1ca1e83 100644 > > --- a/arch/arm64/include/asm/kexec.h > > +++ b/arch/arm64/include/asm/kexec.h > > @@ -90,14 +90,15 @@ static inline void crash_prepare_suspend(void) {} > > static inline void crash_post_resume(void) {} > > #endif > > > > -#ifdef CONFIG_KEXEC_FILE > > #define ARCH_HAS_KIMAGE_ARCH > > > > struct kimage_arch { > > void *dtb; > > unsigned long dtb_mem; > > > + unsigned long kern_reloc; > > This is cache-ing the physical address of an all-architectures value from struct kimage, > in the arch specific part of struct kiamge. Why? > (You must have the struct kimage on hand to access this thing at all!) Because, currently only one physical page is used in order to do reboot: control_code_page; this is where arm64_relocate_new_kernel is copied. So, PA of control_code_page is used as a handler. But with MMU enabled kexec reboot, a number of pages are allocated for reboot purposes, they contain page table, arm64_relocate_new_kernel, and el2 vector table. This is why we need handlers. control_code_page is simply one of the pages that can contains any of the kexec reboot specific data. > If its supposed to be a physical address, please use phys_addr_t. Done that, also changed dtb_mem to phys_addr_t. > > > }; > > > > diff --git a/arch/arm64/kernel/machine_kexec.c b/arch/arm64/kernel/machine_kexec.c > > index 0df8493624e0..9b41da50e6f7 100644 > > --- a/arch/arm64/kernel/machine_kexec.c > > +++ b/arch/arm64/kernel/machine_kexec.c > > @@ -42,6 +42,7 @@ static void _kexec_image_info(const char *func, int line, > > pr_debug(" start: %lx\n", kimage->start); > > pr_debug(" head: %lx\n", kimage->head); > > pr_debug(" nr_segments: %lu\n", kimage->nr_segments); > > + pr_debug(" kern_reloc: %pa\n", &kimage->arch.kern_reloc); > > > > for (i = 0; i < kimage->nr_segments; i++) { > > pr_debug(" segment[%lu]: %016lx - %016lx, 0x%lx bytes, %lu pages\n", > > @@ -58,6 +59,19 @@ void machine_kexec_cleanup(struct kimage *kimage) > > /* Empty routine needed to avoid build errors. */ > > } > > > > +int machine_kexec_post_load(struct kimage *kimage) > > +{ > > + unsigned long kern_reloc; > > + > > + kern_reloc = page_to_phys(kimage->control_code_page); > > kern_reloc should be phys_addr_t. Ok > > > > + memcpy(__va(kern_reloc), arm64_relocate_new_kernel, > > + arm64_relocate_new_kernel_size); > > + kimage->arch.kern_reloc = kern_reloc; > > > Please move the cache maintenance in here too. This will save us doing it late during > kdump. This will also group the mmu-on changes together. OK, but I think we should do it in a separate patch. I assume you mean this code to be moved to load time: 177 /* Flush the reboot_code_buffer in preparation for its execution. */ 178 __flush_dcache_area(reboot_code_buffer, arm64_relocate_new_kernel_size); 179 180 /* 181 * Although we've killed off the secondary CPUs, we don't update 182 * the online mask if we're handling a crash kernel and consequently 183 * need to avoid flush_icache_range(), which will attempt to IPI 184 * the offline CPUs. Therefore, we must use the __* variant here. 185 */ 186 __flush_icache_range((uintptr_t)reboot_code_buffer, 187 arm64_relocate_new_kernel_size); Is it safe to do? We do not know what CPU is going to be executing kexec reboot for us at the load time. Pasha
diff --git a/arch/arm64/include/asm/kexec.h b/arch/arm64/include/asm/kexec.h index 12a561a54128..d15ca1ca1e83 100644 --- a/arch/arm64/include/asm/kexec.h +++ b/arch/arm64/include/asm/kexec.h @@ -90,14 +90,15 @@ static inline void crash_prepare_suspend(void) {} static inline void crash_post_resume(void) {} #endif -#ifdef CONFIG_KEXEC_FILE #define ARCH_HAS_KIMAGE_ARCH struct kimage_arch { void *dtb; unsigned long dtb_mem; + unsigned long kern_reloc; }; +#ifdef CONFIG_KEXEC_FILE extern const struct kexec_file_ops kexec_image_ops; struct kimage; diff --git a/arch/arm64/kernel/machine_kexec.c b/arch/arm64/kernel/machine_kexec.c index 0df8493624e0..9b41da50e6f7 100644 --- a/arch/arm64/kernel/machine_kexec.c +++ b/arch/arm64/kernel/machine_kexec.c @@ -42,6 +42,7 @@ static void _kexec_image_info(const char *func, int line, pr_debug(" start: %lx\n", kimage->start); pr_debug(" head: %lx\n", kimage->head); pr_debug(" nr_segments: %lu\n", kimage->nr_segments); + pr_debug(" kern_reloc: %pa\n", &kimage->arch.kern_reloc); for (i = 0; i < kimage->nr_segments; i++) { pr_debug(" segment[%lu]: %016lx - %016lx, 0x%lx bytes, %lu pages\n", @@ -58,6 +59,19 @@ void machine_kexec_cleanup(struct kimage *kimage) /* Empty routine needed to avoid build errors. */ } +int machine_kexec_post_load(struct kimage *kimage) +{ + unsigned long kern_reloc; + + kern_reloc = page_to_phys(kimage->control_code_page); + memcpy(__va(kern_reloc), arm64_relocate_new_kernel, + arm64_relocate_new_kernel_size); + kimage->arch.kern_reloc = kern_reloc; + + kexec_image_info(kimage); + return 0; +} + /** * machine_kexec_prepare - Prepare for a kexec reboot. * @@ -67,8 +81,6 @@ void machine_kexec_cleanup(struct kimage *kimage) */ int machine_kexec_prepare(struct kimage *kimage) { - kexec_image_info(kimage); - if (kimage->type != KEXEC_TYPE_CRASH && cpus_are_stuck_in_kernel()) { pr_err("Can't kexec: CPUs are stuck in the kernel.\n"); return -EBUSY; @@ -143,8 +155,7 @@ static void kexec_segment_flush(const struct kimage *kimage) */ void machine_kexec(struct kimage *kimage) { - phys_addr_t reboot_code_buffer_phys; - void *reboot_code_buffer; + void *reboot_code_buffer = phys_to_virt(kimage->arch.kern_reloc); bool in_kexec_crash = (kimage == kexec_crash_image); bool stuck_cpus = cpus_are_stuck_in_kernel(); @@ -155,30 +166,8 @@ void machine_kexec(struct kimage *kimage) WARN(in_kexec_crash && (stuck_cpus || smp_crash_stop_failed()), "Some CPUs may be stale, kdump will be unreliable.\n"); - reboot_code_buffer_phys = page_to_phys(kimage->control_code_page); - reboot_code_buffer = phys_to_virt(reboot_code_buffer_phys); - kexec_image_info(kimage); - pr_debug("%s:%d: control_code_page: %p\n", __func__, __LINE__, - kimage->control_code_page); - pr_debug("%s:%d: reboot_code_buffer_phys: %pa\n", __func__, __LINE__, - &reboot_code_buffer_phys); - pr_debug("%s:%d: reboot_code_buffer: %p\n", __func__, __LINE__, - reboot_code_buffer); - pr_debug("%s:%d: relocate_new_kernel: %p\n", __func__, __LINE__, - arm64_relocate_new_kernel); - pr_debug("%s:%d: relocate_new_kernel_size: 0x%lx(%lu) bytes\n", - __func__, __LINE__, arm64_relocate_new_kernel_size, - arm64_relocate_new_kernel_size); - - /* - * Copy arm64_relocate_new_kernel to the reboot_code_buffer for use - * after the kernel is shut down. - */ - memcpy(reboot_code_buffer, arm64_relocate_new_kernel, - arm64_relocate_new_kernel_size); - /* Flush the reboot_code_buffer in preparation for its execution. */ __flush_dcache_area(reboot_code_buffer, arm64_relocate_new_kernel_size); @@ -214,12 +203,8 @@ void machine_kexec(struct kimage *kimage) * userspace (kexec-tools). * In kexec_file case, the kernel starts directly without purgatory. */ - cpu_soft_restart(reboot_code_buffer_phys, kimage->head, kimage->start, -#ifdef CONFIG_KEXEC_FILE - kimage->arch.dtb_mem); -#else - 0); -#endif + cpu_soft_restart(kimage->arch.kern_reloc, kimage->head, kimage->start, + kimage->arch.dtb_mem); BUG(); /* Should never get here. */ }
Currently, kernel relocation function is configured in machine_kexec() at the time of kexec reboot by using control_code_page. This operation, however, is more logical to be done during kexec_load, and thus remove from reboot time. Move, setup of this function to newly added machine_kexec_post_load(). In addition, do some cleanup: add infor about reloction function to kexec_image_info(), and remove extra messages from machine_kexec(). Make dtb_mem, always available, if CONFIG_KEXEC_FILE is not configured dtb_mem is set to zero anyway. Signed-off-by: Pavel Tatashin <pasha.tatashin@soleen.com> --- arch/arm64/include/asm/kexec.h | 3 +- arch/arm64/kernel/machine_kexec.c | 49 +++++++++++-------------------- 2 files changed, 19 insertions(+), 33 deletions(-)