Re: [PATCH 1/2 v6] x86/kexec_file: add e820 entry in case e820 type string matches to io resource name

From: lijiang <lijiang@redhat.com>
To: Borislav Petkov <bp@alien8.de>, Dave Young <dyoung@redhat.com>
Cc: Bjorn Helgaas <helgaas@kernel.org>,
	linux-kernel@vger.kernel.org, kexec@lists.infradead.org,
	x86@kernel.org, tglx@linutronix.de, mingo@redhat.com,
	akpm@linux-foundation.org, bhe@redhat.com
Subject: Re: [PATCH 1/2 v6] x86/kexec_file: add e820 entry in case e820 type string matches to io resource name
Date: Tue, 20 Nov 2018 11:37:26 +0800	[thread overview]
Message-ID: <a1c58bb8-390b-e249-599f-c973d575e1ca@redhat.com> (raw)
In-Reply-To: <20181119102812.GA14688@zn.tnic>

在 2018年11月19日 18:28, Borislav Petkov 写道:
> On Mon, Nov 19, 2018 at 05:55:15PM +0800, Dave Young wrote:
>> Another thing is it is not worth to get the exact info, the 1st kernel
>> reserved part is just fine to be reserved as well in 2nd kernel, no 
>> side effects.  Actually there might be some obscure use cases we
>> do not find which rely those reserved memory ranges so it is better to
>> have.
> 
> That makes sense as an argument. The cleaner thing would be to figure
> out only *which* ranges we're going to need but that is probably harder
> than simply exporting what the first kernel sees. But why we're doing
> it, needs to be in the commit message so that it is clear when bug
> hunting later.
> 
> ...
> 
>> The basic problem is that this device is in PCI segment 1 and
>> the kernel PCI probing cannot find it without all the e820 i/o
>> reservations being present in the e820 table. And the crash kernel
>> does not have those reservations because the kexec command does not
>> pass i/o reservation via the memmap= command line option. (This
>> problem does not show up for other vendors, as SGI is apparently the
>> only one using extended PCI. The lookup of devices in PCI segment 0
>> actually fails for everyone, but devices in segment 0 are then found
>> by some legacy lookup method.) The workaround for this is to fix kexec
>> to pass i/o reserved areas to the crash kernel.
> 
> Yap, this is the *why* I'm looking for. Lianbo, in your next submission,
> please add Dave's explanations to your commit messages.
> Ok. Thank you, Dave and Boris. I will add Dave's explanations to patch log.

BTW: Boris has mentioned the solution which adds the new descriptor 'IORES_DESC_RESERVED'.

Which solution do you prefer? Add the new I/O resource descriptor 'IORES_DESC_RESERVED'(patch v7)
or exactly comparing a string(patch v6)?

These two solutions are good to me. 

Thanks.
Lianbo
> If it says "we need to do X" in the commit message, without a reason
> given *why* we need to, then there's no way for us to know *why* we did
> it, when looking at this months from now. And we absolutely need the
> *why* when staring at the code and fixing the next bug/issue.
> 
> Thx.
> 