crashkernel=x@y option may fail to reserve the required memory region if
KASLR puts kernel into the region. To avoid this uncertainty, making KASLR
skip the required region.
Signed-off-by: Pingfan Liu <[email protected]>
Cc: Thomas Gleixner <[email protected]>
Cc: Ingo Molnar <[email protected]>
Cc: Borislav Petkov <[email protected]>
Cc: "H. Peter Anvin" <[email protected]>
Cc: Baoquan He <[email protected]>
Cc: Will Deacon <[email protected]>
Cc: Nicolas Pitre <[email protected]>
Cc: Pingfan Liu <[email protected]>
Cc: Chao Fan <[email protected]>
Cc: "Kirill A. Shutemov" <[email protected]>
Cc: Ard Biesheuvel <[email protected]>
Cc: [email protected]
---
v1 -> v2: fix some trival format
arch/x86/boot/compressed/kaslr.c | 26 ++++++++++++++++++++++++--
1 file changed, 24 insertions(+), 2 deletions(-)
diff --git a/arch/x86/boot/compressed/kaslr.c b/arch/x86/boot/compressed/kaslr.c
index 9ed9709..e185318 100644
--- a/arch/x86/boot/compressed/kaslr.c
+++ b/arch/x86/boot/compressed/kaslr.c
@@ -109,6 +109,7 @@ enum mem_avoid_index {
MEM_AVOID_BOOTPARAMS,
MEM_AVOID_MEMMAP_BEGIN,
MEM_AVOID_MEMMAP_END = MEM_AVOID_MEMMAP_BEGIN + MAX_MEMMAP_REGIONS - 1,
+ MEM_AVOID_CRASHKERNEL,
MEM_AVOID_MAX,
};
@@ -240,6 +241,25 @@ static void parse_gb_huge_pages(char *param, char *val)
}
}
+/* parse crashkernel=x@y option */
+static void mem_avoid_crashkernel_simple(char *option)
+{
+ unsigned long long crash_size, crash_base;
+ char *cur = option;
+
+ crash_size = memparse(option, &cur);
+ if (option == cur)
+ return;
+
+ if (*cur == '@') {
+ option = cur + 1;
+ crash_base = memparse(option, &cur);
+ if (option == cur)
+ return;
+ mem_avoid[MEM_AVOID_CRASHKERNEL].start = crash_base;
+ mem_avoid[MEM_AVOID_CRASHKERNEL].size = crash_size;
+ }
+}
static void handle_mem_options(void)
{
@@ -250,7 +270,7 @@ static void handle_mem_options(void)
u64 mem_size;
if (!strstr(args, "memmap=") && !strstr(args, "mem=") &&
- !strstr(args, "hugepages"))
+ !strstr(args, "hugepages") && !strstr(args, "crashkernel="))
return;
tmp_cmdline = malloc(len + 1);
@@ -286,6 +306,8 @@ static void handle_mem_options(void)
goto out;
mem_limit = mem_size;
+ } else if (strstr(param, "crashkernel")) {
+ mem_avoid_crashkernel_simple(val);
}
}
@@ -414,7 +436,7 @@ static void mem_avoid_init(unsigned long input, unsigned long input_size,
/* We don't need to set a mapping for setup_data. */
- /* Mark the memmap regions we need to avoid */
+ /* Mark the regions we need to avoid */
handle_mem_options();
#ifdef CONFIG_X86_VERBOSE_BOOTUP
--
2.7.4
Please change subject as:
"x86/boot/KASLR: skip the specified crashkernel region"
Don't see why reserved is needed here.
On 03/13/19 at 12:19pm, Pingfan Liu wrote:
> crashkernel=x@y option may fail to reserve the required memory region if
> KASLR puts kernel into the region. To avoid this uncertainty, making KASLR
> skip the required region.
>
> Signed-off-by: Pingfan Liu <[email protected]>
> Cc: Thomas Gleixner <[email protected]>
> Cc: Ingo Molnar <[email protected]>
> Cc: Borislav Petkov <[email protected]>
> Cc: "H. Peter Anvin" <[email protected]>
> Cc: Baoquan He <[email protected]>
> Cc: Will Deacon <[email protected]>
> Cc: Nicolas Pitre <[email protected]>
> Cc: Pingfan Liu <[email protected]>
> Cc: Chao Fan <[email protected]>
> Cc: "Kirill A. Shutemov" <[email protected]>
> Cc: Ard Biesheuvel <[email protected]>
> Cc: [email protected]
> ---
> v1 -> v2: fix some trival format
>
> arch/x86/boot/compressed/kaslr.c | 26 ++++++++++++++++++++++++--
> 1 file changed, 24 insertions(+), 2 deletions(-)
>
> diff --git a/arch/x86/boot/compressed/kaslr.c b/arch/x86/boot/compressed/kaslr.c
> index 9ed9709..e185318 100644
> --- a/arch/x86/boot/compressed/kaslr.c
> +++ b/arch/x86/boot/compressed/kaslr.c
> @@ -109,6 +109,7 @@ enum mem_avoid_index {
> MEM_AVOID_BOOTPARAMS,
> MEM_AVOID_MEMMAP_BEGIN,
> MEM_AVOID_MEMMAP_END = MEM_AVOID_MEMMAP_BEGIN + MAX_MEMMAP_REGIONS - 1,
> + MEM_AVOID_CRASHKERNEL,
> MEM_AVOID_MAX,
> };
>
> @@ -240,6 +241,25 @@ static void parse_gb_huge_pages(char *param, char *val)
> }
> }
>
> +/* parse crashkernel=x@y option */
> +static void mem_avoid_crashkernel_simple(char *option)
Chao ever mentioned this, I want to ask again, why does it has to be
xxx_simple()?
Except of these, patch looks good to me. It's a nice catch, and only
need a simple fix based on the current code.
Thanks
Baoquan
> +{
> + unsigned long long crash_size, crash_base;
> + char *cur = option;
> +
> + crash_size = memparse(option, &cur);
> + if (option == cur)
> + return;
> +
> + if (*cur == '@') {
> + option = cur + 1;
> + crash_base = memparse(option, &cur);
> + if (option == cur)
> + return;
> + mem_avoid[MEM_AVOID_CRASHKERNEL].start = crash_base;
> + mem_avoid[MEM_AVOID_CRASHKERNEL].size = crash_size;
> + }
> +}
>
> static void handle_mem_options(void)
> {
> @@ -250,7 +270,7 @@ static void handle_mem_options(void)
> u64 mem_size;
>
> if (!strstr(args, "memmap=") && !strstr(args, "mem=") &&
> - !strstr(args, "hugepages"))
> + !strstr(args, "hugepages") && !strstr(args, "crashkernel="))
> return;
>
> tmp_cmdline = malloc(len + 1);
> @@ -286,6 +306,8 @@ static void handle_mem_options(void)
> goto out;
>
> mem_limit = mem_size;
> + } else if (strstr(param, "crashkernel")) {
> + mem_avoid_crashkernel_simple(val);
> }
> }
>
> @@ -414,7 +436,7 @@ static void mem_avoid_init(unsigned long input, unsigned long input_size,
>
> /* We don't need to set a mapping for setup_data. */
>
> - /* Mark the memmap regions we need to avoid */
> + /* Mark the regions we need to avoid */
> handle_mem_options();
>
> #ifdef CONFIG_X86_VERBOSE_BOOTUP
> --
> 2.7.4
>
On Wed, Mar 13, 2019 at 12:19:31PM +0800, Pingfan Liu wrote:
Hi Pingfan,
I wonder your test method and test case.
Do you test it in the Qemu guest or real machine.
Thanks,
Chao Fan
>crashkernel=x@y option may fail to reserve the required memory region if
>KASLR puts kernel into the region. To avoid this uncertainty, making KASLR
>skip the required region.
>
>Signed-off-by: Pingfan Liu <[email protected]>
>Cc: Thomas Gleixner <[email protected]>
>Cc: Ingo Molnar <[email protected]>
>Cc: Borislav Petkov <[email protected]>
>Cc: "H. Peter Anvin" <[email protected]>
>Cc: Baoquan He <[email protected]>
>Cc: Will Deacon <[email protected]>
>Cc: Nicolas Pitre <[email protected]>
>Cc: Pingfan Liu <[email protected]>
>Cc: Chao Fan <[email protected]>
>Cc: "Kirill A. Shutemov" <[email protected]>
>Cc: Ard Biesheuvel <[email protected]>
>Cc: [email protected]
>---
>v1 -> v2: fix some trival format
>
> arch/x86/boot/compressed/kaslr.c | 26 ++++++++++++++++++++++++--
> 1 file changed, 24 insertions(+), 2 deletions(-)
>
>diff --git a/arch/x86/boot/compressed/kaslr.c b/arch/x86/boot/compressed/kaslr.c
>index 9ed9709..e185318 100644
>--- a/arch/x86/boot/compressed/kaslr.c
>+++ b/arch/x86/boot/compressed/kaslr.c
>@@ -109,6 +109,7 @@ enum mem_avoid_index {
> MEM_AVOID_BOOTPARAMS,
> MEM_AVOID_MEMMAP_BEGIN,
> MEM_AVOID_MEMMAP_END = MEM_AVOID_MEMMAP_BEGIN + MAX_MEMMAP_REGIONS - 1,
>+ MEM_AVOID_CRASHKERNEL,
> MEM_AVOID_MAX,
> };
>
>@@ -240,6 +241,25 @@ static void parse_gb_huge_pages(char *param, char *val)
> }
> }
>
>+/* parse crashkernel=x@y option */
>+static void mem_avoid_crashkernel_simple(char *option)
>+{
>+ unsigned long long crash_size, crash_base;
>+ char *cur = option;
>+
>+ crash_size = memparse(option, &cur);
>+ if (option == cur)
>+ return;
>+
>+ if (*cur == '@') {
>+ option = cur + 1;
>+ crash_base = memparse(option, &cur);
>+ if (option == cur)
>+ return;
>+ mem_avoid[MEM_AVOID_CRASHKERNEL].start = crash_base;
>+ mem_avoid[MEM_AVOID_CRASHKERNEL].size = crash_size;
>+ }
>+}
>
> static void handle_mem_options(void)
> {
>@@ -250,7 +270,7 @@ static void handle_mem_options(void)
> u64 mem_size;
>
> if (!strstr(args, "memmap=") && !strstr(args, "mem=") &&
>- !strstr(args, "hugepages"))
>+ !strstr(args, "hugepages") && !strstr(args, "crashkernel="))
> return;
>
> tmp_cmdline = malloc(len + 1);
>@@ -286,6 +306,8 @@ static void handle_mem_options(void)
> goto out;
>
> mem_limit = mem_size;
>+ } else if (strstr(param, "crashkernel")) {
>+ mem_avoid_crashkernel_simple(val);
> }
> }
>
>@@ -414,7 +436,7 @@ static void mem_avoid_init(unsigned long input, unsigned long input_size,
>
> /* We don't need to set a mapping for setup_data. */
>
>- /* Mark the memmap regions we need to avoid */
>+ /* Mark the regions we need to avoid */
> handle_mem_options();
>
> #ifdef CONFIG_X86_VERBOSE_BOOTUP
>--
>2.7.4
>
>
>
On Wed, Mar 13, 2019 at 12:19:31PM +0800, Pingfan Liu wrote:
I tested it in Qemu test with 12G memory, and set crashkernel=6G@6G.
Without this PATCH, it successed to reserve memory just 4 times(total
10 times).
With this PATCH, it successed to reserve memory 15 times(total 15
times).
So I think if you post new version, you can add:
Tested-by: Chao Fan <[email protected]>
Thanks,
Chao Fan
>crashkernel=x@y option may fail to reserve the required memory region if
>KASLR puts kernel into the region. To avoid this uncertainty, making KASLR
>skip the required region.
>
>Signed-off-by: Pingfan Liu <[email protected]>
>Cc: Thomas Gleixner <[email protected]>
>Cc: Ingo Molnar <[email protected]>
>Cc: Borislav Petkov <[email protected]>
>Cc: "H. Peter Anvin" <[email protected]>
>Cc: Baoquan He <[email protected]>
>Cc: Will Deacon <[email protected]>
>Cc: Nicolas Pitre <[email protected]>
>Cc: Pingfan Liu <[email protected]>
>Cc: Chao Fan <[email protected]>
>Cc: "Kirill A. Shutemov" <[email protected]>
>Cc: Ard Biesheuvel <[email protected]>
>Cc: [email protected]
>---
>v1 -> v2: fix some trival format
>
> arch/x86/boot/compressed/kaslr.c | 26 ++++++++++++++++++++++++--
> 1 file changed, 24 insertions(+), 2 deletions(-)
>
>diff --git a/arch/x86/boot/compressed/kaslr.c b/arch/x86/boot/compressed/kaslr.c
>index 9ed9709..e185318 100644
>--- a/arch/x86/boot/compressed/kaslr.c
>+++ b/arch/x86/boot/compressed/kaslr.c
>@@ -109,6 +109,7 @@ enum mem_avoid_index {
> MEM_AVOID_BOOTPARAMS,
> MEM_AVOID_MEMMAP_BEGIN,
> MEM_AVOID_MEMMAP_END = MEM_AVOID_MEMMAP_BEGIN + MAX_MEMMAP_REGIONS - 1,
>+ MEM_AVOID_CRASHKERNEL,
> MEM_AVOID_MAX,
> };
>
>@@ -240,6 +241,25 @@ static void parse_gb_huge_pages(char *param, char *val)
> }
> }
>
>+/* parse crashkernel=x@y option */
>+static void mem_avoid_crashkernel_simple(char *option)
>+{
>+ unsigned long long crash_size, crash_base;
>+ char *cur = option;
>+
>+ crash_size = memparse(option, &cur);
>+ if (option == cur)
>+ return;
>+
>+ if (*cur == '@') {
>+ option = cur + 1;
>+ crash_base = memparse(option, &cur);
>+ if (option == cur)
>+ return;
>+ mem_avoid[MEM_AVOID_CRASHKERNEL].start = crash_base;
>+ mem_avoid[MEM_AVOID_CRASHKERNEL].size = crash_size;
>+ }
>+}
>
> static void handle_mem_options(void)
> {
>@@ -250,7 +270,7 @@ static void handle_mem_options(void)
> u64 mem_size;
>
> if (!strstr(args, "memmap=") && !strstr(args, "mem=") &&
>- !strstr(args, "hugepages"))
>+ !strstr(args, "hugepages") && !strstr(args, "crashkernel="))
> return;
>
> tmp_cmdline = malloc(len + 1);
>@@ -286,6 +306,8 @@ static void handle_mem_options(void)
> goto out;
>
> mem_limit = mem_size;
>+ } else if (strstr(param, "crashkernel")) {
>+ mem_avoid_crashkernel_simple(val);
> }
> }
>
>@@ -414,7 +436,7 @@ static void mem_avoid_init(unsigned long input, unsigned long input_size,
>
> /* We don't need to set a mapping for setup_data. */
>
>- /* Mark the memmap regions we need to avoid */
>+ /* Mark the regions we need to avoid */
> handle_mem_options();
>
> #ifdef CONFIG_X86_VERBOSE_BOOTUP
>--
>2.7.4
>
>
>
On Wed, Mar 20, 2019 at 8:25 AM Baoquan He <[email protected]> wrote:
>
> Please change subject as:
>
> "x86/boot/KASLR: skip the specified crashkernel region"
>
OK.
> Don't see why reserved is needed here.
>
> On 03/13/19 at 12:19pm, Pingfan Liu wrote:
> > crashkernel=x@y option may fail to reserve the required memory region if
> > KASLR puts kernel into the region. To avoid this uncertainty, making KASLR
> > skip the required region.
> >
> > Signed-off-by: Pingfan Liu <[email protected]>
> > Cc: Thomas Gleixner <[email protected]>
> > Cc: Ingo Molnar <[email protected]>
> > Cc: Borislav Petkov <[email protected]>
> > Cc: "H. Peter Anvin" <[email protected]>
> > Cc: Baoquan He <[email protected]>
> > Cc: Will Deacon <[email protected]>
> > Cc: Nicolas Pitre <[email protected]>
> > Cc: Pingfan Liu <[email protected]>
> > Cc: Chao Fan <[email protected]>
> > Cc: "Kirill A. Shutemov" <[email protected]>
> > Cc: Ard Biesheuvel <[email protected]>
> > Cc: [email protected]
> > ---
> > v1 -> v2: fix some trival format
> >
> > arch/x86/boot/compressed/kaslr.c | 26 ++++++++++++++++++++++++--
> > 1 file changed, 24 insertions(+), 2 deletions(-)
> >
> > diff --git a/arch/x86/boot/compressed/kaslr.c b/arch/x86/boot/compressed/kaslr.c
> > index 9ed9709..e185318 100644
> > --- a/arch/x86/boot/compressed/kaslr.c
> > +++ b/arch/x86/boot/compressed/kaslr.c
> > @@ -109,6 +109,7 @@ enum mem_avoid_index {
> > MEM_AVOID_BOOTPARAMS,
> > MEM_AVOID_MEMMAP_BEGIN,
> > MEM_AVOID_MEMMAP_END = MEM_AVOID_MEMMAP_BEGIN + MAX_MEMMAP_REGIONS - 1,
> > + MEM_AVOID_CRASHKERNEL,
> > MEM_AVOID_MAX,
> > };
> >
> > @@ -240,6 +241,25 @@ static void parse_gb_huge_pages(char *param, char *val)
> > }
> > }
> >
> > +/* parse crashkernel=x@y option */
> > +static void mem_avoid_crashkernel_simple(char *option)
>
> Chao ever mentioned this, I want to ask again, why does it has to be
> xxx_simple()?
>
Seems that I had replied Chao's question in another email. The naming
follows the function parse_crashkernel_simple(), as the notes above
the definition
/*
* That function parses "simple" (old) crashkernel command lines like
*
* crashkernel=size[@offset]
*
* It returns 0 on success and -EINVAL on failure.
*/
static int __init parse_crashkernel_simple(char *cmdline,
Do you have alternative suggestion?
> Except of these, patch looks good to me. It's a nice catch, and only
> need a simple fix based on the current code.
>
Thank you for the kindly review.
Regards,
Pingfan
> Thanks
> Baoquan
>
> > +{
> > + unsigned long long crash_size, crash_base;
> > + char *cur = option;
> > +
> > + crash_size = memparse(option, &cur);
> > + if (option == cur)
> > + return;
> > +
> > + if (*cur == '@') {
> > + option = cur + 1;
> > + crash_base = memparse(option, &cur);
> > + if (option == cur)
> > + return;
> > + mem_avoid[MEM_AVOID_CRASHKERNEL].start = crash_base;
> > + mem_avoid[MEM_AVOID_CRASHKERNEL].size = crash_size;
> > + }
> > +}
> >
> > static void handle_mem_options(void)
> > {
> > @@ -250,7 +270,7 @@ static void handle_mem_options(void)
> > u64 mem_size;
> >
> > if (!strstr(args, "memmap=") && !strstr(args, "mem=") &&
> > - !strstr(args, "hugepages"))
> > + !strstr(args, "hugepages") && !strstr(args, "crashkernel="))
> > return;
> >
> > tmp_cmdline = malloc(len + 1);
> > @@ -286,6 +306,8 @@ static void handle_mem_options(void)
> > goto out;
> >
> > mem_limit = mem_size;
> > + } else if (strstr(param, "crashkernel")) {
> > + mem_avoid_crashkernel_simple(val);
> > }
> > }
> >
> > @@ -414,7 +436,7 @@ static void mem_avoid_init(unsigned long input, unsigned long input_size,
> >
> > /* We don't need to set a mapping for setup_data. */
> >
> > - /* Mark the memmap regions we need to avoid */
> > + /* Mark the regions we need to avoid */
> > handle_mem_options();
> >
> > #ifdef CONFIG_X86_VERBOSE_BOOTUP
> > --
> > 2.7.4
> >
On Thu, Mar 21, 2019 at 2:38 PM Chao Fan <[email protected]> wrote:
>
> On Wed, Mar 13, 2019 at 12:19:31PM +0800, Pingfan Liu wrote:
>
> I tested it in Qemu test with 12G memory, and set crashkernel=6G@6G.
> Without this PATCH, it successed to reserve memory just 4 times(total
> 10 times).
> With this PATCH, it successed to reserve memory 15 times(total 15
> times).
>
> So I think if you post new version, you can add:
>
> Tested-by: Chao Fan <[email protected]>
>
Appreciate for your testing. I had done some test on a real machine
with a private patch to narrow down the KASLR range. I think your test
method is more simple and I will add the tested-by you.
Regards,
Pingfan
> Thanks,
> Chao Fan
>
> >crashkernel=x@y option may fail to reserve the required memory region if
> >KASLR puts kernel into the region. To avoid this uncertainty, making KASLR
> >skip the required region.
> >
> >Signed-off-by: Pingfan Liu <[email protected]>
> >Cc: Thomas Gleixner <[email protected]>
> >Cc: Ingo Molnar <[email protected]>
> >Cc: Borislav Petkov <[email protected]>
> >Cc: "H. Peter Anvin" <[email protected]>
> >Cc: Baoquan He <[email protected]>
> >Cc: Will Deacon <[email protected]>
> >Cc: Nicolas Pitre <[email protected]>
> >Cc: Pingfan Liu <[email protected]>
> >Cc: Chao Fan <[email protected]>
> >Cc: "Kirill A. Shutemov" <[email protected]>
> >Cc: Ard Biesheuvel <[email protected]>
> >Cc: [email protected]
> >---
> >v1 -> v2: fix some trival format
> >
> > arch/x86/boot/compressed/kaslr.c | 26 ++++++++++++++++++++++++--
> > 1 file changed, 24 insertions(+), 2 deletions(-)
> >
> >diff --git a/arch/x86/boot/compressed/kaslr.c b/arch/x86/boot/compressed/kaslr.c
> >index 9ed9709..e185318 100644
> >--- a/arch/x86/boot/compressed/kaslr.c
> >+++ b/arch/x86/boot/compressed/kaslr.c
> >@@ -109,6 +109,7 @@ enum mem_avoid_index {
> > MEM_AVOID_BOOTPARAMS,
> > MEM_AVOID_MEMMAP_BEGIN,
> > MEM_AVOID_MEMMAP_END = MEM_AVOID_MEMMAP_BEGIN + MAX_MEMMAP_REGIONS - 1,
> >+ MEM_AVOID_CRASHKERNEL,
> > MEM_AVOID_MAX,
> > };
> >
> >@@ -240,6 +241,25 @@ static void parse_gb_huge_pages(char *param, char *val)
> > }
> > }
> >
> >+/* parse crashkernel=x@y option */
> >+static void mem_avoid_crashkernel_simple(char *option)
> >+{
> >+ unsigned long long crash_size, crash_base;
> >+ char *cur = option;
> >+
> >+ crash_size = memparse(option, &cur);
> >+ if (option == cur)
> >+ return;
> >+
> >+ if (*cur == '@') {
> >+ option = cur + 1;
> >+ crash_base = memparse(option, &cur);
> >+ if (option == cur)
> >+ return;
> >+ mem_avoid[MEM_AVOID_CRASHKERNEL].start = crash_base;
> >+ mem_avoid[MEM_AVOID_CRASHKERNEL].size = crash_size;
> >+ }
> >+}
> >
> > static void handle_mem_options(void)
> > {
> >@@ -250,7 +270,7 @@ static void handle_mem_options(void)
> > u64 mem_size;
> >
> > if (!strstr(args, "memmap=") && !strstr(args, "mem=") &&
> >- !strstr(args, "hugepages"))
> >+ !strstr(args, "hugepages") && !strstr(args, "crashkernel="))
> > return;
> >
> > tmp_cmdline = malloc(len + 1);
> >@@ -286,6 +306,8 @@ static void handle_mem_options(void)
> > goto out;
> >
> > mem_limit = mem_size;
> >+ } else if (strstr(param, "crashkernel")) {
> >+ mem_avoid_crashkernel_simple(val);
> > }
> > }
> >
> >@@ -414,7 +436,7 @@ static void mem_avoid_init(unsigned long input, unsigned long input_size,
> >
> > /* We don't need to set a mapping for setup_data. */
> >
> >- /* Mark the memmap regions we need to avoid */
> >+ /* Mark the regions we need to avoid */
> > handle_mem_options();
> >
> > #ifdef CONFIG_X86_VERBOSE_BOOTUP
> >--
> >2.7.4
> >
> >
> >
>
>
On 03/22/19 at 03:43pm, Pingfan Liu wrote:
> > > +/* parse crashkernel=x@y option */
> > > +static void mem_avoid_crashkernel_simple(char *option)
> >
> > Chao ever mentioned this, I want to ask again, why does it has to be
> > xxx_simple()?
> >
> Seems that I had replied Chao's question in another email. The naming
> follows the function parse_crashkernel_simple(), as the notes above
~~~~~~~~~~~~~~~~~~~~~~~~
Sorry, I don't get. typo?
> the definition
> /*
> * That function parses "simple" (old) crashkernel command lines like
> *
> * crashkernel=size[@offset]
Hmm, should only crashkernel=size@offset be cared? crashkernel=size will
auto finding a place to reserve, and that is after KASLR.
> *
> * It returns 0 on success and -EINVAL on failure.
> */
> static int __init parse_crashkernel_simple(char *cmdline,
>
> Do you have alternative suggestion?
>
> > Except of these, patch looks good to me. It's a nice catch, and only
> > need a simple fix based on the current code.
> >
> Thank you for the kindly review.
>
> Regards,
> Pingfan
>
> > Thanks
> > Baoquan
> >
> > > +{
> > > + unsigned long long crash_size, crash_base;
> > > + char *cur = option;
> > > +
> > > + crash_size = memparse(option, &cur);
> > > + if (option == cur)
> > > + return;
> > > +
> > > + if (*cur == '@') {
> > > + option = cur + 1;
> > > + crash_base = memparse(option, &cur);
> > > + if (option == cur)
> > > + return;
> > > + mem_avoid[MEM_AVOID_CRASHKERNEL].start = crash_base;
> > > + mem_avoid[MEM_AVOID_CRASHKERNEL].size = crash_size;
> > > + }
> > > +}
> > >
> > > static void handle_mem_options(void)
> > > {
> > > @@ -250,7 +270,7 @@ static void handle_mem_options(void)
> > > u64 mem_size;
> > >
> > > if (!strstr(args, "memmap=") && !strstr(args, "mem=") &&
> > > - !strstr(args, "hugepages"))
> > > + !strstr(args, "hugepages") && !strstr(args, "crashkernel="))
> > > return;
> > >
> > > tmp_cmdline = malloc(len + 1);
> > > @@ -286,6 +306,8 @@ static void handle_mem_options(void)
> > > goto out;
> > >
> > > mem_limit = mem_size;
> > > + } else if (strstr(param, "crashkernel")) {
> > > + mem_avoid_crashkernel_simple(val);
> > > }
> > > }
> > >
> > > @@ -414,7 +436,7 @@ static void mem_avoid_init(unsigned long input, unsigned long input_size,
> > >
> > > /* We don't need to set a mapping for setup_data. */
> > >
> > > - /* Mark the memmap regions we need to avoid */
> > > + /* Mark the regions we need to avoid */
> > > handle_mem_options();
> > >
> > > #ifdef CONFIG_X86_VERBOSE_BOOTUP
> > > --
> > > 2.7.4
> > >
On 03/22/19 at 03:52pm, Baoquan He wrote:
> On 03/22/19 at 03:43pm, Pingfan Liu wrote:
> > > > +/* parse crashkernel=x@y option */
> > > > +static void mem_avoid_crashkernel_simple(char *option)
> > >
> > > Chao ever mentioned this, I want to ask again, why does it has to be
> > > xxx_simple()?
> > >
> > Seems that I had replied Chao's question in another email. The naming
> > follows the function parse_crashkernel_simple(), as the notes above
> ~~~~~~~~~~~~~~~~~~~~~~~~
>
> Sorry, I don't get. typo?
OK, I misunderstood it. We do have parse_crashkernel_simple() to handle
crashkernel=size[@offset] case, to differente with other complicated
cases, like crashkernel=size,[high|low],
Then I am fine with this naming. Soryy about the noise.
By the way, do you think if we should take care of this case:
crashkernel=<range1>:<size1>[,<range2>:<size2>,...][@offset]
It can also specify @offset. Not sure if it's too complicated, you may
have a investigation.
These two cases have dependency on your crashkernel=X bug fix patch.
The current code only allow crashkernel= to reserve under 896MB. I
noticed Boris has agreed on the solution. Maybe you can repost a new
version based on the discussion.
http://lkml.kernel.org/r/[email protected]
[PATCHv7] x86/kdump: bugfix, make the behavior of crashkernel=X consistent with kaslr
Thanks
Baoquan
>
> > the definition
> > /*
> > * That function parses "simple" (old) crashkernel command lines like
> > *
> > * crashkernel=size[@offset]
>
> Hmm, should only crashkernel=size@offset be cared? crashkernel=size will
> auto finding a place to reserve, and that is after KASLR.
>
> > *
> > * It returns 0 on success and -EINVAL on failure.
> > */
> > static int __init parse_crashkernel_simple(char *cmdline,
> >
> > Do you have alternative suggestion?
> >
> > > Except of these, patch looks good to me. It's a nice catch, and only
> > > need a simple fix based on the current code.
> > >
> > Thank you for the kindly review.
> >
> > Regards,
> > Pingfan
> >
> > > Thanks
> > > Baoquan
> > >
> > > > +{
> > > > + unsigned long long crash_size, crash_base;
> > > > + char *cur = option;
> > > > +
> > > > + crash_size = memparse(option, &cur);
> > > > + if (option == cur)
> > > > + return;
> > > > +
> > > > + if (*cur == '@') {
> > > > + option = cur + 1;
> > > > + crash_base = memparse(option, &cur);
> > > > + if (option == cur)
> > > > + return;
> > > > + mem_avoid[MEM_AVOID_CRASHKERNEL].start = crash_base;
> > > > + mem_avoid[MEM_AVOID_CRASHKERNEL].size = crash_size;
> > > > + }
> > > > +}
> > > >
> > > > static void handle_mem_options(void)
> > > > {
> > > > @@ -250,7 +270,7 @@ static void handle_mem_options(void)
> > > > u64 mem_size;
> > > >
> > > > if (!strstr(args, "memmap=") && !strstr(args, "mem=") &&
> > > > - !strstr(args, "hugepages"))
> > > > + !strstr(args, "hugepages") && !strstr(args, "crashkernel="))
> > > > return;
> > > >
> > > > tmp_cmdline = malloc(len + 1);
> > > > @@ -286,6 +306,8 @@ static void handle_mem_options(void)
> > > > goto out;
> > > >
> > > > mem_limit = mem_size;
> > > > + } else if (strstr(param, "crashkernel")) {
> > > > + mem_avoid_crashkernel_simple(val);
> > > > }
> > > > }
> > > >
> > > > @@ -414,7 +436,7 @@ static void mem_avoid_init(unsigned long input, unsigned long input_size,
> > > >
> > > > /* We don't need to set a mapping for setup_data. */
> > > >
> > > > - /* Mark the memmap regions we need to avoid */
> > > > + /* Mark the regions we need to avoid */
> > > > handle_mem_options();
> > > >
> > > > #ifdef CONFIG_X86_VERBOSE_BOOTUP
> > > > --
> > > > 2.7.4
> > > >
On Fri, Mar 22, 2019 at 4:34 PM Baoquan He <[email protected]> wrote:
>
> On 03/22/19 at 03:52pm, Baoquan He wrote:
> > On 03/22/19 at 03:43pm, Pingfan Liu wrote:
> > > > > +/* parse crashkernel=x@y option */
> > > > > +static void mem_avoid_crashkernel_simple(char *option)
> > > >
> > > > Chao ever mentioned this, I want to ask again, why does it has to be
> > > > xxx_simple()?
> > > >
> > > Seems that I had replied Chao's question in another email. The naming
> > > follows the function parse_crashkernel_simple(), as the notes above
> > ~~~~~~~~~~~~~~~~~~~~~~~~
> >
> > Sorry, I don't get. typo?
>
> OK, I misunderstood it. We do have parse_crashkernel_simple() to handle
> crashkernel=size[@offset] case, to differente with other complicated
> cases, like crashkernel=size,[high|low],
>
> Then I am fine with this naming. Soryy about the noise.
>
> By the way, do you think if we should take care of this case:
> crashkernel=<range1>:<size1>[,<range2>:<size2>,...][@offset]
>
> It can also specify @offset. Not sure if it's too complicated, you may
> have a investigation.
>
OK, I will try it.
> These two cases have dependency on your crashkernel=X bug fix patch.
No, crashkernel=x@y should have no dependcy on crashkernel=X, the
later one relies on memblock searching.
> The current code only allow crashkernel= to reserve under 896MB. I
> noticed Boris has agreed on the solution. Maybe you can repost a new
> version based on the discussion.
I will sync with Dave to see whether he will post the new version.
Thank you for kindly review.
Regards,
Pingfan
>
> http://lkml.kernel.org/r/[email protected]
> [PATCHv7] x86/kdump: bugfix, make the behavior of crashkernel=X consistent with kaslr
>
> Thanks
> Baoquan
>
> >
> > > the definition
> > > /*
> > > * That function parses "simple" (old) crashkernel command lines like
> > > *
> > > * crashkernel=size[@offset]
> >
> > Hmm, should only crashkernel=size@offset be cared? crashkernel=size will
> > auto finding a place to reserve, and that is after KASLR.
> >
> > > *
> > > * It returns 0 on success and -EINVAL on failure.
> > > */
> > > static int __init parse_crashkernel_simple(char *cmdline,
> > >
> > > Do you have alternative suggestion?
> > >
> > > > Except of these, patch looks good to me. It's a nice catch, and only
> > > > need a simple fix based on the current code.
> > > >
> > > Thank you for the kindly review.
> > >
> > > Regards,
> > > Pingfan
> > >
> > > > Thanks
> > > > Baoquan
> > > >
> > > > > +{
> > > > > + unsigned long long crash_size, crash_base;
> > > > > + char *cur = option;
> > > > > +
> > > > > + crash_size = memparse(option, &cur);
> > > > > + if (option == cur)
> > > > > + return;
> > > > > +
> > > > > + if (*cur == '@') {
> > > > > + option = cur + 1;
> > > > > + crash_base = memparse(option, &cur);
> > > > > + if (option == cur)
> > > > > + return;
> > > > > + mem_avoid[MEM_AVOID_CRASHKERNEL].start = crash_base;
> > > > > + mem_avoid[MEM_AVOID_CRASHKERNEL].size = crash_size;
> > > > > + }
> > > > > +}
> > > > >
> > > > > static void handle_mem_options(void)
> > > > > {
> > > > > @@ -250,7 +270,7 @@ static void handle_mem_options(void)
> > > > > u64 mem_size;
> > > > >
> > > > > if (!strstr(args, "memmap=") && !strstr(args, "mem=") &&
> > > > > - !strstr(args, "hugepages"))
> > > > > + !strstr(args, "hugepages") && !strstr(args, "crashkernel="))
> > > > > return;
> > > > >
> > > > > tmp_cmdline = malloc(len + 1);
> > > > > @@ -286,6 +306,8 @@ static void handle_mem_options(void)
> > > > > goto out;
> > > > >
> > > > > mem_limit = mem_size;
> > > > > + } else if (strstr(param, "crashkernel")) {
> > > > > + mem_avoid_crashkernel_simple(val);
> > > > > }
> > > > > }
> > > > >
> > > > > @@ -414,7 +436,7 @@ static void mem_avoid_init(unsigned long input, unsigned long input_size,
> > > > >
> > > > > /* We don't need to set a mapping for setup_data. */
> > > > >
> > > > > - /* Mark the memmap regions we need to avoid */
> > > > > + /* Mark the regions we need to avoid */
> > > > > handle_mem_options();
> > > > >
> > > > > #ifdef CONFIG_X86_VERBOSE_BOOTUP
> > > > > --
> > > > > 2.7.4
> > > > >
On Fri, Mar 22, 2019 at 4:34 PM Baoquan He <[email protected]> wrote:
>
> On 03/22/19 at 03:52pm, Baoquan He wrote:
> > On 03/22/19 at 03:43pm, Pingfan Liu wrote:
> > > > > +/* parse crashkernel=x@y option */
> > > > > +static void mem_avoid_crashkernel_simple(char *option)
> > > >
> > > > Chao ever mentioned this, I want to ask again, why does it has to be
> > > > xxx_simple()?
> > > >
> > > Seems that I had replied Chao's question in another email. The naming
> > > follows the function parse_crashkernel_simple(), as the notes above
> > ~~~~~~~~~~~~~~~~~~~~~~~~
> >
> > Sorry, I don't get. typo?
>
> OK, I misunderstood it. We do have parse_crashkernel_simple() to handle
> crashkernel=size[@offset] case, to differente with other complicated
> cases, like crashkernel=size,[high|low],
>
> Then I am fine with this naming. Soryy about the noise.
>
> By the way, do you think if we should take care of this case:
> crashkernel=<range1>:<size1>[,<range2>:<size2>,...][@offset]
>
> It can also specify @offset. Not sure if it's too complicated, you may
> have a investigation.
>
In this case, kernel should get the total memory size info. So
process_e820_entries() or process_efi_entries() should be called
twice. One before handle_mem_options(), so crashkernel can evaluate
the reserved size. It is doable, and what is your opinion about the
extra complicate?
Thanks,
Pingfan
[...]
On 03/29/19 at 01:45pm, Pingfan Liu wrote:
> On Fri, Mar 22, 2019 at 4:34 PM Baoquan He <[email protected]> wrote:
> >
> > On 03/22/19 at 03:52pm, Baoquan He wrote:
> > > On 03/22/19 at 03:43pm, Pingfan Liu wrote:
> > > > > > +/* parse crashkernel=x@y option */
> > > > > > +static void mem_avoid_crashkernel_simple(char *option)
> > > > >
> > > > > Chao ever mentioned this, I want to ask again, why does it has to be
> > > > > xxx_simple()?
> > > > >
> > > > Seems that I had replied Chao's question in another email. The naming
> > > > follows the function parse_crashkernel_simple(), as the notes above
> > > ~~~~~~~~~~~~~~~~~~~~~~~~
> > >
> > > Sorry, I don't get. typo?
> >
> > OK, I misunderstood it. We do have parse_crashkernel_simple() to handle
> > crashkernel=size[@offset] case, to differente with other complicated
> > cases, like crashkernel=size,[high|low],
> >
> > Then I am fine with this naming. Soryy about the noise.
> >
> > By the way, do you think if we should take care of this case:
> > crashkernel=<range1>:<size1>[,<range2>:<size2>,...][@offset]
> >
> > It can also specify @offset. Not sure if it's too complicated, you may
> > have a investigation.
> >
> In this case, kernel should get the total memory size info. So
> process_e820_entries() or process_efi_entries() should be called
> twice. One before handle_mem_options(), so crashkernel can evaluate
> the reserved size. It is doable, and what is your opinion about the
You mean calling process_e820_entries to calculate the RAM size in
system? I may not do like that, please check what __find_max_addr() is
doing. Did I get it?
On Fri, Mar 29, 2019 at 2:27 PM Baoquan He <[email protected]> wrote:
>
> On 03/29/19 at 01:45pm, Pingfan Liu wrote:
> > On Fri, Mar 22, 2019 at 4:34 PM Baoquan He <[email protected]> wrote:
> > >
> > > On 03/22/19 at 03:52pm, Baoquan He wrote:
> > > > On 03/22/19 at 03:43pm, Pingfan Liu wrote:
> > > > > > > +/* parse crashkernel=x@y option */
> > > > > > > +static void mem_avoid_crashkernel_simple(char *option)
> > > > > >
> > > > > > Chao ever mentioned this, I want to ask again, why does it has to be
> > > > > > xxx_simple()?
> > > > > >
> > > > > Seems that I had replied Chao's question in another email. The naming
> > > > > follows the function parse_crashkernel_simple(), as the notes above
> > > > ~~~~~~~~~~~~~~~~~~~~~~~~
> > > >
> > > > Sorry, I don't get. typo?
> > >
> > > OK, I misunderstood it. We do have parse_crashkernel_simple() to handle
> > > crashkernel=size[@offset] case, to differente with other complicated
> > > cases, like crashkernel=size,[high|low],
> > >
> > > Then I am fine with this naming. Soryy about the noise.
> > >
> > > By the way, do you think if we should take care of this case:
> > > crashkernel=<range1>:<size1>[,<range2>:<size2>,...][@offset]
> > >
> > > It can also specify @offset. Not sure if it's too complicated, you may
> > > have a investigation.
> > >
> > In this case, kernel should get the total memory size info. So
> > process_e820_entries() or process_efi_entries() should be called
> > twice. One before handle_mem_options(), so crashkernel can evaluate
> > the reserved size. It is doable, and what is your opinion about the
>
> You mean calling process_e820_entries to calculate the RAM size in
> system? I may not do like that, please check what __find_max_addr() is
> doing. Did I get it?
Yes, you got my meaning. But __find_max_addr() relies on the info, fed
by e820__memblock_setup(). It also involves the iteration of all e820
entries to get the max address. No essential difference, right?
On 03/29/19 at 03:25pm, Pingfan Liu wrote:
> On Fri, Mar 29, 2019 at 2:27 PM Baoquan He <[email protected]> wrote:
> >
> > On 03/29/19 at 01:45pm, Pingfan Liu wrote:
> > > On Fri, Mar 22, 2019 at 4:34 PM Baoquan He <[email protected]> wrote:
> > > >
> > > > On 03/22/19 at 03:52pm, Baoquan He wrote:
> > > > > On 03/22/19 at 03:43pm, Pingfan Liu wrote:
> > > > > > > > +/* parse crashkernel=x@y option */
> > > > > > > > +static void mem_avoid_crashkernel_simple(char *option)
> > > > > > >
> > > > > > > Chao ever mentioned this, I want to ask again, why does it has to be
> > > > > > > xxx_simple()?
> > > > > > >
> > > > > > Seems that I had replied Chao's question in another email. The naming
> > > > > > follows the function parse_crashkernel_simple(), as the notes above
> > > > > ~~~~~~~~~~~~~~~~~~~~~~~~
> > > > >
> > > > > Sorry, I don't get. typo?
> > > >
> > > > OK, I misunderstood it. We do have parse_crashkernel_simple() to handle
> > > > crashkernel=size[@offset] case, to differente with other complicated
> > > > cases, like crashkernel=size,[high|low],
> > > >
> > > > Then I am fine with this naming. Soryy about the noise.
> > > >
> > > > By the way, do you think if we should take care of this case:
> > > > crashkernel=<range1>:<size1>[,<range2>:<size2>,...][@offset]
> > > >
> > > > It can also specify @offset. Not sure if it's too complicated, you may
> > > > have a investigation.
> > > >
> > > In this case, kernel should get the total memory size info. So
> > > process_e820_entries() or process_efi_entries() should be called
> > > twice. One before handle_mem_options(), so crashkernel can evaluate
> > > the reserved size. It is doable, and what is your opinion about the
> >
> > You mean calling process_e820_entries to calculate the RAM size in
> > system? I may not do like that, please check what __find_max_addr() is
> > doing. Did I get it?
>
> Yes, you got my meaning. But __find_max_addr() relies on the info, fed
> by e820__memblock_setup(). It also involves the iteration of all e820
> entries to get the max address. No essential difference, right?
Hmm, I would say iterating e820 or efi entries to get the mas addr should be
different with calling process_e820_entries(). The 1st is much simpler,
right?
On Fri, Mar 29, 2019 at 3:34 PM Baoquan He <[email protected]> wrote:
>
> On 03/29/19 at 03:25pm, Pingfan Liu wrote:
> > On Fri, Mar 29, 2019 at 2:27 PM Baoquan He <[email protected]> wrote:
> > >
> > > On 03/29/19 at 01:45pm, Pingfan Liu wrote:
> > > > On Fri, Mar 22, 2019 at 4:34 PM Baoquan He <[email protected]> wrote:
> > > > >
> > > > > On 03/22/19 at 03:52pm, Baoquan He wrote:
> > > > > > On 03/22/19 at 03:43pm, Pingfan Liu wrote:
> > > > > > > > > +/* parse crashkernel=x@y option */
> > > > > > > > > +static void mem_avoid_crashkernel_simple(char *option)
> > > > > > > >
> > > > > > > > Chao ever mentioned this, I want to ask again, why does it has to be
> > > > > > > > xxx_simple()?
> > > > > > > >
> > > > > > > Seems that I had replied Chao's question in another email. The naming
> > > > > > > follows the function parse_crashkernel_simple(), as the notes above
> > > > > > ~~~~~~~~~~~~~~~~~~~~~~~~
> > > > > >
> > > > > > Sorry, I don't get. typo?
> > > > >
> > > > > OK, I misunderstood it. We do have parse_crashkernel_simple() to handle
> > > > > crashkernel=size[@offset] case, to differente with other complicated
> > > > > cases, like crashkernel=size,[high|low],
> > > > >
> > > > > Then I am fine with this naming. Soryy about the noise.
> > > > >
> > > > > By the way, do you think if we should take care of this case:
> > > > > crashkernel=<range1>:<size1>[,<range2>:<size2>,...][@offset]
> > > > >
> > > > > It can also specify @offset. Not sure if it's too complicated, you may
> > > > > have a investigation.
> > > > >
> > > > In this case, kernel should get the total memory size info. So
> > > > process_e820_entries() or process_efi_entries() should be called
> > > > twice. One before handle_mem_options(), so crashkernel can evaluate
> > > > the reserved size. It is doable, and what is your opinion about the
> > >
> > > You mean calling process_e820_entries to calculate the RAM size in
> > > system? I may not do like that, please check what __find_max_addr() is
> > > doing. Did I get it?
> >
> > Yes, you got my meaning. But __find_max_addr() relies on the info, fed
> > by e820__memblock_setup(). It also involves the iteration of all e820
> > entries to get the max address. No essential difference, right?
>
> Hmm, I would say iterating e820 or efi entries to get the mas addr should be
> different with calling process_e820_entries(). The 1st is much simpler,
> right?
>
Yes. My original meaning is to reuse process_e820_entries(), but does
not call process_mem_region() at the first time.
Thanks,
Pingfan
On 03/29/19 at 06:00pm, Pingfan Liu wrote:
> On Fri, Mar 29, 2019 at 3:34 PM Baoquan He <[email protected]> wrote:
> >
> > On 03/29/19 at 03:25pm, Pingfan Liu wrote:
> > > On Fri, Mar 29, 2019 at 2:27 PM Baoquan He <[email protected]> wrote:
> > > >
> > > > On 03/29/19 at 01:45pm, Pingfan Liu wrote:
> > > > > On Fri, Mar 22, 2019 at 4:34 PM Baoquan He <[email protected]> wrote:
> > > > > >
> > > > > > On 03/22/19 at 03:52pm, Baoquan He wrote:
> > > > > > > On 03/22/19 at 03:43pm, Pingfan Liu wrote:
> > > > > > > > > > +/* parse crashkernel=x@y option */
> > > > > > > > > > +static void mem_avoid_crashkernel_simple(char *option)
> > > > > > > > >
> > > > > > > > > Chao ever mentioned this, I want to ask again, why does it has to be
> > > > > > > > > xxx_simple()?
> > > > > > > > >
> > > > > > > > Seems that I had replied Chao's question in another email. The naming
> > > > > > > > follows the function parse_crashkernel_simple(), as the notes above
> > > > > > > ~~~~~~~~~~~~~~~~~~~~~~~~
> > > > > > >
> > > > > > > Sorry, I don't get. typo?
> > > > > >
> > > > > > OK, I misunderstood it. We do have parse_crashkernel_simple() to handle
> > > > > > crashkernel=size[@offset] case, to differente with other complicated
> > > > > > cases, like crashkernel=size,[high|low],
> > > > > >
> > > > > > Then I am fine with this naming. Soryy about the noise.
> > > > > >
> > > > > > By the way, do you think if we should take care of this case:
> > > > > > crashkernel=<range1>:<size1>[,<range2>:<size2>,...][@offset]
> > > > > >
> > > > > > It can also specify @offset. Not sure if it's too complicated, you may
> > > > > > have a investigation.
> > > > > >
> > > > > In this case, kernel should get the total memory size info. So
> > > > > process_e820_entries() or process_efi_entries() should be called
> > > > > twice. One before handle_mem_options(), so crashkernel can evaluate
> > > > > the reserved size. It is doable, and what is your opinion about the
> > > >
> > > > You mean calling process_e820_entries to calculate the RAM size in
> > > > system? I may not do like that, please check what __find_max_addr() is
> > > > doing. Did I get it?
> > >
> > > Yes, you got my meaning. But __find_max_addr() relies on the info, fed
> > > by e820__memblock_setup(). It also involves the iteration of all e820
> > > entries to get the max address. No essential difference, right?
> >
> > Hmm, I would say iterating e820 or efi entries to get the mas addr should be
> > different with calling process_e820_entries(). The 1st is much simpler,
> > right?
> >
> Yes. My original meaning is to reuse process_e820_entries(), but does
> not call process_mem_region() at the first time.
Got it. That would be nice, but it will bring hackery which Thomas and
Boris have clearly expressed disliking. Adding a new function to find
the max addr, it truly doesn't reuse code, while it makes the code paths
and logic are pretty clear. These avoiding cases are bloating code,
however they usually don't happen in a same system. We make a simply and
clear logic to make code strightforward with good code comments and
printing message, it will be easier to debug issue if happened.
So, I think a draft patch is welcomed, we can have a look at the logic,
then polish it later to make a formal patch. Makes sense?
Thanks
Baoquan