Re: [PATCH] x86/boot: Use EFI setup data if provided

Re: [PATCH] x86/boot: Use EFI setup data if provided

[Junichi Nomura]

On Fri, Mar 22, 2019 at 04:23:28PM +0100, Borislav Petkov wrote:
> On Fri, Mar 22, 2019 at 11:03:43AM +0000, Junichi Nomura wrote:
> > Commit 3a63f70bf4c3a ("x86/boot: Early parse RSDP and save it in
> > boot_params") broke kexec boot on EFI systems.  efi_get_rsdp_addr()
> > in the early parsing code tries to search RSDP from EFI table but
> > whose address is virtual.
> > 
> > Since kexec(1) provides physical address of config_table via boot_params,
> > efi_get_rsdp_addr() should look for setup_data in the same way as
> > efi_systab_init() in arch/x86/platform/efi/efi.c does.
> 
> If the kexec kernel should continue to use efi_systab_init() then you
> should make efi_get_rsdp_addr() exit early in the kexec-ed kernel.

I'm not sure which way kexec devel is going. Added kexec list.
Here is the version that exits early in efi_get_rsdp_addr().

[PATCH] x86/boot: Don't try to search RSDP from EFI when kexec-booted

Commit 3a63f70bf4c3a ("x86/boot: Early parse RSDP and save it in
boot_params") broke kexec boot on EFI systems.  efi_get_rsdp_addr()
in the early parsing code tries to search RSDP from EFI table but
whose address is virtual.

Normally kexec(1) provides physical address of config_table via boot_params
and EFI code uses that during initialization.
For the early boot code, we just exit efi_get_rsdp_addr() early if the kernel
is booted by kexec.

Fixes: 3a63f70bf4c3a ("x86/boot: Early parse RSDP and save it in boot_params")
Signed-off-by: Jun'ichi Nomura <j-nomura@ce.jp.nec.com>
Cc: Chao Fan <fanc.fnst@cn.fujitsu.com>
Cc: Borislav Petkov <bp@suse.de>

diff --git a/arch/x86/boot/compressed/acpi.c b/arch/x86/boot/compressed/acpi.c
index 0ef4ad5..1cefc43 100644
--- a/arch/x86/boot/compressed/acpi.c
+++ b/arch/x86/boot/compressed/acpi.c
@@ -44,6 +44,24 @@ static acpi_physical_address get_acpi_rsdp(void)
 	return addr;
 }
 
+static bool is_kexec_booted(void)
+{
+	struct setup_data *data;
+
+	/*
+	 * kexec-tools provides EFI setup data so that kexec-ed kernel
+	 * can find proper tables.
+	 */
+	data = (struct setup_data *) boot_params->hdr.setup_data;
+	while (data) {
+		if (data->type == SETUP_EFI)
+			return true;
+		data = (struct setup_data *) data->next;
+	}
+
+	return false;
+}
+
 /* Search EFI system tables for RSDP. */
 static acpi_physical_address efi_get_rsdp_addr(void)
 {
@@ -57,6 +75,10 @@ static acpi_physical_address efi_get_rsdp_addr(void)
 	int size, i;
 	char *sig;
 
+	/* If the system is kexec-booted, poking EFI systab may not work. */
+	if (is_kexec_booted())
+		return 0;
+
 	ei = &boot_params->efi_info;
 	sig = (char *)&ei->efi_loader_signature;
 

_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: [PATCH] x86/boot: Use EFI setup data if provided

[Dave Young]

On 03/25/19 at 12:27am, Junichi Nomura wrote:
> On Fri, Mar 22, 2019 at 04:23:28PM +0100, Borislav Petkov wrote:
> > On Fri, Mar 22, 2019 at 11:03:43AM +0000, Junichi Nomura wrote:
> > > Commit 3a63f70bf4c3a ("x86/boot: Early parse RSDP and save it in
> > > boot_params") broke kexec boot on EFI systems.  efi_get_rsdp_addr()
> > > in the early parsing code tries to search RSDP from EFI table but
> > > whose address is virtual.
> > > 
> > > Since kexec(1) provides physical address of config_table via boot_params,
> > > efi_get_rsdp_addr() should look for setup_data in the same way as
> > > efi_systab_init() in arch/x86/platform/efi/efi.c does.
> > 
> > If the kexec kernel should continue to use efi_systab_init() then you
> > should make efi_get_rsdp_addr() exit early in the kexec-ed kernel.
> 
> I'm not sure which way kexec devel is going. Added kexec list.
> Here is the version that exits early in efi_get_rsdp_addr().
> 
> [PATCH] x86/boot: Don't try to search RSDP from EFI when kexec-booted
> 
> Commit 3a63f70bf4c3a ("x86/boot: Early parse RSDP and save it in
> boot_params") broke kexec boot on EFI systems.  efi_get_rsdp_addr()
> in the early parsing code tries to search RSDP from EFI table but
> whose address is virtual.
> 
> Normally kexec(1) provides physical address of config_table via boot_params
> and EFI code uses that during initialization.
> For the early boot code, we just exit efi_get_rsdp_addr() early if the kernel
> is booted by kexec.
> 
> Fixes: 3a63f70bf4c3a ("x86/boot: Early parse RSDP and save it in boot_params")
> Signed-off-by: Jun'ichi Nomura <j-nomura@ce.jp.nec.com>
> Cc: Chao Fan <fanc.fnst@cn.fujitsu.com>
> Cc: Borislav Petkov <bp@suse.de>
> 
> diff --git a/arch/x86/boot/compressed/acpi.c b/arch/x86/boot/compressed/acpi.c
> index 0ef4ad5..1cefc43 100644
> --- a/arch/x86/boot/compressed/acpi.c
> +++ b/arch/x86/boot/compressed/acpi.c
> @@ -44,6 +44,24 @@ static acpi_physical_address get_acpi_rsdp(void)
>  	return addr;
>  }
>  
> +static bool is_kexec_booted(void)
> +{
> +	struct setup_data *data;
> +
> +	/*
> +	 * kexec-tools provides EFI setup data so that kexec-ed kernel
> +	 * can find proper tables.
> +	 */
> +	data = (struct setup_data *) boot_params->hdr.setup_data;
> +	while (data) {
> +		if (data->type == SETUP_EFI)
> +			return true;
> +		data = (struct setup_data *) data->next;
> +	}
> +
> +	return false;
> +}
> +
>  /* Search EFI system tables for RSDP. */
>  static acpi_physical_address efi_get_rsdp_addr(void)
>  {
> @@ -57,6 +75,10 @@ static acpi_physical_address efi_get_rsdp_addr(void)
>  	int size, i;
>  	char *sig;
>  
> +	/* If the system is kexec-booted, poking EFI systab may not work. */
> +	if (is_kexec_booted())
> +		return 0;
> +
>  	ei = &boot_params->efi_info;
>  	sig = (char *)&ei->efi_loader_signature;
>  
> 
> _______________________________________________
> kexec mailing list
> kexec@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/kexec

Good catch, this way looks good to me.  But the function
is_kexec_booted can be compiled when #ifdef CONFIG_EFI

Otherwise:

Acked-by: Dave Young <dyoung@redhat.com>

Thanks
Dave

_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: [PATCH] x86/boot: Use EFI setup data if provided

[Dave Young]

On 03/25/19 at 02:01pm, Dave Young wrote:
> On 03/25/19 at 12:27am, Junichi Nomura wrote:
> > On Fri, Mar 22, 2019 at 04:23:28PM +0100, Borislav Petkov wrote:
> > > On Fri, Mar 22, 2019 at 11:03:43AM +0000, Junichi Nomura wrote:
> > > > Commit 3a63f70bf4c3a ("x86/boot: Early parse RSDP and save it in
> > > > boot_params") broke kexec boot on EFI systems.  efi_get_rsdp_addr()
> > > > in the early parsing code tries to search RSDP from EFI table but
> > > > whose address is virtual.
> > > > 
> > > > Since kexec(1) provides physical address of config_table via boot_params,
> > > > efi_get_rsdp_addr() should look for setup_data in the same way as
> > > > efi_systab_init() in arch/x86/platform/efi/efi.c does.
> > > 
> > > If the kexec kernel should continue to use efi_systab_init() then you
> > > should make efi_get_rsdp_addr() exit early in the kexec-ed kernel.
> > 
> > I'm not sure which way kexec devel is going. Added kexec list.
> > Here is the version that exits early in efi_get_rsdp_addr().
> > 
> > [PATCH] x86/boot: Don't try to search RSDP from EFI when kexec-booted
> > 
> > Commit 3a63f70bf4c3a ("x86/boot: Early parse RSDP and save it in
> > boot_params") broke kexec boot on EFI systems.  efi_get_rsdp_addr()
> > in the early parsing code tries to search RSDP from EFI table but
> > whose address is virtual.
> > 
> > Normally kexec(1) provides physical address of config_table via boot_params
> > and EFI code uses that during initialization.
> > For the early boot code, we just exit efi_get_rsdp_addr() early if the kernel
> > is booted by kexec.
> > 
> > Fixes: 3a63f70bf4c3a ("x86/boot: Early parse RSDP and save it in boot_params")
> > Signed-off-by: Jun'ichi Nomura <j-nomura@ce.jp.nec.com>
> > Cc: Chao Fan <fanc.fnst@cn.fujitsu.com>
> > Cc: Borislav Petkov <bp@suse.de>
> > 
> > diff --git a/arch/x86/boot/compressed/acpi.c b/arch/x86/boot/compressed/acpi.c
> > index 0ef4ad5..1cefc43 100644
> > --- a/arch/x86/boot/compressed/acpi.c
> > +++ b/arch/x86/boot/compressed/acpi.c
> > @@ -44,6 +44,24 @@ static acpi_physical_address get_acpi_rsdp(void)
> >  	return addr;
> >  }
> >  
> > +static bool is_kexec_booted(void)
> > +{
> > +	struct setup_data *data;
> > +
> > +	/*
> > +	 * kexec-tools provides EFI setup data so that kexec-ed kernel
> > +	 * can find proper tables.
> > +	 */
> > +	data = (struct setup_data *) boot_params->hdr.setup_data;
> > +	while (data) {
> > +		if (data->type == SETUP_EFI)
> > +			return true;
> > +		data = (struct setup_data *) data->next;
> > +	}
> > +
> > +	return false;
> > +}
> > +
> >  /* Search EFI system tables for RSDP. */
> >  static acpi_physical_address efi_get_rsdp_addr(void)
> >  {
> > @@ -57,6 +75,10 @@ static acpi_physical_address efi_get_rsdp_addr(void)
> >  	int size, i;
> >  	char *sig;
> >  
> > +	/* If the system is kexec-booted, poking EFI systab may not work. */
> > +	if (is_kexec_booted())
> > +		return 0;
> > +
> >  	ei = &boot_params->efi_info;
> >  	sig = (char *)&ei->efi_loader_signature;
> >  
> > 
> > _______________________________________________
> > kexec mailing list
> > kexec@lists.infradead.org
> > http://lists.infradead.org/mailman/listinfo/kexec
> 
> Good catch, this way looks good to me.  But the function
> is_kexec_booted can be compiled when #ifdef CONFIG_EFI
> 
> Otherwise:
> 
> Acked-by: Dave Young <dyoung@redhat.com>
> 

Hold on, I replied too quick.  One question is does the above patch
passed your test?   It can workaround and skip the wrong phys addr
issue, but the acpi early parsing still fails because efi_get_rsdp_addr
return 0? 

If this is the case you may need go with your old patch.

I think normally people do not see this bug, because kernel will set the
rsdp in boot_params->acpi_rsdp_addr.  Maybe you are testing with
different kernel versions, eg.

old kernel kexec to new kernel.

And the old kernel does not set boot_params->acpi_rsdp_addr

Is this correct?

Thanks
Dave

_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: [PATCH] x86/boot: Use EFI setup data if provided

[Kairui Song]

On Mon, Mar 25, 2019 at 2:20 PM Dave Young <dyoung@redhat.com> wrote:
>
> On 03/25/19 at 02:01pm, Dave Young wrote:
> > On 03/25/19 at 12:27am, Junichi Nomura wrote:
> > > On Fri, Mar 22, 2019 at 04:23:28PM +0100, Borislav Petkov wrote:
> > > > On Fri, Mar 22, 2019 at 11:03:43AM +0000, Junichi Nomura wrote:
> > > > > Commit 3a63f70bf4c3a ("x86/boot: Early parse RSDP and save it in
> > > > > boot_params") broke kexec boot on EFI systems.  efi_get_rsdp_addr()
> > > > > in the early parsing code tries to search RSDP from EFI table but
> > > > > whose address is virtual.
> > > > >
> > > > > Since kexec(1) provides physical address of config_table via boot_params,
> > > > > efi_get_rsdp_addr() should look for setup_data in the same way as
> > > > > efi_systab_init() in arch/x86/platform/efi/efi.c does.
> > > >
> > > > If the kexec kernel should continue to use efi_systab_init() then you
> > > > should make efi_get_rsdp_addr() exit early in the kexec-ed kernel.
> > >
> > > I'm not sure which way kexec devel is going. Added kexec list.
> > > Here is the version that exits early in efi_get_rsdp_addr().
> > >
> > > [PATCH] x86/boot: Don't try to search RSDP from EFI when kexec-booted
> > >
> > > Commit 3a63f70bf4c3a ("x86/boot: Early parse RSDP and save it in
> > > boot_params") broke kexec boot on EFI systems.  efi_get_rsdp_addr()
> > > in the early parsing code tries to search RSDP from EFI table but
> > > whose address is virtual.
> > >
> > > Normally kexec(1) provides physical address of config_table via boot_params
> > > and EFI code uses that during initialization.
> > > For the early boot code, we just exit efi_get_rsdp_addr() early if the kernel
> > > is booted by kexec.
> > >
> > > Fixes: 3a63f70bf4c3a ("x86/boot: Early parse RSDP and save it in boot_params")
> > > Signed-off-by: Jun'ichi Nomura <j-nomura@ce.jp.nec.com>
> > > Cc: Chao Fan <fanc.fnst@cn.fujitsu.com>
> > > Cc: Borislav Petkov <bp@suse.de>
> > >
> > > diff --git a/arch/x86/boot/compressed/acpi.c b/arch/x86/boot/compressed/acpi.c
> > > index 0ef4ad5..1cefc43 100644
> > > --- a/arch/x86/boot/compressed/acpi.c
> > > +++ b/arch/x86/boot/compressed/acpi.c
> > > @@ -44,6 +44,24 @@ static acpi_physical_address get_acpi_rsdp(void)
> > >     return addr;
> > >  }
> > >
> > > +static bool is_kexec_booted(void)
> > > +{
> > > +   struct setup_data *data;
> > > +
> > > +   /*
> > > +    * kexec-tools provides EFI setup data so that kexec-ed kernel
> > > +    * can find proper tables.
> > > +    */
> > > +   data = (struct setup_data *) boot_params->hdr.setup_data;
> > > +   while (data) {
> > > +           if (data->type == SETUP_EFI)
> > > +                   return true;
> > > +           data = (struct setup_data *) data->next;
> > > +   }
> > > +
> > > +   return false;
> > > +}
> > > +
> > >  /* Search EFI system tables for RSDP. */
> > >  static acpi_physical_address efi_get_rsdp_addr(void)
> > >  {
> > > @@ -57,6 +75,10 @@ static acpi_physical_address efi_get_rsdp_addr(void)
> > >     int size, i;
> > >     char *sig;
> > >
> > > +   /* If the system is kexec-booted, poking EFI systab may not work. */
> > > +   if (is_kexec_booted())
> > > +           return 0;
> > > +
> > >     ei = &boot_params->efi_info;
> > >     sig = (char *)&ei->efi_loader_signature;
> > >
> > >
> > > _______________________________________________
> > > kexec mailing list
> > > kexec@lists.infradead.org
> > > http://lists.infradead.org/mailman/listinfo/kexec
> >
> > Good catch, this way looks good to me.  But the function
> > is_kexec_booted can be compiled when #ifdef CONFIG_EFI
> >
> > Otherwise:
> >
> > Acked-by: Dave Young <dyoung@redhat.com>
> >
>
> Hold on, I replied too quick.  One question is does the above patch
> passed your test?   It can workaround and skip the wrong phys addr
> issue, but the acpi early parsing still fails because efi_get_rsdp_addr
> return 0?
>
> If this is the case you may need go with your old patch.
>
> I think normally people do not see this bug, because kernel will set the
> rsdp in boot_params->acpi_rsdp_addr.  Maybe you are testing with
> different kernel versions, eg.
>
> old kernel kexec to new kernel.
>
> And the old kernel does not set boot_params->acpi_rsdp_addr
>
> Is this correct?
>
> Thanks
> Dave

Hi Dave, actually only kexec_file_load will always set the
boot_params->acpi_rsdp_addr. Can't guarantee how user space tools will
prepare the boot_prams if kexec_load is used, so it's should very
likely to happen.

And for the patch, I also think the first patch looks better, if we
just return 0 early in efi_get_rsdp_addr aren't we still failing to
parse the rsdp in early code?

-- 
Best Regards,
Kairui Song

_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: [PATCH] x86/boot: Use EFI setup data if provided

[Junichi Nomura]

On 3/25/19 3:19 PM, Dave Young wrote:
> On 03/25/19 at 02:01pm, Dave Young wrote:
>> On 03/25/19 at 12:27am, Junichi Nomura wrote:
>>> On Fri, Mar 22, 2019 at 04:23:28PM +0100, Borislav Petkov wrote:
>>>> On Fri, Mar 22, 2019 at 11:03:43AM +0000, Junichi Nomura wrote:
>>>>> Commit 3a63f70bf4c3a ("x86/boot: Early parse RSDP and save it in
>>>>> boot_params") broke kexec boot on EFI systems.  efi_get_rsdp_addr()
>>>>> in the early parsing code tries to search RSDP from EFI table but
>>>>> whose address is virtual.
>>>>>
>>>>> Since kexec(1) provides physical address of config_table via boot_params,
>>>>> efi_get_rsdp_addr() should look for setup_data in the same way as
>>>>> efi_systab_init() in arch/x86/platform/efi/efi.c does.
>>>>
>>>> If the kexec kernel should continue to use efi_systab_init() then you
>>>> should make efi_get_rsdp_addr() exit early in the kexec-ed kernel.
>>>
>>> I'm not sure which way kexec devel is going. Added kexec list.
>>> Here is the version that exits early in efi_get_rsdp_addr().
>>>
>>> [PATCH] x86/boot: Don't try to search RSDP from EFI when kexec-booted
>>>
>>> Commit 3a63f70bf4c3a ("x86/boot: Early parse RSDP and save it in
>>> boot_params") broke kexec boot on EFI systems.  efi_get_rsdp_addr()
>>> in the early parsing code tries to search RSDP from EFI table but
>>> whose address is virtual.
>>>
>>> Normally kexec(1) provides physical address of config_table via boot_params
>>> and EFI code uses that during initialization.
>>> For the early boot code, we just exit efi_get_rsdp_addr() early if the kernel
>>> is booted by kexec.
>>>
>>> Fixes: 3a63f70bf4c3a ("x86/boot: Early parse RSDP and save it in boot_params")
>>> Signed-off-by: Jun'ichi Nomura <j-nomura@ce.jp.nec.com>
>>> Cc: Chao Fan <fanc.fnst@cn.fujitsu.com>
>>> Cc: Borislav Petkov <bp@suse.de>
>>>
>>> diff --git a/arch/x86/boot/compressed/acpi.c b/arch/x86/boot/compressed/acpi.c
>>> index 0ef4ad5..1cefc43 100644
>>> --- a/arch/x86/boot/compressed/acpi.c
>>> +++ b/arch/x86/boot/compressed/acpi.c
>>> @@ -44,6 +44,24 @@ static acpi_physical_address get_acpi_rsdp(void)
>>>  	return addr;
>>>  }
>>>  
>>> +static bool is_kexec_booted(void)
>>> +{
>>> +	struct setup_data *data;
>>> +
>>> +	/*
>>> +	 * kexec-tools provides EFI setup data so that kexec-ed kernel
>>> +	 * can find proper tables.
>>> +	 */
>>> +	data = (struct setup_data *) boot_params->hdr.setup_data;
>>> +	while (data) {
>>> +		if (data->type == SETUP_EFI)
>>> +			return true;
>>> +		data = (struct setup_data *) data->next;
>>> +	}
>>> +
>>> +	return false;
>>> +}
>>> +
>>>  /* Search EFI system tables for RSDP. */
>>>  static acpi_physical_address efi_get_rsdp_addr(void)
>>>  {
>>> @@ -57,6 +75,10 @@ static acpi_physical_address efi_get_rsdp_addr(void)
>>>  	int size, i;
>>>  	char *sig;
>>>  
>>> +	/* If the system is kexec-booted, poking EFI systab may not work. */
>>> +	if (is_kexec_booted())
>>> +		return 0;
>>> +
>>>  	ei = &boot_params->efi_info;
>>>  	sig = (char *)&ei->efi_loader_signature;
>>>  
>>>
>>> _______________________________________________
>>> kexec mailing list
>>> kexec@lists.infradead.org
>>> http://lists.infradead.org/mailman/listinfo/kexec
>>
>> Good catch, this way looks good to me.  But the function
>> is_kexec_booted can be compiled when #ifdef CONFIG_EFI
>>
>> Otherwise:
>>
>> Acked-by: Dave Young <dyoung@redhat.com>
>>
> 
> Hold on, I replied too quick.  One question is does the above patch
> passed your test?   It can workaround and skip the wrong phys addr
> issue, but the acpi early parsing still fails because efi_get_rsdp_addr
> return 0? 

The patch works for me.
Early parsing fails with the 2nd patch but it boots fine.
EFI initialization is done later without boot_params->acpi_rsdp_addr.
I think that's how v5.0 and earlier kernels work.

> If this is the case you may need go with your old patch.
> 
> I think normally people do not see this bug, because kernel will set the
> rsdp in boot_params->acpi_rsdp_addr.  Maybe you are testing with

I think it's only done for file-based kexec interface.

> different kernel versions, eg.
> 
> old kernel kexec to new kernel.
> 
> And the old kernel does not set boot_params->acpi_rsdp_addr
> 
> Is this correct?

I'm testing kexec from v5.1-rc1 to v5.1-rc1, i.e. same kernel.

-- 
Jun'ichi Nomura, NEC Corporation / NEC Solution Innovators, Ltd.

_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: [PATCH] x86/boot: Use EFI setup data if provided

[Dave Young]

On 03/25/19 at 06:47am, Junichi Nomura wrote:
> On 3/25/19 3:19 PM, Dave Young wrote:
> > On 03/25/19 at 02:01pm, Dave Young wrote:
> >> On 03/25/19 at 12:27am, Junichi Nomura wrote:
> >>> On Fri, Mar 22, 2019 at 04:23:28PM +0100, Borislav Petkov wrote:
> >>>> On Fri, Mar 22, 2019 at 11:03:43AM +0000, Junichi Nomura wrote:
> >>>>> Commit 3a63f70bf4c3a ("x86/boot: Early parse RSDP and save it in
> >>>>> boot_params") broke kexec boot on EFI systems.  efi_get_rsdp_addr()
> >>>>> in the early parsing code tries to search RSDP from EFI table but
> >>>>> whose address is virtual.
> >>>>>
> >>>>> Since kexec(1) provides physical address of config_table via boot_params,
> >>>>> efi_get_rsdp_addr() should look for setup_data in the same way as
> >>>>> efi_systab_init() in arch/x86/platform/efi/efi.c does.
> >>>>
> >>>> If the kexec kernel should continue to use efi_systab_init() then you
> >>>> should make efi_get_rsdp_addr() exit early in the kexec-ed kernel.
> >>>
> >>> I'm not sure which way kexec devel is going. Added kexec list.
> >>> Here is the version that exits early in efi_get_rsdp_addr().
> >>>
> >>> [PATCH] x86/boot: Don't try to search RSDP from EFI when kexec-booted
> >>>
> >>> Commit 3a63f70bf4c3a ("x86/boot: Early parse RSDP and save it in
> >>> boot_params") broke kexec boot on EFI systems.  efi_get_rsdp_addr()
> >>> in the early parsing code tries to search RSDP from EFI table but
> >>> whose address is virtual.
> >>>
> >>> Normally kexec(1) provides physical address of config_table via boot_params
> >>> and EFI code uses that during initialization.
> >>> For the early boot code, we just exit efi_get_rsdp_addr() early if the kernel
> >>> is booted by kexec.
> >>>
> >>> Fixes: 3a63f70bf4c3a ("x86/boot: Early parse RSDP and save it in boot_params")
> >>> Signed-off-by: Jun'ichi Nomura <j-nomura@ce.jp.nec.com>
> >>> Cc: Chao Fan <fanc.fnst@cn.fujitsu.com>
> >>> Cc: Borislav Petkov <bp@suse.de>
> >>>
> >>> diff --git a/arch/x86/boot/compressed/acpi.c b/arch/x86/boot/compressed/acpi.c
> >>> index 0ef4ad5..1cefc43 100644
> >>> --- a/arch/x86/boot/compressed/acpi.c
> >>> +++ b/arch/x86/boot/compressed/acpi.c
> >>> @@ -44,6 +44,24 @@ static acpi_physical_address get_acpi_rsdp(void)
> >>>  	return addr;
> >>>  }
> >>>  
> >>> +static bool is_kexec_booted(void)
> >>> +{
> >>> +	struct setup_data *data;
> >>> +
> >>> +	/*
> >>> +	 * kexec-tools provides EFI setup data so that kexec-ed kernel
> >>> +	 * can find proper tables.
> >>> +	 */
> >>> +	data = (struct setup_data *) boot_params->hdr.setup_data;
> >>> +	while (data) {
> >>> +		if (data->type == SETUP_EFI)
> >>> +			return true;
> >>> +		data = (struct setup_data *) data->next;
> >>> +	}
> >>> +
> >>> +	return false;
> >>> +}
> >>> +
> >>>  /* Search EFI system tables for RSDP. */
> >>>  static acpi_physical_address efi_get_rsdp_addr(void)
> >>>  {
> >>> @@ -57,6 +75,10 @@ static acpi_physical_address efi_get_rsdp_addr(void)
> >>>  	int size, i;
> >>>  	char *sig;
> >>>  
> >>> +	/* If the system is kexec-booted, poking EFI systab may not work. */
> >>> +	if (is_kexec_booted())
> >>> +		return 0;
> >>> +
> >>>  	ei = &boot_params->efi_info;
> >>>  	sig = (char *)&ei->efi_loader_signature;
> >>>  
> >>>
> >>> _______________________________________________
> >>> kexec mailing list
> >>> kexec@lists.infradead.org
> >>> http://lists.infradead.org/mailman/listinfo/kexec
> >>
> >> Good catch, this way looks good to me.  But the function
> >> is_kexec_booted can be compiled when #ifdef CONFIG_EFI
> >>
> >> Otherwise:
> >>
> >> Acked-by: Dave Young <dyoung@redhat.com>
> >>
> > 
> > Hold on, I replied too quick.  One question is does the above patch
> > passed your test?   It can workaround and skip the wrong phys addr
> > issue, but the acpi early parsing still fails because efi_get_rsdp_addr
> > return 0? 
> 
> The patch works for me.
> Early parsing fails with the 2nd patch but it boots fine.
> EFI initialization is done later without boot_params->acpi_rsdp_addr.
> I think that's how v5.0 and earlier kernels work.

Ok, thanks 

> 
> > If this is the case you may need go with your old patch.
> > 
> > I think normally people do not see this bug, because kernel will set the
> > rsdp in boot_params->acpi_rsdp_addr.  Maybe you are testing with
> 
> I think it's only done for file-based kexec interface.

Saw Kairui's another reply, yes, kexec-tools need a patch to fill the
value as well then.

I would vote for a repost of your old patch with some #ifdef

Thanks
Dave

_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

[PATCH v2] x86/boot: Use EFI setup data if provided

[Junichi Nomura]

On 3/25/19 3:59 PM, Dave Young wrote:
> On 03/25/19 at 06:47am, Junichi Nomura wrote:
>> On 3/25/19 3:19 PM, Dave Young wrote:
>>> On 03/25/19 at 02:01pm, Dave Young wrote:
>>> I think normally people do not see this bug, because kernel will set the
>>> rsdp in boot_params->acpi_rsdp_addr.  Maybe you are testing with
>>
>> I think it's only done for file-based kexec interface.
> 
> Saw Kairui's another reply, yes, kexec-tools need a patch to fill the
> value as well then.
> 
> I would vote for a repost of your old patch with some #ifdef

Thanks for comments, Dave, Kairui and Baoquan.

The problem for me is it's a regression in v5.1-rc1, that breaks
existing setup. If early parsing of RSDP is required only for newly
supported configuration, I'm fine such configuration requires
new tools or new options.

This is the 1st version plus #ifdef around the EFI code.
Build tested both with and without CONFIG_EFI to see no warnings.

[PATCH v2] x86/boot: Use EFI setup data if provided

Commit 3a63f70bf4c3a ("x86/boot: Early parse RSDP and save it in
boot_params") broke kexec boot on EFI systems.  efi_get_rsdp_addr()
in the early parsing code tries to search RSDP from EFI table but
whose address is virtual.

Since kexec(1) provides physical address of config_table via boot_params,
efi_get_rsdp_addr() should look for setup_data in the same way as
efi_systab_init() in arch/x86/platform/efi/efi.c does.

Fixes: 3a63f70bf4c3a ("x86/boot: Early parse RSDP and save it in boot_params")
Signed-off-by: Jun'ichi Nomura <j-nomura@ce.jp.nec.com>
Acked-by: Dave Young <dyoung@redhat.com>
Cc: Chao Fan <fanc.fnst@cn.fujitsu.com>
Cc: Borislav Petkov <bp@suse.de>

diff --git a/arch/x86/boot/compressed/acpi.c b/arch/x86/boot/compressed/acpi.c
--- a/arch/x86/boot/compressed/acpi.c
+++ b/arch/x86/boot/compressed/acpi.c
@@ -44,6 +44,24 @@ static acpi_physical_address get_acpi_rsdp(void)
 	return addr;
 }
 
+#ifdef CONFIG_EFI
+static unsigned long efi_get_setup_data_addr(void)
+{
+	struct setup_data *data;
+	u64 pa_data;
+
+	pa_data = boot_params->hdr.setup_data;
+	while (pa_data) {
+		data = (struct setup_data *) pa_data;
+		if (data->type == SETUP_EFI)
+			return pa_data + sizeof(struct setup_data);
+		pa_data = data->next;
+	}
+
+	return 0;
+}
+#endif
+
 /* Search EFI system tables for RSDP. */
 static acpi_physical_address efi_get_rsdp_addr(void)
 {
@@ -53,10 +71,12 @@ static acpi_physical_address efi_get_rsdp_addr(void)
 	unsigned long systab, systab_tables, config_tables;
 	unsigned int nr_tables;
 	struct efi_info *ei;
+	struct efi_setup_data *esd;
 	bool efi_64;
 	int size, i;
 	char *sig;
 
+	esd = (struct efi_setup_data *) efi_get_setup_data_addr();
 	ei = &boot_params->efi_info;
 	sig = (char *)&ei->efi_loader_signature;
 
@@ -86,13 +106,13 @@ static acpi_physical_address efi_get_rsdp_addr(void)
 	if (efi_64) {
 		efi_system_table_64_t *stbl = (efi_system_table_64_t *)systab;
 
-		config_tables	= stbl->tables;
+		config_tables	= esd ? esd->tables : stbl->tables;
 		nr_tables	= stbl->nr_tables;
 		size		= sizeof(efi_config_table_64_t);
 	} else {
 		efi_system_table_32_t *stbl = (efi_system_table_32_t *)systab;
 
-		config_tables	= stbl->tables;
+		config_tables	= esd ? esd->tables : stbl->tables;
 		nr_tables	= stbl->nr_tables;
 		size		= sizeof(efi_config_table_32_t);
 	}

_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: [PATCH v2] x86/boot: Use EFI setup data if provided

[Boris Petkov]

On March 25, 2019 9:27:21 AM GMT+01:00, Junichi Nomura <j-nomura@ce.jp.nec.com> wrote:
>On 3/25/19 3:59 PM, Dave Young wrote:
>> On 03/25/19 at 06:47am, Junichi Nomura wrote:
>>> On 3/25/19 3:19 PM, Dave Young wrote:
>>>> On 03/25/19 at 02:01pm, Dave Young wrote:
>>>> I think normally people do not see this bug, because kernel will
>set the
>>>> rsdp in boot_params->acpi_rsdp_addr.  Maybe you are testing with
>>>
>>> I think it's only done for file-based kexec interface.
>> 
>> Saw Kairui's another reply, yes, kexec-tools need a patch to fill the
>> value as well then.
>> 
>> I would vote for a repost of your old patch with some #ifdef
>
>Thanks for comments, Dave, Kairui and Baoquan.
>
>The problem for me is it's a regression in v5.1-rc1, that breaks
>existing setup. If early parsing of RSDP is required only for newly
>supported configuration, I'm fine such configuration requires
>new tools or new options.
>
>This is the 1st version plus #ifdef around the EFI code.

I'm going to repeat that again until you get it:

If the kexec kernel should continue to use efi_systab_init() then you
should make efi_get_rsdp_addr() exit early in the kexec-ed kernel.
-- 
Sent from a small device: formatting sux and brevity is inevitable. 

_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

[PATCH v2] x86/boot: Don't try to search RSDP from EFI when kexec-booted

[Junichi Nomura]

On 3/25/19 5:54 PM, Boris Petkov wrote:
> I'm going to repeat that again until you get it:
> 
> If the kexec kernel should continue to use efi_systab_init() then you
> should make efi_get_rsdp_addr() exit early in the kexec-ed kernel.

Do you think this one is ok? Either works for me.

[PATCH v2] x86/boot: Don't try to search RSDP from EFI when kexec-booted

Commit 3a63f70bf4c3a ("x86/boot: Early parse RSDP and save it in
boot_params") broke kexec boot on EFI systems.  efi_get_rsdp_addr()
in the early parsing code tries to search RSDP from EFI table but
whose address is virtual.

Normally kexec(1) provides physical address of config_table via boot_params
and EFI code uses that during initialization.
For the early boot code, we just exit efi_get_rsdp_addr() early if the kernel
is booted by kexec.

Fixes: 3a63f70bf4c3a ("x86/boot: Early parse RSDP and save it in boot_params")
Signed-off-by: Jun'ichi Nomura <j-nomura@ce.jp.nec.com>
Cc: Chao Fan <fanc.fnst@cn.fujitsu.com>
Cc: Borislav Petkov <bp@suse.de>

diff --git a/arch/x86/boot/compressed/acpi.c b/arch/x86/boot/compressed/acpi.c
--- a/arch/x86/boot/compressed/acpi.c
+++ b/arch/x86/boot/compressed/acpi.c
@@ -44,6 +44,26 @@ static acpi_physical_address get_acpi_rsdp(void)
 	return addr;
 }
 
+#ifdef CONFIG_EFI
+static bool is_kexec_booted(void)
+{
+	struct setup_data *data;
+
+	/*
+	 * kexec-tools provides EFI setup data so that kexec-ed kernel
+	 * can find proper tables.
+	 */
+	data = (struct setup_data *) boot_params->hdr.setup_data;
+	while (data) {
+		if (data->type == SETUP_EFI)
+			return true;
+		data = (struct setup_data *) data->next;
+	}
+
+	return false;
+}
+#endif
+
 /* Search EFI system tables for RSDP. */
 static acpi_physical_address efi_get_rsdp_addr(void)
 {
@@ -57,6 +77,10 @@ static acpi_physical_address efi_get_rsdp_addr(void)
 	int size, i;
 	char *sig;
 
+	/* If the system is kexec-booted, poking EFI systab may not work. */
+	if (is_kexec_booted())
+		return 0;
+
 	ei = &boot_params->efi_info;
 	sig = (char *)&ei->efi_loader_signature;
 

_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: [PATCH v2] x86/boot: Use EFI setup data if provided

[Dave Young]

On 03/25/19 at 09:54am, Boris Petkov wrote:
> On March 25, 2019 9:27:21 AM GMT+01:00, Junichi Nomura <j-nomura@ce.jp.nec.com> wrote:
> >On 3/25/19 3:59 PM, Dave Young wrote:
> >> On 03/25/19 at 06:47am, Junichi Nomura wrote:
> >>> On 3/25/19 3:19 PM, Dave Young wrote:
> >>>> On 03/25/19 at 02:01pm, Dave Young wrote:
> >>>> I think normally people do not see this bug, because kernel will
> >set the
> >>>> rsdp in boot_params->acpi_rsdp_addr.  Maybe you are testing with
> >>>
> >>> I think it's only done for file-based kexec interface.
> >> 
> >> Saw Kairui's another reply, yes, kexec-tools need a patch to fill the
> >> value as well then.
> >> 
> >> I would vote for a repost of your old patch with some #ifdef
> >
> >Thanks for comments, Dave, Kairui and Baoquan.
> >
> >The problem for me is it's a regression in v5.1-rc1, that breaks
> >existing setup. If early parsing of RSDP is required only for newly
> >supported configuration, I'm fine such configuration requires
> >new tools or new options.
> >
> >This is the 1st version plus #ifdef around the EFI code.
> 
> I'm going to repeat that again until you get it:
> 
> If the kexec kernel should continue to use efi_systab_init() then you
> should make efi_get_rsdp_addr() exit early in the kexec-ed kernel.

In that way, early parsing will fail in kexeced kernel, am I missing
something?  The early code become complicated but since we have already
the early acpi parsing why not to make it consistent in kexeced kernel?

Thanks
Dave

_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: [PATCH v2] x86/boot: Use EFI setup data if provided

[Junichi Nomura]

On 3/25/19 7:15 PM, Dave Young wrote:
> On 03/25/19 at 09:54am, Boris Petkov wrote:
>> On March 25, 2019 9:27:21 AM GMT+01:00, Junichi Nomura <j-nomura@ce.jp.nec.com> wrote:
>>> On 3/25/19 3:59 PM, Dave Young wrote:
>>>> On 03/25/19 at 06:47am, Junichi Nomura wrote:
>>>>> On 3/25/19 3:19 PM, Dave Young wrote:
>>>>>> On 03/25/19 at 02:01pm, Dave Young wrote:
>>>>>> I think normally people do not see this bug, because kernel will
>>> set the
>>>>>> rsdp in boot_params->acpi_rsdp_addr.  Maybe you are testing with
>>>>>
>>>>> I think it's only done for file-based kexec interface.
>>>>
>>>> Saw Kairui's another reply, yes, kexec-tools need a patch to fill the
>>>> value as well then.
>>>>
>>>> I would vote for a repost of your old patch with some #ifdef
>>>
>>> Thanks for comments, Dave, Kairui and Baoquan.
>>>
>>> The problem for me is it's a regression in v5.1-rc1, that breaks
>>> existing setup. If early parsing of RSDP is required only for newly
>>> supported configuration, I'm fine such configuration requires
>>> new tools or new options.
>>>
>>> This is the 1st version plus #ifdef around the EFI code.
>>
>> I'm going to repeat that again until you get it:
>>
>> If the kexec kernel should continue to use efi_systab_init() then you
>> should make efi_get_rsdp_addr() exit early in the kexec-ed kernel.
> 
> In that way, early parsing will fail in kexeced kernel, am I missing
> something?  The early code become complicated but since we have already
> the early acpi parsing why not to make it consistent in kexeced kernel?

AFAIU, early parsing is new code in v5.1-rc1 to support kexec on systems
with hotpluggable memory with KASLR enabled. For systems that requires the
new feature, it may be ok to say "you need to use another kexec interface"
and/or "you need new kexec-tools".

-- 
Jun'ichi Nomura, NEC Corporation / NEC Solution Innovators, Ltd.

_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: [PATCH v2] x86/boot: Use EFI setup data if provided

[Dave Young]

On 03/25/19 at 10:36am, Junichi Nomura wrote:
> On 3/25/19 7:15 PM, Dave Young wrote:
> > On 03/25/19 at 09:54am, Boris Petkov wrote:
> >> On March 25, 2019 9:27:21 AM GMT+01:00, Junichi Nomura <j-nomura@ce.jp.nec.com> wrote:
> >>> On 3/25/19 3:59 PM, Dave Young wrote:
> >>>> On 03/25/19 at 06:47am, Junichi Nomura wrote:
> >>>>> On 3/25/19 3:19 PM, Dave Young wrote:
> >>>>>> On 03/25/19 at 02:01pm, Dave Young wrote:
> >>>>>> I think normally people do not see this bug, because kernel will
> >>> set the
> >>>>>> rsdp in boot_params->acpi_rsdp_addr.  Maybe you are testing with
> >>>>>
> >>>>> I think it's only done for file-based kexec interface.
> >>>>
> >>>> Saw Kairui's another reply, yes, kexec-tools need a patch to fill the
> >>>> value as well then.
> >>>>
> >>>> I would vote for a repost of your old patch with some #ifdef
> >>>
> >>> Thanks for comments, Dave, Kairui and Baoquan.
> >>>
> >>> The problem for me is it's a regression in v5.1-rc1, that breaks
> >>> existing setup. If early parsing of RSDP is required only for newly
> >>> supported configuration, I'm fine such configuration requires
> >>> new tools or new options.
> >>>
> >>> This is the 1st version plus #ifdef around the EFI code.
> >>
> >> I'm going to repeat that again until you get it:
> >>
> >> If the kexec kernel should continue to use efi_systab_init() then you
> >> should make efi_get_rsdp_addr() exit early in the kexec-ed kernel.
> > 
> > In that way, early parsing will fail in kexeced kernel, am I missing
> > something?  The early code become complicated but since we have already
> > the early acpi parsing why not to make it consistent in kexeced kernel?
> 
> AFAIU, early parsing is new code in v5.1-rc1 to support kexec on systems
> with hotpluggable memory with KASLR enabled. For systems that requires the
> new feature, it may be ok to say "you need to use another kexec interface"
> and/or "you need new kexec-tools".

Ok, it makes some sense, Kairui mentioned he will work on a kexec-tools
patch then this becomes a must.

Thanks
Dave

_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: [PATCH v2] x86/boot: Use EFI setup data if provided

[Borislav Petkov]

On Mon, Mar 25, 2019 at 10:36:33AM +0000, Junichi Nomura wrote:
> AFAIU, early parsing is new code in v5.1-rc1 to support kexec on systems
> with hotpluggable memory with KASLR enabled. For systems that requires the
> new feature, it may be ok to say "you need to use another kexec interface"
> and/or "you need new kexec-tools".

No, this exactly should *not* happen. kexec is already full of duct tape
- don't need any more of that.

So I suggested that efi_get_rsdp_addr() should exit early on in the
kexeced kernel but making this all play nice with the kexec-ed kernel,
as Dave suggests, is better.

Now, my next question is: why does the RDSP address need to come from
kexec(1) (by way of efi_setup_data) and why can't the kexec'ed kernel
figure it out itself by parsing the EFI tables in a similar way to
efi_get_rsdp_addr ?

-- 
Regards/Gruss,
    Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.

_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: [PATCH v2] x86/boot: Use EFI setup data if provided

[Dave Young]

On 03/25/19 at 01:01pm, Borislav Petkov wrote:
> On Mon, Mar 25, 2019 at 10:36:33AM +0000, Junichi Nomura wrote:
> > AFAIU, early parsing is new code in v5.1-rc1 to support kexec on systems
> > with hotpluggable memory with KASLR enabled. For systems that requires the
> > new feature, it may be ok to say "you need to use another kexec interface"
> > and/or "you need new kexec-tools".
> 
> No, this exactly should *not* happen. kexec is already full of duct tape
> - don't need any more of that.
> 
> So I suggested that efi_get_rsdp_addr() should exit early on in the
> kexeced kernel but making this all play nice with the kexec-ed kernel,
> as Dave suggests, is better.
> 
> Now, my next question is: why does the RDSP address need to come from
> kexec(1) (by way of efi_setup_data) and why can't the kexec'ed kernel
> figure it out itself by parsing the EFI tables in a similar way to
> efi_get_rsdp_addr ?

efi_enter_virtual_mode() can only run once because of efi firmware/spec
limitation,  and after entered virtual mode, efi firmware just updated
the original efi sys table, for example the original
systab64->fw_vendor, systab64->tables, and even smbios (only found on
some HPE machine) are changed from physcial address to virtual address.

In the current efi_get_rsdp_addr, it assumes the efi config tables
address is not touched (as physical addresses), it will break then.

Kexec saved the original physical addresses, and pass them to kexeced
kernel via x86 setup_data, so  both the early parsing or efi init code
need to get those physical values from setup_data.

Thanks
Dave

_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: [PATCH v2] x86/boot: Use EFI setup data if provided

[Borislav Petkov]

On Mon, Mar 25, 2019 at 08:23:02PM +0800, Dave Young wrote:
> efi_enter_virtual_mode() can only run once because of efi firmware/spec
> limitation,  and after entered virtual mode, efi firmware just updated

I should remember that - I did it at the time.

> Kexec saved the original physical addresses, and pass them to kexeced
> kernel via x86 setup_data, so  both the early parsing or efi init code
> need to get those physical values from setup_data.

So efi_get_rsdp_addr() needs to be refactored in such a way so that at
least the loop towards the end gets carved out into a separate function
- __efi_get_rsdp_addr() or so - which gets config_tables, nr_tables and
size as arguments and finds the RSDP address in the kexec-ed kernel.

So we'd need something like that:

acpi_physical_address get_rsdp_addr(void)
{
        acpi_physical_address pa;

        pa = get_acpi_rsdp();

        if (!pa)
                pa = boot_params->acpi_rsdp_addr;

        if (!pa)
                pa = efi_get_rsdp_addr();

	if (!pa)
		pa = kexec_get_rdsp_addr();		<--- new function

        if (!pa)
                pa = bios_get_rsdp_addr();

        return pa;
}

which would get config_tables from setup_data and call
__efi_get_rsdp_addr() to dig it out in the kexec'ed kernel.

Junichi, ask if it is still unclear what needs to be done.

Thx.

-- 
Regards/Gruss,
    Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.

_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: [PATCH v2] x86/boot: Use EFI setup data if provided

[Junichi Nomura]

On 3/25/19 9:32 PM, Borislav Petkov wrote:
> On Mon, Mar 25, 2019 at 08:23:02PM +0800, Dave Young wrote:
>> Kexec saved the original physical addresses, and pass them to kexeced
>> kernel via x86 setup_data, so  both the early parsing or efi init code
>> need to get those physical values from setup_data.
> 
> So efi_get_rsdp_addr() needs to be refactored in such a way so that at
> least the loop towards the end gets carved out into a separate function
> - __efi_get_rsdp_addr() or so - which gets config_tables, nr_tables and
> size as arguments and finds the RSDP address in the kexec-ed kernel.

Since we still need to read systab for nr_tables and do signature
check to determine if it's 32bit or 64bit for kexec-ed kernel,
everything except the address of config_tables are common between
normal boot and kexec boot.

> So we'd need something like that:
> 
> acpi_physical_address get_rsdp_addr(void)
> {
>         acpi_physical_address pa;
> 
>         pa = get_acpi_rsdp();
> 
>         if (!pa)
>                 pa = boot_params->acpi_rsdp_addr;
> 
>         if (!pa)
>                 pa = efi_get_rsdp_addr();
> 
> 	if (!pa)
> 		pa = kexec_get_rdsp_addr();		<--- new function
> 
>         if (!pa)
>                 pa = bios_get_rsdp_addr();
> 
>         return pa;
> }
> 
> which would get config_tables from setup_data and call
> __efi_get_rsdp_addr() to dig it out in the kexec'ed kernel.
> 
> Junichi, ask if it is still unclear what needs to be done.

efi_get_rsdp_addr() and kexec_get_rsdp_addr() could be implemented
like this (sorry about the pseudo code):

  /* This is also used to check if the kernel is kexec-ed. */
  unsigned long efi_get_setup_data_addr(void) {
    return the address of efi_setup_data if kexec-ed or 0 if not;
  }

  acpi_physical_address __efi_get_rsdp_addr(unsigned long config_tables) {
    // Do mostly same as the current efi_get_rsdp_addr().
    // If config_tables is non-zero, use it instead of systab->tables.
  }

  acpi_physical_address efi_get_rsdp_addr(void) {
    if (efi_get_setup_data_addr())
      return 0;
    return __efi_get_rsdp_addr(0);
  }

  acpi_physical_address kexec_get_rsdp_addr(void) {
    esd = (struct efi_setup_data *) efi_get_setup_data_addr();
    if (esd && esd->tables)
      return __efi_get_rsdp_addr((unsigned long) esd->tables);
    return 0;
  }

I don't think it looks nice.. Does this match what you envisage?

-- 
Jun'ichi Nomura, NEC Corporation / NEC Solution Innovators, Ltd.

_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: [PATCH v2] x86/boot: Use EFI setup data if provided

[Dave Young]

On 03/25/19 at 11:10pm, Junichi Nomura wrote:
> On 3/25/19 9:32 PM, Borislav Petkov wrote:
> > On Mon, Mar 25, 2019 at 08:23:02PM +0800, Dave Young wrote:
> >> Kexec saved the original physical addresses, and pass them to kexeced
> >> kernel via x86 setup_data, so  both the early parsing or efi init code
> >> need to get those physical values from setup_data.
> > 
> > So efi_get_rsdp_addr() needs to be refactored in such a way so that at
> > least the loop towards the end gets carved out into a separate function
> > - __efi_get_rsdp_addr() or so - which gets config_tables, nr_tables and
> > size as arguments and finds the RSDP address in the kexec-ed kernel.
> 
> Since we still need to read systab for nr_tables and do signature
> check to determine if it's 32bit or 64bit for kexec-ed kernel,
> everything except the address of config_tables are common between
> normal boot and kexec boot.

Hmm, the efi/kexec support only added for 64bit, so no need check 32bit for
kexec.

Thanks
Dave

_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: [PATCH v2] x86/boot: Use EFI setup data if provided

[Borislav Petkov]

On Mon, Mar 25, 2019 at 11:10:01PM +0000, Junichi Nomura wrote:
> efi_get_rsdp_addr() and kexec_get_rsdp_addr() could be implemented
> like this (sorry about the pseudo code):

This doesn't look like what I suggested:

> So efi_get_rsdp_addr() needs to be refactored in such a way so that at
> least the loop towards the end gets carved out into a separate function
> - __efi_get_rsdp_addr() or so - which gets config_tables, nr_tables and
> size as arguments and finds the RSDP address in the kexec-ed kernel.

You need to carve out the loop at the end and make it into a separate
__efi_get_rsdp_addr() function which gets the physical or the virtual
address.

-- 
Regards/Gruss,
    Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.

_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: [PATCH v2] x86/boot: Use EFI setup data if provided

[bhe]

Hi Junichi,

On 03/26/19 at 02:57pm, Borislav Petkov wrote:
> On Mon, Mar 25, 2019 at 11:10:01PM +0000, Junichi Nomura wrote:
> > efi_get_rsdp_addr() and kexec_get_rsdp_addr() could be implemented
> > like this (sorry about the pseudo code):
> 
> This doesn't look like what I suggested:
> 
> > So efi_get_rsdp_addr() needs to be refactored in such a way so that at
> > least the loop towards the end gets carved out into a separate function
> > - __efi_get_rsdp_addr() or so - which gets config_tables, nr_tables and
> > size as arguments and finds the RSDP address in the kexec-ed kernel.
> 
> You need to carve out the loop at the end and make it into a separate
> __efi_get_rsdp_addr() function which gets the physical or the virtual
> address.

I guess Boris is suggesting code like below. Please correct me if I am
wrong.

static acpi_physical_address _efi_get_rsdp_addr(efi_config_table tbl, ...)
{
	/* Get EFI tables from systab. */
	for (i = 0; i < nr_tables; i++) {
		...
	}
	return rsdp_addr;
}

static acpi_physical_address efi_get_rsdp_addr(void)
{
	...
	/* Get systab from boot params. */		
	...
	/* Handle EFI bitness properly */
	...
	return _efi_get_rsdp_addr();
}


static acpi_physical_address kexec_get_rsdp_addr(void)
{
	if (!is_kexec_booted)
		return 0;

	efi_get_setup_data_addr();
	...
	/* Handle EFI bitness properly */
	...
	return _efi_get_rsdp_addr();
}

acpi_physical_address get_rsdp_addr(void)
{
        acpi_physical_address pa;

        pa = get_acpi_rsdp();

        if (!pa)
                pa = boot_params->acpi_rsdp_addr;


	/**
	/*I think here we should check if it's kexec booted firstly.
	 * Skip it if not kexec. this can avoid the wrong kexec virt
	 * addr parsing./
        if (!pa)
                pa = kexec_get_rdsp_addr();             <--- new function

        if (!pa)
                pa = efi_get_rsdp_addr();

        if (!pa)
                pa = bios_get_rsdp_addr();

        return pa;
}

_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: [PATCH v2] x86/boot: Use EFI setup data if provided

[Borislav Petkov]

On Wed, Mar 27, 2019 at 09:48:52AM +0800, bhe@redhat.com wrote:
> I guess Boris is suggesting code like below. Please correct me if I am
> wrong.

Yap, exactly.

Thx.

-- 
Regards/Gruss,
    Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.

_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: [PATCH v2] x86/boot: Use EFI setup data if provided

[Junichi Nomura]

On 2019/03/27 10:48, bhe@redhat.com wrote:
>>> So efi_get_rsdp_addr() needs to be refactored in such a way so that at
>>> least the loop towards the end gets carved out into a separate function
>>> - __efi_get_rsdp_addr() or so - which gets config_tables, nr_tables and
>>> size as arguments and finds the RSDP address in the kexec-ed kernel.
>>
>> You need to carve out the loop at the end and make it into a separate
>> __efi_get_rsdp_addr() function which gets the physical or the virtual
>> address.
> 
> I guess Boris is suggesting code like below. Please correct me if I am
> wrong.
> 
> static acpi_physical_address _efi_get_rsdp_addr(efi_config_table tbl, ...)
> {
> 	/* Get EFI tables from systab. */
> 	for (i = 0; i < nr_tables; i++) {
> 		...
> 	}
> 	return rsdp_addr;
> }
> 
> static acpi_physical_address efi_get_rsdp_addr(void)
> {
> 	...
> 	/* Get systab from boot params. */		
> 	...
> 	/* Handle EFI bitness properly */
> 	...
> 	return _efi_get_rsdp_addr();
> }
> 
> 
> static acpi_physical_address kexec_get_rsdp_addr(void)
> {
> 	if (!is_kexec_booted)
> 		return 0;
> 
> 	efi_get_setup_data_addr();
> 	...
> 	/* Handle EFI bitness properly */
> 	...
> 	return _efi_get_rsdp_addr();
> }

I still don't get it... We still need systab for kexec case as well
to get nr_tables. Don't we?

> acpi_physical_address get_rsdp_addr(void)
> {
>          acpi_physical_address pa;
> 
>          pa = get_acpi_rsdp();
> 
>          if (!pa)
>                  pa = boot_params->acpi_rsdp_addr;
> 
> 
> 	/**
> 	/*I think here we should check if it's kexec booted firstly.
> 	 * Skip it if not kexec. this can avoid the wrong kexec virt
> 	 * addr parsing./
>          if (!pa)
>                  pa = kexec_get_rdsp_addr();             <--- new function
> 
>          if (!pa)
>                  pa = efi_get_rsdp_addr();
> 

Shouldn't t efi_get_rsdp_addr() check "is_kexec_booted" and exit
early so that it never tries to use virtual config_tables pointer
if for some unknown resason kexec_get_rsdp_addr() failed.

Currently I check "is_kexec_booted" by subset of efi_get_setup_data_addr().
Do you know a simpler way to check "is_kexec_booted"?

>          if (!pa)
>                  pa = bios_get_rsdp_addr();
> 
>          return pa;
> }

-- 
Jun'ichi Nomura, NEC Corporation / NEC Solution Innovators, Ltd.
_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: [PATCH v2] x86/boot: Use EFI setup data if provided

[Chao Fan]

On Thu, Mar 28, 2019 at 04:17:16AM +0000, Junichi Nomura wrote:
>On 2019/03/27 10:48, bhe@redhat.com wrote:
>>>> So efi_get_rsdp_addr() needs to be refactored in such a way so that at
>>>> least the loop towards the end gets carved out into a separate function
>>>> - __efi_get_rsdp_addr() or so - which gets config_tables, nr_tables and
>>>> size as arguments and finds the RSDP address in the kexec-ed kernel.
>>>
>>> You need to carve out the loop at the end and make it into a separate
>>> __efi_get_rsdp_addr() function which gets the physical or the virtual
>>> address.
>> 
>> I guess Boris is suggesting code like below. Please correct me if I am
>> wrong.
>> 
>> static acpi_physical_address _efi_get_rsdp_addr(efi_config_table tbl, ...)
>> {
>> 	/* Get EFI tables from systab. */
>> 	for (i = 0; i < nr_tables; i++) {
>> 		...
>> 	}
>> 	return rsdp_addr;
>> }
>> 
>> static acpi_physical_address efi_get_rsdp_addr(void)
>> {
>> 	...
>> 	/* Get systab from boot params. */		
>> 	...
>> 	/* Handle EFI bitness properly */
>> 	...
>> 	return _efi_get_rsdp_addr();
>> }
>> 
>> 
>> static acpi_physical_address kexec_get_rsdp_addr(void)
>> {
>> 	if (!is_kexec_booted)
>> 		return 0;
>> 
>> 	efi_get_setup_data_addr();
>> 	...
>> 	/* Handle EFI bitness properly */
>> 	...
>> 	return _efi_get_rsdp_addr();
>> }
>
>I still don't get it... We still need systab for kexec case as well
>to get nr_tables. Don't we?
>
>> acpi_physical_address get_rsdp_addr(void)
>> {
>>          acpi_physical_address pa;
>> 
>>          pa = get_acpi_rsdp();
>> 
>>          if (!pa)
>>                  pa = boot_params->acpi_rsdp_addr;
>> 
>> 
>> 	/**
>> 	/*I think here we should check if it's kexec booted firstly.
>> 	 * Skip it if not kexec. this can avoid the wrong kexec virt
>> 	 * addr parsing./
>>          if (!pa)
>>                  pa = kexec_get_rdsp_addr();             <--- new function
>> 
>>          if (!pa)
>>                  pa = efi_get_rsdp_addr();
>> 
>
>Shouldn't t efi_get_rsdp_addr() check "is_kexec_booted" and exit
>early so that it never tries to use virtual config_tables pointer
>if for some unknown resason kexec_get_rsdp_addr() failed.

In my understanding, in EFI issue, RSDP is from efi_get_rsdp_addr() or
kexec_get_rdsp_addr()(only in kexec environment). So in these two functions
only one can be excuted, so how about this:

if (is_kexec_booted)
	pa = kexec_get_rdsp_addr();	<--- new function
else
	pa = efi_get_rsdp_addr();

And also split the same code as _efi_get_rsdp_addr().
If there is something wrong in my understanding, please let me know.

Thanks,
Chao Fan

>
>Currently I check "is_kexec_booted" by subset of efi_get_setup_data_addr().
>Do you know a simpler way to check "is_kexec_booted"?
>
>>          if (!pa)
>>                  pa = bios_get_rsdp_addr();
>> 
>>          return pa;
>> }
>
>-- 
>Jun'ichi Nomura, NEC Corporation / NEC Solution Innovators, Ltd.
>



_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: [PATCH v2] x86/boot: Use EFI setup data if provided

[bhe]

On 03/28/19 at 04:17am, Junichi Nomura wrote:
> On 2019/03/27 10:48, bhe@redhat.com wrote:
> >>> So efi_get_rsdp_addr() needs to be refactored in such a way so that at
> >>> least the loop towards the end gets carved out into a separate function
> >>> - __efi_get_rsdp_addr() or so - which gets config_tables, nr_tables and
> >>> size as arguments and finds the RSDP address in the kexec-ed kernel.
> >>
> >> You need to carve out the loop at the end and make it into a separate
> >> __efi_get_rsdp_addr() function which gets the physical or the virtual
> >> address.
> > 
> > I guess Boris is suggesting code like below. Please correct me if I am
> > wrong.
> > 
> > static acpi_physical_address _efi_get_rsdp_addr(efi_config_table tbl, ...)
> > {
> > 	/* Get EFI tables from systab. */
> > 	for (i = 0; i < nr_tables; i++) {
> > 		...
> > 	}
> > 	return rsdp_addr;
> > }
> > 
> > static acpi_physical_address efi_get_rsdp_addr(void)
> > {
> > 	...
> > 	/* Get systab from boot params. */		
> > 	...
> > 	/* Handle EFI bitness properly */
> > 	...
> > 	return _efi_get_rsdp_addr();
> > }
> > 
> > 
> > static acpi_physical_address kexec_get_rsdp_addr(void)
> > {
> > 	if (!is_kexec_booted)
> > 		return 0;
> > 
> > 	efi_get_setup_data_addr();
> > 	...
> > 	/* Handle EFI bitness properly */
> > 	...
> > 	return _efi_get_rsdp_addr();
> > }
> 
> I still don't get it... We still need systab for kexec case as well
> to get nr_tables. Don't we?

Yes, simpler.

As Dave replied in another mail, efi/kexec is only added for x86_64. See
how it does in setup_linux_system_parameters() of kexec_tools utility,
and we only have bzImage64 handling in kernel for kexec_file loading,
see prepare_add_efi_setup_data().

You may only need to get kexec ei_info to use directly.

> 
> > acpi_physical_address get_rsdp_addr(void)
> > {
> >          acpi_physical_address pa;
> > 
> >          pa = get_acpi_rsdp();
> > 
> >          if (!pa)
> >                  pa = boot_params->acpi_rsdp_addr;
> > 
> > 
> > 	/**
> > 	/*I think here we should check if it's kexec booted firstly.
> > 	 * Skip it if not kexec. this can avoid the wrong kexec virt
> > 	 * addr parsing./
> >          if (!pa)
> >                  pa = kexec_get_rdsp_addr();             <--- new function
> > 
> >          if (!pa)
> >                  pa = efi_get_rsdp_addr();
> > 
> 
> Shouldn't t efi_get_rsdp_addr() check "is_kexec_booted" and exit
> early so that it never tries to use virtual config_tables pointer
> if for some unknown resason kexec_get_rsdp_addr() failed.

Well, you can just call your efi_get_setup_data_addr() in
kexec_get_rdsp_addr(), if succeed, go ahead to read ei_info and call
_efi_get_rsdp_addr(). If failed, return to try efi_get_rsdp_addr().

> 
> Currently I check "is_kexec_booted" by subset of efi_get_setup_data_addr().
> Do you know a simpler way to check "is_kexec_booted"?

There seems to be no another way to check, I think your way is good.
Tryint to get it in kexec_get_rdsp_addr() earlier, you don't need to
judge specifically in efi_get_rsdp_addr() again.

> 
> >          if (!pa)
> >                  pa = bios_get_rsdp_addr();
> > 
> >          return pa;
> > }
> 
> -- 
> Jun'ichi Nomura, NEC Corporation / NEC Solution Innovators, Ltd.

_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: [PATCH v2] x86/boot: Use EFI setup data if provided

[Junichi Nomura]

On 2019/03/28 15:43, bhe@redhat.com wrote:
> On 03/28/19 at 04:17am, Junichi Nomura wrote:
>> I still don't get it... We still need systab for kexec case as well
>> to get nr_tables. Don't we?
> 
> Yes, simpler.
> 
> As Dave replied in another mail, efi/kexec is only added for x86_64. See
> how it does in setup_linux_system_parameters() of kexec_tools utility,
> and we only have bzImage64 handling in kernel for kexec_file loading,
> see prepare_add_efi_setup_data().
> 
> You may only need to get kexec ei_info to use directly.

OK, let me try. How does this look?


Commit 3a63f70bf4c3a ("x86/boot: Early parse RSDP and save it in
boot_params") broke kexec boot on EFI systems.  efi_get_rsdp_addr()
in the early parsing code tries to search RSDP from EFI table but
that will crash because the table address is virtual when the kernel
was booted by kexec.

Since kexec(1) provides physical address of the table via efi_setup_data,
early boot code of kexec-ed kernel should look for setup_data in the same
way as efi_systab_init() in arch/x86/platform/efi/efi.c does.

Fixes: 3a63f70bf4c3a ("x86/boot: Early parse RSDP and save it in boot_params")
Signed-off-by: Jun'ichi Nomura <j-nomura@ce.jp.nec.com>
Cc: Chao Fan <fanc.fnst@cn.fujitsu.com>
Cc: Borislav Petkov <bp@suse.de>
Cc: Dave Young <dyoung@redhat.com>
Cc: Baoquan He <bhe@redhat.com>

diff --git a/arch/x86/boot/compressed/acpi.c b/arch/x86/boot/compressed/acpi.c
--- a/arch/x86/boot/compressed/acpi.c
+++ b/arch/x86/boot/compressed/acpi.c
@@ -44,17 +44,106 @@ static acpi_physical_address get_acpi_rsdp(void)
 	return addr;
 }
 
+static unsigned long efi_get_kexec_setup_data_addr(void)
+{
+#ifdef CONFIG_EFI
+	struct setup_data *data;
+	u64 pa_data;
+
+	pa_data = boot_params->hdr.setup_data;
+	while (pa_data) {
+		data = (struct setup_data *) pa_data;
+		if (data->type == SETUP_EFI)
+			return pa_data + sizeof(struct setup_data);
+		pa_data = data->next;
+	}
+#endif
+	return 0;
+}
+
 /* Search EFI system tables for RSDP. */
-static acpi_physical_address efi_get_rsdp_addr(void)
+static acpi_physical_address __efi_get_rsdp_addr(unsigned long config_tables, unsigned int nr_tables, int size, bool efi_64)
 {
 	acpi_physical_address rsdp_addr = 0;
 
 #ifdef CONFIG_EFI
-	unsigned long systab, systab_tables, config_tables;
+	int i;
+
+	/* Get EFI tables from systab. */
+	for (i = 0; i < nr_tables; i++) {
+		acpi_physical_address table;
+		efi_guid_t guid;
+
+		config_tables += size;
+
+		if (efi_64) {
+			efi_config_table_64_t *tbl = (efi_config_table_64_t *)config_tables;
+
+			guid  = tbl->guid;
+			table = tbl->table;
+
+			if (!IS_ENABLED(CONFIG_X86_64) && table >> 32) {
+				debug_putstr("Error getting RSDP address: EFI config table located above 4GB.\n");
+				return 0;
+			}
+		} else {
+			efi_config_table_32_t *tbl = (efi_config_table_32_t *)config_tables;
+
+			guid  = tbl->guid;
+			table = tbl->table;
+		}
+
+		if (!(efi_guidcmp(guid, ACPI_TABLE_GUID)))
+			rsdp_addr = table;
+		else if (!(efi_guidcmp(guid, ACPI_20_TABLE_GUID)))
+			return table;
+	}
+#endif
+	return rsdp_addr;
+}
+
+static acpi_physical_address kexec_get_rsdp_addr(void)
+{
+#ifdef CONFIG_EFI
+	struct efi_setup_data *esd;
+	efi_system_table_64_t *systab;
+	struct efi_info *ei;
+	char *sig;
+
+	esd = (struct efi_setup_data *) efi_get_kexec_setup_data_addr();
+	if (!esd)
+		return 0;
+	if (!esd->tables) {
+		debug_putstr("Wrong kexec SETUP_EFI data.\n");
+		return 0;
+	}
+
+	ei = &boot_params->efi_info;
+	sig = (char *)&ei->efi_loader_signature;
+	if (strncmp(sig, EFI64_LOADER_SIGNATURE, 4)) {
+		debug_putstr("Wrong EFI loader signature.\n");
+		return 0;
+	}
+
+	/* Get systab from boot params. */
+	systab = (efi_system_table_64_t *) (ei->efi_systab | ((__u64)ei->efi_systab_hi << 32));
+	if (!systab)
+		error("EFI system table not found.");
+
+	return __efi_get_rsdp_addr((unsigned long) esd->tables, systab->nr_tables, sizeof(efi_config_table_64_t), true);
+#else
+	return 0;
+#endif
+}
+
+static acpi_physical_address efi_get_rsdp_addr(void)
+{
+#ifdef CONFIG_EFI
+	unsigned long systab, config_tables;
 	unsigned int nr_tables;
 	struct efi_info *ei;
 	bool efi_64;
-	int size, i;
+	int size;
 	char *sig;
 
 	ei = &boot_params->efi_info;
@@ -100,37 +189,10 @@ static acpi_physical_address efi_get_rsdp_addr(void)
 	if (!config_tables)
 		error("EFI config tables not found.");
 
-	/* Get EFI tables from systab. */
-	for (i = 0; i < nr_tables; i++) {
-		acpi_physical_address table;
-		efi_guid_t guid;
-
-		config_tables += size;
-
-		if (efi_64) {
-			efi_config_table_64_t *tbl = (efi_config_table_64_t *)config_tables;
-
-			guid  = tbl->guid;
-			table = tbl->table;
-
-			if (!IS_ENABLED(CONFIG_X86_64) && table >> 32) {
-				debug_putstr("Error getting RSDP address: EFI config table located above 4GB.\n");
-				return 0;
-			}
-		} else {
-			efi_config_table_32_t *tbl = (efi_config_table_32_t *)config_tables;
-
-			guid  = tbl->guid;
-			table = tbl->table;
-		}
-
-		if (!(efi_guidcmp(guid, ACPI_TABLE_GUID)))
-			rsdp_addr = table;
-		else if (!(efi_guidcmp(guid, ACPI_20_TABLE_GUID)))
-			return table;
-	}
+	return __efi_get_rsdp_addr(config_tables, nr_tables, size, efi_64);
+#else
+	return 0;
 #endif
-	return rsdp_addr;
 }
 
 static u8 compute_checksum(u8 *buffer, u32 length)
@@ -221,6 +283,9 @@ acpi_physical_address get_rsdp_addr(void)
 		pa = boot_params->acpi_rsdp_addr;
 
 	if (!pa)
+		pa = kexec_get_rsdp_addr();
+
+	if (!pa)
 		pa = efi_get_rsdp_addr();
 
 	if (!pa)

_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: [PATCH v2] x86/boot: Use EFI setup data if provided

[Borislav Petkov]

On Thu, Mar 28, 2019 at 07:43:38AM +0000, Junichi Nomura wrote:
>  /* Search EFI system tables for RSDP. */
> -static acpi_physical_address efi_get_rsdp_addr(void)
> +static acpi_physical_address __efi_get_rsdp_addr(unsigned long config_tables, unsigned int nr_tables, int size, bool efi_64)

Break this line like this:

static acpi_physical_address
__efi_get_rsdp_addr(unsigned long config_tables, unsigned int nr_tables,
                    int size, bool efi_64)


>  {
>  	acpi_physical_address rsdp_addr = 0;

You don't need that variable and can return "table" or 0 after the endif
below.

>  #ifdef CONFIG_EFI
> -	unsigned long systab, systab_tables, config_tables;
> +	int i;
> +
> +	/* Get EFI tables from systab. */
> +	for (i = 0; i < nr_tables; i++) {
> +		acpi_physical_address table;
> +		efi_guid_t guid;
> +
> +		config_tables += size;
> +
> +		if (efi_64) {
> +			efi_config_table_64_t *tbl = (efi_config_table_64_t *)config_tables;
> +
> +			guid  = tbl->guid;
> +			table = tbl->table;
> +
> +			if (!IS_ENABLED(CONFIG_X86_64) && table >> 32) {
> +				debug_putstr("Error getting RSDP address: EFI config table located above 4GB.\n");
> +				return 0;
> +			}
> +		} else {
> +			efi_config_table_32_t *tbl = (efi_config_table_32_t *)config_tables;
> +
> +			guid  = tbl->guid;
> +			table = tbl->table;
> +		}
> +
> +		if (!(efi_guidcmp(guid, ACPI_TABLE_GUID)))
> +			rsdp_addr = table;
> +		else if (!(efi_guidcmp(guid, ACPI_20_TABLE_GUID)))
> +			return table;
> +	}
> +#endif
> +	return rsdp_addr;
> +}
> +
> +static acpi_physical_address kexec_get_rsdp_addr(void)
> +{
> +#ifdef CONFIG_EFI
> +	struct efi_setup_data *esd;
> +	efi_system_table_64_t *systab;
> +	struct efi_info *ei;
> +	char *sig;

Please sort function local variables declaration in a reverse christmas
tree order:

	<type A> longest_variable_name;
	<type B> shorter_var_name;
	<type C> even_shorter;
	<type D> i;

> +
> +	esd = (struct efi_setup_data *) efi_get_kexec_setup_data_addr();
> +	if (!esd)
> +		return 0;

<---- newline here.

> +	if (!esd->tables) {
> +		debug_putstr("Wrong kexec SETUP_EFI data.\n");
> +		return 0;
> +	}
> +
> +	ei = &boot_params->efi_info;
> +	sig = (char *)&ei->efi_loader_signature;
> +	if (strncmp(sig, EFI64_LOADER_SIGNATURE, 4)) {
> +		debug_putstr("Wrong EFI loader signature.\n");

  "Wrong kexec EFI loader signature."

so that it differs from the other error message and we can know where we
are when debugging.

> +		return 0;
> +	}
> +
> +	/* Get systab from boot params. */
> +	systab = (efi_system_table_64_t *) (ei->efi_systab | ((__u64)ei->efi_systab_hi << 32));
> +	if (!systab)
> +		error("EFI system table not found.");

Ditto.

> +
> +	return __efi_get_rsdp_addr((unsigned long) esd->tables, systab->nr_tables, sizeof(efi_config_table_64_t), true);

You can easily break that line - no need to let it stick out.

> +#else
> +	return 0;
> +#endif
> +}

Other than those nitpicks, yes, it looks ok to me. Please send a proper
patch.

Thx.

-- 
Regards/Gruss,
    Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.

_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: [PATCH v2] x86/boot: Use EFI setup data if provided

[Junichi Nomura]

On 3/29/19 12:52 AM, Borislav Petkov wrote:
> static acpi_physical_address
> __efi_get_rsdp_addr(unsigned long config_tables, unsigned int nr_tables,
>                     int size, bool efi_64)
> 
> 
>>  {
>>  	acpi_physical_address rsdp_addr = 0;
> 
> You don't need that variable and can return "table" or 0 after the endif
> below.

I could do that but it will slightly change the current logic.

Existing code does this:

                if (!(efi_guidcmp(guid, ACPI_TABLE_GUID)))
                        rsdp_addr = table;
                else if (!(efi_guidcmp(guid, ACPI_20_TABLE_GUID)))
                        return table;

I thought it was to return the table for ACPI_20_TABLE_GUID
if both tables exist.  If we remove rsdp_addr, the code will be:

                if (!(efi_guidcmp(guid, ACPI_TABLE_GUID)))
                        return table;
                else if (!(efi_guidcmp(guid, ACPI_20_TABLE_GUID)))
                        return table;

So if there are 2 tables, we return the one that comes first.
Is it ok?

-- 
Jun'ichi Nomura, NEC Corporation / NEC Solution Innovators, Ltd.
_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: [PATCH v2] x86/boot: Use EFI setup data if provided

[Borislav Petkov]

On Fri, Mar 29, 2019 at 03:05:52AM +0000, Junichi Nomura wrote:
> > You don't need that variable and can return "table" or 0 after the endif
> > below.
> 
> I could do that but it will slightly change the current logic.
> 
> Existing code does this:
> 
>                 if (!(efi_guidcmp(guid, ACPI_TABLE_GUID)))
>                         rsdp_addr = table;
>                 else if (!(efi_guidcmp(guid, ACPI_20_TABLE_GUID)))
>                         return table;
> 
> I thought it was to return the table for ACPI_20_TABLE_GUID
> if both tables exist.  If we remove rsdp_addr, the code will be:
> 
>                 if (!(efi_guidcmp(guid, ACPI_TABLE_GUID)))
>                         return table;
>                 else if (!(efi_guidcmp(guid, ACPI_20_TABLE_GUID)))
>                         return table;
> 
> So if there are 2 tables, we return the one that comes first.
> Is it ok?

That's a good question.

Chao, what was the intention there, ACPI_20_TABLE_GUID is the preferred
table to return? If so, why?

-- 
Regards/Gruss,
    Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.

_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: [PATCH v2] x86/boot: Use EFI setup data if provided

[Chao Fan]

On Fri, Mar 29, 2019 at 09:39:20AM +0100, Borislav Petkov wrote:
>On Fri, Mar 29, 2019 at 03:05:52AM +0000, Junichi Nomura wrote:
>> > You don't need that variable and can return "table" or 0 after the endif
>> > below.
>> 
>> I could do that but it will slightly change the current logic.
>> 
>> Existing code does this:
>> 
>>                 if (!(efi_guidcmp(guid, ACPI_TABLE_GUID)))
>>                         rsdp_addr = table;
>>                 else if (!(efi_guidcmp(guid, ACPI_20_TABLE_GUID)))
>>                         return table;
>> 
>> I thought it was to return the table for ACPI_20_TABLE_GUID
>> if both tables exist.  If we remove rsdp_addr, the code will be:
>> 
>>                 if (!(efi_guidcmp(guid, ACPI_TABLE_GUID)))
>>                         return table;
>>                 else if (!(efi_guidcmp(guid, ACPI_20_TABLE_GUID)))
>>                         return table;
>> 
>> So if there are 2 tables, we return the one that comes first.
>> Is it ok?
>
>That's a good question.
>
>Chao, what was the intention there, ACPI_20_TABLE_GUID is the preferred
>table to return? If so, why?

Yes, ACPI_20_TABLE_GUID is preferred.

ACPI_20 means version 2.0 and later versions, ACPI_TABLE_GUID is version 1.0
which is earlier than 2003, it's too old. Version 2.0 has more features than 1.0.
Sure the new version is preferred.

So many codes prefers ACPI20, such as in drivers/acpi/osl.c where kernel
parses RSDP ACPI20 firstly. Documentation/ABI/testing/sysfs-firmware-efi says in
/sys/firmware/efi/systab, ACPI20 comes before ACPI. So that kexec-tools
code kexec/arch/i386/crashdump-x86.c can easily get ACPI_20(if there is
ACPI_20) before ACPI 1.0.
But in my code, I am not sure which version will be found firstly, so I
write this logical, if ACPI20 found, return directly, then consider ACPI 1.0.

Thanks,
Chao Fan

>-- 
>Regards/Gruss,
>    Boris.
>
>Good mailing practices for 400: avoid top-posting and trim the reply.
>
>



_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: [PATCH v2] x86/boot: Use EFI setup data if provided

[Borislav Petkov]

On Fri, Mar 29, 2019 at 05:05:50PM +0800, Chao Fan wrote:
> But in my code, I am not sure which version will be found firstly, so I
> write this logical, if ACPI20 found, return directly, then consider ACPI 1.0.

Thanks.

Junichi, please add a shorter version of that as a comment to the code,
above the function name so that it is clear why we're preferring the 2.0
version.

Thanks.

-- 
Regards/Gruss,
    Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.

_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: [PATCH v2] x86/boot: Use EFI setup data if provided

[Junichi Nomura]

On 3/29/19 6:16 PM, Borislav Petkov wrote:
> On Fri, Mar 29, 2019 at 05:05:50PM +0800, Chao Fan wrote:
>> But in my code, I am not sure which version will be found firstly, so I
>> write this logical, if ACPI20 found, return directly, then consider ACPI 1.0.
> 
> Thanks.
> 
> Junichi, please add a shorter version of that as a comment to the code,
> above the function name so that it is clear why we're preferring the 2.0
> version.

Sure, I'll add this above __efi_get_rsdp_addr().

/*
 * Search EFI system tables for RSDP.  If both ACPI_20_TABLE_GUID and
 * ACPI_TABLE_GUID are found, take the former, which has more features.
 */

-- 
Jun'ichi Nomura, NEC Corporation / NEC Solution Innovators, Ltd.
_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: [PATCH v2] x86/boot: Use EFI setup data if provided

[Chao Fan]

On Fri, Mar 29, 2019 at 09:37:00AM +0000, Junichi Nomura wrote:
>On 3/29/19 6:16 PM, Borislav Petkov wrote:
>> On Fri, Mar 29, 2019 at 05:05:50PM +0800, Chao Fan wrote:
>>> But in my code, I am not sure which version will be found firstly, so I
>>> write this logical, if ACPI20 found, return directly, then consider ACPI 1.0.
>> 
>> Thanks.
>> 
>> Junichi, please add a shorter version of that as a comment to the code,
>> above the function name so that it is clear why we're preferring the 2.0
>> version.
>
>Sure, I'll add this above __efi_get_rsdp_addr().
>
>/*
> * Search EFI system tables for RSDP.  If both ACPI_20_TABLE_GUID and
> * ACPI_TABLE_GUID are found, take the former, which has more features.
> */
>

I notice in my host machine, the two tables are the same:
[17:38:11] cfan@localhost /home/cfan (0)
> sudo cat /sys/firmware/efi/systab
[sudo] password for cfan:
MPS=0xfd420
ACPI20=0xdb807000
ACPI=0xdb807000
SMBIOS=0xf04c0

But in my qemu guest, they are different and the address of ACPI20 is
higher than ACPI 1.0:
[root@localhost ~]# cat /sys/firmware/efi/systab
ACPI20=0xbfbfa014
ACPI=0xbfbfa000
SMBIOS=0xbfbcc000

In this condition, ACPI 1.0 comes before ACPI 2.0. So I suggested you
to keep this logical.

Thanks,
Chao Fan

>-- 
>Jun'ichi Nomura, NEC Corporation / NEC Solution Innovators, Ltd.
>



_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: [PATCH v2] x86/boot: Use EFI setup data if provided

[Junichi Nomura]

On 3/29/19 6:44 PM, Chao Fan wrote:
> But in my qemu guest, they are different and the address of ACPI20 is
> higher than ACPI 1.0:
> [root@localhost ~]# cat /sys/firmware/efi/systab
> ACPI20=0xbfbfa014
> ACPI=0xbfbfa000
> SMBIOS=0xbfbcc000
> 
> In this condition, ACPI 1.0 comes before ACPI 2.0. So I suggested you
> to keep this logical.

Thank you for the information. The proposed patch keeps the logic
and should work fine on that case as well.

-- 
Jun'ichi Nomura, NEC Corporation / NEC Solution Innovators, Ltd.

_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

[PATCH] x86/boot: Use efi_setup_data for searching RSDP on kexec-ed kernel

[Junichi Nomura]

Commit 3a63f70bf4c3a ("x86/boot: Early parse RSDP and save it in
boot_params") broke kexec boot on EFI systems.  efi_get_rsdp_addr()
in the early parsing code tries to search RSDP from EFI table but
that will crash because the table address is virtual when the kernel
was booted by kexec.

In the case of kexec, physical address of EFI tables is provided
via efi_setup_data in boot_params, which is set up by kexec(1).

Factor out the table parsing code and use different pointers depending
on whether the kernel is booted by kexec or not.

Fixes: 3a63f70bf4c3a ("x86/boot: Early parse RSDP and save it in boot_params")
Signed-off-by: Jun'ichi Nomura <j-nomura@ce.jp.nec.com>
Cc: Chao Fan <fanc.fnst@cn.fujitsu.com>
Cc: Borislav Petkov <bp@suse.de>
Cc: Dave Young <dyoung@redhat.com>
Cc: Baoquan He <bhe@redhat.com>

diff --git a/arch/x86/boot/compressed/acpi.c b/arch/x86/boot/compressed/acpi.c
--- a/arch/x86/boot/compressed/acpi.c
+++ b/arch/x86/boot/compressed/acpi.c
@@ -44,17 +44,109 @@ static acpi_physical_address get_acpi_rsdp(void)
 	return addr;
 }
 
+#ifdef CONFIG_EFI
+static unsigned long efi_get_kexec_setup_data_addr(void)
+{
+	struct setup_data *data;
+	u64 pa_data;
+
+	pa_data = boot_params->hdr.setup_data;
+	while (pa_data) {
+		data = (struct setup_data *) pa_data;
+		if (data->type == SETUP_EFI)
+			return pa_data + sizeof(struct setup_data);
+		pa_data = data->next;
+	}
+	return 0;
+}
+
 /* Search EFI system tables for RSDP. */
-static acpi_physical_address efi_get_rsdp_addr(void)
+static acpi_physical_address
+__efi_get_rsdp_addr(unsigned long config_tables, unsigned int nr_tables,
+		    bool efi_64)
 {
 	acpi_physical_address rsdp_addr = 0;
+	int i;
+
+	/* Get EFI tables from systab. */
+	for (i = 0; i < nr_tables; i++) {
+		acpi_physical_address table;
+		efi_guid_t guid;
+
+		if (efi_64) {
+			efi_config_table_64_t *tbl = (efi_config_table_64_t *) config_tables + i;
+
+			guid  = tbl->guid;
+			table = tbl->table;
+
+			if (!IS_ENABLED(CONFIG_X86_64) && table >> 32) {
+				debug_putstr("Error getting RSDP address: EFI config table located above 4GB.\n");
+				return 0;
+			}
+		} else {
+			efi_config_table_32_t *tbl = (efi_config_table_32_t *) config_tables + i;
+
+			guid  = tbl->guid;
+			table = tbl->table;
+		}
+
+		if (!(efi_guidcmp(guid, ACPI_TABLE_GUID)))
+			rsdp_addr = table;
+		else if (!(efi_guidcmp(guid, ACPI_20_TABLE_GUID)))
+			return table;
+	}
+
+	return rsdp_addr;
+}
+#endif
 
+static acpi_physical_address kexec_get_rsdp_addr(void)
+{
 #ifdef CONFIG_EFI
-	unsigned long systab, systab_tables, config_tables;
+	efi_system_table_64_t *systab;
+	struct efi_setup_data *esd;
+	struct efi_info *ei;
+	char *sig;
+
+	esd = (struct efi_setup_data *) efi_get_kexec_setup_data_addr();
+	if (!esd)
+		return 0;
+
+	if (!esd->tables) {
+		debug_putstr("Wrong kexec SETUP_EFI data.\n");
+		return 0;
+	}
+
+	ei = &boot_params->efi_info;
+	sig = (char *)&ei->efi_loader_signature;
+	/*
+	 * EFI/kexec support is only added for 64bit. So we don't have to
+	 * care 32bit case.
+	 */
+	if (strncmp(sig, EFI64_LOADER_SIGNATURE, 4)) {
+		debug_putstr("Wrong kexec EFI loader signature.\n");
+		return 0;
+	}
+
+	/* Get systab from boot params. */
+	systab = (efi_system_table_64_t *) (ei->efi_systab | ((__u64)ei->efi_systab_hi << 32));
+	if (!systab)
+		error("EFI system table not found in kexec boot_params.");
+
+	return __efi_get_rsdp_addr((unsigned long) esd->tables,
+				   systab->nr_tables, true);
+#else
+	return 0;
+#endif
+}
+
+static acpi_physical_address efi_get_rsdp_addr(void)
+{
+#ifdef CONFIG_EFI
+	unsigned long systab, config_tables;
 	unsigned int nr_tables;
 	struct efi_info *ei;
 	bool efi_64;
-	int size, i;
 	char *sig;
 
 	ei = &boot_params->efi_info;
@@ -88,49 +180,20 @@ static acpi_physical_address efi_get_rsdp_addr(void)
 
 		config_tables	= stbl->tables;
 		nr_tables	= stbl->nr_tables;
-		size		= sizeof(efi_config_table_64_t);
 	} else {
 		efi_system_table_32_t *stbl = (efi_system_table_32_t *)systab;
 
 		config_tables	= stbl->tables;
 		nr_tables	= stbl->nr_tables;
-		size		= sizeof(efi_config_table_32_t);
 	}
 
 	if (!config_tables)
 		error("EFI config tables not found.");
 
-	/* Get EFI tables from systab. */
-	for (i = 0; i < nr_tables; i++) {
-		acpi_physical_address table;
-		efi_guid_t guid;
-
-		config_tables += size;
-
-		if (efi_64) {
-			efi_config_table_64_t *tbl = (efi_config_table_64_t *)config_tables;
-
-			guid  = tbl->guid;
-			table = tbl->table;
-
-			if (!IS_ENABLED(CONFIG_X86_64) && table >> 32) {
-				debug_putstr("Error getting RSDP address: EFI config table located above 4GB.\n");
-				return 0;
-			}
-		} else {
-			efi_config_table_32_t *tbl = (efi_config_table_32_t *)config_tables;
-
-			guid  = tbl->guid;
-			table = tbl->table;
-		}
-
-		if (!(efi_guidcmp(guid, ACPI_TABLE_GUID)))
-			rsdp_addr = table;
-		else if (!(efi_guidcmp(guid, ACPI_20_TABLE_GUID)))
-			return table;
-	}
+	return __efi_get_rsdp_addr(config_tables, nr_tables, efi_64);
+#else
+	return 0;
 #endif
-	return rsdp_addr;
 }
 
 static u8 compute_checksum(u8 *buffer, u32 length)
@@ -221,6 +284,9 @@ acpi_physical_address get_rsdp_addr(void)
 		pa = boot_params->acpi_rsdp_addr;
 
 	if (!pa)
+		pa = kexec_get_rsdp_addr();
+
+	if (!pa)
 		pa = efi_get_rsdp_addr();
 
 	if (!pa)

_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: [PATCH] x86/boot: Use efi_setup_data for searching RSDP on kexec-ed kernel

[bhe]

On 03/29/19 at 07:20am, Junichi Nomura wrote:
> Commit 3a63f70bf4c3a ("x86/boot: Early parse RSDP and save it in
> boot_params") broke kexec boot on EFI systems.  efi_get_rsdp_addr()
> in the early parsing code tries to search RSDP from EFI table but
> that will crash because the table address is virtual when the kernel
> was booted by kexec.
> 
> In the case of kexec, physical address of EFI tables is provided
> via efi_setup_data in boot_params, which is set up by kexec(1).
> 
> Factor out the table parsing code and use different pointers depending
> on whether the kernel is booted by kexec or not.
> 
> Fixes: 3a63f70bf4c3a ("x86/boot: Early parse RSDP and save it in boot_params")
> Signed-off-by: Jun'ichi Nomura <j-nomura@ce.jp.nec.com>
> Cc: Chao Fan <fanc.fnst@cn.fujitsu.com>
> Cc: Borislav Petkov <bp@suse.de>
> Cc: Dave Young <dyoung@redhat.com>
> Cc: Baoquan He <bhe@redhat.com>
> 
> diff --git a/arch/x86/boot/compressed/acpi.c b/arch/x86/boot/compressed/acpi.c
> --- a/arch/x86/boot/compressed/acpi.c
> +++ b/arch/x86/boot/compressed/acpi.c
> @@ -44,17 +44,109 @@ static acpi_physical_address get_acpi_rsdp(void)
>  	return addr;
>  }
>  
> +#ifdef CONFIG_EFI
> +static unsigned long efi_get_kexec_setup_data_addr(void)
> +{
> +	struct setup_data *data;
> +	u64 pa_data;
> +
> +	pa_data = boot_params->hdr.setup_data;
> +	while (pa_data) {
> +		data = (struct setup_data *) pa_data;
> +		if (data->type == SETUP_EFI)
> +			return pa_data + sizeof(struct setup_data);
> +		pa_data = data->next;
> +	}
> +	return 0;
> +}
> +
>  /* Search EFI system tables for RSDP. */
> -static acpi_physical_address efi_get_rsdp_addr(void)
> +static acpi_physical_address
> +__efi_get_rsdp_addr(unsigned long config_tables, unsigned int nr_tables,
> +		    bool efi_64)
>  {
>  	acpi_physical_address rsdp_addr = 0;
> +	int i;
> +
> +	/* Get EFI tables from systab. */
> +	for (i = 0; i < nr_tables; i++) {
> +		acpi_physical_address table;
> +		efi_guid_t guid;
> +
> +		if (efi_64) {
> +			efi_config_table_64_t *tbl = (efi_config_table_64_t *) config_tables + i;
> +
> +			guid  = tbl->guid;
> +			table = tbl->table;
> +
> +			if (!IS_ENABLED(CONFIG_X86_64) && table >> 32) {
> +				debug_putstr("Error getting RSDP address: EFI config table located above 4GB.\n");
> +				return 0;
> +			}
> +		} else {
> +			efi_config_table_32_t *tbl = (efi_config_table_32_t *) config_tables + i;
> +
> +			guid  = tbl->guid;
> +			table = tbl->table;
> +		}
> +
> +		if (!(efi_guidcmp(guid, ACPI_TABLE_GUID)))
> +			rsdp_addr = table;
> +		else if (!(efi_guidcmp(guid, ACPI_20_TABLE_GUID)))
> +			return table;
> +	}
> +
> +	return rsdp_addr;
> +}
> +#endif
>  
> +static acpi_physical_address kexec_get_rsdp_addr(void)
> +{
>  #ifdef CONFIG_EFI
> -	unsigned long systab, systab_tables, config_tables;
> +	efi_system_table_64_t *systab;
> +	struct efi_setup_data *esd;
> +	struct efi_info *ei;
> +	char *sig;
> +
> +	esd = (struct efi_setup_data *) efi_get_kexec_setup_data_addr();
> +	if (!esd)
> +		return 0;
> +
> +	if (!esd->tables) {
> +		debug_putstr("Wrong kexec SETUP_EFI data.\n");
> +		return 0;
> +	}
> +
> +	ei = &boot_params->efi_info;
> +	sig = (char *)&ei->efi_loader_signature;
> +	/*
> +	 * EFI/kexec support is only added for 64bit. So we don't have to
> +	 * care 32bit case.
> +	 */
I would put the doc above kexec_get_rsdp_addr(). Other than this, it
looks good to me. Thanks for the effort.

Acked-by: Baoquan He <bhe@redhat.com>


> +	if (strncmp(sig, EFI64_LOADER_SIGNATURE, 4)) {
> +		debug_putstr("Wrong kexec EFI loader signature.\n");
> +		return 0;
> +	}
> +
> +	/* Get systab from boot params. */
> +	systab = (efi_system_table_64_t *) (ei->efi_systab | ((__u64)ei->efi_systab_hi << 32));
> +	if (!systab)
> +		error("EFI system table not found in kexec boot_params.");
> +
> +	return __efi_get_rsdp_addr((unsigned long) esd->tables,
> +				   systab->nr_tables, true);
> +#else
> +	return 0;
> +#endif
> +}
> +
> +static acpi_physical_address efi_get_rsdp_addr(void)
> +{
> +#ifdef CONFIG_EFI
> +	unsigned long systab, config_tables;
>  	unsigned int nr_tables;
>  	struct efi_info *ei;
>  	bool efi_64;
> -	int size, i;
>  	char *sig;
>  
>  	ei = &boot_params->efi_info;
> @@ -88,49 +180,20 @@ static acpi_physical_address efi_get_rsdp_addr(void)
>  
>  		config_tables	= stbl->tables;
>  		nr_tables	= stbl->nr_tables;
> -		size		= sizeof(efi_config_table_64_t);
>  	} else {
>  		efi_system_table_32_t *stbl = (efi_system_table_32_t *)systab;
>  
>  		config_tables	= stbl->tables;
>  		nr_tables	= stbl->nr_tables;
> -		size		= sizeof(efi_config_table_32_t);
>  	}
>  
>  	if (!config_tables)
>  		error("EFI config tables not found.");
>  
> -	/* Get EFI tables from systab. */
> -	for (i = 0; i < nr_tables; i++) {
> -		acpi_physical_address table;
> -		efi_guid_t guid;
> -
> -		config_tables += size;
> -
> -		if (efi_64) {
> -			efi_config_table_64_t *tbl = (efi_config_table_64_t *)config_tables;
> -
> -			guid  = tbl->guid;
> -			table = tbl->table;
> -
> -			if (!IS_ENABLED(CONFIG_X86_64) && table >> 32) {
> -				debug_putstr("Error getting RSDP address: EFI config table located above 4GB.\n");
> -				return 0;
> -			}
> -		} else {
> -			efi_config_table_32_t *tbl = (efi_config_table_32_t *)config_tables;
> -
> -			guid  = tbl->guid;
> -			table = tbl->table;
> -		}
> -
> -		if (!(efi_guidcmp(guid, ACPI_TABLE_GUID)))
> -			rsdp_addr = table;
> -		else if (!(efi_guidcmp(guid, ACPI_20_TABLE_GUID)))
> -			return table;
> -	}
> +	return __efi_get_rsdp_addr(config_tables, nr_tables, efi_64);
> +#else
> +	return 0;
>  #endif
> -	return rsdp_addr;
>  }
>  
>  static u8 compute_checksum(u8 *buffer, u32 length)
> @@ -221,6 +284,9 @@ acpi_physical_address get_rsdp_addr(void)
>  		pa = boot_params->acpi_rsdp_addr;
>  
>  	if (!pa)
> +		pa = kexec_get_rsdp_addr();
> +
> +	if (!pa)
>  		pa = efi_get_rsdp_addr();
>  
>  	if (!pa)

_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: [PATCH] x86/boot: Use efi_setup_data for searching RSDP on kexec-ed kernel

[Chao Fan]

On Fri, Mar 29, 2019 at 07:20:38AM +0000, Junichi Nomura wrote:
>Commit 3a63f70bf4c3a ("x86/boot: Early parse RSDP and save it in
>boot_params") broke kexec boot on EFI systems.  efi_get_rsdp_addr()
>in the early parsing code tries to search RSDP from EFI table but
>that will crash because the table address is virtual when the kernel
>was booted by kexec.
[...]
>-			guid  = tbl->guid;
>-			table = tbl->table;
>-		}
>-
>-		if (!(efi_guidcmp(guid, ACPI_TABLE_GUID)))
>-			rsdp_addr = table;
>-		else if (!(efi_guidcmp(guid, ACPI_20_TABLE_GUID)))
>-			return table;
>-	}
>+	return __efi_get_rsdp_addr(config_tables, nr_tables, efi_64);
>+#else
>+	return 0;
> #endif
>-	return rsdp_addr;

I remeber the rsdp_addr is defined before #ifdef CONFIG_EFI
If so, you don't need
#else
	return 0;

BY the way, what's your patch based on? I like add patch on my local
branch and then review code, but failed.
I try to use 'patch -p1 <' your patch to the latest tip master branch,
but failed.

Thanks,
Chao Fan

> }
> 
> static u8 compute_checksum(u8 *buffer, u32 length)
>@@ -221,6 +284,9 @@ acpi_physical_address get_rsdp_addr(void)
> 		pa = boot_params->acpi_rsdp_addr;
> 
> 	if (!pa)
>+		pa = kexec_get_rsdp_addr();
>+
>+	if (!pa)
> 		pa = efi_get_rsdp_addr();
> 
> 	if (!pa)
>
>



_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: [PATCH] x86/boot: Use efi_setup_data for searching RSDP on kexec-ed kernel

[Junichi Nomura]

On 3/29/19 5:29 PM, Chao Fan wrote:
> On Fri, Mar 29, 2019 at 07:20:38AM +0000, Junichi Nomura wrote:
>> +	return __efi_get_rsdp_addr(config_tables, nr_tables, efi_64);
>> +#else
>> +	return 0;
>> #endif
>> -	return rsdp_addr;
> 
> I remeber the rsdp_addr is defined before #ifdef CONFIG_EFI
> If so, you don't need
> #else
> 	return 0;

I moved the whole __efi_get_rsdp_addr() to the inside of #ifdef CONFIG_EFI
and both kexec_get_rsdp_addr() and efi_get_rsdp_addr() just return 0 if
CONFIG_EFI is not defined.

> BY the way, what's your patch based on? I like add patch on my local
> branch and then review code, but failed.
> I try to use 'patch -p1 <' your patch to the latest tip master branch,
> but failed.

The patch is based on Linus's v5.1-rc2.

-- 
Jun'ichi Nomura, NEC Corporation / NEC Solution Innovators, Ltd.

_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: [PATCH] x86/boot: Use efi_setup_data for searching RSDP on kexec-ed kernel

[Chao Fan]

On Fri, Mar 29, 2019 at 08:39:45AM +0000, Junichi Nomura wrote:
>On 3/29/19 5:29 PM, Chao Fan wrote:
>> On Fri, Mar 29, 2019 at 07:20:38AM +0000, Junichi Nomura wrote:
>>> +	return __efi_get_rsdp_addr(config_tables, nr_tables, efi_64);
>>> +#else
>>> +	return 0;
>>> #endif
>>> -	return rsdp_addr;
>> 
>> I remeber the rsdp_addr is defined before #ifdef CONFIG_EFI
>> If so, you don't need
>> #else
>> 	return 0;
>
>I moved the whole __efi_get_rsdp_addr() to the inside of #ifdef CONFIG_EFI
>and both kexec_get_rsdp_addr() and efi_get_rsdp_addr() just return 0 if
>CONFIG_EFI is not defined.

Ah, got it. I will add the patch and do a simple test.
>
>> BY the way, what's your patch based on? I like add patch on my local
>> branch and then review code, but failed.
>> I try to use 'patch -p1 <' your patch to the latest tip master branch,
>> but failed.
>
>The patch is based on Linus's v5.1-rc2.

Thanks,
Chao Fan

>
>-- 
>Jun'ichi Nomura, NEC Corporation / NEC Solution Innovators, Ltd.
>
>



_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: [PATCH] x86/boot: Use efi_setup_data for searching RSDP on kexec-ed kernel

[bhe]

On 03/29/19 at 04:29pm, Chao Fan wrote:
> On Fri, Mar 29, 2019 at 07:20:38AM +0000, Junichi Nomura wrote:
> >Commit 3a63f70bf4c3a ("x86/boot: Early parse RSDP and save it in
> >boot_params") broke kexec boot on EFI systems.  efi_get_rsdp_addr()
> >in the early parsing code tries to search RSDP from EFI table but
> >that will crash because the table address is virtual when the kernel
> >was booted by kexec.
> [...]
> >-			guid  = tbl->guid;
> >-			table = tbl->table;
> >-		}
> >-
> >-		if (!(efi_guidcmp(guid, ACPI_TABLE_GUID)))
> >-			rsdp_addr = table;
> >-		else if (!(efi_guidcmp(guid, ACPI_20_TABLE_GUID)))
> >-			return table;
> >-	}
> >+	return __efi_get_rsdp_addr(config_tables, nr_tables, efi_64);
> >+#else
> >+	return 0;
> > #endif
> >-	return rsdp_addr;
> 
> I remeber the rsdp_addr is defined before #ifdef CONFIG_EFI
> If so, you don't need
> #else
> 	return 0;

it doesn't change the old logic. Junichi moved rsdp_addr definition inside
the CONFIG_EFI iedeffery block.
> 
> BY the way, what's your patch based on? I like add patch on my local
> branch and then review code, but failed.
> I try to use 'patch -p1 <' your patch to the latest tip master branch,
> but failed.
> 
> Thanks,
> Chao Fan
> 
> > }
> > 
> > static u8 compute_checksum(u8 *buffer, u32 length)
> >@@ -221,6 +284,9 @@ acpi_physical_address get_rsdp_addr(void)
> > 		pa = boot_params->acpi_rsdp_addr;
> > 
> > 	if (!pa)
> >+		pa = kexec_get_rsdp_addr();
> >+
> >+	if (!pa)
> > 		pa = efi_get_rsdp_addr();
> > 
> > 	if (!pa)
> >
> >
> 
> 

_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: [PATCH] x86/boot: Use efi_setup_data for searching RSDP on kexec-ed kernel

[Chao Fan]

On Fri, Mar 29, 2019 at 05:16:55PM +0800, bhe@redhat.com wrote:
>On 03/29/19 at 04:29pm, Chao Fan wrote:
>> On Fri, Mar 29, 2019 at 07:20:38AM +0000, Junichi Nomura wrote:
>> >Commit 3a63f70bf4c3a ("x86/boot: Early parse RSDP and save it in
>> >boot_params") broke kexec boot on EFI systems.  efi_get_rsdp_addr()
>> >in the early parsing code tries to search RSDP from EFI table but
>> >that will crash because the table address is virtual when the kernel
>> >was booted by kexec.
>> [...]
>> >-			guid  = tbl->guid;
>> >-			table = tbl->table;
>> >-		}
>> >-
>> >-		if (!(efi_guidcmp(guid, ACPI_TABLE_GUID)))
>> >-			rsdp_addr = table;
>> >-		else if (!(efi_guidcmp(guid, ACPI_20_TABLE_GUID)))
>> >-			return table;
>> >-	}
>> >+	return __efi_get_rsdp_addr(config_tables, nr_tables, efi_64);
>> >+#else
>> >+	return 0;
>> > #endif
>> >-	return rsdp_addr;
>> 
>> I remeber the rsdp_addr is defined before #ifdef CONFIG_EFI
>> If so, you don't need
>> #else
>> 	return 0;
>
>it doesn't change the old logic. Junichi moved rsdp_addr definition inside
>the CONFIG_EFI iedeffery block.

Yes, got it. Thanks for your explain.

Thanks,
Chao Fan
>> 
>> BY the way, what's your patch based on? I like add patch on my local
>> branch and then review code, but failed.
>> I try to use 'patch -p1 <' your patch to the latest tip master branch,
>> but failed.
>> 
>> Thanks,
>> Chao Fan
>> 
>> > }
>> > 
>> > static u8 compute_checksum(u8 *buffer, u32 length)
>> >@@ -221,6 +284,9 @@ acpi_physical_address get_rsdp_addr(void)
>> > 		pa = boot_params->acpi_rsdp_addr;
>> > 
>> > 	if (!pa)
>> >+		pa = kexec_get_rsdp_addr();
>> >+
>> >+	if (!pa)
>> > 		pa = efi_get_rsdp_addr();
>> > 
>> > 	if (!pa)
>> >
>> >
>> 
>> 
>
>



_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

[PATCH v2] x86/boot: Use efi_setup_data for searching RSDP on kexec-ed kernel

[Junichi Nomura]

Commit 3a63f70bf4c3a ("x86/boot: Early parse RSDP and save it in
boot_params") broke kexec boot on EFI systems.  efi_get_rsdp_addr()
in the early parsing code tries to search RSDP from EFI table but
that will crash because the table address is virtual when the kernel
was booted by kexec.

In the case of kexec, physical address of EFI tables is provided
via efi_setup_data in boot_params, which is set up by kexec(1).

Factor out the table parsing code and use different pointers depending
on whether the kernel is booted by kexec or not.

Fixes: 3a63f70bf4c3a ("x86/boot: Early parse RSDP and save it in boot_params")
Signed-off-by: Jun'ichi Nomura <j-nomura@ce.jp.nec.com>
Acked-by: Baoquan He <bhe@redhat.com>
Cc: Chao Fan <fanc.fnst@cn.fujitsu.com>
Cc: Borislav Petkov <bp@suse.de>
Cc: Dave Young <dyoung@redhat.com>

--
v2: Added comments above __efi_get_rsdp_addr() and kexec_get_rsdp_addr() 

diff --git a/arch/x86/boot/compressed/acpi.c b/arch/x86/boot/compressed/acpi.c
--- a/arch/x86/boot/compressed/acpi.c
+++ b/arch/x86/boot/compressed/acpi.c
@@ -44,17 +44,112 @@ static acpi_physical_address get_acpi_rsdp(void)
 	return addr;
 }
 
-/* Search EFI system tables for RSDP. */
-static acpi_physical_address efi_get_rsdp_addr(void)
+#ifdef CONFIG_EFI
+static unsigned long efi_get_kexec_setup_data_addr(void)
+{
+	struct setup_data *data;
+	u64 pa_data;
+
+	pa_data = boot_params->hdr.setup_data;
+	while (pa_data) {
+		data = (struct setup_data *) pa_data;
+		if (data->type == SETUP_EFI)
+			return pa_data + sizeof(struct setup_data);
+		pa_data = data->next;
+	}
+	return 0;
+}
+
+/*
+ * Search EFI system tables for RSDP.  If both ACPI_20_TABLE_GUID and
+ * ACPI_TABLE_GUID are found, take the former, which has more features.
+ */
+static acpi_physical_address
+__efi_get_rsdp_addr(unsigned long config_tables, unsigned int nr_tables,
+		    bool efi_64)
 {
 	acpi_physical_address rsdp_addr = 0;
+	int i;
+
+	/* Get EFI tables from systab. */
+	for (i = 0; i < nr_tables; i++) {
+		acpi_physical_address table;
+		efi_guid_t guid;
+
+		if (efi_64) {
+			efi_config_table_64_t *tbl = (efi_config_table_64_t *) config_tables + i;
+
+			guid  = tbl->guid;
+			table = tbl->table;
+
+			if (!IS_ENABLED(CONFIG_X86_64) && table >> 32) {
+				debug_putstr("Error getting RSDP address: EFI config table located above 4GB.\n");
+				return 0;
+			}
+		} else {
+			efi_config_table_32_t *tbl = (efi_config_table_32_t *) config_tables + i;
 
+			guid  = tbl->guid;
+			table = tbl->table;
+		}
+
+		if (!(efi_guidcmp(guid, ACPI_TABLE_GUID)))
+			rsdp_addr = table;
+		else if (!(efi_guidcmp(guid, ACPI_20_TABLE_GUID)))
+			return table;
+	}
+
+	return rsdp_addr;
+}
+#endif
+
+/*
+ * EFI/kexec support is only added for 64bit. So we don't have to
+ * care 32bit case.
+ */
+static acpi_physical_address kexec_get_rsdp_addr(void)
+{
 #ifdef CONFIG_EFI
-	unsigned long systab, systab_tables, config_tables;
+	efi_system_table_64_t *systab;
+	struct efi_setup_data *esd;
+	struct efi_info *ei;
+	char *sig;
+
+	esd = (struct efi_setup_data *) efi_get_kexec_setup_data_addr();
+	if (!esd)
+		return 0;
+
+	if (!esd->tables) {
+		debug_putstr("Wrong kexec SETUP_EFI data.\n");
+		return 0;
+	}
+
+	ei = &boot_params->efi_info;
+	sig = (char *)&ei->efi_loader_signature;
+	if (strncmp(sig, EFI64_LOADER_SIGNATURE, 4)) {
+		debug_putstr("Wrong kexec EFI loader signature.\n");
+		return 0;
+	}
+
+	/* Get systab from boot params. */
+	systab = (efi_system_table_64_t *) (ei->efi_systab | ((__u64)ei->efi_systab_hi << 32));
+	if (!systab)
+		error("EFI system table not found in kexec boot_params.");
+
+	return __efi_get_rsdp_addr((unsigned long) esd->tables,
+				   systab->nr_tables, true);
+#else
+	return 0;
+#endif
+}
+
+static acpi_physical_address efi_get_rsdp_addr(void)
+{
+#ifdef CONFIG_EFI
+	unsigned long systab, config_tables;
 	unsigned int nr_tables;
 	struct efi_info *ei;
 	bool efi_64;
-	int size, i;
 	char *sig;
 
 	ei = &boot_params->efi_info;
@@ -88,49 +183,20 @@ static acpi_physical_address efi_get_rsdp_addr(void)
 
 		config_tables	= stbl->tables;
 		nr_tables	= stbl->nr_tables;
-		size		= sizeof(efi_config_table_64_t);
 	} else {
 		efi_system_table_32_t *stbl = (efi_system_table_32_t *)systab;
 
 		config_tables	= stbl->tables;
 		nr_tables	= stbl->nr_tables;
-		size		= sizeof(efi_config_table_32_t);
 	}
 
 	if (!config_tables)
 		error("EFI config tables not found.");
 
-	/* Get EFI tables from systab. */
-	for (i = 0; i < nr_tables; i++) {
-		acpi_physical_address table;
-		efi_guid_t guid;
-
-		config_tables += size;
-
-		if (efi_64) {
-			efi_config_table_64_t *tbl = (efi_config_table_64_t *)config_tables;
-
-			guid  = tbl->guid;
-			table = tbl->table;
-
-			if (!IS_ENABLED(CONFIG_X86_64) && table >> 32) {
-				debug_putstr("Error getting RSDP address: EFI config table located above 4GB.\n");
-				return 0;
-			}
-		} else {
-			efi_config_table_32_t *tbl = (efi_config_table_32_t *)config_tables;
-
-			guid  = tbl->guid;
-			table = tbl->table;
-		}
-
-		if (!(efi_guidcmp(guid, ACPI_TABLE_GUID)))
-			rsdp_addr = table;
-		else if (!(efi_guidcmp(guid, ACPI_20_TABLE_GUID)))
-			return table;
-	}
+	return __efi_get_rsdp_addr(config_tables, nr_tables, efi_64);
+#else
+	return 0;
 #endif
-	return rsdp_addr;
 }
 
 static u8 compute_checksum(u8 *buffer, u32 length)
@@ -221,6 +287,9 @@ acpi_physical_address get_rsdp_addr(void)
 		pa = boot_params->acpi_rsdp_addr;
 
 	if (!pa)
+		pa = kexec_get_rsdp_addr();
+
+	if (!pa)
 		pa = efi_get_rsdp_addr();
 
 	if (!pa)

_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: [PATCH v2] x86/boot: Use efi_setup_data for searching RSDP on kexec-ed kernel

[Chao Fan]

Hi,

I have test your PATCH in Qemu guest:
----------
Fedora 29 (Workstation Edition)
Kernel 5.1.0-rc3+ on an x86_64 (ttyS0)

localhost login: root
Password:
Last login: Tue Apr  2 05:30:33 on ttyS0
[root@localhost ~]# cd /boot
[root@localhost boot]# ls
config-4.18.16-300.fc29.x86_64
efi
extlinux
grub2
initramfs-0-rescue-858ff1f5d0cb453898ae6c7f77c68ba7.img
initramfs-4.18.16-300.fc29.x86_64.img
initramfs-4.18.16-300.fc29.x86_64kdump.img
initramfs-5.1.0-rc3+.img
initramfs-5.1.0-rc3+kdump.img
loader
lost+found
System.map
System.map-4.18.16-300.fc29.x86_64
System.map-5.1.0-rc3+
vmlinuz
vmlinuz-0-rescue-858ff1f5d0cb453898ae6c7f77c68ba7
vmlinuz-4.18.16-300.fc29.x86_64
vmlinuz-5.1.0-rc3+
[root@localhost boot]# kexec -l vmlinuz-5.1.0-rc3+ --initrd=initramfs-5.1.0-rc3+.img --reuse-cmdline
[root@localhost boot]# kexec -e
[   77.933760] Unregister pv shared memory for cpu 1
[   77.933763] Unregister pv shared memory for cpu 2
[   77.933766] Unregister pv shared memory for cpu 5
[   77.933825] Unregister pv shared memory for cpu 7
[   77.933882] Unregister pv shared memory for cpu 3
[   77.933904] Unregister pv shared memory for cpu 8
[   77.936147] Unregister pv shared memory for cpu 4
[   77.936199] Unregister pv shared memory for cpu 9
[   77.945822] Unregister pv shared memory for cpu 0
[   77.946308] Unregister pv shared memory for cpu 6
[   77.947420] sd 0:0:0:0: [sda] Synchronizing SCSI cache
[   77.989030] kexec_core: Starting new kernel
early console in extract_kernel
input_data: 0x000000017f6033b1
input_len: 0x00000000008412d4
output: 0x000000017e000000
output_len: 0x0000000001e15844
kernel_total_size: 0x0000000001e2c000
trampoline_32bit: 0x000000000009d000
booted via startup_64()


Physical KASLR disabled: no suitable memory region!
------

I am not sure whether I have done the right test.
This guest is booted from EFI. Here we can see the kexeced kernel
has completed the compressed boot stage. So I think your PATCH works.

Thanks,
Chao Fan




_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: [PATCH v2] x86/boot: Use efi_setup_data for searching RSDP on kexec-ed kernel

[Junichi Nomura]

On Tue, Apr 02, 2019 at 05:41:49PM +0800, Chao Fan wrote:
> [   77.989030] kexec_core: Starting new kernel
> early console in extract_kernel
> input_data: 0x000000017f6033b1
> input_len: 0x00000000008412d4
> output: 0x000000017e000000
> output_len: 0x0000000001e15844
> kernel_total_size: 0x0000000001e2c000
> trampoline_32bit: 0x000000000009d000
> booted via startup_64()
> 
> 
> Physical KASLR disabled: no suitable memory region!
> ------
> 
> I am not sure whether I have done the right test.
> This guest is booted from EFI. Here we can see the kexeced kernel
> has completed the compressed boot stage. So I think your PATCH works.

Thanks for testing.  If your test bed doesn't boot even with the patch,
you could check what was found as RSDP with a debug patch like below.

diff --git a/arch/x86/boot/compressed/misc.c b/arch/x86/boot/compressed/misc.c
--- a/arch/x86/boot/compressed/misc.c
+++ b/arch/x86/boot/compressed/misc.c
@@ -379,6 +379,7 @@ asmlinkage __visible void *extract_kernel(void *rmode, memptr heap,
 	debug_putaddr(output);
 	debug_putaddr(output_len);
 	debug_putaddr(kernel_total_size);
+	debug_putaddr(boot_params->acpi_rsdp_addr);
 
 #ifdef CONFIG_X86_64
 	/* Report address of 32-bit trampoline */

_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: [PATCH v2] x86/boot: Use efi_setup_data for searching RSDP on kexec-ed kernel

[Chao Fan]

On Tue, Apr 02, 2019 at 09:53:51AM +0000, Junichi Nomura wrote:
>On Tue, Apr 02, 2019 at 05:41:49PM +0800, Chao Fan wrote:
>> [   77.989030] kexec_core: Starting new kernel
>> early console in extract_kernel
>> input_data: 0x000000017f6033b1
>> input_len: 0x00000000008412d4
>> output: 0x000000017e000000
>> output_len: 0x0000000001e15844
>> kernel_total_size: 0x0000000001e2c000
>> trampoline_32bit: 0x000000000009d000
>> booted via startup_64()
>> 
>> 
>> Physical KASLR disabled: no suitable memory region!
>> ------
>> 
>> I am not sure whether I have done the right test.
>> This guest is booted from EFI. Here we can see the kexeced kernel
>> has completed the compressed boot stage. So I think your PATCH works.
>
>Thanks for testing.  If your test bed doesn't boot even with the patch,
>you could check what was found as RSDP with a debug patch like below.

Oh no, it booted. I just put the compressed stag log.

Thanks,
Chao Fan

>
>diff --git a/arch/x86/boot/compressed/misc.c b/arch/x86/boot/compressed/misc.c
>--- a/arch/x86/boot/compressed/misc.c
>+++ b/arch/x86/boot/compressed/misc.c
>@@ -379,6 +379,7 @@ asmlinkage __visible void *extract_kernel(void *rmode, memptr heap,
> 	debug_putaddr(output);
> 	debug_putaddr(output_len);
> 	debug_putaddr(kernel_total_size);
>+	debug_putaddr(boot_params->acpi_rsdp_addr);
> 
> #ifdef CONFIG_X86_64
> 	/* Report address of 32-bit trampoline */
>
>



_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: [PATCH v2] x86/boot: Use efi_setup_data for searching RSDP on kexec-ed kernel

[Junichi Nomura]

On 4/2/19 8:06 PM, Chao Fan wrote:
> On Tue, Apr 02, 2019 at 09:53:51AM +0000, Junichi Nomura wrote:
>> On Tue, Apr 02, 2019 at 05:41:49PM +0800, Chao Fan wrote:
>>> [   77.989030] kexec_core: Starting new kernel
>>> early console in extract_kernel
>>> input_data: 0x000000017f6033b1
>>> input_len: 0x00000000008412d4
>>> output: 0x000000017e000000
>>> output_len: 0x0000000001e15844
>>> kernel_total_size: 0x0000000001e2c000
>>> trampoline_32bit: 0x000000000009d000
>>> booted via startup_64()
>>>
>>>
>>> Physical KASLR disabled: no suitable memory region!
>>> ------
>>>
>>> I am not sure whether I have done the right test.
>>> This guest is booted from EFI. Here we can see the kexeced kernel
>>> has completed the compressed boot stage. So I think your PATCH works.
>>
>> Thanks for testing.  If your test bed doesn't boot even with the patch,
>> you could check what was found as RSDP with a debug patch like below.
> 
> Oh no, it booted. I just put the compressed stag log.

Ah, then I think the patch worked as expected. Thanks.

-- 
Jun'ichi Nomura, NEC Corporation / NEC Solution Innovators, Ltd.

_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: [PATCH v2] x86/boot: Use efi_setup_data for searching RSDP on kexec-ed kernel

[Dave Young]

On 04/01/19 at 12:08am, Junichi Nomura wrote:
> Commit 3a63f70bf4c3a ("x86/boot: Early parse RSDP and save it in
> boot_params") broke kexec boot on EFI systems.  efi_get_rsdp_addr()
> in the early parsing code tries to search RSDP from EFI table but
> that will crash because the table address is virtual when the kernel
> was booted by kexec.
> 
> In the case of kexec, physical address of EFI tables is provided
> via efi_setup_data in boot_params, which is set up by kexec(1).
> 
> Factor out the table parsing code and use different pointers depending
> on whether the kernel is booted by kexec or not.
> 
> Fixes: 3a63f70bf4c3a ("x86/boot: Early parse RSDP and save it in boot_params")
> Signed-off-by: Jun'ichi Nomura <j-nomura@ce.jp.nec.com>
> Acked-by: Baoquan He <bhe@redhat.com>
> Cc: Chao Fan <fanc.fnst@cn.fujitsu.com>
> Cc: Borislav Petkov <bp@suse.de>
> Cc: Dave Young <dyoung@redhat.com>
> 
> --
> v2: Added comments above __efi_get_rsdp_addr() and kexec_get_rsdp_addr() 
> 
> diff --git a/arch/x86/boot/compressed/acpi.c b/arch/x86/boot/compressed/acpi.c
> --- a/arch/x86/boot/compressed/acpi.c
> +++ b/arch/x86/boot/compressed/acpi.c
> @@ -44,17 +44,112 @@ static acpi_physical_address get_acpi_rsdp(void)
>  	return addr;
>  }
>  
> -/* Search EFI system tables for RSDP. */
> -static acpi_physical_address efi_get_rsdp_addr(void)
> +#ifdef CONFIG_EFI
> +static unsigned long efi_get_kexec_setup_data_addr(void)
> +{
> +	struct setup_data *data;
> +	u64 pa_data;
> +
> +	pa_data = boot_params->hdr.setup_data;
> +	while (pa_data) {
> +		data = (struct setup_data *) pa_data;
> +		if (data->type == SETUP_EFI)
> +			return pa_data + sizeof(struct setup_data);
> +		pa_data = data->next;
> +	}
> +	return 0;
> +}
> +
> +/*
> + * Search EFI system tables for RSDP.  If both ACPI_20_TABLE_GUID and
> + * ACPI_TABLE_GUID are found, take the former, which has more features.
> + */
> +static acpi_physical_address
> +__efi_get_rsdp_addr(unsigned long config_tables, unsigned int nr_tables,
> +		    bool efi_64)
>  {
>  	acpi_physical_address rsdp_addr = 0;
> +	int i;
> +
> +	/* Get EFI tables from systab. */
> +	for (i = 0; i < nr_tables; i++) {
> +		acpi_physical_address table;
> +		efi_guid_t guid;
> +
> +		if (efi_64) {
> +			efi_config_table_64_t *tbl = (efi_config_table_64_t *) config_tables + i;
> +
> +			guid  = tbl->guid;
> +			table = tbl->table;
> +
> +			if (!IS_ENABLED(CONFIG_X86_64) && table >> 32) {
> +				debug_putstr("Error getting RSDP address: EFI config table located above 4GB.\n");
> +				return 0;
> +			}
> +		} else {
> +			efi_config_table_32_t *tbl = (efi_config_table_32_t *) config_tables + i;
>  
> +			guid  = tbl->guid;
> +			table = tbl->table;
> +		}
> +
> +		if (!(efi_guidcmp(guid, ACPI_TABLE_GUID)))
> +			rsdp_addr = table;
> +		else if (!(efi_guidcmp(guid, ACPI_20_TABLE_GUID)))
> +			return table;
> +	}
> +
> +	return rsdp_addr;
> +}
> +#endif
> +
> +/*
> + * EFI/kexec support is only added for 64bit. So we don't have to
> + * care 32bit case.
> + */
> +static acpi_physical_address kexec_get_rsdp_addr(void)
> +{
>  #ifdef CONFIG_EFI
> -	unsigned long systab, systab_tables, config_tables;
> +	efi_system_table_64_t *systab;
> +	struct efi_setup_data *esd;
> +	struct efi_info *ei;
> +	char *sig;
> +
> +	esd = (struct efi_setup_data *) efi_get_kexec_setup_data_addr();
> +	if (!esd)
> +		return 0;
> +
> +	if (!esd->tables) {
> +		debug_putstr("Wrong kexec SETUP_EFI data.\n");
> +		return 0;
> +	}
> +
> +	ei = &boot_params->efi_info;
> +	sig = (char *)&ei->efi_loader_signature;
> +	if (strncmp(sig, EFI64_LOADER_SIGNATURE, 4)) {
> +		debug_putstr("Wrong kexec EFI loader signature.\n");
> +		return 0;
> +	}
> +
> +	/* Get systab from boot params. */
> +	systab = (efi_system_table_64_t *) (ei->efi_systab | ((__u64)ei->efi_systab_hi << 32));
> +	if (!systab)
> +		error("EFI system table not found in kexec boot_params.");
> +
> +	return __efi_get_rsdp_addr((unsigned long) esd->tables,
> +				   systab->nr_tables, true);
> +#else
> +	return 0;
> +#endif
> +}
> +
> +static acpi_physical_address efi_get_rsdp_addr(void)
> +{
> +#ifdef CONFIG_EFI
> +	unsigned long systab, config_tables;
>  	unsigned int nr_tables;
>  	struct efi_info *ei;
>  	bool efi_64;
> -	int size, i;
>  	char *sig;
>  
>  	ei = &boot_params->efi_info;
> @@ -88,49 +183,20 @@ static acpi_physical_address efi_get_rsdp_addr(void)
>  
>  		config_tables	= stbl->tables;
>  		nr_tables	= stbl->nr_tables;
> -		size		= sizeof(efi_config_table_64_t);
>  	} else {
>  		efi_system_table_32_t *stbl = (efi_system_table_32_t *)systab;
>  
>  		config_tables	= stbl->tables;
>  		nr_tables	= stbl->nr_tables;
> -		size		= sizeof(efi_config_table_32_t);
>  	}
>  
>  	if (!config_tables)
>  		error("EFI config tables not found.");
>  
> -	/* Get EFI tables from systab. */
> -	for (i = 0; i < nr_tables; i++) {
> -		acpi_physical_address table;
> -		efi_guid_t guid;
> -
> -		config_tables += size;
> -
> -		if (efi_64) {
> -			efi_config_table_64_t *tbl = (efi_config_table_64_t *)config_tables;
> -
> -			guid  = tbl->guid;
> -			table = tbl->table;
> -
> -			if (!IS_ENABLED(CONFIG_X86_64) && table >> 32) {
> -				debug_putstr("Error getting RSDP address: EFI config table located above 4GB.\n");
> -				return 0;
> -			}
> -		} else {
> -			efi_config_table_32_t *tbl = (efi_config_table_32_t *)config_tables;
> -
> -			guid  = tbl->guid;
> -			table = tbl->table;
> -		}
> -
> -		if (!(efi_guidcmp(guid, ACPI_TABLE_GUID)))
> -			rsdp_addr = table;
> -		else if (!(efi_guidcmp(guid, ACPI_20_TABLE_GUID)))
> -			return table;
> -	}
> +	return __efi_get_rsdp_addr(config_tables, nr_tables, efi_64);
> +#else
> +	return 0;
>  #endif
> -	return rsdp_addr;
>  }
>  
>  static u8 compute_checksum(u8 *buffer, u32 length)
> @@ -221,6 +287,9 @@ acpi_physical_address get_rsdp_addr(void)
>  		pa = boot_params->acpi_rsdp_addr;
>  
>  	if (!pa)
> +		pa = kexec_get_rsdp_addr();
> +
> +	if (!pa)
>  		pa = efi_get_rsdp_addr();
>  
>  	if (!pa)

I failed to kexec reboot on my laptop, kernel panics too quick,  I'm not sure this is
caused by your patch though.

Actually there are something probably i915 changes break kexec,  the
above test is with "nomodeset" which should work.

Let me do more testing and update here tomorrow.

Thanks
Dave


_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: [PATCH v2] x86/boot: Use efi_setup_data for searching RSDP on kexec-ed kernel

[Chao Fan]

On Tue, Apr 02, 2019 at 08:03:19PM +0800, Dave Young wrote:
>On 04/01/19 at 12:08am, Junichi Nomura wrote:
>> Commit 3a63f70bf4c3a ("x86/boot: Early parse RSDP and save it in
>> boot_params") broke kexec boot on EFI systems.  efi_get_rsdp_addr()
>> in the early parsing code tries to search RSDP from EFI table but
>> that will crash because the table address is virtual when the kernel
>> was booted by kexec.
>> 
>> In the case of kexec, physical address of EFI tables is provided
>> via efi_setup_data in boot_params, which is set up by kexec(1).
>> 
>> Factor out the table parsing code and use different pointers depending
>> on whether the kernel is booted by kexec or not.
>> 
>> Fixes: 3a63f70bf4c3a ("x86/boot: Early parse RSDP and save it in boot_params")
>> Signed-off-by: Jun'ichi Nomura <j-nomura@ce.jp.nec.com>
>> Acked-by: Baoquan He <bhe@redhat.com>
>> Cc: Chao Fan <fanc.fnst@cn.fujitsu.com>
>> Cc: Borislav Petkov <bp@suse.de>
>> Cc: Dave Young <dyoung@redhat.com>
[...]
>
>I failed to kexec reboot on my laptop, kernel panics too quick,  I'm not sure this is
>caused by your patch though.
>
>Actually there are something probably i915 changes break kexec,  the
>above test is with "nomodeset" which should work.
>
>Let me do more testing and update here tomorrow.
>

Hi Dave,

Last day I was testing the normal kexec, today I have tested the kdump
issue. Since the kdump has set "nokaslr" to cmdline, so I drop from
KDUMP_COMMANDLINE_APPEND
And it booted OK, so the PATCH works in both normal kexec and kdump.

[root@localhost ~]# echo 1 > /proc/sys/kernel/sysrq
[root@localhost ~]# echo c > /proc/sysrq-trigger
[   67.776136] sysrq: Trigger a crash
[   67.777412] Kernel panic - not syncing: sysrq triggered crash
[   67.779429] CPU: 1 PID: 1652 Comm: bash Kdump: loaded Not tainted 5.1.0-rc3+ #4
[   67.780755] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 0.0.0 02/06/2015
[   67.782062] Call Trace:
[   67.782490]  dump_stack+0x5c/0x80
[   67.783049]  panic+0x101/0x2a7
[   67.783560]  ? printk+0x58/0x6f
[   67.784091]  sysrq_handle_crash+0x11/0x11
[   67.784762]  __handle_sysrq.cold.7+0x45/0xf2
[   67.785467]  write_sysrq_trigger+0x2b/0x30
[   67.786087]  proc_reg_write+0x39/0x60
[   67.786597]  vfs_write+0xa5/0x1a0
[   67.787061]  ksys_write+0x4f/0xb0
[   67.787492]  do_syscall_64+0x5b/0x160
[   67.788010]  entry_SYSCALL_64_after_hwframe+0x44/0xa9
[   67.788740] RIP: 0033:0x7f66266fbed8
[   67.789239] Code: 89 02 48 c7 c0 ff ff ff ff eb b3 0f 1f 80 00 00 00 00 f3 0f 1e fa 48 8d 05 45 78 0d 00 8b 00 85 c0 75 17 b8 01 00 00 00 0f 05 <48> 3d 00 f0 ff ff 77 58 c3 0f 1f 80 00 00 00 00 41 54 49 89 d4 55
[   67.791325] RSP: 002b:00007ffecdaf6138 EFLAGS: 00000246 ORIG_RAX: 0000000000000001
[   67.792084] RAX: ffffffffffffffda RBX: 0000000000000002 RCX: 00007f66266fbed8
[   67.792820] RDX: 0000000000000002 RSI: 000055dcc8d29880 RDI: 0000000000000001
[   67.793515] RBP: 000055dcc8d29880 R08: 000000000000000a R09: 00007ffecdaf5cc0
[   67.794276] R10: 000000000000000a R11: 0000000000000246 R12: 00007f66267cf780
[   67.795017] R13: 0000000000000002 R14: 00007f66267ca740 R15: 0000000000000002
early console in extract_kernel
input_data: 0x00000000376033b1
input_len: 0x00000000008412d4
output: 0x0000000036000000
output_len: 0x0000000001e15844
kernel_total_size: 0x0000000001e2c000
trampoline_32bit: 0x000000000009d000
booted via startup_64()


Physical KASLR disabled: no suitable memory region!

Virtual KASLR using RDRAND RDTSC...

Decompressing Linux... Parsing ELF... Performing relocations... done.
Booting the kernel.
[...]
         Starting Kdump Vmcore Save Service...
kdump: dump target is /dev/mapper/fedora-root
kdump: saving to /sysroot//var/crash/127.0.0.1-2019-04-03-01:28:01/
[    3.551609] EXT4-fs (dm-0): re-mounted. Opts: (null)
kdump: saving vmcore-dmesg.txt
kdump: saving vmcore-dmesg.txt complete
kdump: saving vmcore
Copying data                                      : [100.0 %] |           eta: 0s
kdump: saving vmcore complete

Thanks,
Chao Fan

>Thanks
>Dave
>
>
>



_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: [PATCH v2] x86/boot: Use efi_setup_data for searching RSDP on kexec-ed kernel

[Dave Young]

On 04/03/19 at 01:35pm, Chao Fan wrote:
> On Tue, Apr 02, 2019 at 08:03:19PM +0800, Dave Young wrote:
> >On 04/01/19 at 12:08am, Junichi Nomura wrote:
> >> Commit 3a63f70bf4c3a ("x86/boot: Early parse RSDP and save it in
> >> boot_params") broke kexec boot on EFI systems.  efi_get_rsdp_addr()
> >> in the early parsing code tries to search RSDP from EFI table but
> >> that will crash because the table address is virtual when the kernel
> >> was booted by kexec.
> >> 
> >> In the case of kexec, physical address of EFI tables is provided
> >> via efi_setup_data in boot_params, which is set up by kexec(1).
> >> 
> >> Factor out the table parsing code and use different pointers depending
> >> on whether the kernel is booted by kexec or not.
> >> 
> >> Fixes: 3a63f70bf4c3a ("x86/boot: Early parse RSDP and save it in boot_params")
> >> Signed-off-by: Jun'ichi Nomura <j-nomura@ce.jp.nec.com>
> >> Acked-by: Baoquan He <bhe@redhat.com>
> >> Cc: Chao Fan <fanc.fnst@cn.fujitsu.com>
> >> Cc: Borislav Petkov <bp@suse.de>
> >> Cc: Dave Young <dyoung@redhat.com>
> [...]
> >
> >I failed to kexec reboot on my laptop, kernel panics too quick,  I'm not sure this is
> >caused by your patch though.
> >
> >Actually there are something probably i915 changes break kexec,  the
> >above test is with "nomodeset" which should work.
> >
> >Let me do more testing and update here tomorrow.
> >
> 
> Hi Dave,
> 
> Last day I was testing the normal kexec, today I have tested the kdump
> issue. Since the kdump has set "nokaslr" to cmdline, so I drop from
> KDUMP_COMMANDLINE_APPEND
> And it booted OK, so the PATCH works in both normal kexec and kdump.
> 

Actually I got some different kexec test results.

Yesterday, with my installed kernel (based on git head several weeks
ago), kexec kernel panics.

Then I tried latest mainline with git pull, everything works, (with or
without the patch, and can not reproduce the bug this patch is fixing)

Today, test again, kexec reboot hangs (with or without your patch), but
kdump works always (with or without the patch)

It is weird to me. Probably I need find out why I can not reproduce the
bug this patch is addressing first.

earlyprintk seems not working for me anymore, it is not easy to debug on
laptop now.

But the patch itself is clear, I think it should be good.  There might be
other things broken.

Thanks
Dave

_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: [PATCH v2] x86/boot: Use efi_setup_data for searching RSDP on kexec-ed kernel

[Dave Young]

On 04/03/19 at 01:53pm, Dave Young wrote:
> On 04/03/19 at 01:35pm, Chao Fan wrote:
> > On Tue, Apr 02, 2019 at 08:03:19PM +0800, Dave Young wrote:
> > >On 04/01/19 at 12:08am, Junichi Nomura wrote:
> > >> Commit 3a63f70bf4c3a ("x86/boot: Early parse RSDP and save it in
> > >> boot_params") broke kexec boot on EFI systems.  efi_get_rsdp_addr()
> > >> in the early parsing code tries to search RSDP from EFI table but
> > >> that will crash because the table address is virtual when the kernel
> > >> was booted by kexec.
> > >> 
> > >> In the case of kexec, physical address of EFI tables is provided
> > >> via efi_setup_data in boot_params, which is set up by kexec(1).
> > >> 
> > >> Factor out the table parsing code and use different pointers depending
> > >> on whether the kernel is booted by kexec or not.
> > >> 
> > >> Fixes: 3a63f70bf4c3a ("x86/boot: Early parse RSDP and save it in boot_params")
> > >> Signed-off-by: Jun'ichi Nomura <j-nomura@ce.jp.nec.com>
> > >> Acked-by: Baoquan He <bhe@redhat.com>
> > >> Cc: Chao Fan <fanc.fnst@cn.fujitsu.com>
> > >> Cc: Borislav Petkov <bp@suse.de>
> > >> Cc: Dave Young <dyoung@redhat.com>
> > [...]
> > >
> > >I failed to kexec reboot on my laptop, kernel panics too quick,  I'm not sure this is
> > >caused by your patch though.
> > >
> > >Actually there are something probably i915 changes break kexec,  the
> > >above test is with "nomodeset" which should work.
> > >
> > >Let me do more testing and update here tomorrow.
> > >
> > 
> > Hi Dave,
> > 
> > Last day I was testing the normal kexec, today I have tested the kdump
> > issue. Since the kdump has set "nokaslr" to cmdline, so I drop from
> > KDUMP_COMMANDLINE_APPEND
> > And it booted OK, so the PATCH works in both normal kexec and kdump.
> > 
> 
> Actually I got some different kexec test results.
> 
> Yesterday, with my installed kernel (based on git head several weeks
> ago), kexec kernel panics.
> 
> Then I tried latest mainline with git pull, everything works, (with or
> without the patch, and can not reproduce the bug this patch is fixing)
> 
> Today, test again, kexec reboot hangs (with or without your patch), but
> kdump works always (with or without the patch)
> 
> It is weird to me. Probably I need find out why I can not reproduce the
> bug this patch is addressing first.
> 
> earlyprintk seems not working for me anymore, it is not easy to debug on
> laptop now.
> 
> But the patch itself is clear, I think it should be good.  There might be
> other things broken.

Disable your immovable mem code then everything works for me.  There
might be something wrong in the code.  Also "nokaslr" does not help, it
should be another problem 

diff --git a/arch/x86/boot/compressed/kaslr.c b/arch/x86/boot/compressed/kaslr.c
index 2e53c056ba20..e760c9159662 100644
--- a/arch/x86/boot/compressed/kaslr.c
+++ b/arch/x86/boot/compressed/kaslr.c
@@ -416,7 +416,7 @@ static void mem_avoid_init(unsigned long input, unsigned long input_size,
 	handle_mem_options();
 
 	/* Enumerate the immovable memory regions */
-	num_immovable_mem = count_immovable_mem_regions();
+	/*num_immovable_mem = count_immovable_mem_regions();*/
 
 #ifdef CONFIG_X86_VERBOSE_BOOTUP
 	/* Make sure video RAM can be used. */
diff --git a/arch/x86/boot/compressed/misc.c b/arch/x86/boot/compressed/misc.c
index c0d6c560df69..1bc6f46d3aa7 100644
--- a/arch/x86/boot/compressed/misc.c
+++ b/arch/x86/boot/compressed/misc.c
@@ -352,7 +352,7 @@ asmlinkage __visible void *extract_kernel(void *rmode, memptr heap,
 	boot_params->hdr.loadflags &= ~KASLR_FLAG;
 
 	/* Save RSDP address for later use. */
-	boot_params->acpi_rsdp_addr = get_rsdp_addr();
+/*	boot_params->acpi_rsdp_addr = get_rsdp_addr(); */
 
 	sanitize_boot_params(boot_params);
 

_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: [PATCH v2] x86/boot: Use efi_setup_data for searching RSDP on kexec-ed kernel

[Chao Fan]

On Wed, Apr 03, 2019 at 02:39:11PM +0800, Dave Young wrote:
>> Actually I got some different kexec test results.
>> 
>> Yesterday, with my installed kernel (based on git head several weeks
>> ago), kexec kernel panics.
>> 
>> Then I tried latest mainline with git pull, everything works, (with or
>> without the patch, and can not reproduce the bug this patch is fixing)
>> 
>> Today, test again, kexec reboot hangs (with or without your patch), but
>> kdump works always (with or without the patch)
>> 
>> It is weird to me. Probably I need find out why I can not reproduce the
>> bug this patch is addressing first.
>> 
>> earlyprintk seems not working for me anymore, it is not easy to debug on
>> laptop now.
>> 
>> But the patch itself is clear, I think it should be good.  There might be
>> other things broken.
>
>Disable your immovable mem code then everything works for me.  There
>might be something wrong in the code.  Also "nokaslr" does not help, it
>should be another problem 

If "nokaslr" doesn't help, so I think
>+      /*num_immovable_mem = count_immovable_mem_regions();*/
also doesn't help. I think the problem is from get_rsdp_addr().

You test it in your laptop, I only tested in Qemu guest.
I have no laptop avaliable, so I will look for a physical machine
to test it.

Thanks,
Chao Fan

>
>diff --git a/arch/x86/boot/compressed/kaslr.c b/arch/x86/boot/compressed/kaslr.c
>index 2e53c056ba20..e760c9159662 100644
>--- a/arch/x86/boot/compressed/kaslr.c
>+++ b/arch/x86/boot/compressed/kaslr.c
>@@ -416,7 +416,7 @@ static void mem_avoid_init(unsigned long input, unsigned long input_size,
> 	handle_mem_options();
> 
> 	/* Enumerate the immovable memory regions */
>-	num_immovable_mem = count_immovable_mem_regions();
>+	/*num_immovable_mem = count_immovable_mem_regions();*/
> 
> #ifdef CONFIG_X86_VERBOSE_BOOTUP
> 	/* Make sure video RAM can be used. */
>diff --git a/arch/x86/boot/compressed/misc.c b/arch/x86/boot/compressed/misc.c
>index c0d6c560df69..1bc6f46d3aa7 100644
>--- a/arch/x86/boot/compressed/misc.c
>+++ b/arch/x86/boot/compressed/misc.c
>@@ -352,7 +352,7 @@ asmlinkage __visible void *extract_kernel(void *rmode, memptr heap,
> 	boot_params->hdr.loadflags &= ~KASLR_FLAG;
> 
> 	/* Save RSDP address for later use. */
>-	boot_params->acpi_rsdp_addr = get_rsdp_addr();
>+/*	boot_params->acpi_rsdp_addr = get_rsdp_addr(); */
> 
> 	sanitize_boot_params(boot_params);
> 
>
>



_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: [PATCH v2] x86/boot: Use efi_setup_data for searching RSDP on kexec-ed kernel

[bhe]

On 04/03/19 at 03:30pm, Chao Fan wrote:
> On Wed, Apr 03, 2019 at 02:39:11PM +0800, Dave Young wrote:
> >> Actually I got some different kexec test results.
> >> 
> >> Yesterday, with my installed kernel (based on git head several weeks
> >> ago), kexec kernel panics.
> >> 
> >> Then I tried latest mainline with git pull, everything works, (with or
> >> without the patch, and can not reproduce the bug this patch is fixing)
> >> 
> >> Today, test again, kexec reboot hangs (with or without your patch), but
> >> kdump works always (with or without the patch)
> >> 
> >> It is weird to me. Probably I need find out why I can not reproduce the
> >> bug this patch is addressing first.
> >> 
> >> earlyprintk seems not working for me anymore, it is not easy to debug on
> >> laptop now.
> >> 
> >> But the patch itself is clear, I think it should be good.  There might be
> >> other things broken.
> >
> >Disable your immovable mem code then everything works for me.  There
> >might be something wrong in the code.  Also "nokaslr" does not help, it
> >should be another problem 
> 
> If "nokaslr" doesn't help, so I think
> >+      /*num_immovable_mem = count_immovable_mem_regions();*/
> also doesn't help. I think the problem is from get_rsdp_addr().

Yes, seems get_rsdp_addr() has issue in this case. I am wondering if we
can adjust the postion of get_rsdp_addr() calling. If nokaslr is
specified, no need to get rsdp?

> 
> You test it in your laptop, I only tested in Qemu guest.
> I have no laptop avaliable, so I will look for a physical machine
> to test it.
> 
> Thanks,
> Chao Fan
> 
> >
> >diff --git a/arch/x86/boot/compressed/kaslr.c b/arch/x86/boot/compressed/kaslr.c
> >index 2e53c056ba20..e760c9159662 100644
> >--- a/arch/x86/boot/compressed/kaslr.c
> >+++ b/arch/x86/boot/compressed/kaslr.c
> >@@ -416,7 +416,7 @@ static void mem_avoid_init(unsigned long input, unsigned long input_size,
> > 	handle_mem_options();
> > 
> > 	/* Enumerate the immovable memory regions */
> >-	num_immovable_mem = count_immovable_mem_regions();
> >+	/*num_immovable_mem = count_immovable_mem_regions();*/
> > 
> > #ifdef CONFIG_X86_VERBOSE_BOOTUP
> > 	/* Make sure video RAM can be used. */
> >diff --git a/arch/x86/boot/compressed/misc.c b/arch/x86/boot/compressed/misc.c
> >index c0d6c560df69..1bc6f46d3aa7 100644
> >--- a/arch/x86/boot/compressed/misc.c
> >+++ b/arch/x86/boot/compressed/misc.c
> >@@ -352,7 +352,7 @@ asmlinkage __visible void *extract_kernel(void *rmode, memptr heap,
> > 	boot_params->hdr.loadflags &= ~KASLR_FLAG;
> > 
> > 	/* Save RSDP address for later use. */
> >-	boot_params->acpi_rsdp_addr = get_rsdp_addr();
> >+/*	boot_params->acpi_rsdp_addr = get_rsdp_addr(); */
> > 
> > 	sanitize_boot_params(boot_params);
> > 
> >
> >
> 
> 

_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: [PATCH v2] x86/boot: Use efi_setup_data for searching RSDP on kexec-ed kernel

[Dave Young]

On 04/03/19 at 03:50pm, Baoquan He wrote:
> On 04/03/19 at 03:30pm, Chao Fan wrote:
> > On Wed, Apr 03, 2019 at 02:39:11PM +0800, Dave Young wrote:
> > >> Actually I got some different kexec test results.
> > >> 
> > >> Yesterday, with my installed kernel (based on git head several weeks
> > >> ago), kexec kernel panics.
> > >> 
> > >> Then I tried latest mainline with git pull, everything works, (with or
> > >> without the patch, and can not reproduce the bug this patch is fixing)
> > >> 
> > >> Today, test again, kexec reboot hangs (with or without your patch), but
> > >> kdump works always (with or without the patch)
> > >> 
> > >> It is weird to me. Probably I need find out why I can not reproduce the
> > >> bug this patch is addressing first.
> > >> 
> > >> earlyprintk seems not working for me anymore, it is not easy to debug on
> > >> laptop now.
> > >> 
> > >> But the patch itself is clear, I think it should be good.  There might be
> > >> other things broken.
> > >
> > >Disable your immovable mem code then everything works for me.  There
> > >might be something wrong in the code.  Also "nokaslr" does not help, it
> > >should be another problem 
> > 
> > If "nokaslr" doesn't help, so I think
> > >+      /*num_immovable_mem = count_immovable_mem_regions();*/
> > also doesn't help. I think the problem is from get_rsdp_addr().
> 
> Yes, seems get_rsdp_addr() has issue in this case. I am wondering if we
> can adjust the postion of get_rsdp_addr() calling. If nokaslr is
> specified, no need to get rsdp?

I assumed the early code is only be useful for kaslr, if this is true
then no need to get rsdp in case nokaslr.

But I vaguely remember Boris want the boot_params rsdp pointer be a
general thing, I did not followed up these patch discussions, need to see how Boris thinks about this.

Thanks
Dave

_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: [PATCH v2] x86/boot: Use efi_setup_data for searching RSDP on kexec-ed kernel

[Dave Young]

On 04/03/19 at 04:23pm, Dave Young wrote:
> On 04/03/19 at 03:50pm, Baoquan He wrote:
> > On 04/03/19 at 03:30pm, Chao Fan wrote:
> > > On Wed, Apr 03, 2019 at 02:39:11PM +0800, Dave Young wrote:
> > > >> Actually I got some different kexec test results.
> > > >> 
> > > >> Yesterday, with my installed kernel (based on git head several weeks
> > > >> ago), kexec kernel panics.
> > > >> 
> > > >> Then I tried latest mainline with git pull, everything works, (with or
> > > >> without the patch, and can not reproduce the bug this patch is fixing)
> > > >> 
> > > >> Today, test again, kexec reboot hangs (with or without your patch), but
> > > >> kdump works always (with or without the patch)
> > > >> 
> > > >> It is weird to me. Probably I need find out why I can not reproduce the
> > > >> bug this patch is addressing first.
> > > >> 
> > > >> earlyprintk seems not working for me anymore, it is not easy to debug on
> > > >> laptop now.
> > > >> 
> > > >> But the patch itself is clear, I think it should be good.  There might be
> > > >> other things broken.
> > > >
> > > >Disable your immovable mem code then everything works for me.  There
> > > >might be something wrong in the code.  Also "nokaslr" does not help, it
> > > >should be another problem 
> > > 
> > > If "nokaslr" doesn't help, so I think
> > > >+      /*num_immovable_mem = count_immovable_mem_regions();*/
> > > also doesn't help. I think the problem is from get_rsdp_addr().
> > 
> > Yes, seems get_rsdp_addr() has issue in this case. I am wondering if we
> > can adjust the postion of get_rsdp_addr() calling. If nokaslr is
> > specified, no need to get rsdp?
> 
> I assumed the early code is only be useful for kaslr, if this is true
> then no need to get rsdp in case nokaslr.
> 
> But I vaguely remember Boris want the boot_params rsdp pointer be a
> general thing, I did not followed up these patch discussions, need to see how Boris thinks about this.
> 
Personally I would like to have a cmdline to bypass the early acpi code
because early code is hard to debug, if we have something weird happens
we can try to gate out these code, and then get some idea..

_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: [PATCH v2] x86/boot: Use efi_setup_data for searching RSDP on kexec-ed kernel

[Borislav Petkov]

On Wed, Apr 03, 2019 at 04:26:34PM +0800, Dave Young wrote:
> Personally I would like to have a cmdline to bypass the early acpi code
> because early code is hard to debug, if we have something weird happens
> we can try to gate out these code, and then get some idea..

No, this early crap better work on all machines by default.

Also, Chao, are you telling me that you've only tested in a VM? Or
have you actually tested your code on an actual machine with memory
hotremove?

-- 
Regards/Gruss,
    Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.

_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: [PATCH v2] x86/boot: Use efi_setup_data for searching RSDP on kexec-ed kernel

[Chao Fan]

On Wed, Apr 03, 2019 at 06:14:29PM +0200, Borislav Petkov wrote:
>On Wed, Apr 03, 2019 at 04:26:34PM +0800, Dave Young wrote:
>> Personally I would like to have a cmdline to bypass the early acpi code
>> because early code is hard to debug, if we have something weird happens
>> we can try to gate out these code, and then get some idea..
>
>No, this early crap better work on all machines by default.
>
>Also, Chao, are you telling me that you've only tested in a VM? Or

In VM, with all cases. In real machine, test boot and RSDP parsing,
not hotremove memory.

Thanks,
Chao Fan

>have you actually tested your code on an actual machine with memory
>hotremove?
>
>-- 
>Regards/Gruss,
>    Boris.
>
>Good mailing practices for 400: avoid top-posting and trim the reply.
>
>



_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: [PATCH v2] x86/boot: Use efi_setup_data for searching RSDP on kexec-ed kernel

[Chao Fan]

On Wed, Apr 03, 2019 at 03:50:15PM +0800, bhe@redhat.com wrote:
>On 04/03/19 at 03:30pm, Chao Fan wrote:
>> On Wed, Apr 03, 2019 at 02:39:11PM +0800, Dave Young wrote:
>> >> Actually I got some different kexec test results.
>> >> 
>> >> Yesterday, with my installed kernel (based on git head several weeks
>> >> ago), kexec kernel panics.
>> >> 
>> >> Then I tried latest mainline with git pull, everything works, (with or
>> >> without the patch, and can not reproduce the bug this patch is fixing)
>> >> 
>> >> Today, test again, kexec reboot hangs (with or without your patch), but
>> >> kdump works always (with or without the patch)
>> >> 
>> >> It is weird to me. Probably I need find out why I can not reproduce the
>> >> bug this patch is addressing first.
>> >> 
>> >> earlyprintk seems not working for me anymore, it is not easy to debug on
>> >> laptop now.
>> >> 
>> >> But the patch itself is clear, I think it should be good.  There might be
>> >> other things broken.
>> >
>> >Disable your immovable mem code then everything works for me.  There
>> >might be something wrong in the code.  Also "nokaslr" does not help, it
>> >should be another problem 
>> 
>> If "nokaslr" doesn't help, so I think
>> >+      /*num_immovable_mem = count_immovable_mem_regions();*/
>> also doesn't help. I think the problem is from get_rsdp_addr().
>
>Yes, seems get_rsdp_addr() has issue in this case. I am wondering if we
>can adjust the postion of get_rsdp_addr() calling. If nokaslr is
>specified, no need to get rsdp?

I think no, RSDP parsing dosn't only work for KASLR.

Thanks,
Chao Fan

>
>> 
>> You test it in your laptop, I only tested in Qemu guest.
>> I have no laptop avaliable, so I will look for a physical machine
>> to test it.
>> 
>> Thanks,
>> Chao Fan
>> 
>> >
>> >diff --git a/arch/x86/boot/compressed/kaslr.c b/arch/x86/boot/compressed/kaslr.c
>> >index 2e53c056ba20..e760c9159662 100644
>> >--- a/arch/x86/boot/compressed/kaslr.c
>> >+++ b/arch/x86/boot/compressed/kaslr.c
>> >@@ -416,7 +416,7 @@ static void mem_avoid_init(unsigned long input, unsigned long input_size,
>> > 	handle_mem_options();
>> > 
>> > 	/* Enumerate the immovable memory regions */
>> >-	num_immovable_mem = count_immovable_mem_regions();
>> >+	/*num_immovable_mem = count_immovable_mem_regions();*/
>> > 
>> > #ifdef CONFIG_X86_VERBOSE_BOOTUP
>> > 	/* Make sure video RAM can be used. */
>> >diff --git a/arch/x86/boot/compressed/misc.c b/arch/x86/boot/compressed/misc.c
>> >index c0d6c560df69..1bc6f46d3aa7 100644
>> >--- a/arch/x86/boot/compressed/misc.c
>> >+++ b/arch/x86/boot/compressed/misc.c
>> >@@ -352,7 +352,7 @@ asmlinkage __visible void *extract_kernel(void *rmode, memptr heap,
>> > 	boot_params->hdr.loadflags &= ~KASLR_FLAG;
>> > 
>> > 	/* Save RSDP address for later use. */
>> >-	boot_params->acpi_rsdp_addr = get_rsdp_addr();
>> >+/*	boot_params->acpi_rsdp_addr = get_rsdp_addr(); */
>> > 
>> > 	sanitize_boot_params(boot_params);
>> > 
>> >
>> >
>> 
>> 
>
>



_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: [PATCH v2] x86/boot: Use efi_setup_data for searching RSDP on kexec-ed kernel

[Chao Fan]

On Wed, Apr 03, 2019 at 01:53:40PM +0800, Dave Young wrote:
>On 04/03/19 at 01:35pm, Chao Fan wrote:
>> On Tue, Apr 02, 2019 at 08:03:19PM +0800, Dave Young wrote:
>> >On 04/01/19 at 12:08am, Junichi Nomura wrote:
>> >> Commit 3a63f70bf4c3a ("x86/boot: Early parse RSDP and save it in
>> >> boot_params") broke kexec boot on EFI systems.  efi_get_rsdp_addr()
>> >> in the early parsing code tries to search RSDP from EFI table but
>> >> that will crash because the table address is virtual when the kernel
>> >> was booted by kexec.
>> >> 
>> >> In the case of kexec, physical address of EFI tables is provided
>> >> via efi_setup_data in boot_params, which is set up by kexec(1).
>> >> 
>> >> Factor out the table parsing code and use different pointers depending
>> >> on whether the kernel is booted by kexec or not.
>> >> 
>> >> Fixes: 3a63f70bf4c3a ("x86/boot: Early parse RSDP and save it in boot_params")
>> >> Signed-off-by: Jun'ichi Nomura <j-nomura@ce.jp.nec.com>
>> >> Acked-by: Baoquan He <bhe@redhat.com>
>> >> Cc: Chao Fan <fanc.fnst@cn.fujitsu.com>
>> >> Cc: Borislav Petkov <bp@suse.de>
>> >> Cc: Dave Young <dyoung@redhat.com>
>> [...]
>> >
>> >I failed to kexec reboot on my laptop, kernel panics too quick,  I'm not sure this is
>> >caused by your patch though.
>> >
>> >Actually there are something probably i915 changes break kexec,  the
>> >above test is with "nomodeset" which should work.
>> >
>> >Let me do more testing and update here tomorrow.
>> >
>> 
>> Hi Dave,
>> 
>> Last day I was testing the normal kexec, today I have tested the kdump
>> issue. Since the kdump has set "nokaslr" to cmdline, so I drop from
>> KDUMP_COMMANDLINE_APPEND
>> And it booted OK, so the PATCH works in both normal kexec and kdump.
>> 
>
>Actually I got some different kexec test results.
>
>Yesterday, with my installed kernel (based on git head several weeks
>ago), kexec kernel panics.
>
>Then I tried latest mainline with git pull, everything works, (with or
>without the patch, and can not reproduce the bug this patch is fixing)
>
>Today, test again, kexec reboot hangs (with or without your patch), but
>kdump works always (with or without the patch)

That's so strange, I'm puzzled by your test.
Withour ant changes, just test again, the result of yesterday and today
are different?

>
>It is weird to me. Probably I need find out why I can not reproduce the
>bug this patch is addressing first.

Hmm, is your laptop booted from EFI or legacy BIOS?
>
>earlyprintk seems not working for me anymore, it is not easy to debug on
>laptop now.

Another computer can help print the console message.
I remeber you are good at debug on two computers.

Thanks,
Chao Fan

>
>But the patch itself is clear, I think it should be good.  There might be
>other things broken.
>
>Thanks
>Dave
>
>



_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: [PATCH v2] x86/boot: Use efi_setup_data for searching RSDP on kexec-ed kernel

[Dave Young]

On 04/03/19 at 03:21pm, Chao Fan wrote:
> On Wed, Apr 03, 2019 at 01:53:40PM +0800, Dave Young wrote:
> >On 04/03/19 at 01:35pm, Chao Fan wrote:
> >> On Tue, Apr 02, 2019 at 08:03:19PM +0800, Dave Young wrote:
> >> >On 04/01/19 at 12:08am, Junichi Nomura wrote:
> >> >> Commit 3a63f70bf4c3a ("x86/boot: Early parse RSDP and save it in
> >> >> boot_params") broke kexec boot on EFI systems.  efi_get_rsdp_addr()
> >> >> in the early parsing code tries to search RSDP from EFI table but
> >> >> that will crash because the table address is virtual when the kernel
> >> >> was booted by kexec.
> >> >> 
> >> >> In the case of kexec, physical address of EFI tables is provided
> >> >> via efi_setup_data in boot_params, which is set up by kexec(1).
> >> >> 
> >> >> Factor out the table parsing code and use different pointers depending
> >> >> on whether the kernel is booted by kexec or not.
> >> >> 
> >> >> Fixes: 3a63f70bf4c3a ("x86/boot: Early parse RSDP and save it in boot_params")
> >> >> Signed-off-by: Jun'ichi Nomura <j-nomura@ce.jp.nec.com>
> >> >> Acked-by: Baoquan He <bhe@redhat.com>
> >> >> Cc: Chao Fan <fanc.fnst@cn.fujitsu.com>
> >> >> Cc: Borislav Petkov <bp@suse.de>
> >> >> Cc: Dave Young <dyoung@redhat.com>
> >> [...]
> >> >
> >> >I failed to kexec reboot on my laptop, kernel panics too quick,  I'm not sure this is
> >> >caused by your patch though.
> >> >
> >> >Actually there are something probably i915 changes break kexec,  the
> >> >above test is with "nomodeset" which should work.
> >> >
> >> >Let me do more testing and update here tomorrow.
> >> >
> >> 
> >> Hi Dave,
> >> 
> >> Last day I was testing the normal kexec, today I have tested the kdump
> >> issue. Since the kdump has set "nokaslr" to cmdline, so I drop from
> >> KDUMP_COMMANDLINE_APPEND
> >> And it booted OK, so the PATCH works in both normal kexec and kdump.
> >> 
> >
> >Actually I got some different kexec test results.
> >
> >Yesterday, with my installed kernel (based on git head several weeks
> >ago), kexec kernel panics.
> >
> >Then I tried latest mainline with git pull, everything works, (with or
> >without the patch, and can not reproduce the bug this patch is fixing)
> >
> >Today, test again, kexec reboot hangs (with or without your patch), but
> >kdump works always (with or without the patch)
> 
> That's so strange, I'm puzzled by your test.
> Withour ant changes, just test again, the result of yesterday and today
> are different?
> 
> >
> >It is weird to me. Probably I need find out why I can not reproduce the
> >bug this patch is addressing first.
> 
> Hmm, is your laptop booted from EFI or legacy BIOS?

EFI booted.

> >
> >earlyprintk seems not working for me anymore, it is not easy to debug on
> >laptop now.
> 
> Another computer can help print the console message.
> I remeber you are good at debug on two computers.

I used to use earlyprintk, but seems earlyprintk can not work so early.
So I only can try to comment out code manually.

It seems to be still some bug in early acpi code because if I 
return 0 in get rsdp function it just works.

This guess make sense to me for now:
1. nokaslr does not work because it may only avoid randomization, but it
does not avoid running the early acpi parsing code.

2. kexec does not work without the patch we talked about, because I just
reproduced Junichi reported bug, sometimes hang, or reset, it is
understandable.

3. kexec does not work with the patch, so there is still bugs in the
code. 

So several things to do is:

Fix 1. (separate issue),  Baoquan or either of you may want to fix it :)

Fix 3.  need more debugging, have you or Junichi run tests on more real
hardware, maybe it is easier to reproduce on real hardware, I'm glad to
help to try test patch or provide any help. 

Thanks
Dave

_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: [PATCH v2] x86/boot: Use efi_setup_data for searching RSDP on kexec-ed kernel

[Chao Fan]

On Wed, Apr 03, 2019 at 04:09:16PM +0800, Dave Young wrote:
>On 04/03/19 at 03:21pm, Chao Fan wrote:
>> On Wed, Apr 03, 2019 at 01:53:40PM +0800, Dave Young wrote:
>> >On 04/03/19 at 01:35pm, Chao Fan wrote:
>> >> On Tue, Apr 02, 2019 at 08:03:19PM +0800, Dave Young wrote:
>> >> >On 04/01/19 at 12:08am, Junichi Nomura wrote:
>> >> >> Commit 3a63f70bf4c3a ("x86/boot: Early parse RSDP and save it in
>> >> >> boot_params") broke kexec boot on EFI systems.  efi_get_rsdp_addr()
>> >> >> in the early parsing code tries to search RSDP from EFI table but
>> >> >> that will crash because the table address is virtual when the kernel
>> >> >> was booted by kexec.
>> >> >> 
>> >> >> In the case of kexec, physical address of EFI tables is provided
>> >> >> via efi_setup_data in boot_params, which is set up by kexec(1).
>> >> >> 
>> >> >> Factor out the table parsing code and use different pointers depending
>> >> >> on whether the kernel is booted by kexec or not.
>> >> >> 
>> >> >> Fixes: 3a63f70bf4c3a ("x86/boot: Early parse RSDP and save it in boot_params")
>> >> >> Signed-off-by: Jun'ichi Nomura <j-nomura@ce.jp.nec.com>
>> >> >> Acked-by: Baoquan He <bhe@redhat.com>
>> >> >> Cc: Chao Fan <fanc.fnst@cn.fujitsu.com>
>> >> >> Cc: Borislav Petkov <bp@suse.de>
>> >> >> Cc: Dave Young <dyoung@redhat.com>
>> >> [...]
>> >> >
>> >> >I failed to kexec reboot on my laptop, kernel panics too quick,  I'm not sure this is
>> >> >caused by your patch though.
>> >> >
>> >> >Actually there are something probably i915 changes break kexec,  the
>> >> >above test is with "nomodeset" which should work.
>> >> >
>> >> >Let me do more testing and update here tomorrow.
>> >> >
>> >> 
>> >> Hi Dave,
>> >> 
>> >> Last day I was testing the normal kexec, today I have tested the kdump
>> >> issue. Since the kdump has set "nokaslr" to cmdline, so I drop from
>> >> KDUMP_COMMANDLINE_APPEND
>> >> And it booted OK, so the PATCH works in both normal kexec and kdump.
>> >> 
>> >
>> >Actually I got some different kexec test results.
>> >
>> >Yesterday, with my installed kernel (based on git head several weeks
>> >ago), kexec kernel panics.
>> >
>> >Then I tried latest mainline with git pull, everything works, (with or
>> >without the patch, and can not reproduce the bug this patch is fixing)
>> >
>> >Today, test again, kexec reboot hangs (with or without your patch), but
>> >kdump works always (with or without the patch)
>> 
>> That's so strange, I'm puzzled by your test.
>> Withour ant changes, just test again, the result of yesterday and today
>> are different?
>> 
>> >
>> >It is weird to me. Probably I need find out why I can not reproduce the
>> >bug this patch is addressing first.
>> 
>> Hmm, is your laptop booted from EFI or legacy BIOS?
>
>EFI booted.
>
>> >
>> >earlyprintk seems not working for me anymore, it is not easy to debug on
>> >laptop now.
>> 
>> Another computer can help print the console message.
>> I remeber you are good at debug on two computers.
>
>I used to use earlyprintk, but seems earlyprintk can not work so early.
>So I only can try to comment out code manually.

Ah, I got it. If you want to use earlyprintk to print message of
get_rsdp_addr(), you should change some code.
Because get_rsdp_addr() is called before earlyprintk initialed.

I am still testing, but in my debug stage, I change like this:
diff --git a/arch/x86/boot/compressed/misc.c b/arch/x86/boot/compressed/misc.c
index c0d6c560df69..dbbe8d9a5792 100644
--- a/arch/x86/boot/compressed/misc.c
+++ b/arch/x86/boot/compressed/misc.c
@@ -352,7 +352,6 @@ asmlinkage __visible void *extract_kernel(void *rmode, memptr heap,
        boot_params->hdr.loadflags &= ~KASLR_FLAG;

        /* Save RSDP address for later use. */
-       boot_params->acpi_rsdp_addr = get_rsdp_addr();

        sanitize_boot_params(boot_params);

@@ -368,6 +367,7 @@ asmlinkage __visible void *extract_kernel(void *rmode, memptr heap,
        cols = boot_params->screen_info.orig_video_cols;

        console_init();
+       boot_params->acpi_rsdp_addr = get_rsdp_addr();
        debug_putstr("early console in extract_kernel\n");

        free_mem_ptr     = heap;        /* Heap */

Call this function after console_init().
That will make earlyprintk works.

>
>It seems to be still some bug in early acpi code because if I 
>return 0 in get rsdp function it just works.
>
>This guess make sense to me for now:
>1. nokaslr does not work because it may only avoid randomization, but it
>does not avoid running the early acpi parsing code.
>
>2. kexec does not work without the patch we talked about, because I just
>reproduced Junichi reported bug, sometimes hang, or reset, it is
>understandable.
>
>3. kexec does not work with the patch, so there is still bugs in the
>code. 
>
>So several things to do is:
>
>Fix 1. (separate issue),  Baoquan or either of you may want to fix it :)

For 1, I think it's not needed to fix. Since not only KASLR needs RSDP.
So nokaslr should not stop the acpi parsing code.
>
>Fix 3.  need more debugging, have you or Junichi run tests on more real
>hardware, maybe it is easier to reproduce on real hardware, I'm glad to
>help to try test patch or provide any help. 

I am still testing in real hardware.

Thanks,
Chao Fan

>
>Thanks
>Dave
>
>



_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: [PATCH v2] x86/boot: Use efi_setup_data for searching RSDP on kexec-ed kernel

[Chao Fan]

On Wed, Apr 03, 2019 at 04:23:06PM +0800, Chao Fan wrote:
>On Wed, Apr 03, 2019 at 04:09:16PM +0800, Dave Young wrote:
>>On 04/03/19 at 03:21pm, Chao Fan wrote:
>>> On Wed, Apr 03, 2019 at 01:53:40PM +0800, Dave Young wrote:
>>> >On 04/03/19 at 01:35pm, Chao Fan wrote:
>>> >> On Tue, Apr 02, 2019 at 08:03:19PM +0800, Dave Young wrote:
>>> >> >On 04/01/19 at 12:08am, Junichi Nomura wrote:
[...]
>>> >
>>> >It is weird to me. Probably I need find out why I can not reproduce the
>>> >bug this patch is addressing first.
>>> 
>>> Hmm, is your laptop booted from EFI or legacy BIOS?
>>
>>EFI booted.
>>
>>> >
>>> >earlyprintk seems not working for me anymore, it is not easy to debug on
>>> >laptop now.
>>> 
>>> Another computer can help print the console message.
>>> I remeber you are good at debug on two computers.
>>
>>I used to use earlyprintk, but seems earlyprintk can not work so early.
>>So I only can try to comment out code manually.
>
>Ah, I got it. If you want to use earlyprintk to print message of
>get_rsdp_addr(), you should change some code.
>Because get_rsdp_addr() is called before earlyprintk initialed.
>
>I am still testing, but in my debug stage, I change like this:
>diff --git a/arch/x86/boot/compressed/misc.c b/arch/x86/boot/compressed/misc.c
>index c0d6c560df69..dbbe8d9a5792 100644
>--- a/arch/x86/boot/compressed/misc.c
>+++ b/arch/x86/boot/compressed/misc.c
>@@ -352,7 +352,6 @@ asmlinkage __visible void *extract_kernel(void *rmode, memptr heap,
>        boot_params->hdr.loadflags &= ~KASLR_FLAG;
>
>        /* Save RSDP address for later use. */
>-       boot_params->acpi_rsdp_addr = get_rsdp_addr();
>
>        sanitize_boot_params(boot_params);
>
>@@ -368,6 +367,7 @@ asmlinkage __visible void *extract_kernel(void *rmode, memptr heap,
>        cols = boot_params->screen_info.orig_video_cols;
>
>        console_init();
>+       boot_params->acpi_rsdp_addr = get_rsdp_addr();
>        debug_putstr("early console in extract_kernel\n");
>
>        free_mem_ptr     = heap;        /* Heap */
>
>Call this function after console_init().
>That will make earlyprintk works.
>
>>
>>It seems to be still some bug in early acpi code because if I 
>>return 0 in get rsdp function it just works.
>>
>>This guess make sense to me for now:
>>1. nokaslr does not work because it may only avoid randomization, but it
>>does not avoid running the early acpi parsing code.
>>
>>2. kexec does not work without the patch we talked about, because I just
>>reproduced Junichi reported bug, sometimes hang, or reset, it is
>>understandable.
>>
>>3. kexec does not work with the patch, so there is still bugs in the
>>code. 
>>
>>So several things to do is:
>>
>>Fix 1. (separate issue),  Baoquan or either of you may want to fix it :)
>
>For 1, I think it's not needed to fix. Since not only KASLR needs RSDP.
>So nokaslr should not stop the acpi parsing code.
>>
>>Fix 3.  need more debugging, have you or Junichi run tests on more real
>>hardware, maybe it is easier to reproduce on real hardware, I'm glad to
>>help to try test patch or provide any help. 
>
>I am still testing in real hardware.

Hi Dave,

I find a Fujitsu Desktop PC to test it.
Without this PATCH, it failed to kexec and kdump.
With this PATCH, it succeed to kexec.
But failed to kdump. From the log, I think it didn't jump to the second
kernel, just reboot after panic. I have not figured out what's the
problem, but it seems not caused by this PATCH.
So I still think this PATCH works for the Fujitsu Desktop PC.

As for your issue, I think there may be some problems related to specified
hardware. Are you using a Lenovo laptop?

And I am not sure how Nomura tested it.

Thanks,
Chao Fan

>
>Thanks,
>Chao Fan
>
>>
>>Thanks
>>Dave
>>
>>



_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: [PATCH v2] x86/boot: Use efi_setup_data for searching RSDP on kexec-ed kernel

[Chao Fan]

On Wed, Apr 03, 2019 at 05:02:11PM +0800, Chao Fan wrote:
>On Wed, Apr 03, 2019 at 04:23:06PM +0800, Chao Fan wrote:
>>On Wed, Apr 03, 2019 at 04:09:16PM +0800, Dave Young wrote:
>>>On 04/03/19 at 03:21pm, Chao Fan wrote:
>>>> On Wed, Apr 03, 2019 at 01:53:40PM +0800, Dave Young wrote:
>>>> >On 04/03/19 at 01:35pm, Chao Fan wrote:
>>>> >> On Tue, Apr 02, 2019 at 08:03:19PM +0800, Dave Young wrote:
>>>> >> >On 04/01/19 at 12:08am, Junichi Nomura wrote:
>[...]
>>>> >
>>>> >It is weird to me. Probably I need find out why I can not reproduce the
>>>> >bug this patch is addressing first.
>>>> 
>>>> Hmm, is your laptop booted from EFI or legacy BIOS?
>>>
>>>EFI booted.
>>>
>>>> >
>>>> >earlyprintk seems not working for me anymore, it is not easy to debug on
>>>> >laptop now.
>>>> 
>>>> Another computer can help print the console message.
>>>> I remeber you are good at debug on two computers.
>>>
>>>I used to use earlyprintk, but seems earlyprintk can not work so early.
>>>So I only can try to comment out code manually.
>>
>>Ah, I got it. If you want to use earlyprintk to print message of
>>get_rsdp_addr(), you should change some code.
>>Because get_rsdp_addr() is called before earlyprintk initialed.
>>
>>I am still testing, but in my debug stage, I change like this:
>>diff --git a/arch/x86/boot/compressed/misc.c b/arch/x86/boot/compressed/misc.c
>>index c0d6c560df69..dbbe8d9a5792 100644
>>--- a/arch/x86/boot/compressed/misc.c
>>+++ b/arch/x86/boot/compressed/misc.c
>>@@ -352,7 +352,6 @@ asmlinkage __visible void *extract_kernel(void *rmode, memptr heap,
>>        boot_params->hdr.loadflags &= ~KASLR_FLAG;
>>
>>        /* Save RSDP address for later use. */
>>-       boot_params->acpi_rsdp_addr = get_rsdp_addr();
>>
>>        sanitize_boot_params(boot_params);
>>
>>@@ -368,6 +367,7 @@ asmlinkage __visible void *extract_kernel(void *rmode, memptr heap,
>>        cols = boot_params->screen_info.orig_video_cols;
>>
>>        console_init();
>>+       boot_params->acpi_rsdp_addr = get_rsdp_addr();
>>        debug_putstr("early console in extract_kernel\n");
>>
>>        free_mem_ptr     = heap;        /* Heap */
>>
>>Call this function after console_init().
>>That will make earlyprintk works.
>>
>>>
>>>It seems to be still some bug in early acpi code because if I 
>>>return 0 in get rsdp function it just works.
>>>
>>>This guess make sense to me for now:
>>>1. nokaslr does not work because it may only avoid randomization, but it
>>>does not avoid running the early acpi parsing code.
>>>
>>>2. kexec does not work without the patch we talked about, because I just
>>>reproduced Junichi reported bug, sometimes hang, or reset, it is
>>>understandable.
>>>
>>>3. kexec does not work with the patch, so there is still bugs in the
>>>code. 
>>>
>>>So several things to do is:
>>>
>>>Fix 1. (separate issue),  Baoquan or either of you may want to fix it :)
>>
>>For 1, I think it's not needed to fix. Since not only KASLR needs RSDP.
>>So nokaslr should not stop the acpi parsing code.
>>>
>>>Fix 3.  need more debugging, have you or Junichi run tests on more real
>>>hardware, maybe it is easier to reproduce on real hardware, I'm glad to
>>>help to try test patch or provide any help. 
>>
>>I am still testing in real hardware.
>
>Hi Dave,
>
>I find a Fujitsu Desktop PC to test it.
>Without this PATCH, it failed to kexec and kdump.
>With this PATCH, it succeed to kexec.
>But failed to kdump. From the log, I think it didn't jump to the second
>kernel, just reboot after panic. I have not figured out what's the
>problem, but it seems not caused by this PATCH.

I comment out this line:
diff --git a/arch/x86/boot/compressed/misc.c b/arch/x86/boot/compressed/misc.c
index c0d6c560df69..4d826abe659c 100644
--- a/arch/x86/boot/compressed/misc.c
+++ b/arch/x86/boot/compressed/misc.c
@@ -352,7 +352,7 @@ asmlinkage __visible void *extract_kernel(void *rmode, memptr heap,
        boot_params->hdr.loadflags &= ~KASLR_FLAG;

        /* Save RSDP address for later use. */
-       boot_params->acpi_rsdp_addr = get_rsdp_addr();
+       //boot_params->acpi_rsdp_addr = get_rsdp_addr();

        sanitize_boot_params(boot_params);

Kdump also failed to jump to the second kernel and just hang.
So I think it's another issue. As for this PATCH, I think it works.
So for now, we only find your laptop will meet the problem, so maybe
you can debug for more information.

Thanks,
Chao Fan

>So I still think this PATCH works for the Fujitsu Desktop PC.
>
>As for your issue, I think there may be some problems related to specified
>hardware. Are you using a Lenovo laptop?
>
>And I am not sure how Nomura tested it.
>
>Thanks,
>Chao Fan
>
>>
>>Thanks,
>>Chao Fan
>>
>>>
>>>Thanks
>>>Dave
>>>
>>>



_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: [PATCH v2] x86/boot: Use efi_setup_data for searching RSDP on kexec-ed kernel

[Junichi Nomura]

Hi Dave and Chao,

On 4/3/19 6:02 PM, Chao Fan wrote:
> On Wed, Apr 03, 2019 at 04:23:06PM +0800, Chao Fan wrote:
>> On Wed, Apr 03, 2019 at 04:09:16PM +0800, Dave Young wrote:
>>> Fix 3.  need more debugging, have you or Junichi run tests on more real
>>> hardware, maybe it is easier to reproduce on real hardware, I'm glad to
>>> help to try test patch or provide any help. 
>>
>> I am still testing in real hardware.
> 
> Hi Dave,
> 
> I find a Fujitsu Desktop PC to test it.
> Without this PATCH, it failed to kexec and kdump.
> With this PATCH, it succeed to kexec.
> But failed to kdump. From the log, I think it didn't jump to the second
> kernel, just reboot after panic. I have not figured out what's the
> problem, but it seems not caused by this PATCH.
> So I still think this PATCH works for the Fujitsu Desktop PC.
> 
> As for your issue, I think there may be some problems related to specified
> hardware. Are you using a Lenovo laptop?
> 
> And I am not sure how Nomura tested it.

I've tested 3 different models of EFI-booted baremetal servers with both
normal kexec and panic kexec.  So far as I've tried Linus's v5.1-rc3,
the problem always reproduced without the patch and disappears with the patch.

-- 
Jun'ichi Nomura, NEC Corporation / NEC Solution Innovators, Ltd.

_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: [PATCH v2] x86/boot: Use efi_setup_data for searching RSDP on kexec-ed kernel

[Dave Young]

On 04/04/19 at 01:23am, Junichi Nomura wrote:
> Hi Dave and Chao,
> 
> On 4/3/19 6:02 PM, Chao Fan wrote:
> > On Wed, Apr 03, 2019 at 04:23:06PM +0800, Chao Fan wrote:
> >> On Wed, Apr 03, 2019 at 04:09:16PM +0800, Dave Young wrote:
> >>> Fix 3.  need more debugging, have you or Junichi run tests on more real
> >>> hardware, maybe it is easier to reproduce on real hardware, I'm glad to
> >>> help to try test patch or provide any help. 
> >>
> >> I am still testing in real hardware.
> > 
> > Hi Dave,
> > 
> > I find a Fujitsu Desktop PC to test it.
> > Without this PATCH, it failed to kexec and kdump.
> > With this PATCH, it succeed to kexec.
> > But failed to kdump. From the log, I think it didn't jump to the second
> > kernel, just reboot after panic. I have not figured out what's the
> > problem, but it seems not caused by this PATCH.
> > So I still think this PATCH works for the Fujitsu Desktop PC.
> > 
> > As for your issue, I think there may be some problems related to specified
> > hardware. Are you using a Lenovo laptop?
> > 
> > And I am not sure how Nomura tested it.
> 
> I've tested 3 different models of EFI-booted baremetal servers with both
> normal kexec and panic kexec.  So far as I've tried Linus's v5.1-rc3,
> the problem always reproduced without the patch and disappears with the patch.

Hmm, both of my two laptops (Thinkpad T480s and T420) failed to boot with kexec.

I will see if I can find something, but it may need more time because
early console does not work especially after kexec.

Thanks
Dave

_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: [PATCH v2] x86/boot: Use efi_setup_data for searching RSDP on kexec-ed kernel

[bhe]

On 04/04/19 at 10:52am, Dave Young wrote:
> On 04/04/19 at 01:23am, Junichi Nomura wrote:
> > Hi Dave and Chao,
> > 
> > On 4/3/19 6:02 PM, Chao Fan wrote:
> > > On Wed, Apr 03, 2019 at 04:23:06PM +0800, Chao Fan wrote:
> > >> On Wed, Apr 03, 2019 at 04:09:16PM +0800, Dave Young wrote:
> > >>> Fix 3.  need more debugging, have you or Junichi run tests on more real
> > >>> hardware, maybe it is easier to reproduce on real hardware, I'm glad to
> > >>> help to try test patch or provide any help. 
> > >>
> > >> I am still testing in real hardware.
> > > 
> > > Hi Dave,
> > > 
> > > I find a Fujitsu Desktop PC to test it.
> > > Without this PATCH, it failed to kexec and kdump.
> > > With this PATCH, it succeed to kexec.
> > > But failed to kdump. From the log, I think it didn't jump to the second
> > > kernel, just reboot after panic. I have not figured out what's the
> > > problem, but it seems not caused by this PATCH.
> > > So I still think this PATCH works for the Fujitsu Desktop PC.
> > > 
> > > As for your issue, I think there may be some problems related to specified
> > > hardware. Are you using a Lenovo laptop?
> > > 
> > > And I am not sure how Nomura tested it.
> > 
> > I've tested 3 different models of EFI-booted baremetal servers with both
> > normal kexec and panic kexec.  So far as I've tried Linus's v5.1-rc3,
> > the problem always reproduced without the patch and disappears with the patch.
> 
> Hmm, both of my two laptops (Thinkpad T480s and T420) failed to boot with kexec.
> 
> I will see if I can find something, but it may need more time because
> early console does not work especially after kexec.

Dave, can you try below patch to print debugging message and hang kernel
to check the outputting? The hang is necessary, otherwise later printk
printking will overwrite it.

diff --git a/arch/x86/boot/compressed/misc.c b/arch/x86/boot/compressed/misc.c
index c0d6c560df69..68119547c4aa 100644
--- a/arch/x86/boot/compressed/misc.c
+++ b/arch/x86/boot/compressed/misc.c
@@ -351,9 +351,6 @@ asmlinkage __visible void *extract_kernel(void *rmode, memptr heap,
 	/* Clear flags intended for solely in-kernel use. */
 	boot_params->hdr.loadflags &= ~KASLR_FLAG;
 
-	/* Save RSDP address for later use. */
-	boot_params->acpi_rsdp_addr = get_rsdp_addr();
-
 	sanitize_boot_params(boot_params);
 
 	if (boot_params->screen_info.orig_video_mode == 7) {
@@ -370,6 +367,10 @@ asmlinkage __visible void *extract_kernel(void *rmode, memptr heap,
 	console_init();
 	debug_putstr("early console in extract_kernel\n");
 
+	error("Hang kernel for kexec debugging");
+	/* Save RSDP address for later use. */
+	boot_params->acpi_rsdp_addr = get_rsdp_addr();
+
 	free_mem_ptr     = heap;	/* Heap */
 	free_mem_end_ptr = heap + BOOT_HEAP_SIZE;
 

_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: [PATCH v2] x86/boot: Use efi_setup_data for searching RSDP on kexec-ed kernel

[bhe]

On 04/04/19 at 11:00am, Baoquan He wrote:
> On 04/04/19 at 10:52am, Dave Young wrote:
> > On 04/04/19 at 01:23am, Junichi Nomura wrote:
> > > Hi Dave and Chao,
> > > 
> > > On 4/3/19 6:02 PM, Chao Fan wrote:
> > > > On Wed, Apr 03, 2019 at 04:23:06PM +0800, Chao Fan wrote:
> > > >> On Wed, Apr 03, 2019 at 04:09:16PM +0800, Dave Young wrote:
> > > >>> Fix 3.  need more debugging, have you or Junichi run tests on more real
> > > >>> hardware, maybe it is easier to reproduce on real hardware, I'm glad to
> > > >>> help to try test patch or provide any help. 
> > > >>
> > > >> I am still testing in real hardware.
> > > > 
> > > > Hi Dave,
> > > > 
> > > > I find a Fujitsu Desktop PC to test it.
> > > > Without this PATCH, it failed to kexec and kdump.
> > > > With this PATCH, it succeed to kexec.
> > > > But failed to kdump. From the log, I think it didn't jump to the second
> > > > kernel, just reboot after panic. I have not figured out what's the
> > > > problem, but it seems not caused by this PATCH.
> > > > So I still think this PATCH works for the Fujitsu Desktop PC.
> > > > 
> > > > As for your issue, I think there may be some problems related to specified
> > > > hardware. Are you using a Lenovo laptop?
> > > > 
> > > > And I am not sure how Nomura tested it.
> > > 
> > > I've tested 3 different models of EFI-booted baremetal servers with both
> > > normal kexec and panic kexec.  So far as I've tried Linus's v5.1-rc3,
> > > the problem always reproduced without the patch and disappears with the patch.
> > 
> > Hmm, both of my two laptops (Thinkpad T480s and T420) failed to boot with kexec.
> > 
> > I will see if I can find something, but it may need more time because
> > early console does not work especially after kexec.
> 
> Dave, can you try below patch to print debugging message and hang kernel
> to check the outputting? The hang is necessary, otherwise later printk
> printking will overwrite it.
> 
> diff --git a/arch/x86/boot/compressed/misc.c b/arch/x86/boot/compressed/misc.c
> index c0d6c560df69..68119547c4aa 100644
> --- a/arch/x86/boot/compressed/misc.c
> +++ b/arch/x86/boot/compressed/misc.c
> @@ -351,9 +351,6 @@ asmlinkage __visible void *extract_kernel(void *rmode, memptr heap,
>  	/* Clear flags intended for solely in-kernel use. */
>  	boot_params->hdr.loadflags &= ~KASLR_FLAG;
>  
> -	/* Save RSDP address for later use. */
> -	boot_params->acpi_rsdp_addr = get_rsdp_addr();
> -
>  	sanitize_boot_params(boot_params);
>  
>  	if (boot_params->screen_info.orig_video_mode == 7) {
> @@ -370,6 +367,10 @@ asmlinkage __visible void *extract_kernel(void *rmode, memptr heap,
>  	console_init();
>  	debug_putstr("early console in extract_kernel\n");
>  
 +	/* Save RSDP address for later use. */
 +	boot_params->acpi_rsdp_addr = get_rsdp_addr();
 +
 +	error("Hang kernel for kexec debugging");

Sorry, here I means calling error() to hang kernel after calling
get_rsdp_addr().


>  	free_mem_ptr     = heap;	/* Heap */
>  	free_mem_end_ptr = heap + BOOT_HEAP_SIZE;
>  

_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: [PATCH v2] x86/boot: Use efi_setup_data for searching RSDP on kexec-ed kernel

[Dave Young]

On 04/04/19 at 11:10am, Baoquan He wrote:
> On 04/04/19 at 11:00am, Baoquan He wrote:
> > On 04/04/19 at 10:52am, Dave Young wrote:
> > > On 04/04/19 at 01:23am, Junichi Nomura wrote:
> > > > Hi Dave and Chao,
> > > > 
> > > > On 4/3/19 6:02 PM, Chao Fan wrote:
> > > > > On Wed, Apr 03, 2019 at 04:23:06PM +0800, Chao Fan wrote:
> > > > >> On Wed, Apr 03, 2019 at 04:09:16PM +0800, Dave Young wrote:
> > > > >>> Fix 3.  need more debugging, have you or Junichi run tests on more real
> > > > >>> hardware, maybe it is easier to reproduce on real hardware, I'm glad to
> > > > >>> help to try test patch or provide any help. 
> > > > >>
> > > > >> I am still testing in real hardware.
> > > > > 
> > > > > Hi Dave,
> > > > > 
> > > > > I find a Fujitsu Desktop PC to test it.
> > > > > Without this PATCH, it failed to kexec and kdump.
> > > > > With this PATCH, it succeed to kexec.
> > > > > But failed to kdump. From the log, I think it didn't jump to the second
> > > > > kernel, just reboot after panic. I have not figured out what's the
> > > > > problem, but it seems not caused by this PATCH.
> > > > > So I still think this PATCH works for the Fujitsu Desktop PC.
> > > > > 
> > > > > As for your issue, I think there may be some problems related to specified
> > > > > hardware. Are you using a Lenovo laptop?
> > > > > 
> > > > > And I am not sure how Nomura tested it.
> > > > 
> > > > I've tested 3 different models of EFI-booted baremetal servers with both
> > > > normal kexec and panic kexec.  So far as I've tried Linus's v5.1-rc3,
> > > > the problem always reproduced without the patch and disappears with the patch.
> > > 
> > > Hmm, both of my two laptops (Thinkpad T480s and T420) failed to boot with kexec.
> > > 
> > > I will see if I can find something, but it may need more time because
> > > early console does not work especially after kexec.
> > 
> > Dave, can you try below patch to print debugging message and hang kernel
> > to check the outputting? The hang is necessary, otherwise later printk
> > printking will overwrite it.
> > 
> > diff --git a/arch/x86/boot/compressed/misc.c b/arch/x86/boot/compressed/misc.c
> > index c0d6c560df69..68119547c4aa 100644
> > --- a/arch/x86/boot/compressed/misc.c
> > +++ b/arch/x86/boot/compressed/misc.c
> > @@ -351,9 +351,6 @@ asmlinkage __visible void *extract_kernel(void *rmode, memptr heap,
> >  	/* Clear flags intended for solely in-kernel use. */
> >  	boot_params->hdr.loadflags &= ~KASLR_FLAG;
> >  
> > -	/* Save RSDP address for later use. */
> > -	boot_params->acpi_rsdp_addr = get_rsdp_addr();
> > -
> >  	sanitize_boot_params(boot_params);
> >  
> >  	if (boot_params->screen_info.orig_video_mode == 7) {
> > @@ -370,6 +367,10 @@ asmlinkage __visible void *extract_kernel(void *rmode, memptr heap,
> >  	console_init();
> >  	debug_putstr("early console in extract_kernel\n");
> >  
>  +	/* Save RSDP address for later use. */
>  +	boot_params->acpi_rsdp_addr = get_rsdp_addr();
>  +
>  +	error("Hang kernel for kexec debugging");
> 
> Sorry, here I means calling error() to hang kernel after calling
> get_rsdp_addr().

Thanks, it did not hang, it always reset to firmware/grub boot menu.
I'm pretty sure now the bug exists in get_rsdp_addr().

_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: [PATCH v2] x86/boot: Use efi_setup_data for searching RSDP on kexec-ed kernel

[Dave Young]

On 04/04/19 at 11:22am, Dave Young wrote:
> On 04/04/19 at 11:10am, Baoquan He wrote:
> > On 04/04/19 at 11:00am, Baoquan He wrote:
> > > On 04/04/19 at 10:52am, Dave Young wrote:
> > > > On 04/04/19 at 01:23am, Junichi Nomura wrote:
> > > > > Hi Dave and Chao,
> > > > > 
> > > > > On 4/3/19 6:02 PM, Chao Fan wrote:
> > > > > > On Wed, Apr 03, 2019 at 04:23:06PM +0800, Chao Fan wrote:
> > > > > >> On Wed, Apr 03, 2019 at 04:09:16PM +0800, Dave Young wrote:
> > > > > >>> Fix 3.  need more debugging, have you or Junichi run tests on more real
> > > > > >>> hardware, maybe it is easier to reproduce on real hardware, I'm glad to
> > > > > >>> help to try test patch or provide any help. 
> > > > > >>
> > > > > >> I am still testing in real hardware.
> > > > > > 
> > > > > > Hi Dave,
> > > > > > 
> > > > > > I find a Fujitsu Desktop PC to test it.
> > > > > > Without this PATCH, it failed to kexec and kdump.
> > > > > > With this PATCH, it succeed to kexec.
> > > > > > But failed to kdump. From the log, I think it didn't jump to the second
> > > > > > kernel, just reboot after panic. I have not figured out what's the
> > > > > > problem, but it seems not caused by this PATCH.
> > > > > > So I still think this PATCH works for the Fujitsu Desktop PC.
> > > > > > 
> > > > > > As for your issue, I think there may be some problems related to specified
> > > > > > hardware. Are you using a Lenovo laptop?
> > > > > > 
> > > > > > And I am not sure how Nomura tested it.
> > > > > 
> > > > > I've tested 3 different models of EFI-booted baremetal servers with both
> > > > > normal kexec and panic kexec.  So far as I've tried Linus's v5.1-rc3,
> > > > > the problem always reproduced without the patch and disappears with the patch.
> > > > 
> > > > Hmm, both of my two laptops (Thinkpad T480s and T420) failed to boot with kexec.
> > > > 
> > > > I will see if I can find something, but it may need more time because
> > > > early console does not work especially after kexec.
> > > 
> > > Dave, can you try below patch to print debugging message and hang kernel
> > > to check the outputting? The hang is necessary, otherwise later printk
> > > printking will overwrite it.
> > > 
> > > diff --git a/arch/x86/boot/compressed/misc.c b/arch/x86/boot/compressed/misc.c
> > > index c0d6c560df69..68119547c4aa 100644
> > > --- a/arch/x86/boot/compressed/misc.c
> > > +++ b/arch/x86/boot/compressed/misc.c
> > > @@ -351,9 +351,6 @@ asmlinkage __visible void *extract_kernel(void *rmode, memptr heap,
> > >  	/* Clear flags intended for solely in-kernel use. */
> > >  	boot_params->hdr.loadflags &= ~KASLR_FLAG;
> > >  
> > > -	/* Save RSDP address for later use. */
> > > -	boot_params->acpi_rsdp_addr = get_rsdp_addr();
> > > -
> > >  	sanitize_boot_params(boot_params);
> > >  
> > >  	if (boot_params->screen_info.orig_video_mode == 7) {
> > > @@ -370,6 +367,10 @@ asmlinkage __visible void *extract_kernel(void *rmode, memptr heap,
> > >  	console_init();
> > >  	debug_putstr("early console in extract_kernel\n");
> > >  
> >  +	/* Save RSDP address for later use. */
> >  +	boot_params->acpi_rsdp_addr = get_rsdp_addr();
> >  +
> >  +	error("Hang kernel for kexec debugging");
> > 
> > Sorry, here I means calling error() to hang kernel after calling
> > get_rsdp_addr().
> 
> Thanks, it did not hang, it always reset to firmware/grub boot menu.
> I'm pretty sure now the bug exists in get_rsdp_addr().

static acpi_physical_address kexec_get_rsdp_addr(void)
{
...
        /* Get systab from boot params. */
        systab = (efi_system_table_64_t *) (ei->efi_systab | ((__u64)ei->efi_systab_hi << 32));
        if (!systab)
              error("EFI system table not found in kexec boot_params.");
 
...
  -> add error("hang me") here will have a hang
...
        return __efi_get_rsdp_addr((unsigned long)esd->tables,
                                   systab->nr_tables, true);
 
But add error("hang me") in __efi_get_rsdp_addr it did not hang.
 
It seems reference the systab pointer cause a system reset.
 
A question is does the identity mapping covered the memory address of
systab?
 
In my case it is 0xdad9ef18
 
If the memory is mapped on demand, then there will be problems, it
should cover setup_data and efi table space.
 
Thanks
Dave


_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: [PATCH v2] x86/boot: Use efi_setup_data for searching RSDP on kexec-ed kernel

[Chao Fan]

On Thu, Apr 04, 2019 at 02:41:30PM +0800, Dave Young wrote:
>On 04/04/19 at 11:22am, Dave Young wrote:
>> On 04/04/19 at 11:10am, Baoquan He wrote:
>> > On 04/04/19 at 11:00am, Baoquan He wrote:
>> > > On 04/04/19 at 10:52am, Dave Young wrote:
>> > > > On 04/04/19 at 01:23am, Junichi Nomura wrote:
>> >  +	/* Save RSDP address for later use. */
>> >  +	boot_params->acpi_rsdp_addr = get_rsdp_addr();
>> >  +
>> >  +	error("Hang kernel for kexec debugging");
>> > 
>> > Sorry, here I means calling error() to hang kernel after calling
>> > get_rsdp_addr().
>> 
>> Thanks, it did not hang, it always reset to firmware/grub boot menu.
>> I'm pretty sure now the bug exists in get_rsdp_addr().
>
>static acpi_physical_address kexec_get_rsdp_addr(void)
>{
>...
>        /* Get systab from boot params. */
>        systab = (efi_system_table_64_t *) (ei->efi_systab | ((__u64)ei->efi_systab_hi << 32));
>        if (!systab)
>              error("EFI system table not found in kexec boot_params.");
> 
>...
>  -> add error("hang me") here will have a hang
>...
>        return __efi_get_rsdp_addr((unsigned long)esd->tables,
>                                   systab->nr_tables, true);
> 

I have an idea, but not sure whether is a problem.
In code of Nomura:

#if defined(CONFIG_EFI) && defined(CONFIG_X86_64)
[...]
        if (strncmp(sig, EFI64_LOADER_SIGNATURE, 4)) {
                debug_putstr("Wrong kexec EFI loader signature.\n");
                return 0;
        }

        /* Get systab from boot params. */
        systab = (efi_system_table_64_t *) (ei->efi_systab | ((__u64)ei->efi_systab_hi << 32));
[...]
#endif

After review agian, I wonder what will happen if 32bit-efi boot 64bit
OS.

Ever meet a problem:
https://lkml.org/lkml/2019/2/8/845

It's a efi32 bootloader to boot a 64bit OS, then a problem happened.


Thanks,
Chao Fan

>But add error("hang me") in __efi_get_rsdp_addr it did not hang.
> 
>It seems reference the systab pointer cause a system reset.
> 
>A question is does the identity mapping covered the memory address of
>systab?
> 
>In my case it is 0xdad9ef18
> 
>If the memory is mapped on demand, then there will be problems, it
>should cover setup_data and efi table space.
> 
>Thanks
>Dave
>
>
>



_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: [PATCH v2] x86/boot: Use efi_setup_data for searching RSDP on kexec-ed kernel

[Dave Young]

Hi Chao,
On 04/04/19 at 03:20pm, Chao Fan wrote:
> On Thu, Apr 04, 2019 at 02:41:30PM +0800, Dave Young wrote:
> >On 04/04/19 at 11:22am, Dave Young wrote:
> >> On 04/04/19 at 11:10am, Baoquan He wrote:
> >> > On 04/04/19 at 11:00am, Baoquan He wrote:
> >> > > On 04/04/19 at 10:52am, Dave Young wrote:
> >> > > > On 04/04/19 at 01:23am, Junichi Nomura wrote:
> >> >  +	/* Save RSDP address for later use. */
> >> >  +	boot_params->acpi_rsdp_addr = get_rsdp_addr();
> >> >  +
> >> >  +	error("Hang kernel for kexec debugging");
> >> > 
> >> > Sorry, here I means calling error() to hang kernel after calling
> >> > get_rsdp_addr().
> >> 
> >> Thanks, it did not hang, it always reset to firmware/grub boot menu.
> >> I'm pretty sure now the bug exists in get_rsdp_addr().
> >
> >static acpi_physical_address kexec_get_rsdp_addr(void)
> >{
> >...
> >        /* Get systab from boot params. */
> >        systab = (efi_system_table_64_t *) (ei->efi_systab | ((__u64)ei->efi_systab_hi << 32));
> >        if (!systab)
> >              error("EFI system table not found in kexec boot_params.");
> > 
> >...
> >  -> add error("hang me") here will have a hang
> >...
> >        return __efi_get_rsdp_addr((unsigned long)esd->tables,
> >                                   systab->nr_tables, true);
> > 
> 
> I have an idea, but not sure whether is a problem.
> In code of Nomura:
> 
> #if defined(CONFIG_EFI) && defined(CONFIG_X86_64)
> [...]
>         if (strncmp(sig, EFI64_LOADER_SIGNATURE, 4)) {
>                 debug_putstr("Wrong kexec EFI loader signature.\n");
>                 return 0;
>         }
> 
>         /* Get systab from boot params. */
>         systab = (efi_system_table_64_t *) (ei->efi_systab | ((__u64)ei->efi_systab_hi << 32));
> [...]
> #endif
> 
> After review agian, I wonder what will happen if 32bit-efi boot 64bit
> OS.

It is hard to find 32bit efi hardware, I can confirm all the laptop I
have are 64bit efi.

In case 32bit efi EFI64_LOADER_SIGNATURE checking will be false.

_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: [PATCH v2] x86/boot: Use efi_setup_data for searching RSDP on kexec-ed kernel

[Chao Fan]

On Thu, Apr 04, 2019 at 03:41:02PM +0800, Dave Young wrote:
>Hi Chao,
>On 04/04/19 at 03:20pm, Chao Fan wrote:
>> On Thu, Apr 04, 2019 at 02:41:30PM +0800, Dave Young wrote:
>> >On 04/04/19 at 11:22am, Dave Young wrote:
>> >> On 04/04/19 at 11:10am, Baoquan He wrote:
>> >> > On 04/04/19 at 11:00am, Baoquan He wrote:
>> >> > > On 04/04/19 at 10:52am, Dave Young wrote:
>> >> > > > On 04/04/19 at 01:23am, Junichi Nomura wrote:
>> >> >  +	/* Save RSDP address for later use. */
>> >> >  +	boot_params->acpi_rsdp_addr = get_rsdp_addr();
>> >> >  +
>> >> >  +	error("Hang kernel for kexec debugging");
>> >> > 
>> >> > Sorry, here I means calling error() to hang kernel after calling
>> >> > get_rsdp_addr().
>> >> 
>> >> Thanks, it did not hang, it always reset to firmware/grub boot menu.
>> >> I'm pretty sure now the bug exists in get_rsdp_addr().
>> >
>> >static acpi_physical_address kexec_get_rsdp_addr(void)
>> >{
>> >...
>> >        /* Get systab from boot params. */
>> >        systab = (efi_system_table_64_t *) (ei->efi_systab | ((__u64)ei->efi_systab_hi << 32));
>> >        if (!systab)
>> >              error("EFI system table not found in kexec boot_params.");
>> > 
>> >...
>> >  -> add error("hang me") here will have a hang
>> >...
>> >        return __efi_get_rsdp_addr((unsigned long)esd->tables,
>> >                                   systab->nr_tables, true);
>> > 
>> 
>> I have an idea, but not sure whether is a problem.
>> In code of Nomura:
>> 
>> #if defined(CONFIG_EFI) && defined(CONFIG_X86_64)
>> [...]
>>         if (strncmp(sig, EFI64_LOADER_SIGNATURE, 4)) {
>>                 debug_putstr("Wrong kexec EFI loader signature.\n");
>>                 return 0;
>>         }
>> 
>>         /* Get systab from boot params. */
>>         systab = (efi_system_table_64_t *) (ei->efi_systab | ((__u64)ei->efi_systab_hi << 32));
>> [...]
>> #endif
>> 
>> After review agian, I wonder what will happen if 32bit-efi boot 64bit
>> OS.
>
>It is hard to find 32bit efi hardware, I can confirm all the laptop I
>have are 64bit efi.
>
>In case 32bit efi EFI64_LOADER_SIGNATURE checking will be false.

Yes, just when reviewing code, I notice this problem.

Thanks,
Chao Fan
>
>



_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: [PATCH v2] x86/boot: Use efi_setup_data for searching RSDP on kexec-ed kernel

[Borislav Petkov]

On Thu, Apr 04, 2019 at 03:41:02PM +0800, Dave Young wrote:
> It is hard to find 32bit efi hardware, I can confirm all the laptop I
> have are 64bit efi.

You can use 32-bit OVMF with qemu, like in the link Chao gave a couple
of mails earlier.

-- 
Regards/Gruss,
    Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.

_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: [PATCH v2] x86/boot: Use efi_setup_data for searching RSDP on kexec-ed kernel

[Dave Young]

On 04/04/19 at 02:22pm, Borislav Petkov wrote:
> On Thu, Apr 04, 2019 at 03:41:02PM +0800, Dave Young wrote:
> > It is hard to find 32bit efi hardware, I can confirm all the laptop I
> > have are 64bit efi.
> 
> You can use 32-bit OVMF with qemu, like in the link Chao gave a couple
> of mails earlier.

Since my case is not 32bit related, let me continue to get what happens
on 64bit laptops first.  32bit test can be done after that. 

Thanks
Dave

_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: [PATCH v2] x86/boot: Use efi_setup_data for searching RSDP on kexec-ed kernel

[Dave Young]

> >
> >Today, test again, kexec reboot hangs (with or without your patch), but
> >kdump works always (with or without the patch)
> 
> That's so strange, I'm puzzled by your test.
> Withour ant changes, just test again, the result of yesterday and today
> are different?
> 

Yes, I'm also puzzled, not 100% sure if I have some wrong test steps or
something, can only say for today's test results now :(

_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

[PATCH v3] x86/boot: Use efi_setup_data for searching RSDP on kexec-ed kernel

[Junichi Nomura]

Commit 3a63f70bf4c3a ("x86/boot: Early parse RSDP and save it in
boot_params") broke kexec boot on EFI systems.  efi_get_rsdp_addr()
in the early parsing code tries to search RSDP from EFI table but
that will crash because the table address is virtual when the kernel
was booted by kexec.

In the case of kexec, physical address of EFI tables is provided
via efi_setup_data in boot_params, which is set up by kexec(1).

Factor out the table parsing code and use different pointers depending
on whether the kernel is booted by kexec or not.

Fixes: 3a63f70bf4c3a ("x86/boot: Early parse RSDP and save it in boot_params")
Signed-off-by: Jun'ichi Nomura <j-nomura@ce.jp.nec.com>
Acked-by: Baoquan He <bhe@redhat.com>
Tested-by: Chao Fan <fanc.fnst@cn.fujitsu.com>
Cc: Borislav Petkov <bp@suse.de>
Cc: Dave Young <dyoung@redhat.com>

--
v2: Added comments above __efi_get_rsdp_addr() and kexec_get_rsdp_addr() 

v3: Properly ifdef out 64bit-only kexec code to avoid 32bit build warnings

diff --git a/arch/x86/boot/compressed/acpi.c b/arch/x86/boot/compressed/acpi.c
index 0ef4ad5..d9f9abd 100644
--- a/arch/x86/boot/compressed/acpi.c
+++ b/arch/x86/boot/compressed/acpi.c
@@ -44,17 +44,114 @@ static acpi_physical_address get_acpi_rsdp(void)
 	return addr;
 }
 
-/* Search EFI system tables for RSDP. */
-static acpi_physical_address efi_get_rsdp_addr(void)
+#if defined(CONFIG_EFI) && defined(CONFIG_X86_64)
+static unsigned long efi_get_kexec_setup_data_addr(void)
+{
+	struct setup_data *data;
+	u64 pa_data;
+
+	pa_data = boot_params->hdr.setup_data;
+	while (pa_data) {
+		data = (struct setup_data *) pa_data;
+		if (data->type == SETUP_EFI)
+			return pa_data + sizeof(struct setup_data);
+		pa_data = data->next;
+	}
+	return 0;
+}
+#endif
+
+#ifdef CONFIG_EFI
+/*
+ * Search EFI system tables for RSDP.  If both ACPI_20_TABLE_GUID and
+ * ACPI_TABLE_GUID are found, take the former, which has more features.
+ */
+static acpi_physical_address
+__efi_get_rsdp_addr(unsigned long config_tables, unsigned int nr_tables,
+		    bool efi_64)
 {
 	acpi_physical_address rsdp_addr = 0;
+	int i;
 
+	/* Get EFI tables from systab. */
+	for (i = 0; i < nr_tables; i++) {
+		acpi_physical_address table;
+		efi_guid_t guid;
+
+		if (efi_64) {
+			efi_config_table_64_t *tbl = (efi_config_table_64_t *) config_tables + i;
+
+			guid  = tbl->guid;
+			table = tbl->table;
+
+			if (!IS_ENABLED(CONFIG_X86_64) && table >> 32) {
+				debug_putstr("Error getting RSDP address: EFI config table located above 4GB.\n");
+				return 0;
+			}
+		} else {
+			efi_config_table_32_t *tbl = (efi_config_table_32_t *) config_tables + i;
+
+			guid  = tbl->guid;
+			table = tbl->table;
+		}
+
+		if (!(efi_guidcmp(guid, ACPI_TABLE_GUID)))
+			rsdp_addr = table;
+		else if (!(efi_guidcmp(guid, ACPI_20_TABLE_GUID)))
+			return table;
+	}
+
+	return rsdp_addr;
+}
+#endif
+
+/*
+ * EFI/kexec support is only added for 64bit. So we don't have to
+ * care 32bit case.
+ */
+static acpi_physical_address kexec_get_rsdp_addr(void)
+{
+#if defined(CONFIG_EFI) && defined(CONFIG_X86_64)
+	efi_system_table_64_t *systab;
+	struct efi_setup_data *esd;
+	struct efi_info *ei;
+	char *sig;
+
+	esd = (struct efi_setup_data *) efi_get_kexec_setup_data_addr();
+	if (!esd)
+		return 0;
+
+	if (!esd->tables) {
+		debug_putstr("Wrong kexec SETUP_EFI data.\n");
+		return 0;
+	}
+
+	ei = &boot_params->efi_info;
+	sig = (char *)&ei->efi_loader_signature;
+	if (strncmp(sig, EFI64_LOADER_SIGNATURE, 4)) {
+		debug_putstr("Wrong kexec EFI loader signature.\n");
+		return 0;
+	}
+
+	/* Get systab from boot params. */
+	systab = (efi_system_table_64_t *) (ei->efi_systab | ((__u64)ei->efi_systab_hi << 32));
+	if (!systab)
+		error("EFI system table not found in kexec boot_params.");
+
+	return __efi_get_rsdp_addr((unsigned long) esd->tables,
+				   systab->nr_tables, true);
+#else
+	return 0;
+#endif
+}
+
+static acpi_physical_address efi_get_rsdp_addr(void)
+{
 #ifdef CONFIG_EFI
-	unsigned long systab, systab_tables, config_tables;
+	unsigned long systab, config_tables;
 	unsigned int nr_tables;
 	struct efi_info *ei;
 	bool efi_64;
-	int size, i;
 	char *sig;
 
 	ei = &boot_params->efi_info;
@@ -88,49 +185,20 @@ static acpi_physical_address efi_get_rsdp_addr(void)
 
 		config_tables	= stbl->tables;
 		nr_tables	= stbl->nr_tables;
-		size		= sizeof(efi_config_table_64_t);
 	} else {
 		efi_system_table_32_t *stbl = (efi_system_table_32_t *)systab;
 
 		config_tables	= stbl->tables;
 		nr_tables	= stbl->nr_tables;
-		size		= sizeof(efi_config_table_32_t);
 	}
 
 	if (!config_tables)
 		error("EFI config tables not found.");
 
-	/* Get EFI tables from systab. */
-	for (i = 0; i < nr_tables; i++) {
-		acpi_physical_address table;
-		efi_guid_t guid;
-
-		config_tables += size;
-
-		if (efi_64) {
-			efi_config_table_64_t *tbl = (efi_config_table_64_t *)config_tables;
-
-			guid  = tbl->guid;
-			table = tbl->table;
-
-			if (!IS_ENABLED(CONFIG_X86_64) && table >> 32) {
-				debug_putstr("Error getting RSDP address: EFI config table located above 4GB.\n");
-				return 0;
-			}
-		} else {
-			efi_config_table_32_t *tbl = (efi_config_table_32_t *)config_tables;
-
-			guid  = tbl->guid;
-			table = tbl->table;
-		}
-
-		if (!(efi_guidcmp(guid, ACPI_TABLE_GUID)))
-			rsdp_addr = table;
-		else if (!(efi_guidcmp(guid, ACPI_20_TABLE_GUID)))
-			return table;
-	}
+	return __efi_get_rsdp_addr(config_tables, nr_tables, efi_64);
+#else
+	return 0;
 #endif
-	return rsdp_addr;
 }
 
 static u8 compute_checksum(u8 *buffer, u32 length)
@@ -221,6 +289,9 @@ acpi_physical_address get_rsdp_addr(void)
 		pa = boot_params->acpi_rsdp_addr;
 
 	if (!pa)
+		pa = kexec_get_rsdp_addr();
+
+	if (!pa)
 		pa = efi_get_rsdp_addr();
 
 	if (!pa)

_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: [PATCH v3] x86/boot: Use efi_setup_data for searching RSDP on kexec-ed kernel

[Dave Young]

Hi Junichi,

While we are still debuggin the bug I reproduced, but for this patch
I have still some comments inline.

BTW, it would be good to start a new thread when you send V4 :)

On 04/02/19 at 10:25am, Junichi Nomura wrote:
> Commit 3a63f70bf4c3a ("x86/boot: Early parse RSDP and save it in
> boot_params") broke kexec boot on EFI systems.  efi_get_rsdp_addr()
> in the early parsing code tries to search RSDP from EFI table but
> that will crash because the table address is virtual when the kernel
> was booted by kexec.
> 
> In the case of kexec, physical address of EFI tables is provided
> via efi_setup_data in boot_params, which is set up by kexec(1).
> 
> Factor out the table parsing code and use different pointers depending
> on whether the kernel is booted by kexec or not.
> 
> Fixes: 3a63f70bf4c3a ("x86/boot: Early parse RSDP and save it in boot_params")
> Signed-off-by: Jun'ichi Nomura <j-nomura@ce.jp.nec.com>
> Acked-by: Baoquan He <bhe@redhat.com>
> Tested-by: Chao Fan <fanc.fnst@cn.fujitsu.com>
> Cc: Borislav Petkov <bp@suse.de>
> Cc: Dave Young <dyoung@redhat.com>
> 
> --
> v2: Added comments above __efi_get_rsdp_addr() and kexec_get_rsdp_addr() 
> 
> v3: Properly ifdef out 64bit-only kexec code to avoid 32bit build warnings
> 
> diff --git a/arch/x86/boot/compressed/acpi.c b/arch/x86/boot/compressed/acpi.c
> index 0ef4ad5..d9f9abd 100644
> --- a/arch/x86/boot/compressed/acpi.c
> +++ b/arch/x86/boot/compressed/acpi.c
> @@ -44,17 +44,114 @@ static acpi_physical_address get_acpi_rsdp(void)
>  	return addr;
>  }
>  
> -/* Search EFI system tables for RSDP. */
> -static acpi_physical_address efi_get_rsdp_addr(void)
> +#if defined(CONFIG_EFI) && defined(CONFIG_X86_64)
> +static unsigned long efi_get_kexec_setup_data_addr(void)
> +{
> +	struct setup_data *data;
> +	u64 pa_data;
> +
> +	pa_data = boot_params->hdr.setup_data;
> +	while (pa_data) {
> +		data = (struct setup_data *) pa_data;
> +		if (data->type == SETUP_EFI)
> +			return pa_data + sizeof(struct setup_data);
> +		pa_data = data->next;
> +	}
> +	return 0;
> +}
> +#endif
> +
> +#ifdef CONFIG_EFI
> +/*
> + * Search EFI system tables for RSDP.  If both ACPI_20_TABLE_GUID and
> + * ACPI_TABLE_GUID are found, take the former, which has more features.
> + */
> +static acpi_physical_address
> +__efi_get_rsdp_addr(unsigned long config_tables, unsigned int nr_tables,
> +		    bool efi_64)
>  {
>  	acpi_physical_address rsdp_addr = 0;
> +	int i;
>  
> +	/* Get EFI tables from systab. */
> +	for (i = 0; i < nr_tables; i++) {
> +		acpi_physical_address table;
> +		efi_guid_t guid;
> +
> +		if (efi_64) {
> +			efi_config_table_64_t *tbl = (efi_config_table_64_t *) config_tables + i;
> +
> +			guid  = tbl->guid;
> +			table = tbl->table;
> +
> +			if (!IS_ENABLED(CONFIG_X86_64) && table >> 32) {
> +				debug_putstr("Error getting RSDP address: EFI config table located above 4GB.\n");
> +				return 0;
> +			}
> +		} else {
> +			efi_config_table_32_t *tbl = (efi_config_table_32_t *) config_tables + i;
> +
> +			guid  = tbl->guid;
> +			table = tbl->table;
> +		}
> +
> +		if (!(efi_guidcmp(guid, ACPI_TABLE_GUID)))
> +			rsdp_addr = table;
> +		else if (!(efi_guidcmp(guid, ACPI_20_TABLE_GUID)))
> +			return table;
> +	}
> +
> +	return rsdp_addr;
> +}
> +#endif
> +
> +/*
> + * EFI/kexec support is only added for 64bit. So we don't have to
> + * care 32bit case.
> + */
> +static acpi_physical_address kexec_get_rsdp_addr(void)
> +{
> +#if defined(CONFIG_EFI) && defined(CONFIG_X86_64)
> +	efi_system_table_64_t *systab;
> +	struct efi_setup_data *esd;
> +	struct efi_info *ei;
> +	char *sig;
> +
> +	esd = (struct efi_setup_data *) efi_get_kexec_setup_data_addr();
> +	if (!esd)
> +		return 0;
> +
> +	if (!esd->tables) {
> +		debug_putstr("Wrong kexec SETUP_EFI data.\n");
> +		return 0;
> +	}
> +
> +	ei = &boot_params->efi_info;
> +	sig = (char *)&ei->efi_loader_signature;
> +	if (strncmp(sig, EFI64_LOADER_SIGNATURE, 4)) {
> +		debug_putstr("Wrong kexec EFI loader signature.\n");
> +		return 0;
> +	}
> +
> +	/* Get systab from boot params. */
> +	systab = (efi_system_table_64_t *) (ei->efi_systab | ((__u64)ei->efi_systab_hi << 32));
> +	if (!systab)
> +		error("EFI system table not found in kexec boot_params.");

Instead of hang the system, return 0 should be better

> +
> +	return __efi_get_rsdp_addr((unsigned long) esd->tables,
> +				   systab->nr_tables, true);
> +#else
> +	return 0;
> +#endif
> +}
> +
> +static acpi_physical_address efi_get_rsdp_addr(void)
> +{
>  #ifdef CONFIG_EFI
> -	unsigned long systab, systab_tables, config_tables;
> +	unsigned long systab, config_tables;
>  	unsigned int nr_tables;
>  	struct efi_info *ei;
>  	bool efi_64;
> -	int size, i;
>  	char *sig;
>  
>  	ei = &boot_params->efi_info;
> @@ -88,49 +185,20 @@ static acpi_physical_address efi_get_rsdp_addr(void)
>  
>  		config_tables	= stbl->tables;
>  		nr_tables	= stbl->nr_tables;
> -		size		= sizeof(efi_config_table_64_t);
>  	} else {
>  		efi_system_table_32_t *stbl = (efi_system_table_32_t *)systab;
>  
>  		config_tables	= stbl->tables;
>  		nr_tables	= stbl->nr_tables;
> -		size		= sizeof(efi_config_table_32_t);
>  	}
>  
>  	if (!config_tables)
>  		error("EFI config tables not found.");
>  
> -	/* Get EFI tables from systab. */
> -	for (i = 0; i < nr_tables; i++) {
> -		acpi_physical_address table;
> -		efi_guid_t guid;
> -
> -		config_tables += size;
> -
> -		if (efi_64) {
> -			efi_config_table_64_t *tbl = (efi_config_table_64_t *)config_tables;
> -
> -			guid  = tbl->guid;
> -			table = tbl->table;
> -
> -			if (!IS_ENABLED(CONFIG_X86_64) && table >> 32) {
> -				debug_putstr("Error getting RSDP address: EFI config table located above 4GB.\n");
> -				return 0;
> -			}
> -		} else {
> -			efi_config_table_32_t *tbl = (efi_config_table_32_t *)config_tables;
> -
> -			guid  = tbl->guid;
> -			table = tbl->table;
> -		}
> -
> -		if (!(efi_guidcmp(guid, ACPI_TABLE_GUID)))
> -			rsdp_addr = table;
> -		else if (!(efi_guidcmp(guid, ACPI_20_TABLE_GUID)))
> -			return table;
> -	}
> +	return __efi_get_rsdp_addr(config_tables, nr_tables, efi_64);
> +#else
> +	return 0;
>  #endif
> -	return rsdp_addr;
>  }
>  
>  static u8 compute_checksum(u8 *buffer, u32 length)
> @@ -221,6 +289,9 @@ acpi_physical_address get_rsdp_addr(void)
>  		pa = boot_params->acpi_rsdp_addr;
>  
>  	if (!pa)
> +		pa = kexec_get_rsdp_addr();
> +
> +	if (!pa)
>  		pa = efi_get_rsdp_addr();

In case kexec_get_rsdp_addr failed, above code will try efi_get.. again,
but that will also fail for same issue this patch is fixing.  In case
kexec we should avoid the efi_get_rsdp_addr.

>  
>  	if (!pa)
> 
> _______________________________________________
> kexec mailing list
> kexec@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/kexec

Thanks
Dave

_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: [PATCH v3] x86/boot: Use efi_setup_data for searching RSDP on kexec-ed kernel

[Borislav Petkov]

On Thu, Apr 04, 2019 at 03:32:33PM +0800, Dave Young wrote:
> BTW, it would be good to start a new thread when you send V4 :)

Yes please.

> > +	/* Get systab from boot params. */
> > +	systab = (efi_system_table_64_t *) (ei->efi_systab | ((__u64)ei->efi_systab_hi << 32));
> > +	if (!systab)
> > +		error("EFI system table not found in kexec boot_params.");
> 
> Instead of hang the system, return 0 should be better

Can the kexec kernel even function if we fail here?

-- 
Regards/Gruss,
    Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.

_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: [PATCH v3] x86/boot: Use efi_setup_data for searching RSDP on kexec-ed kernel

[Dave Young]

On 04/04/19 at 02:24pm, Borislav Petkov wrote:
> On Thu, Apr 04, 2019 at 03:32:33PM +0800, Dave Young wrote:
> > BTW, it would be good to start a new thread when you send V4 :)
> 
> Yes please.
> 
> > > +	/* Get systab from boot params. */
> > > +	systab = (efi_system_table_64_t *) (ei->efi_systab | ((__u64)ei->efi_systab_hi << 32));
> > > +	if (!systab)
> > > +		error("EFI system table not found in kexec boot_params.");
> > 
> > Instead of hang the system, return 0 should be better
> 
> Can the kexec kernel even function if we fail here?

It only bootable when people provide acpi_rsdp via cmdline or
boot_params.  According to current code logic, cmdline/boot_param is
handled first so kexec boot will not work in this case.

The early code hang can make people confused, it is hard to
say what happens and not easy to debug.   But if we return 0 then kernel
just continue to boot, and fail later because of no acpi root pointer, at
least we can have some kernel boot log.

Thanks
Dave

_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: [PATCH v3] x86/boot: Use efi_setup_data for searching RSDP on kexec-ed kernel

[Borislav Petkov]

On Thu, Apr 04, 2019 at 10:12:41PM +0800, Dave Young wrote:
> The early code hang can make people confused, it is hard to
> say what happens and not easy to debug.   But if we return 0 then kernel
> just continue to boot, and fail later because of no acpi root pointer, at
> least we can have some kernel boot log.

Is it clear from that boot log where we failed?

-- 
Regards/Gruss,
    Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.

_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: [PATCH v3] x86/boot: Use efi_setup_data for searching RSDP on kexec-ed kernel

[Dave Young]

On 04/04/19 at 04:41pm, Borislav Petkov wrote:
> On Thu, Apr 04, 2019 at 10:12:41PM +0800, Dave Young wrote:
> > The early code hang can make people confused, it is hard to
> > say what happens and not easy to debug.   But if we return 0 then kernel
> > just continue to boot, and fail later because of no acpi root pointer, at
> > least we can have some kernel boot log.
> 
> Is it clear from that boot log where we failed?

The early boot log in compress/*.c is not visible for kexec EFI boot so
the log is useless unless serial is usable.

the early putstr only works for legacy boot with some ioport outb()
callbacks unless serial is available.  For EFI boot it does not work,
and for kexec, it is even worse because 1st kernel booted with kms mode
setting, it is not like the firmware initialized video mode any more
when we run kexec reboot.

_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: [PATCH v3] x86/boot: Use efi_setup_data for searching RSDP on kexec-ed kernel

[Junichi Nomura]

On 4/4/19 4:32 PM, Dave Young wrote:
> While we are still debuggin the bug I reproduced, but for this patch
> I have still some comments inline.
> 
> BTW, it would be good to start a new thread when you send V4 :)

Sure.

>> +	/* Get systab from boot params. */
>> +	systab = (efi_system_table_64_t *) (ei->efi_systab | ((__u64)ei->efi_systab_hi << 32));
>> +	if (!systab)
>> +		error("EFI system table not found in kexec boot_params.");
> 
> Instead of hang the system, return 0 should be better

OK. I'll make it something like:
	if (!systab) {
		debug_putstr(...);
		return 0;
	}

>>  	if (!pa)
>> +		pa = kexec_get_rsdp_addr();
>> +
>> +	if (!pa)
>>  		pa = efi_get_rsdp_addr();
> 
> In case kexec_get_rsdp_addr failed, above code will try efi_get.. again,
> but that will also fail for same issue this patch is fixing.  In case
> kexec we should avoid the efi_get_rsdp_addr.

Yeah, I'll set efi_kexec_booted to true in kexec_get_rsdp_addr() if it's
kexec booted so that efi_get_rsdp_addr() can exit early in such a case.

-- 
Jun'ichi Nomura, NEC Corporation / NEC Solution Innovators, Ltd.

_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: [PATCH v2] x86/boot: Use EFI setup data if provided

[bhe]

On 03/28/19 at 07:43am, Junichi Nomura wrote:
> On 2019/03/28 15:43, bhe@redhat.com wrote:
> > On 03/28/19 at 04:17am, Junichi Nomura wrote:
> >> I still don't get it... We still need systab for kexec case as well
> >> to get nr_tables. Don't we?
> > 
> > Yes, simpler.
> > 
> > As Dave replied in another mail, efi/kexec is only added for x86_64. See
> > how it does in setup_linux_system_parameters() of kexec_tools utility,
> > and we only have bzImage64 handling in kernel for kexec_file loading,
> > see prepare_add_efi_setup_data().
> > 
> > You may only need to get kexec ei_info to use directly.
> 
> OK, let me try. How does this look?

Yes, it looks great. Thanks.

Some tiny concenrns added in inline comments, please check.

> 
> Commit 3a63f70bf4c3a ("x86/boot: Early parse RSDP and save it in
> boot_params") broke kexec boot on EFI systems.  efi_get_rsdp_addr()
> in the early parsing code tries to search RSDP from EFI table but
> that will crash because the table address is virtual when the kernel
> was booted by kexec.
> 
> Since kexec(1) provides physical address of the table via efi_setup_data,
> early boot code of kexec-ed kernel should look for setup_data in the same
> way as efi_systab_init() in arch/x86/platform/efi/efi.c does.
> 
> Fixes: 3a63f70bf4c3a ("x86/boot: Early parse RSDP and save it in boot_params")
> Signed-off-by: Jun'ichi Nomura <j-nomura@ce.jp.nec.com>
> Cc: Chao Fan <fanc.fnst@cn.fujitsu.com>
> Cc: Borislav Petkov <bp@suse.de>
> Cc: Dave Young <dyoung@redhat.com>
> Cc: Baoquan He <bhe@redhat.com>
> 
> diff --git a/arch/x86/boot/compressed/acpi.c b/arch/x86/boot/compressed/acpi.c
> --- a/arch/x86/boot/compressed/acpi.c
> +++ b/arch/x86/boot/compressed/acpi.c
> @@ -44,17 +44,106 @@ static acpi_physical_address get_acpi_rsdp(void)
>  	return addr;
>  }
>  
> +static unsigned long efi_get_kexec_setup_data_addr(void)
> +{
> +#ifdef CONFIG_EFI
> +	struct setup_data *data;
> +	u64 pa_data;
> +
> +	pa_data = boot_params->hdr.setup_data;
> +	while (pa_data) {
> +		data = (struct setup_data *) pa_data;
> +		if (data->type == SETUP_EFI)
> +			return pa_data + sizeof(struct setup_data);
> +		pa_data = data->next;
> +	}
> +#endif
> +	return 0;
> +}
> +
>  /* Search EFI system tables for RSDP. */
> -static acpi_physical_address efi_get_rsdp_addr(void)
> +static acpi_physical_address __efi_get_rsdp_addr(unsigned long config_tables, unsigned int nr_tables, int size, bool efi_64)

Here parameter 'size' and 'efi_64' seems a little duplicated on
functionality. Only passing efi_64 can deduce the size? Personal
opinion.

>  {
>  	acpi_physical_address rsdp_addr = 0;
>  
>  #ifdef CONFIG_EFI
> -	unsigned long systab, systab_tables, config_tables;
> +	int i;
> +
> +	/* Get EFI tables from systab. */
> +	for (i = 0; i < nr_tables; i++) {
> +		acpi_physical_address table;
> +		efi_guid_t guid;
> +
> +		config_tables += size;
> +
> +		if (efi_64) {
> +			efi_config_table_64_t *tbl = (efi_config_table_64_t *)config_tables;
> +
> +			guid  = tbl->guid;
> +			table = tbl->table;
> +
> +			if (!IS_ENABLED(CONFIG_X86_64) && table >> 32) {
> +				debug_putstr("Error getting RSDP address: EFI config table located above 4GB.\n");
> +				return 0;
> +			}
> +		} else {
> +			efi_config_table_32_t *tbl = (efi_config_table_32_t *)config_tables;
> +
> +			guid  = tbl->guid;
> +			table = tbl->table;
> +		}
> +
> +		if (!(efi_guidcmp(guid, ACPI_TABLE_GUID)))
> +			rsdp_addr = table;
> +		else if (!(efi_guidcmp(guid, ACPI_20_TABLE_GUID)))
> +			return table;
> +	}
> +#endif
> +	return rsdp_addr;
> +}
> +

It might be worth adding code comments here to tell why we only care
about 64bit kexec booting?

> +static acpi_physical_address kexec_get_rsdp_addr(void)
> +{
> +#ifdef CONFIG_EFI
> +	struct efi_setup_data *esd;
> +	efi_system_table_64_t *systab;
> +	struct efi_info *ei;
> +	char *sig;
> +
> +	esd = (struct efi_setup_data *) efi_get_kexec_setup_data_addr();
> +	if (!esd)
> +		return 0;
> +	if (!esd->tables) {
> +		debug_putstr("Wrong kexec SETUP_EFI data.\n");
> +		return 0;
> +	}
> +
> +	ei = &boot_params->efi_info;
> +	sig = (char *)&ei->efi_loader_signature;
> +	if (strncmp(sig, EFI64_LOADER_SIGNATURE, 4)) {
> +		debug_putstr("Wrong EFI loader signature.\n");
> +		return 0;
> +	}
> +
> +	/* Get systab from boot params. */
> +	systab = (efi_system_table_64_t *) (ei->efi_systab | ((__u64)ei->efi_systab_hi << 32));
> +	if (!systab)
> +		error("EFI system table not found.");
> +
> +	return __efi_get_rsdp_addr((unsigned long) esd->tables, systab->nr_tables, sizeof(efi_config_table_64_t), true);
> +#else
> +	return 0;
> +#endif
> +}
> +
> +static acpi_physical_address efi_get_rsdp_addr(void)
> +{
> +#ifdef CONFIG_EFI
> +	unsigned long systab, config_tables;
>  	unsigned int nr_tables;
>  	struct efi_info *ei;
>  	bool efi_64;
> -	int size, i;
> +	int size;
>  	char *sig;
>  
>  	ei = &boot_params->efi_info;
> @@ -100,37 +189,10 @@ static acpi_physical_address efi_get_rsdp_addr(void)
>  	if (!config_tables)
>  		error("EFI config tables not found.");
>  
> -	/* Get EFI tables from systab. */
> -	for (i = 0; i < nr_tables; i++) {
> -		acpi_physical_address table;
> -		efi_guid_t guid;
> -
> -		config_tables += size;
> -
> -		if (efi_64) {
> -			efi_config_table_64_t *tbl = (efi_config_table_64_t *)config_tables;
> -
> -			guid  = tbl->guid;
> -			table = tbl->table;
> -
> -			if (!IS_ENABLED(CONFIG_X86_64) && table >> 32) {
> -				debug_putstr("Error getting RSDP address: EFI config table located above 4GB.\n");
> -				return 0;
> -			}
> -		} else {
> -			efi_config_table_32_t *tbl = (efi_config_table_32_t *)config_tables;
> -
> -			guid  = tbl->guid;
> -			table = tbl->table;
> -		}
> -
> -		if (!(efi_guidcmp(guid, ACPI_TABLE_GUID)))
> -			rsdp_addr = table;
> -		else if (!(efi_guidcmp(guid, ACPI_20_TABLE_GUID)))
> -			return table;
> -	}
> +	return __efi_get_rsdp_addr(config_tables, nr_tables, size, efi_64);
> +#else
> +	return 0;
>  #endif
> -	return rsdp_addr;
>  }
>  
>  static u8 compute_checksum(u8 *buffer, u32 length)
> @@ -221,6 +283,9 @@ acpi_physical_address get_rsdp_addr(void)
>  		pa = boot_params->acpi_rsdp_addr;
>  
>  	if (!pa)
> +		pa = kexec_get_rsdp_addr();
> +
> +	if (!pa)
>  		pa = efi_get_rsdp_addr();
>  
>  	if (!pa)

_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: [PATCH v2] x86/boot: Use EFI setup data if provided

[Junichi Nomura]

On 3/29/19 8:11 AM, bhe@redhat.com wrote:
> On 03/28/19 at 07:43am, Junichi Nomura wrote:
>>  /* Search EFI system tables for RSDP. */
>> -static acpi_physical_address efi_get_rsdp_addr(void)
>> +static acpi_physical_address __efi_get_rsdp_addr(unsigned long config_tables, unsigned int nr_tables, int size, bool efi_64)
> 
> Here parameter 'size' and 'efi_64' seems a little duplicated on
> functionality. Only passing efi_64 can deduce the size? Personal
> opinion.

Yes. But I'm not sure which is preferred.

The current code is:

__efi_get_rsdp_addr() {
        for (i = 0; i < nr_tables; i++) {
                config_tables += size;
                if (efi_64) {
                        efi_config_table_64_t *tbl = (efi_config_table_64_t *)config_tables;
                        // get guid and table
                } else {
                        efi_config_table_32_t *tbl = (efi_config_table_32_t *)config_tables;
                        // get guid and table
                }
                // check guid and return table if it's valid
        }
}

If we remove "size" parameter, that will become:

__efi_get_rsdp_addr() {
        for (i = 0; i < nr_tables; i++) {
                if (efi_64) {
                        efi_config_table_64_t *tbl;
                        config_tables += sizeof(efi_config_table_64_t);
                        tbl = (efi_config_table_64_t *)config_tables;
                        // get guid and table
                } else {
                        efi_config_table_32_t *tbl;
                        config_tables += sizeof(efi_config_table_32_t);
                        tbl = (efi_config_table_32_t *)config_tables;
                        // get guid and table
                }
                // check guid and return table if it's valid
        }
}

Or we could create 2 functions, __efi_get_rsdp_addr32() and __efi_get_rsdp_addr64(),
and let efi_get_rsdp_addr() to choose which one to use based on signature.

> It might be worth adding code comments here to tell why we only care
> about 64bit kexec booting?

I think so. I'll add a comment.

-- 
Jun'ichi Nomura, NEC Corporation / NEC Solution Innovators, Ltd.

_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: [PATCH v2] x86/boot: Use EFI setup data if provided

[bhe]

On 03/29/19 at 03:34am, Junichi Nomura wrote:
> On 3/29/19 8:11 AM, bhe@redhat.com wrote:
> > On 03/28/19 at 07:43am, Junichi Nomura wrote:
> >>  /* Search EFI system tables for RSDP. */
> >> -static acpi_physical_address efi_get_rsdp_addr(void)
> >> +static acpi_physical_address __efi_get_rsdp_addr(unsigned long config_tables, unsigned int nr_tables, int size, bool efi_64)
> > 
> > Here parameter 'size' and 'efi_64' seems a little duplicated on
> > functionality. Only passing efi_64 can deduce the size? Personal
> > opinion.
> 
> Yes. But I'm not sure which is preferred.
> 
> The current code is:
> 

I would like to change them like below two cases. I personally prefer
the 2nd one. Feel free to take one you like better.

1)
 __efi_get_rsdp_addr() {

	size = efi_64 ? sizeof(efi_config_table_64_t):sizeof(efi_config_table_32_t);;

Or if/else sytle.

         for (i = 0; i < nr_tables; i++) {
                 config_tables += size;
                 if (efi_64) {
                         efi_config_table_64_t *tbl = (efi_config_table_64_t *)config_tables;
                         // get guid and table
                 } else {
                         efi_config_table_32_t *tbl = (efi_config_table_32_t *)config_tables;
                         // get guid and table
                 }
                 // check guid and return table if it's valid
         }
 }
 
2)
 __efi_get_rsdp_addr() {
         for (i = 0; i < nr_tables; i++) {
                 if (efi_64) {
			efi_config_table_64_t *tbl = (efi_config_table_64_t *)config_tables + i;
                         // get guid and table
                 } else {
			efi_config_table_32_t *tbl = (efi_config_table_32_t *)config_tables + i;
                         // get guid and table
                 }
                 // check guid and return table if it's valid
         }
 }
> 
> Or we could create 2 functions, __efi_get_rsdp_addr32() and __efi_get_rsdp_addr64(),
> and let efi_get_rsdp_addr() to choose which one to use based on signature.
> 
> > It might be worth adding code comments here to tell why we only care
> > about 64bit kexec booting?
> 
> I think so. I'll add a comment.
> 
> -- 
> Jun'ichi Nomura, NEC Corporation / NEC Solution Innovators, Ltd.

_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: [PATCH v2] x86/boot: Use EFI setup data if provided

[Junichi Nomura]

On 3/29/19 12:52 PM, bhe@redhat.com wrote:
> 2)
>  __efi_get_rsdp_addr() {
>          for (i = 0; i < nr_tables; i++) {
>                  if (efi_64) {
> 			efi_config_table_64_t *tbl = (efi_config_table_64_t *)config_tables + i;
>                          // get guid and table
>                  } else {
> 			efi_config_table_32_t *tbl = (efi_config_table_32_t *)config_tables + i;
>                          // get guid and table
>                  }
>                  // check guid and return table if it's valid
>          }
>  }

Thanks. This looks nice and doesn't conflict the direction of refactoring, I think.

-- 
Jun'ichi Nomura, NEC Corporation / NEC Solution Innovators, Ltd.

_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec