Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Fri, 26 Aug 2022 12:35:18 +0800
From:   Baoquan He <bhe@redhat.com>
To:     Eric DeVolder <eric.devolder@oracle.com>
Cc:     linux-kernel@vger.kernel.org, x86@kernel.org,
        kexec@lists.infradead.org, ebiederm@xmission.com,
        dyoung@redhat.com, vgoyal@redhat.com, tglx@linutronix.de,
        mingo@redhat.com, bp@alien8.de, dave.hansen@linux.intel.com,
        hpa@zytor.com, nramas@linux.microsoft.com, thomas.lendacky@amd.com,
        robh@kernel.org, efault@gmx.de, rppt@kernel.org, david@redhat.com,
        sourabhjain@linux.ibm.com, konrad.wilk@oracle.com,
        boris.ostrovsky@oracle.com
Subject: Re: [PATCH v10 8/8] x86/crash: Add x86 crash hotplug support
Message-ID: <YwhNhrlmFH6ps3BN@MiWiFi-R3L-srv>
References: <20220721181747.1640-1-eric.devolder@oracle.com>
 <20220721181747.1640-9-eric.devolder@oracle.com>
 <Yvbxgvk+5HqEl7J2@MiWiFi-R3L-srv>
 <0395a745-edbb-275a-f37f-c6e799388da1@oracle.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <0395a745-edbb-275a-f37f-c6e799388da1@oracle.com>
Precedence: bulk

On 08/16/22 at 10:23am, Eric DeVolder wrote:
> 
> 
> On 8/12/22 19:34, Baoquan He wrote:
> > On 07/21/22 at 02:17pm, Eric DeVolder wrote:
> > ...snip....
> > > diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
> > > index e58798f636d4..bb59596c8bea 100644
> > > --- a/arch/x86/Kconfig
> > > +++ b/arch/x86/Kconfig
> > > @@ -2065,6 +2065,17 @@ config CRASH_DUMP
> > >   	  (CONFIG_RELOCATABLE=y).
> > >   	  For more details see Documentation/admin-guide/kdump/kdump.rst
> > > +config CRASH_MAX_MEMORY_RANGES
> > > +	depends on CRASH_DUMP && KEXEC_FILE && (HOTPLUG_CPU || MEMORY_HOTPLUG)
> > > +	int
> > > +	default 32768
> > 
> > Do we need to enforce the value with page align and minimal size? I
> 
> Are you asking about the value CRASH_MAX_MEMORY_RANGES? This value represents
> the maximum number of memory ranges, and there Elf64_Phdrs, that we need to
> allow for elfcorehdr memory. So I'm not sure what the concern for alignment
> is. I suppose we could also institute a minimum size for this value, say 1024.
> 
> > checked crash_load_segments() in arch/x86/kernel/crash.c, it does the
> > page size aligning in kexec_add_buffer(). And in
> > load_crashdump_segments() of
> > kexec-tools/kexec/arch/i386/crashdump-x86.c, it creates elfcorehdr at
> > below code, the align is 1024, and in generic add_buffer()
> > implementation, it enforces the memsz page aligned, and changes the
> > passed align as page alignment.
> > 
> > 
> > 	elfcorehdr = add_buffer(info, tmp, bufsz, memsz, align, min_base,
> >                                                          max_addr, -1);
> > 
> > Maybe we should at least mention this in the help text to notice people.
> 
> Unfortunately I do not yet understand the concern being raised.

Oops, never mind, I misunderstood CRASH_MAX_MEMORY_RANGES. Thought it's
the range vlaue of buffer containing elfcorehdr, I must be dizzy when
reading this part.

> 
> > 
...snip...
> > > diff --git a/arch/x86/kernel/crash.c b/arch/x86/kernel/crash.c
> > > index 9ceb93c176a6..55dda4fcde6e 100644
> > > --- a/arch/x86/kernel/crash.c
> > > +++ b/arch/x86/kernel/crash.c
> > > @@ -25,6 +25,7 @@
> > >   #include <linux/slab.h>
> > >   #include <linux/vmalloc.h>
> > >   #include <linux/memblock.h>
> > > +#include <linux/highmem.h>
> > >   #include <asm/processor.h>
> > >   #include <asm/hardirq.h>
> > > @@ -397,7 +398,17 @@ int crash_load_segments(struct kimage *image)
> > >   	image->elf_headers = kbuf.buffer;
> > >   	image->elf_headers_sz = kbuf.bufsz;
> > > +#if defined(CONFIG_HOTPLUG_CPU) || defined(CONFIG_MEMORY_HOTPLUG)
> > > +	/* Ensure elfcorehdr segment large enough for hotplug changes */
> > > +	kbuf.memsz = (CONFIG_NR_CPUS_DEFAULT + CONFIG_CRASH_MAX_MEMORY_RANGES) * sizeof(Elf64_Phdr);
> > 
> > Do we need to break the line to 80 chars?
> 
> Sure, I will do so.
> 
> > 
> > > +	/* For marking as usable to crash kernel */
> > > +	image->elf_headers_sz = kbuf.memsz;
> > 
> > Do we need this code comment?
> 
> Well, it did take me a while to figure this particular item out in order for all
> this code to work right (else the crash kernel would fail at boot time). So I
> think it best to keep this comment.
> 
> > 
> > > +	/* Record the index of the elfcorehdr segment */
> > > +	image->elfcorehdr_index = image->nr_segments;
> > 
> > And this place?
> 
> Not necessarily needed, but I've found it useful.
> 
> > 
> > > +	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);
> > > @@ -412,3 +423,107 @@ int crash_load_segments(struct kimage *image)
> > >   	return ret;
> > >   }
> > >   #endif /* CONFIG_KEXEC_FILE */
> > > +
> > > +#if defined(CONFIG_HOTPLUG_CPU) || defined(CONFIG_MEMORY_HOTPLUG)
> > > +void *arch_map_crash_pages(unsigned long paddr, unsigned long size)
> > > +{
> > > +	/*
> > > +	 * NOTE: The addresses and sizes passed to this routine have
> > > +	 * already been fully aligned on page boundaries. There is no
> > > +	 * need for massaging the address or size.
> > > +	 */
> > 
> > Can we move the code comment above function interface?
> 
> Yes
> 
> > 
> > > +	void *ptr = NULL;
> > > +
> > > +	/* NOTE: requires arch_kexec_[un]protect_crashkres() for write access */
> > 
> > Do we need this code comment? On ARCH where proctionion is made, we
> > surely need to the protect/unprotect.
> 
> I will remove this; I've mentioned this in handle_hotplug_event() where these
> protect/unprotect functions are called.
> 
> > 
> > > +	if (size > 0) {
> > > +		struct page *page = pfn_to_page(paddr >> PAGE_SHIFT);
> > > +
> > > +		ptr = kmap_local_page(page);
> > > +	}
> > > +
> > > +	return ptr;
> > > +}
> > > +
> > > +void arch_unmap_crash_pages(void **ptr)
> > > +{
> > > +	if (ptr) {
> > > +		if (*ptr)
> > > +			kunmap_local(*ptr);
> > > +		*ptr = NULL;
> > > +	}
> > > +}
> > > +
> > > +/**
> > > + * arch_crash_handle_hotplug_event() - Handle hotplug elfcorehdr changes
> > > + * @image: the active struct kimage
> > > + * @hp_action: the hot un/plug action being handled
> > > + * @cpu: when KEXEC_CRASH_HP_ADD/REMOVE_CPU, the cpu affected
> > > + *
> > > + * To accurately reflect hot un/plug changes, the elfcorehdr (which
> > > + * is passed to the crash kernel via the elfcorehdr= parameter)
> > > + * must be updated with the new list of CPUs and memories. The new
> > > + * elfcorehdr is prepared in a kernel buffer, and then it is
> > > + * written on top of the existing/old elfcorehdr.
> > > + *
> > > + * For hotplug changes to elfcorehdr to work, two conditions are
> > > + * needed:
> > > + * First, the segment containing the elfcorehdr must be large enough
> > > + * to permit a growing number of resources. See the
> > > + * CONFIG_CRASH_MAX_MEMORY_RANGES description.
> > > + * Second, purgatory must explicitly exclude the elfcorehdr from the
> > > + * list of segments it checks (since the elfcorehdr changes and thus
> > > + * would require an update to purgatory itself to update the digest).
> > 
> > Isn't this generic concept to crash hotplug? Should we move it out to
> > some generic place?
> 
> Yes, so I will relocate this.
> 
> > 
> > > + *
> > > + */
> > > +void arch_crash_handle_hotplug_event(struct kimage *image,
> > > +	unsigned int hp_action, unsigned int cpu)
> > 
> > The passed in 'cpu' is not used at all, what is it added for? I didn't
> > see explanation about it.
> 
> Well its not used for x86, but as I recall, Sourabh Jain needed it for the PowerPC handler.

Then better mention this in log or add code comment, otherwise confusion
could be caused.