linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Baoquan He <bhe@redhat.com>
To: Lianbo Jiang <lijiang@redhat.com>
Cc: linux-kernel@vger.kernel.org, kexec@lists.infradead.org,
	x86@kernel.org, tglx@linutronix.de, mingo@redhat.com,
	bp@alien8.de, akpm@linux-foundation.org, dyoung@redhat.com
Subject: Re: [PATCH 1/2 v5] x86/kexec_file: add e820 entry in case e820 type string matches to io resource name
Date: Wed, 7 Nov 2018 13:17:45 +0800	[thread overview]
Message-ID: <20181107051745.GP27491@MiWiFi-R3L-srv> (raw)
In-Reply-To: <20181107050019.6663-2-lijiang@redhat.com>

Hi Lianbo,

On 11/07/18 at 01:00pm, Lianbo Jiang wrote:
> kdump uses walk_iomem_res_desc() to iterate io resources, then adds matched
> desc to e820 table for kdump kernel.
> 
> But IORES_DESC_NONE resource type includes several different e820 types,

As we discussed privately, better say more clearly what is
"IORES_DESC_NONE resource type includes several different e820 types".

You can tell:

When convert e820 type into iores descriptors, several e820 types are
all converted to IORES_DESC_NONE in function e820_type_to_iores_desc().

Here may no need to list below code if you have told clearly what
happened. Otherwise could you tell what it mean about the sentence:
"IORES_DESC_NONE resource type includes several different e820 types".

They are diffeent types, why one type includes another type, how does it
include? Why it matters when one type includes another type? Why do you
care?

static unsigned long __init e820_type_to_iores_desc(struct e820_entry *entry)                                                                     
{
        switch (entry->type) {
        case E820_TYPE_ACPI:            return IORES_DESC_ACPI_TABLES;
        case E820_TYPE_NVS:             return IORES_DESC_ACPI_NV_STORAGE;
        case E820_TYPE_PMEM:            return IORES_DESC_PERSISTENT_MEMORY;
        case E820_TYPE_PRAM:            return IORES_DESC_PERSISTENT_MEMORY_LEGACY;
        case E820_TYPE_RESERVED_KERN:   /* Fall-through: */
        case E820_TYPE_RAM:             /* Fall-through: */
        case E820_TYPE_UNUSABLE:        /* Fall-through: */
        case E820_TYPE_RESERVED:        /* Fall-through: */
        default:                        return IORES_DESC_NONE;
        }
}

> we need add exact e820 type to kdump kernel e820 table, thus it also needs

Why we need add exact e820 type to kdump kernel e820 table? just because 
"IORES_DESC_NONE resource type includes several different e820 types"?
Why we didn't need it before, why need it now?

Add it for what? Fix a bug/add a feature?

> an extra checking in memmap_entry_callback() to match the e820 type and
> resource name.

This paragraph only tells how you fix it. "IORES_DESC_NONE resource type
includes several different e820 types" only tells a fact. So what's
impacted by the including? Why i'ts impacted?

> 
> Suggested-by: Dave Young <dyoung@redhat.com>
> Signed-off-by: Lianbo Jiang <lijiang@redhat.com>
> ---
>  arch/x86/include/asm/e820/api.h | 2 ++
>  arch/x86/kernel/crash.c         | 6 +++++-
>  arch/x86/kernel/e820.c          | 2 +-
>  kernel/resource.c               | 1 +
>  4 files changed, 9 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/x86/include/asm/e820/api.h b/arch/x86/include/asm/e820/api.h
> index 62be73b23d5c..6d5451b36e80 100644
> --- a/arch/x86/include/asm/e820/api.h
> +++ b/arch/x86/include/asm/e820/api.h
> @@ -42,6 +42,8 @@ extern void e820__register_nosave_regions(unsigned long limit_pfn);
>  
>  extern int  e820__get_entry_type(u64 start, u64 end);
>  
> +extern const char *e820_type_to_string(struct e820_entry *entry);
> +
>  /*
>   * Returns true iff the specified range [start,end) is completely contained inside
>   * the ISA region.
> diff --git a/arch/x86/kernel/crash.c b/arch/x86/kernel/crash.c
> index f631a3f15587..ae724a6e0a5f 100644
> --- a/arch/x86/kernel/crash.c
> +++ b/arch/x86/kernel/crash.c
> @@ -37,6 +37,7 @@
>  #include <asm/reboot.h>
>  #include <asm/virtext.h>
>  #include <asm/intel_pt.h>
> +#include <asm/e820/api.h>
>  
>  /* Used while preparing memory map entries for second kernel */
>  struct crash_memmap_data {
> @@ -314,11 +315,14 @@ static int memmap_entry_callback(struct resource *res, void *arg)
>  	struct crash_memmap_data *cmd = arg;
>  	struct boot_params *params = cmd->params;
>  	struct e820_entry ei;
> +	const char *name;
>  
>  	ei.addr = res->start;
>  	ei.size = resource_size(res);
>  	ei.type = cmd->type;
> -	add_e820_entry(params, &ei);
> +	name = e820_type_to_string(&ei);
> +	if (res->name && !strcmp(name, res->name))
> +		add_e820_entry(params, &ei);
>  
>  	return 0;
>  }
> diff --git a/arch/x86/kernel/e820.c b/arch/x86/kernel/e820.c
> index 50895c2f937d..4c1fe4f8db1e 100644
> --- a/arch/x86/kernel/e820.c
> +++ b/arch/x86/kernel/e820.c
> @@ -1011,7 +1011,7 @@ void __init e820__finish_early_params(void)
>  	}
>  }
>  
> -static const char *__init e820_type_to_string(struct e820_entry *entry)
> +const char *e820_type_to_string(struct e820_entry *entry)
>  {
>  	switch (entry->type) {
>  	case E820_TYPE_RESERVED_KERN:	/* Fall-through: */
> diff --git a/kernel/resource.c b/kernel/resource.c
> index b3a3a1fc499e..6285a6b4de6c 100644
> --- a/kernel/resource.c
> +++ b/kernel/resource.c
> @@ -366,6 +366,7 @@ static int find_next_iomem_res(resource_size_t start, resource_size_t end,
>  	res->end = min(end, p->end);
>  	res->flags = p->flags;
>  	res->desc = p->desc;
> +	res->name = p->name;
>  	return 0;
>  }
>  
> -- 
> 2.17.1
> 

  reply	other threads:[~2018-11-07  5:17 UTC|newest]

Thread overview: 9+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-11-07  5:00 [PATCH 0/2 v5] add reserved e820 ranges to the kdump kernel e820 table Lianbo Jiang
2018-11-07  5:00 ` [PATCH 1/2 v5] x86/kexec_file: add e820 entry in case e820 type string matches to io resource name Lianbo Jiang
2018-11-07  5:17   ` Baoquan He [this message]
2018-11-07  5:00 ` [PATCH 2/2 v5] x86/kexec_file: add reserved e820 ranges to kdump kernel e820 table Lianbo Jiang
2018-11-07  5:23   ` Baoquan He
2018-11-07  9:10     ` lijiang
2018-11-07  9:35       ` Dave Young
2018-11-09  6:51       ` Baoquan He
2018-11-09  7:13         ` lijiang

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20181107051745.GP27491@MiWiFi-R3L-srv \
    --to=bhe@redhat.com \
    --cc=akpm@linux-foundation.org \
    --cc=bp@alien8.de \
    --cc=dyoung@redhat.com \
    --cc=kexec@lists.infradead.org \
    --cc=lijiang@redhat.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=mingo@redhat.com \
    --cc=tglx@linutronix.de \
    --cc=x86@kernel.org \
    --subject='Re: [PATCH 1/2 v5] x86/kexec_file: add e820 entry in case e820 type string matches to io resource name' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).