Messages in this thread | | | Date | Fri, 7 Oct 2022 15:00:52 -0500 | Subject | Re: [PATCH v12 7/7] x86/crash: Add x86 crash hotplug support | From | Eric DeVolder <> |
| |
On 10/4/22 04:10, Sourabh Jain wrote: > > On 30/09/22 21:06, Eric DeVolder wrote: >> >> >> On 9/28/22 11:07, Borislav Petkov wrote: >>> On Tue, Sep 13, 2022 at 02:12:31PM -0500, Eric DeVolder wrote: >>>> This topic was discussed previously https://lkml.org/lkml/2022/3/3/372. >>> >>> Please do not use lkml.org to refer to lkml messages. We have a >>> perfectly fine archival system at lore.kernel.org. You simply do >>> >>> https://lore.kernel.org/r/<Message-ID> >>> >>> when you want to point to a previous mail. >> >> ok, thanks for pointing that out to me. >>> >>>> David points out that terminology is tricky here due to differing behaviors. >>>> And perhaps that is your point in asking for guidance text. It can be >>>> complicated >>> >>> Which means you need an explanation how to use this even more. >>> >>> And why is CONFIG_CRASH_MAX_MEMORY_RANGES even a Kconfig item and not >>> something you discover from the hardware? >> >> No, is the short answer. >> >>> >>> Your help text talks about System RAM entries in /proc/iomem which means >>> that those entries are present somewhere in the kernel and you can read >>> them out and do the proper calculations dynamically instead of doing the >>> static CONFIG_NR_CPUS_DEFAULT + CONFIG_CRASH_MAX_MEMORY_RANGES thing. >> >> The intent is to compute the max size buffer needed to contain a maximum populated elfcorehdr, >> which is primarily based on the number of CPUs and memory regions. Thus far I (and others >> involved) have not found a kernel method to determine the maximum number of memory regions >> possible (if you are aware of one, please let me know!). Thus CONFIG_CRASH_MAX_MEMORY_RANGES was >> born (rather borrowed from kexec-tools). >> >> So no dynamic computation is possible, yet. >> >>> >>>> , but it all comes down to System RAM entries. >>>> >>>> I could perhaps offer an overly simplified example such that for 1GiB block >>>> size, for example, the CRASH_MAX_MEMORY_RANGES of 32768 would allow for 32TiB >>>> of memory? >>> >>> Yes, and stick it somewhere in Documentation/admin-guide/kdump/ and >>> refer to it in that help text so that people can find it and read how to >>> use your new option. >>> >> ok >> >>>> The kbuf.bufsz value is obtained via a call to prepare_elf_headers(); I can >>>> not initialize it at its declaration. >>> >>> Sorry, I meant this: >>> >>> diff --git a/arch/x86/kernel/crash.c b/arch/x86/kernel/crash.c >>> index 8fc7d678ac72..ee6fd9f1b2b9 100644 >>> --- a/arch/x86/kernel/crash.c >>> +++ b/arch/x86/kernel/crash.c >>> @@ -395,8 +395,9 @@ int crash_load_segments(struct kimage *image) >>> if (ret) >>> return ret; >>> - image->elf_headers = kbuf.buffer; >>> - image->elf_headers_sz = kbuf.bufsz; >>> + image->elf_headers = kbuf.buffer; >>> + image->elf_headers_sz = kbuf.bufsz; >>> + kbuf.memsz = kbuf.bufsz; >>> #if defined(CONFIG_HOTPLUG_CPU) || defined(CONFIG_MEMORY_HOTPLUG) >>> /* Ensure elfcorehdr segment large enough for hotplug changes */ >>> @@ -407,9 +408,8 @@ int crash_load_segments(struct kimage *image) >>> image->elf_headers_sz = kbuf.memsz; >>> image->elfcorehdr_index = image->nr_segments; >>> image->elfcorehdr_index_valid = true; >>> -#else >>> - kbuf.memsz = kbuf.bufsz; >>> #endif >>> + >>> kbuf.buf_align = ELF_CORE_HEADER_ALIGN; >>> kbuf.mem = KEXEC_BUF_MEM_UNKNOWN; >>> ret = kexec_add_buffer(&kbuf); >>> >> ok >> >>>> I'm at a loss as to what to do differently here. You've raised this issue >>>> before and I went back and looked at the suggestions then and I don't see >>>> how that applies to this situation. How is this situation different than the >>>> #ifdef CONFIG_KEXEC_FILE that immediately preceeds it? >>> >>> See the diff at the end. I'm not saying this is how you should do it >>> but it should give you a better idea. The logic being, the functions >>> in the .c file don't really need ifdeffery around them - you're adding >>> 1-2 functions and crash.c is not that big - so they can be built in >>> unconditionally. You'd need the ifdeffery *in the header only* when >>> crash.c is not being built. >> ok; I've overlooked that scenario. >>> >>> But I've done it with ifdeffery in the .c file now because yes, the >>> kexec code is a minefield of ifdeffery. Hell, there's ifdeffery even in >>> the headers for structs. Ifdeffery you don't really need. Someone should >>> clean that up and simplify this immensely. >> >> ok >> >>> >>>> Currently there is a concurrent effort for PPC support by Sourabh >>>> Jain, and in that effort arch_map_crash_pages() is using __va(paddr). >>> >>> Why? >>> >>>> I do not know the nuances between kmap_local_page() and __va() to >>>> answer the question. >>> >>> kmap_local_page() is a generic interface and it should work on any arch. >>> >>> And it is documented even: >>> >>> $ git grep kmap_local_page Documentation/ >>> >>>> If kmap_local_page() works for all archs, then I'm happy to drop these >>>> arch_ variants and use it directly. >>> >>> Yes, pls do. >> >> I'll check with Sourabh to see if PPC can work with kmap_local_page(). > I think kmap_local_page do support on PowerPC. But can you explain why we need this > function here, aren't the reserve memory already available to use?
On x86, attempts to access the elfcorehdr without mapping it did not work (resulted in a fault).
Let me know if using kmap_local_page() in place of __va() in arch_map_crash_pages(). If it does, then I can eliminate arch_un/map_crash_pages() and use kmap_local_page() directly.
Thanks, eric > > Thanks, > Sourabh Jain
| |