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;
Message-ID: <7d0697ee-d6e8-dad1-ca77-f2e8104b0b0f@linux.ibm.com>
Date:   Tue, 4 Oct 2022 14:40:30 +0530
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.2.1
Subject: Re: [PATCH v12 7/7] x86/crash: Add x86 crash hotplug support
Content-Language: en-US
To:     Eric DeVolder <eric.devolder@oracle.com>,
        Borislav Petkov <bp@alien8.de>
Cc:     linux-kernel@vger.kernel.org, x86@kernel.org,
        kexec@lists.infradead.org, ebiederm@xmission.com,
        dyoung@redhat.com, bhe@redhat.com, vgoyal@redhat.com,
        tglx@linutronix.de, mingo@redhat.com, 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,
        konrad.wilk@oracle.com, boris.ostrovsky@oracle.com
References: <20220909210509.6286-1-eric.devolder@oracle.com>
 <20220909210509.6286-8-eric.devolder@oracle.com>
 <Yx7XEcXZ8PwwQW95@nazgul.tnic>
 <cb343eef-46be-2d67-b93a-84c75be86325@oracle.com> <YzRxPAoN+XmOfJzV@zn.tnic>
 <fd08c13d-a917-4cd6-85ec-267e0fe74c41@oracle.com>
From:   Sourabh Jain <sourabhjain@linux.ibm.com>
In-Reply-To: <fd08c13d-a917-4cd6-85ec-267e0fe74c41@oracle.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit
MIME-Version: 1.0
Precedence: bulk


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?

Thanks,
Sourabh Jain