This patch series fix the kexec panic on efi=noruntime or efi=old_map
and leverage acpi_rsdp_addr to make the second kernel boot up properly.
Kairui Song (2):
x86, kexec_file_load: Don't setup EFI info if EFI runtime is not
enabled
x86, kexec_file_load: make it work with efi=noruntime or efi=old_map
Update from V1:
- Add a cover letter and fix some type in commit message
- Previous patches are not sent in a single thread
arch/x86/kernel/kexec-bzimage64.c | 24 ++++++++++++++++++++++++
drivers/acpi/acpica/tbxfroot.c | 3 +--
include/acpi/acpixf.h | 2 +-
3 files changed, 26 insertions(+), 3 deletions(-)
--
2.20.1
Currently with "efi=noruntime" in kernel command line, calling
kexec_file_load will raise below problem:
[ 97.967067] BUG: unable to handle kernel NULL pointer dereference at 0000000000000000
[ 97.967894] #PF error: [normal kernel read fault]
...
[ 97.980456] Call Trace:
[ 97.980724] efi_runtime_map_copy+0x28/0x30
[ 97.981267] bzImage64_load+0x688/0x872
[ 97.981794] arch_kexec_kernel_image_load+0x6d/0x70
[ 97.982441] kimage_file_alloc_init+0x13e/0x220
[ 97.983035] __x64_sys_kexec_file_load+0x144/0x290
[ 97.983586] do_syscall_64+0x55/0x1a0
[ 97.983962] entry_SYSCALL_64_after_hwframe+0x44/0xa9
When efi runtime is not enabled, efi memmap is not mapped, so just skip
EFI info setup.
Suggested-by: Dave Young <[email protected]>
Signed-off-by: Kairui Song <[email protected]>
---
arch/x86/kernel/kexec-bzimage64.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/arch/x86/kernel/kexec-bzimage64.c b/arch/x86/kernel/kexec-bzimage64.c
index 0d5efa34f359..53917a3ebf94 100644
--- a/arch/x86/kernel/kexec-bzimage64.c
+++ b/arch/x86/kernel/kexec-bzimage64.c
@@ -167,6 +167,9 @@ setup_efi_state(struct boot_params *params, unsigned long params_load_addr,
struct efi_info *current_ei = &boot_params.efi_info;
struct efi_info *ei = ¶ms->efi_info;
+ if (!efi_enabled(EFI_RUNTIME_SERVICES))
+ return 0;
+
if (!current_ei->efi_memmap_size)
return 0;
--
2.20.1
When efi=noruntime or efi=oldmap is used, EFI services won't be available
in the second kernel, therefore the second kernel will not be able to get
the ACPI RSDP address from firmware by calling EFI services and won't
boot. Previously we are expecting the user to set the acpi_rsdp=<addr>
on kernel command line for second kernel as there was no way to pass RSDP
address to second kernel.
After commit e6e094e053af ('x86/acpi, x86/boot: Take RSDP address from
boot params if available'), now it's possible to set an acpi_rsdp_addr
parameter in the boot_params passed to second kernel, this commit make
use of it, detect and set the RSDP address when it's required for second
kernel to boot.
Tested with an EFI enabled KVM VM with efi=noruntime.
Suggested-by: Dave Young <[email protected]>
Signed-off-by: Kairui Song <[email protected]>
---
arch/x86/kernel/kexec-bzimage64.c | 21 +++++++++++++++++++++
drivers/acpi/acpica/tbxfroot.c | 3 +--
include/acpi/acpixf.h | 2 +-
3 files changed, 23 insertions(+), 3 deletions(-)
diff --git a/arch/x86/kernel/kexec-bzimage64.c b/arch/x86/kernel/kexec-bzimage64.c
index 53917a3ebf94..0a90dcbd041f 100644
--- a/arch/x86/kernel/kexec-bzimage64.c
+++ b/arch/x86/kernel/kexec-bzimage64.c
@@ -20,6 +20,7 @@
#include <linux/mm.h>
#include <linux/efi.h>
#include <linux/verification.h>
+#include <linux/acpi.h>
#include <asm/bootparam.h>
#include <asm/setup.h>
@@ -255,8 +256,28 @@ setup_boot_parameters(struct kimage *image, struct boot_params *params,
/* Setup EFI state */
setup_efi_state(params, params_load_addr, efi_map_offset, efi_map_sz,
efi_setup_data_offset);
+
+#ifdef CONFIG_ACPI
+ /* Setup ACPI RSDP pointer in case EFI is not available in second kernel */
+ if (!acpi_disabled && (!efi_enabled(EFI_RUNTIME_SERVICES) || efi_enabled(EFI_OLD_MEMMAP))) {
+ /* Copied from acpi_os_get_root_pointer accordingly */
+ params->acpi_rsdp_addr = boot_params.acpi_rsdp_addr;
+ if (!params->acpi_rsdp_addr) {
+ if (efi_enabled(EFI_CONFIG_TABLES)) {
+ if (efi.acpi20 != EFI_INVALID_TABLE_ADDR)
+ params->acpi_rsdp_addr = efi.acpi20;
+ else if (efi.acpi != EFI_INVALID_TABLE_ADDR)
+ params->acpi_rsdp_addr = efi.acpi;
+ } else if (IS_ENABLED(CONFIG_ACPI_LEGACY_TABLES_LOOKUP)) {
+ acpi_find_root_pointer(¶ms->acpi_rsdp_addr);
+ }
+ }
+ if (!params->acpi_rsdp_addr)
+ pr_warn("RSDP is not available for second kernel\n");
+ }
#endif
+#endif
/* Setup EDD info */
memcpy(params->eddbuf, boot_params.eddbuf,
EDDMAXNR * sizeof(struct edd_info));
diff --git a/drivers/acpi/acpica/tbxfroot.c b/drivers/acpi/acpica/tbxfroot.c
index 483d0ce5180a..dac1e34a931c 100644
--- a/drivers/acpi/acpica/tbxfroot.c
+++ b/drivers/acpi/acpica/tbxfroot.c
@@ -108,8 +108,7 @@ acpi_status acpi_tb_validate_rsdp(struct acpi_table_rsdp *rsdp)
*
******************************************************************************/
-acpi_status ACPI_INIT_FUNCTION
-acpi_find_root_pointer(acpi_physical_address *table_address)
+acpi_status acpi_find_root_pointer(acpi_physical_address *table_address)
{
u8 *table_ptr;
u8 *mem_rover;
diff --git a/include/acpi/acpixf.h b/include/acpi/acpixf.h
index 7aa38b648564..869d75ecaf7d 100644
--- a/include/acpi/acpixf.h
+++ b/include/acpi/acpixf.h
@@ -474,7 +474,7 @@ ACPI_EXTERNAL_RETURN_STATUS(acpi_status ACPI_INIT_FUNCTION
ACPI_EXTERNAL_RETURN_STATUS(acpi_status ACPI_INIT_FUNCTION
acpi_reallocate_root_table(void))
-ACPI_EXTERNAL_RETURN_STATUS(acpi_status ACPI_INIT_FUNCTION
+ACPI_EXTERNAL_RETURN_STATUS(acpi_status
acpi_find_root_pointer(acpi_physical_address
*rsdp_address))
ACPI_EXTERNAL_RETURN_STATUS(acpi_status
--
2.20.1
On Wed, Jan 16, 2019 at 7:10 AM Borislav Petkov <[email protected]> wrote:
> > +#ifdef CONFIG_ACPI
> > + /* Setup ACPI RSDP pointer in case EFI is not available in second kernel */
> > + if (!acpi_disabled && (!efi_enabled(EFI_RUNTIME_SERVICES) || efi_enabled(EFI_OLD_MEMMAP))) {
> > + /* Copied from acpi_os_get_root_pointer accordingly */
> > + params->acpi_rsdp_addr = boot_params.acpi_rsdp_addr;
> > + if (!params->acpi_rsdp_addr) {
> > + if (efi_enabled(EFI_CONFIG_TABLES)) {
> > + if (efi.acpi20 != EFI_INVALID_TABLE_ADDR)
> > + params->acpi_rsdp_addr = efi.acpi20;
> > + else if (efi.acpi != EFI_INVALID_TABLE_ADDR)
> > + params->acpi_rsdp_addr = efi.acpi;
> > + } else if (IS_ENABLED(CONFIG_ACPI_LEGACY_TABLES_LOOKUP)) {
> > + acpi_find_root_pointer(¶ms->acpi_rsdp_addr);
> > + }
> > + }
> > + if (!params->acpi_rsdp_addr)
> > + pr_warn("RSDP is not available for second kernel\n");
> > + }
> > #endif
>
> Amazing the amount of ACPI RDSP parsing and fiddling patches flying
> around these days...
>
> In any case, this needs to be synchronized with:
>
> https://lkml.kernel.org/r/[email protected]
>
> and checked whether the above can be used instead of sprinkling of ACPI
> parsing code left and right.
>
> Thx.
Hi thanks for the suggestion.
I didn't see a way to reuse things in that patch series, situation is
different, in that patch it needs to get RSDP in very early boot stage
so it did everything from scratch, in this patch kexec_file_load need
to get RSDP too, but everything is well setup so things are a lot
easier, just read from current boot_prams, efi and fallback to
acpi_find_root_pointer should be good.
And that patch series also need to consider boot_params.acpi_rsdp_addr
value, or it won't work if the system is rebooted with kexec, efi is
disabled and acpi_rsdp is provided by boot_params (Xen PVH guests also
need to get acpi_rsdp from boot_params according to commit message of
ae7e1238e68f2a472a125673ab506d49158c1889). Will add some comment and
discuss.
>
> --
> Regards/Gruss,
> Boris.
>
> Good mailing practices for 400: avoid top-posting and trim the reply.
--
Best Regards,
Kairui Song
On Tue, Jan 15, 2019 at 05:58:34PM +0800, Kairui Song wrote:
> When efi=noruntime or efi=oldmap is used, EFI services won't be available
> in the second kernel, therefore the second kernel will not be able to get
> the ACPI RSDP address from firmware by calling EFI services and won't
> boot. Previously we are expecting the user to set the acpi_rsdp=<addr>
> on kernel command line for second kernel as there was no way to pass RSDP
> address to second kernel.
>
> After commit e6e094e053af ('x86/acpi, x86/boot: Take RSDP address from
> boot params if available'), now it's possible to set an acpi_rsdp_addr
> parameter in the boot_params passed to second kernel, this commit make
> use of it, detect and set the RSDP address when it's required for second
> kernel to boot.
>
> Tested with an EFI enabled KVM VM with efi=noruntime.
>
> Suggested-by: Dave Young <[email protected]>
> Signed-off-by: Kairui Song <[email protected]>
> ---
> arch/x86/kernel/kexec-bzimage64.c | 21 +++++++++++++++++++++
> drivers/acpi/acpica/tbxfroot.c | 3 +--
> include/acpi/acpixf.h | 2 +-
> 3 files changed, 23 insertions(+), 3 deletions(-)
>
> diff --git a/arch/x86/kernel/kexec-bzimage64.c b/arch/x86/kernel/kexec-bzimage64.c
> index 53917a3ebf94..0a90dcbd041f 100644
> --- a/arch/x86/kernel/kexec-bzimage64.c
> +++ b/arch/x86/kernel/kexec-bzimage64.c
> @@ -20,6 +20,7 @@
> #include <linux/mm.h>
> #include <linux/efi.h>
> #include <linux/verification.h>
> +#include <linux/acpi.h>
>
> #include <asm/bootparam.h>
> #include <asm/setup.h>
> @@ -255,8 +256,28 @@ setup_boot_parameters(struct kimage *image, struct boot_params *params,
> /* Setup EFI state */
> setup_efi_state(params, params_load_addr, efi_map_offset, efi_map_sz,
> efi_setup_data_offset);
> +
> +#ifdef CONFIG_ACPI
> + /* Setup ACPI RSDP pointer in case EFI is not available in second kernel */
> + if (!acpi_disabled && (!efi_enabled(EFI_RUNTIME_SERVICES) || efi_enabled(EFI_OLD_MEMMAP))) {
> + /* Copied from acpi_os_get_root_pointer accordingly */
> + params->acpi_rsdp_addr = boot_params.acpi_rsdp_addr;
> + if (!params->acpi_rsdp_addr) {
> + if (efi_enabled(EFI_CONFIG_TABLES)) {
> + if (efi.acpi20 != EFI_INVALID_TABLE_ADDR)
> + params->acpi_rsdp_addr = efi.acpi20;
> + else if (efi.acpi != EFI_INVALID_TABLE_ADDR)
> + params->acpi_rsdp_addr = efi.acpi;
> + } else if (IS_ENABLED(CONFIG_ACPI_LEGACY_TABLES_LOOKUP)) {
> + acpi_find_root_pointer(¶ms->acpi_rsdp_addr);
> + }
> + }
> + if (!params->acpi_rsdp_addr)
> + pr_warn("RSDP is not available for second kernel\n");
> + }
> #endif
Amazing the amount of ACPI RDSP parsing and fiddling patches flying
around these days...
In any case, this needs to be synchronized with:
https://lkml.kernel.org/r/[email protected]
and checked whether the above can be used instead of sprinkling of ACPI
parsing code left and right.
Thx.
--
Regards/Gruss,
Boris.
Good mailing practices for 400: avoid top-posting and trim the reply.
On 01/16/19 at 01:09pm, Kairui Song wrote:
> On Wed, Jan 16, 2019 at 11:32 AM Dave Young <[email protected]> wrote:
> >
> > On 01/16/19 at 12:10am, Borislav Petkov wrote:
> > > On Tue, Jan 15, 2019 at 05:58:34PM +0800, Kairui Song wrote:
> > > > When efi=noruntime or efi=oldmap is used, EFI services won't be available
> > > > in the second kernel, therefore the second kernel will not be able to get
> > > > the ACPI RSDP address from firmware by calling EFI services and won't
> > > > boot. Previously we are expecting the user to set the acpi_rsdp=<addr>
> > > > on kernel command line for second kernel as there was no way to pass RSDP
> > > > address to second kernel.
> > > >
> > > > After commit e6e094e053af ('x86/acpi, x86/boot: Take RSDP address from
> > > > boot params if available'), now it's possible to set an acpi_rsdp_addr
> > > > parameter in the boot_params passed to second kernel, this commit make
> > > > use of it, detect and set the RSDP address when it's required for second
> > > > kernel to boot.
> > > >
> > > > Tested with an EFI enabled KVM VM with efi=noruntime.
> > > >
> > > > Suggested-by: Dave Young <[email protected]>
> > > > Signed-off-by: Kairui Song <[email protected]>
> > > > ---
> > > > arch/x86/kernel/kexec-bzimage64.c | 21 +++++++++++++++++++++
> > > > drivers/acpi/acpica/tbxfroot.c | 3 +--
> > > > include/acpi/acpixf.h | 2 +-
> > > > 3 files changed, 23 insertions(+), 3 deletions(-)
> > > >
> > > > diff --git a/arch/x86/kernel/kexec-bzimage64.c b/arch/x86/kernel/kexec-bzimage64.c
> > > > index 53917a3ebf94..0a90dcbd041f 100644
> > > > --- a/arch/x86/kernel/kexec-bzimage64.c
> > > > +++ b/arch/x86/kernel/kexec-bzimage64.c
> > > > @@ -20,6 +20,7 @@
> > > > #include <linux/mm.h>
> > > > #include <linux/efi.h>
> > > > #include <linux/verification.h>
> > > > +#include <linux/acpi.h>
> > > >
> > > > #include <asm/bootparam.h>
> > > > #include <asm/setup.h>
> > > > @@ -255,8 +256,28 @@ setup_boot_parameters(struct kimage *image, struct boot_params *params,
> > > > /* Setup EFI state */
> > > > setup_efi_state(params, params_load_addr, efi_map_offset, efi_map_sz,
> > > > efi_setup_data_offset);
> > > > +
> > > > +#ifdef CONFIG_ACPI
> > > > + /* Setup ACPI RSDP pointer in case EFI is not available in second kernel */
> > > > + if (!acpi_disabled && (!efi_enabled(EFI_RUNTIME_SERVICES) || efi_enabled(EFI_OLD_MEMMAP))) {
> > > > + /* Copied from acpi_os_get_root_pointer accordingly */
> > > > + params->acpi_rsdp_addr = boot_params.acpi_rsdp_addr;
> > > > + if (!params->acpi_rsdp_addr) {
> > > > + if (efi_enabled(EFI_CONFIG_TABLES)) {
> > > > + if (efi.acpi20 != EFI_INVALID_TABLE_ADDR)
> > > > + params->acpi_rsdp_addr = efi.acpi20;
> > > > + else if (efi.acpi != EFI_INVALID_TABLE_ADDR)
> > > > + params->acpi_rsdp_addr = efi.acpi;
> > > > + } else if (IS_ENABLED(CONFIG_ACPI_LEGACY_TABLES_LOOKUP)) {
> > > > + acpi_find_root_pointer(¶ms->acpi_rsdp_addr);
> > > > + }
> > > > + }
> > > > + if (!params->acpi_rsdp_addr)
> > > > + pr_warn("RSDP is not available for second kernel\n");
> > > > + }
> > > > #endif
> > >
> > > Amazing the amount of ACPI RDSP parsing and fiddling patches flying
> > > around these days...
> > >
> > > In any case, this needs to be synchronized with:
> > >
> > > https://lkml.kernel.org/r/[email protected]
> > >
> > > and checked whether the above can be used instead of sprinkling of ACPI
> > > parsing code left and right.
> >
> > Both Baoquan and Chao are cced for comments.
> > The above KASLR patches seems some early code to parsing acpi, but I think in this
> > patch just call acpi function to get the root pointer no need to add the
> > duplicate logic of if/else/else if.
> >
> > Kairui, do you have any reason for the checking? Is there some simple
> > acpi function to just return the root pointer?
>
> Hi, I'm afraid that would require moving multiple structure and
> function out of .init,
> acpi_os_get_root_pointer is an ideal function to do the job, but it's
> in .init and (on x86) it will call x86_init.acpi.get_root_pointer (by
> calling acpi_arch_get_root_pointer) which will also be freed after
> init, unless I change the x86_init, move they out of .init which is
> not a good idea.
>
> Maybe I could split acpi_os_get_root_pointer into two, one gets freed
> after init, one for later use. But the "acpi_rsdp_addr" trick only
> works for x86, and this would change more common acpi driver code so
> not sure if a good idea.
Can the acpi root pointer be saved after initialized? If that is good
then a new function to get it would be easier. But need opinion from
acpi people.
Thanks
Dave
On 01/16/19 at 12:10am, Borislav Petkov wrote:
> On Tue, Jan 15, 2019 at 05:58:34PM +0800, Kairui Song wrote:
> > When efi=noruntime or efi=oldmap is used, EFI services won't be available
> > in the second kernel, therefore the second kernel will not be able to get
> > the ACPI RSDP address from firmware by calling EFI services and won't
> > boot. Previously we are expecting the user to set the acpi_rsdp=<addr>
> > on kernel command line for second kernel as there was no way to pass RSDP
> > address to second kernel.
> >
> > After commit e6e094e053af ('x86/acpi, x86/boot: Take RSDP address from
> > boot params if available'), now it's possible to set an acpi_rsdp_addr
> > parameter in the boot_params passed to second kernel, this commit make
> > use of it, detect and set the RSDP address when it's required for second
> > kernel to boot.
> >
> > Tested with an EFI enabled KVM VM with efi=noruntime.
> >
> > Suggested-by: Dave Young <[email protected]>
> > Signed-off-by: Kairui Song <[email protected]>
> > ---
> > arch/x86/kernel/kexec-bzimage64.c | 21 +++++++++++++++++++++
> > drivers/acpi/acpica/tbxfroot.c | 3 +--
> > include/acpi/acpixf.h | 2 +-
> > 3 files changed, 23 insertions(+), 3 deletions(-)
> >
> > diff --git a/arch/x86/kernel/kexec-bzimage64.c b/arch/x86/kernel/kexec-bzimage64.c
> > index 53917a3ebf94..0a90dcbd041f 100644
> > --- a/arch/x86/kernel/kexec-bzimage64.c
> > +++ b/arch/x86/kernel/kexec-bzimage64.c
> > @@ -20,6 +20,7 @@
> > #include <linux/mm.h>
> > #include <linux/efi.h>
> > #include <linux/verification.h>
> > +#include <linux/acpi.h>
> >
> > #include <asm/bootparam.h>
> > #include <asm/setup.h>
> > @@ -255,8 +256,28 @@ setup_boot_parameters(struct kimage *image, struct boot_params *params,
> > /* Setup EFI state */
> > setup_efi_state(params, params_load_addr, efi_map_offset, efi_map_sz,
> > efi_setup_data_offset);
> > +
> > +#ifdef CONFIG_ACPI
> > + /* Setup ACPI RSDP pointer in case EFI is not available in second kernel */
> > + if (!acpi_disabled && (!efi_enabled(EFI_RUNTIME_SERVICES) || efi_enabled(EFI_OLD_MEMMAP))) {
> > + /* Copied from acpi_os_get_root_pointer accordingly */
> > + params->acpi_rsdp_addr = boot_params.acpi_rsdp_addr;
> > + if (!params->acpi_rsdp_addr) {
> > + if (efi_enabled(EFI_CONFIG_TABLES)) {
> > + if (efi.acpi20 != EFI_INVALID_TABLE_ADDR)
> > + params->acpi_rsdp_addr = efi.acpi20;
> > + else if (efi.acpi != EFI_INVALID_TABLE_ADDR)
> > + params->acpi_rsdp_addr = efi.acpi;
> > + } else if (IS_ENABLED(CONFIG_ACPI_LEGACY_TABLES_LOOKUP)) {
> > + acpi_find_root_pointer(¶ms->acpi_rsdp_addr);
> > + }
> > + }
> > + if (!params->acpi_rsdp_addr)
> > + pr_warn("RSDP is not available for second kernel\n");
> > + }
> > #endif
>
> Amazing the amount of ACPI RDSP parsing and fiddling patches flying
> around these days...
>
> In any case, this needs to be synchronized with:
>
> https://lkml.kernel.org/r/[email protected]
>
> and checked whether the above can be used instead of sprinkling of ACPI
> parsing code left and right.
Both Baoquan and Chao are cced for comments.
The above KASLR patches seems some early code to parsing acpi, but I think in this
patch just call acpi function to get the root pointer no need to add the
duplicate logic of if/else/else if.
Kairui, do you have any reason for the checking? Is there some simple
acpi function to just return the root pointer?
>
> Thx.
>
> --
> Regards/Gruss,
> Boris.
>
> Good mailing practices for 400: avoid top-posting and trim the reply.
Thanks
Dave
On Wed, Jan 16, 2019 at 11:32 AM Dave Young <[email protected]> wrote:
>
> On 01/16/19 at 12:10am, Borislav Petkov wrote:
> > On Tue, Jan 15, 2019 at 05:58:34PM +0800, Kairui Song wrote:
> > > When efi=noruntime or efi=oldmap is used, EFI services won't be available
> > > in the second kernel, therefore the second kernel will not be able to get
> > > the ACPI RSDP address from firmware by calling EFI services and won't
> > > boot. Previously we are expecting the user to set the acpi_rsdp=<addr>
> > > on kernel command line for second kernel as there was no way to pass RSDP
> > > address to second kernel.
> > >
> > > After commit e6e094e053af ('x86/acpi, x86/boot: Take RSDP address from
> > > boot params if available'), now it's possible to set an acpi_rsdp_addr
> > > parameter in the boot_params passed to second kernel, this commit make
> > > use of it, detect and set the RSDP address when it's required for second
> > > kernel to boot.
> > >
> > > Tested with an EFI enabled KVM VM with efi=noruntime.
> > >
> > > Suggested-by: Dave Young <[email protected]>
> > > Signed-off-by: Kairui Song <[email protected]>
> > > ---
> > > arch/x86/kernel/kexec-bzimage64.c | 21 +++++++++++++++++++++
> > > drivers/acpi/acpica/tbxfroot.c | 3 +--
> > > include/acpi/acpixf.h | 2 +-
> > > 3 files changed, 23 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/arch/x86/kernel/kexec-bzimage64.c b/arch/x86/kernel/kexec-bzimage64.c
> > > index 53917a3ebf94..0a90dcbd041f 100644
> > > --- a/arch/x86/kernel/kexec-bzimage64.c
> > > +++ b/arch/x86/kernel/kexec-bzimage64.c
> > > @@ -20,6 +20,7 @@
> > > #include <linux/mm.h>
> > > #include <linux/efi.h>
> > > #include <linux/verification.h>
> > > +#include <linux/acpi.h>
> > >
> > > #include <asm/bootparam.h>
> > > #include <asm/setup.h>
> > > @@ -255,8 +256,28 @@ setup_boot_parameters(struct kimage *image, struct boot_params *params,
> > > /* Setup EFI state */
> > > setup_efi_state(params, params_load_addr, efi_map_offset, efi_map_sz,
> > > efi_setup_data_offset);
> > > +
> > > +#ifdef CONFIG_ACPI
> > > + /* Setup ACPI RSDP pointer in case EFI is not available in second kernel */
> > > + if (!acpi_disabled && (!efi_enabled(EFI_RUNTIME_SERVICES) || efi_enabled(EFI_OLD_MEMMAP))) {
> > > + /* Copied from acpi_os_get_root_pointer accordingly */
> > > + params->acpi_rsdp_addr = boot_params.acpi_rsdp_addr;
> > > + if (!params->acpi_rsdp_addr) {
> > > + if (efi_enabled(EFI_CONFIG_TABLES)) {
> > > + if (efi.acpi20 != EFI_INVALID_TABLE_ADDR)
> > > + params->acpi_rsdp_addr = efi.acpi20;
> > > + else if (efi.acpi != EFI_INVALID_TABLE_ADDR)
> > > + params->acpi_rsdp_addr = efi.acpi;
> > > + } else if (IS_ENABLED(CONFIG_ACPI_LEGACY_TABLES_LOOKUP)) {
> > > + acpi_find_root_pointer(¶ms->acpi_rsdp_addr);
> > > + }
> > > + }
> > > + if (!params->acpi_rsdp_addr)
> > > + pr_warn("RSDP is not available for second kernel\n");
> > > + }
> > > #endif
> >
> > Amazing the amount of ACPI RDSP parsing and fiddling patches flying
> > around these days...
> >
> > In any case, this needs to be synchronized with:
> >
> > https://lkml.kernel.org/r/[email protected]
> >
> > and checked whether the above can be used instead of sprinkling of ACPI
> > parsing code left and right.
>
> Both Baoquan and Chao are cced for comments.
> The above KASLR patches seems some early code to parsing acpi, but I think in this
> patch just call acpi function to get the root pointer no need to add the
> duplicate logic of if/else/else if.
>
> Kairui, do you have any reason for the checking? Is there some simple
> acpi function to just return the root pointer?
Hi, I'm afraid that would require moving multiple structure and
function out of .init,
acpi_os_get_root_pointer is an ideal function to do the job, but it's
in .init and (on x86) it will call x86_init.acpi.get_root_pointer (by
calling acpi_arch_get_root_pointer) which will also be freed after
init, unless I change the x86_init, move they out of .init which is
not a good idea.
Maybe I could split acpi_os_get_root_pointer into two, one gets freed
after init, one for later use. But the "acpi_rsdp_addr" trick only
works for x86, and this would change more common acpi driver code so
not sure if a good idea.
>
> >
> > Thx.
> >
> > --
> > Regards/Gruss,
> > Boris.
> >
> > Good mailing practices for 400: avoid top-posting and trim the reply.
>
> Thanks
> Dave
--
Best Regards,
Kairui Song
On Wed, Jan 16, 2019 at 03:08:42PM +0800, Kairui Song wrote:
> I didn't see a way to reuse things in that patch series, situation is
> different, in that patch it needs to get RSDP in very early boot stage
> so it did everything from scratch, in this patch kexec_file_load need
> to get RSDP too, but everything is well setup so things are a lot
> easier, just read from current boot_prams, efi and fallback to
> acpi_find_root_pointer should be good.
No no. Early code should find out that venerable RSDP thing once and
will save it somewhere for further use. No gazillion parsings of it.
Just once and share it with the rest of the code that needs it.
--
Regards/Gruss,
Boris.
Good mailing practices for 400: avoid top-posting and trim the reply.
On 1/16/2019 7:51 AM, Dave Young wrote:
> On 01/16/19 at 01:09pm, Kairui Song wrote:
>> On Wed, Jan 16, 2019 at 11:32 AM Dave Young <[email protected]> wrote:
>>> On 01/16/19 at 12:10am, Borislav Petkov wrote:
>>>> On Tue, Jan 15, 2019 at 05:58:34PM +0800, Kairui Song wrote:
>>>>> When efi=noruntime or efi=oldmap is used, EFI services won't be available
>>>>> in the second kernel, therefore the second kernel will not be able to get
>>>>> the ACPI RSDP address from firmware by calling EFI services and won't
>>>>> boot. Previously we are expecting the user to set the acpi_rsdp=<addr>
>>>>> on kernel command line for second kernel as there was no way to pass RSDP
>>>>> address to second kernel.
>>>>>
>>>>> After commit e6e094e053af ('x86/acpi, x86/boot: Take RSDP address from
>>>>> boot params if available'), now it's possible to set an acpi_rsdp_addr
>>>>> parameter in the boot_params passed to second kernel, this commit make
>>>>> use of it, detect and set the RSDP address when it's required for second
>>>>> kernel to boot.
>>>>>
>>>>> Tested with an EFI enabled KVM VM with efi=noruntime.
>>>>>
>>>>> Suggested-by: Dave Young <[email protected]>
>>>>> Signed-off-by: Kairui Song <[email protected]>
>>>>> ---
>>>>> arch/x86/kernel/kexec-bzimage64.c | 21 +++++++++++++++++++++
>>>>> drivers/acpi/acpica/tbxfroot.c | 3 +--
>>>>> include/acpi/acpixf.h | 2 +-
>>>>> 3 files changed, 23 insertions(+), 3 deletions(-)
>>>>>
>>>>> diff --git a/arch/x86/kernel/kexec-bzimage64.c b/arch/x86/kernel/kexec-bzimage64.c
>>>>> index 53917a3ebf94..0a90dcbd041f 100644
>>>>> --- a/arch/x86/kernel/kexec-bzimage64.c
>>>>> +++ b/arch/x86/kernel/kexec-bzimage64.c
>>>>> @@ -20,6 +20,7 @@
>>>>> #include <linux/mm.h>
>>>>> #include <linux/efi.h>
>>>>> #include <linux/verification.h>
>>>>> +#include <linux/acpi.h>
>>>>>
>>>>> #include <asm/bootparam.h>
>>>>> #include <asm/setup.h>
>>>>> @@ -255,8 +256,28 @@ setup_boot_parameters(struct kimage *image, struct boot_params *params,
>>>>> /* Setup EFI state */
>>>>> setup_efi_state(params, params_load_addr, efi_map_offset, efi_map_sz,
>>>>> efi_setup_data_offset);
>>>>> +
>>>>> +#ifdef CONFIG_ACPI
>>>>> + /* Setup ACPI RSDP pointer in case EFI is not available in second kernel */
>>>>> + if (!acpi_disabled && (!efi_enabled(EFI_RUNTIME_SERVICES) || efi_enabled(EFI_OLD_MEMMAP))) {
>>>>> + /* Copied from acpi_os_get_root_pointer accordingly */
>>>>> + params->acpi_rsdp_addr = boot_params.acpi_rsdp_addr;
>>>>> + if (!params->acpi_rsdp_addr) {
>>>>> + if (efi_enabled(EFI_CONFIG_TABLES)) {
>>>>> + if (efi.acpi20 != EFI_INVALID_TABLE_ADDR)
>>>>> + params->acpi_rsdp_addr = efi.acpi20;
>>>>> + else if (efi.acpi != EFI_INVALID_TABLE_ADDR)
>>>>> + params->acpi_rsdp_addr = efi.acpi;
>>>>> + } else if (IS_ENABLED(CONFIG_ACPI_LEGACY_TABLES_LOOKUP)) {
>>>>> + acpi_find_root_pointer(¶ms->acpi_rsdp_addr);
>>>>> + }
>>>>> + }
>>>>> + if (!params->acpi_rsdp_addr)
>>>>> + pr_warn("RSDP is not available for second kernel\n");
>>>>> + }
>>>>> #endif
>>>> Amazing the amount of ACPI RDSP parsing and fiddling patches flying
>>>> around these days...
>>>>
>>>> In any case, this needs to be synchronized with:
>>>>
>>>> https://lkml.kernel.org/r/[email protected]
>>>>
>>>> and checked whether the above can be used instead of sprinkling of ACPI
>>>> parsing code left and right.
>>> Both Baoquan and Chao are cced for comments.
>>> The above KASLR patches seems some early code to parsing acpi, but I think in this
>>> patch just call acpi function to get the root pointer no need to add the
>>> duplicate logic of if/else/else if.
>>>
>>> Kairui, do you have any reason for the checking? Is there some simple
>>> acpi function to just return the root pointer?
>> Hi, I'm afraid that would require moving multiple structure and
>> function out of .init,
>> acpi_os_get_root_pointer is an ideal function to do the job, but it's
>> in .init and (on x86) it will call x86_init.acpi.get_root_pointer (by
>> calling acpi_arch_get_root_pointer) which will also be freed after
>> init, unless I change the x86_init, move they out of .init which is
>> not a good idea.
>>
>> Maybe I could split acpi_os_get_root_pointer into two, one gets freed
>> after init, one for later use. But the "acpi_rsdp_addr" trick only
>> works for x86, and this would change more common acpi driver code so
>> not sure if a good idea.
> Can the acpi root pointer be saved after initialized? If that is good
> then a new function to get it would be easier. But need opinion from
> acpi people.
For that, please repost the series with a CC to [email protected].
Thanks!
On Wed, Jan 16, 2019 at 5:46 PM Borislav Petkov <[email protected]> wrote:
>
> On Wed, Jan 16, 2019 at 03:08:42PM +0800, Kairui Song wrote:
> > I didn't see a way to reuse things in that patch series, situation is
> > different, in that patch it needs to get RSDP in very early boot stage
> > so it did everything from scratch, in this patch kexec_file_load need
> > to get RSDP too, but everything is well setup so things are a lot
> > easier, just read from current boot_prams, efi and fallback to
> > acpi_find_root_pointer should be good.
>
> No no. Early code should find out that venerable RSDP thing once and
> will save it somewhere for further use. No gazillion parsings of it.
> Just once and share it with the rest of the code that needs it.
>
How about we refill the boot_params.acpi_rsdp_addr if it is not valid
in early code, so it could be used as a reliable RSDP address source?
That should make things easier.
But if early code should parse it and store it should be done in
Chao's patch, or I can post another patch to do it if Chao's patch is
merged.
For now I think good to have something like this in this patch series
to always keep storing acpi_rsdp in late code,
acpi_os_get_root_pointer_late (maybe comeup with a better name later)
could be used anytime to get RSDP and no extra parsing:
--- a/drivers/acpi/osl.c
+++ b/drivers/acpi/osl.c
@@ -180,8 +180,8 @@ void acpi_os_vprintf(const char *fmt, va_list args)
#endif
}
-#ifdef CONFIG_KEXEC
static unsigned long acpi_rsdp;
+#ifdef CONFIG_KEXEC
static int __init setup_acpi_rsdp(char *arg)
{
return kstrtoul(arg, 16, &acpi_rsdp);
@@ -189,28 +189,38 @@ static int __init setup_acpi_rsdp(char *arg)
early_param("acpi_rsdp", setup_acpi_rsdp);
#endif
+acpi_physical_address acpi_os_get_root_pointer_late(void) {
+ return acpi_rsdp;
+}
+
acpi_physical_address __init acpi_os_get_root_pointer(void)
{
acpi_physical_address pa;
-#ifdef CONFIG_KEXEC
if (acpi_rsdp)
return acpi_rsdp;
-#endif
+
pa = acpi_arch_get_root_pointer();
- if (pa)
+ if (pa) {
+ acpi_rsdp = pa;
return pa;
+ }
if (efi_enabled(EFI_CONFIG_TABLES)) {
- if (efi.acpi20 != EFI_INVALID_TABLE_ADDR)
+ if (efi.acpi20 != EFI_INVALID_TABLE_ADDR) {
+ acpi_rsdp = efi.acpi20;
return efi.acpi20;
- if (efi.acpi != EFI_INVALID_TABLE_ADDR)
+ }
+ if (efi.acpi != EFI_INVALID_TABLE_ADDR) {
+ acpi_rsdp = efi.acpi;
return efi.acpi;
+ }
pr_err(PREFIX "System description tables not found\n");
} else if (IS_ENABLED(CONFIG_ACPI_LEGACY_TABLES_LOOKUP)) {
acpi_find_root_pointer(&pa);
}
+ acpi_rsdp = pa;
return pa;
}
> --
> Regards/Gruss,
> Boris.
>
> Good mailing practices for 400: avoid top-posting and trim the reply.
--
Best Regards,
Kairui Song
On Thu, Jan 17, 2019 at 03:41:13PM +0800, Kairui Song wrote:
>On Wed, Jan 16, 2019 at 5:46 PM Borislav Petkov <[email protected]> wrote:
>>
>> On Wed, Jan 16, 2019 at 03:08:42PM +0800, Kairui Song wrote:
>> > I didn't see a way to reuse things in that patch series, situation is
>> > different, in that patch it needs to get RSDP in very early boot stage
>> > so it did everything from scratch, in this patch kexec_file_load need
>> > to get RSDP too, but everything is well setup so things are a lot
>> > easier, just read from current boot_prams, efi and fallback to
>> > acpi_find_root_pointer should be good.
>>
>> No no. Early code should find out that venerable RSDP thing once and
>> will save it somewhere for further use. No gazillion parsings of it.
>> Just once and share it with the rest of the code that needs it.
>>
>
>How about we refill the boot_params.acpi_rsdp_addr if it is not valid
>in early code, so it could be used as a reliable RSDP address source?
>That should make things easier.
I think it's OK.
Try to read it, if get RSDP, use it.
If not, search in EFI/BIOS/... and refill the RSDP to
boot_params.acpi_rsdp_addr.
By the way, I search kernel code, I didn't find other code fill and
use it, only you(KEXEC) are trying to fill it.
If I miss something, please let me know.
Thanks,
Chao Fan
>
>But if early code should parse it and store it should be done in
>Chao's patch, or I can post another patch to do it if Chao's patch is
>merged.
>
>For now I think good to have something like this in this patch series
>to always keep storing acpi_rsdp in late code,
>acpi_os_get_root_pointer_late (maybe comeup with a better name later)
>could be used anytime to get RSDP and no extra parsing:
>
>--- a/drivers/acpi/osl.c
>+++ b/drivers/acpi/osl.c
>@@ -180,8 +180,8 @@ void acpi_os_vprintf(const char *fmt, va_list args)
> #endif
> }
>
>-#ifdef CONFIG_KEXEC
> static unsigned long acpi_rsdp;
>+#ifdef CONFIG_KEXEC
> static int __init setup_acpi_rsdp(char *arg)
> {
> return kstrtoul(arg, 16, &acpi_rsdp);
>@@ -189,28 +189,38 @@ static int __init setup_acpi_rsdp(char *arg)
> early_param("acpi_rsdp", setup_acpi_rsdp);
> #endif
>
>+acpi_physical_address acpi_os_get_root_pointer_late(void) {
>+ return acpi_rsdp;
>+}
>+
> acpi_physical_address __init acpi_os_get_root_pointer(void)
> {
> acpi_physical_address pa;
>
>-#ifdef CONFIG_KEXEC
> if (acpi_rsdp)
> return acpi_rsdp;
>-#endif
>+
> pa = acpi_arch_get_root_pointer();
>- if (pa)
>+ if (pa) {
>+ acpi_rsdp = pa;
> return pa;
>+ }
>
> if (efi_enabled(EFI_CONFIG_TABLES)) {
>- if (efi.acpi20 != EFI_INVALID_TABLE_ADDR)
>+ if (efi.acpi20 != EFI_INVALID_TABLE_ADDR) {
>+ acpi_rsdp = efi.acpi20;
> return efi.acpi20;
>- if (efi.acpi != EFI_INVALID_TABLE_ADDR)
>+ }
>+ if (efi.acpi != EFI_INVALID_TABLE_ADDR) {
>+ acpi_rsdp = efi.acpi;
> return efi.acpi;
>+ }
> pr_err(PREFIX "System description tables not found\n");
> } else if (IS_ENABLED(CONFIG_ACPI_LEGACY_TABLES_LOOKUP)) {
> acpi_find_root_pointer(&pa);
> }
>
> + acpi_rsdp = pa;
> return pa;
> }
>
>> --
>> Regards/Gruss,
>> Boris.
>>
>> Good mailing practices for 400: avoid top-posting and trim the reply.
>--
>Best Regards,
>Kairui Song
>
>
On Thu, Jan 17, 2019 at 3:51 PM Chao Fan <[email protected]> wrote:
>
> On Thu, Jan 17, 2019 at 03:41:13PM +0800, Kairui Song wrote:
> >On Wed, Jan 16, 2019 at 5:46 PM Borislav Petkov <[email protected]> wrote:
> >>
> >> On Wed, Jan 16, 2019 at 03:08:42PM +0800, Kairui Song wrote:
> >> > I didn't see a way to reuse things in that patch series, situation is
> >> > different, in that patch it needs to get RSDP in very early boot stage
> >> > so it did everything from scratch, in this patch kexec_file_load need
> >> > to get RSDP too, but everything is well setup so things are a lot
> >> > easier, just read from current boot_prams, efi and fallback to
> >> > acpi_find_root_pointer should be good.
> >>
> >> No no. Early code should find out that venerable RSDP thing once and
> >> will save it somewhere for further use. No gazillion parsings of it.
> >> Just once and share it with the rest of the code that needs it.
> >>
> >
> >How about we refill the boot_params.acpi_rsdp_addr if it is not valid
> >in early code, so it could be used as a reliable RSDP address source?
> >That should make things easier.
>
> I think it's OK.
> Try to read it, if get RSDP, use it.
> If not, search in EFI/BIOS/... and refill the RSDP to
> boot_params.acpi_rsdp_addr.
> By the way, I search kernel code, I didn't find other code fill and
> use it, only you(KEXEC) are trying to fill it.
> If I miss something, please let me know.
Yes, kexec would read RSDP again to pass it to second kernel, and only
if EFI is disabled (efi=noruntime/old_map, else second kernel will get
rsdp just fine). Not sure if any other component would use it.
>
> Thanks,
> Chao Fan
>
--
Best Regards,
Kairui Song
+ linux-acpi list
On 01/17/19 at 03:49pm, Chao Fan wrote:
> On Thu, Jan 17, 2019 at 03:41:13PM +0800, Kairui Song wrote:
> >On Wed, Jan 16, 2019 at 5:46 PM Borislav Petkov <[email protected]> wrote:
> >>
> >> On Wed, Jan 16, 2019 at 03:08:42PM +0800, Kairui Song wrote:
> >> > I didn't see a way to reuse things in that patch series, situation is
> >> > different, in that patch it needs to get RSDP in very early boot stage
> >> > so it did everything from scratch, in this patch kexec_file_load need
> >> > to get RSDP too, but everything is well setup so things are a lot
> >> > easier, just read from current boot_prams, efi and fallback to
> >> > acpi_find_root_pointer should be good.
> >>
> >> No no. Early code should find out that venerable RSDP thing once and
> >> will save it somewhere for further use. No gazillion parsings of it.
> >> Just once and share it with the rest of the code that needs it.
> >>
> >
> >How about we refill the boot_params.acpi_rsdp_addr if it is not valid
> >in early code, so it could be used as a reliable RSDP address source?
> >That should make things easier.
>
> I think it's OK.
> Try to read it, if get RSDP, use it.
> If not, search in EFI/BIOS/... and refill the RSDP to
> boot_params.acpi_rsdp_addr.
> By the way, I search kernel code, I didn't find other code fill and
> use it, only you(KEXEC) are trying to fill it.
> If I miss something, please let me know.
>
> Thanks,
> Chao Fan
>
> >
> >But if early code should parse it and store it should be done in
> >Chao's patch, or I can post another patch to do it if Chao's patch is
> >merged.
> >
> >For now I think good to have something like this in this patch series
> >to always keep storing acpi_rsdp in late code,
> >acpi_os_get_root_pointer_late (maybe comeup with a better name later)
> >could be used anytime to get RSDP and no extra parsing:
> >
> >--- a/drivers/acpi/osl.c
> >+++ b/drivers/acpi/osl.c
> >@@ -180,8 +180,8 @@ void acpi_os_vprintf(const char *fmt, va_list args)
> > #endif
> > }
> >
> >-#ifdef CONFIG_KEXEC
> > static unsigned long acpi_rsdp;
> >+#ifdef CONFIG_KEXEC
> > static int __init setup_acpi_rsdp(char *arg)
> > {
> > return kstrtoul(arg, 16, &acpi_rsdp);
> >@@ -189,28 +189,38 @@ static int __init setup_acpi_rsdp(char *arg)
> > early_param("acpi_rsdp", setup_acpi_rsdp);
> > #endif
> >
> >+acpi_physical_address acpi_os_get_root_pointer_late(void) {
> >+ return acpi_rsdp;
> >+}
> >+
> > acpi_physical_address __init acpi_os_get_root_pointer(void)
> > {
> > acpi_physical_address pa;
> >
> >-#ifdef CONFIG_KEXEC
> > if (acpi_rsdp)
> > return acpi_rsdp;
> >-#endif
> >+
> > pa = acpi_arch_get_root_pointer();
> >- if (pa)
> >+ if (pa) {
> >+ acpi_rsdp = pa;
> > return pa;
> >+ }
> >
> > if (efi_enabled(EFI_CONFIG_TABLES)) {
> >- if (efi.acpi20 != EFI_INVALID_TABLE_ADDR)
> >+ if (efi.acpi20 != EFI_INVALID_TABLE_ADDR) {
> >+ acpi_rsdp = efi.acpi20;
> > return efi.acpi20;
> >- if (efi.acpi != EFI_INVALID_TABLE_ADDR)
> >+ }
> >+ if (efi.acpi != EFI_INVALID_TABLE_ADDR) {
> >+ acpi_rsdp = efi.acpi;
> > return efi.acpi;
> >+ }
> > pr_err(PREFIX "System description tables not found\n");
> > } else if (IS_ENABLED(CONFIG_ACPI_LEGACY_TABLES_LOOKUP)) {
> > acpi_find_root_pointer(&pa);
> > }
> >
> > + acpi_rsdp = pa;
> > return pa;
> > }
> >
> >> --
> >> Regards/Gruss,
> >> Boris.
> >>
> >> Good mailing practices for 400: avoid top-posting and trim the reply.
> >--
> >Best Regards,
> >Kairui Song
> >
> >
>
>
Add linux-acpi list
On 01/17/19 at 03:41pm, Kairui Song wrote:
> On Wed, Jan 16, 2019 at 5:46 PM Borislav Petkov <[email protected]> wrote:
> >
> > On Wed, Jan 16, 2019 at 03:08:42PM +0800, Kairui Song wrote:
> > > I didn't see a way to reuse things in that patch series, situation is
> > > different, in that patch it needs to get RSDP in very early boot stage
> > > so it did everything from scratch, in this patch kexec_file_load need
> > > to get RSDP too, but everything is well setup so things are a lot
> > > easier, just read from current boot_prams, efi and fallback to
> > > acpi_find_root_pointer should be good.
> >
> > No no. Early code should find out that venerable RSDP thing once and
> > will save it somewhere for further use. No gazillion parsings of it.
> > Just once and share it with the rest of the code that needs it.
> >
>
> How about we refill the boot_params.acpi_rsdp_addr if it is not valid
> in early code, so it could be used as a reliable RSDP address source?
> That should make things easier.
>
> But if early code should parse it and store it should be done in
> Chao's patch, or I can post another patch to do it if Chao's patch is
> merged.
>
> For now I think good to have something like this in this patch series
> to always keep storing acpi_rsdp in late code,
> acpi_os_get_root_pointer_late (maybe comeup with a better name later)
> could be used anytime to get RSDP and no extra parsing:
>
> --- a/drivers/acpi/osl.c
> +++ b/drivers/acpi/osl.c
> @@ -180,8 +180,8 @@ void acpi_os_vprintf(const char *fmt, va_list args)
> #endif
> }
>
> -#ifdef CONFIG_KEXEC
> static unsigned long acpi_rsdp;
> +#ifdef CONFIG_KEXEC
> static int __init setup_acpi_rsdp(char *arg)
> {
> return kstrtoul(arg, 16, &acpi_rsdp);
> @@ -189,28 +189,38 @@ static int __init setup_acpi_rsdp(char *arg)
> early_param("acpi_rsdp", setup_acpi_rsdp);
> #endif
>
> +acpi_physical_address acpi_os_get_root_pointer_late(void) {
> + return acpi_rsdp;
> +}
> +
> acpi_physical_address __init acpi_os_get_root_pointer(void)
> {
> acpi_physical_address pa;
>
> -#ifdef CONFIG_KEXEC
> if (acpi_rsdp)
> return acpi_rsdp;
> -#endif
> +
> pa = acpi_arch_get_root_pointer();
> - if (pa)
> + if (pa) {
> + acpi_rsdp = pa;
> return pa;
> + }
>
> if (efi_enabled(EFI_CONFIG_TABLES)) {
> - if (efi.acpi20 != EFI_INVALID_TABLE_ADDR)
> + if (efi.acpi20 != EFI_INVALID_TABLE_ADDR) {
> + acpi_rsdp = efi.acpi20;
> return efi.acpi20;
> - if (efi.acpi != EFI_INVALID_TABLE_ADDR)
> + }
> + if (efi.acpi != EFI_INVALID_TABLE_ADDR) {
> + acpi_rsdp = efi.acpi;
> return efi.acpi;
> + }
> pr_err(PREFIX "System description tables not found\n");
> } else if (IS_ENABLED(CONFIG_ACPI_LEGACY_TABLES_LOOKUP)) {
> acpi_find_root_pointer(&pa);
> }
>
> + acpi_rsdp = pa;
> return pa;
> }
>
> > --
> > Regards/Gruss,
> > Boris.
> >
> > Good mailing practices for 400: avoid top-posting and trim the reply.
> --
> Best Regards,
> Kairui Song
On Thu, Jan 17, 2019 at 9:53 AM Dave Young <[email protected]> wrote:
>
> Add linux-acpi list
Well, thanks, but please resend the patches with a CC to linux-acpi.
> On 01/17/19 at 03:41pm, Kairui Song wrote:
> > On Wed, Jan 16, 2019 at 5:46 PM Borislav Petkov <[email protected]> wrote:
> > >
> > > On Wed, Jan 16, 2019 at 03:08:42PM +0800, Kairui Song wrote:
> > > > I didn't see a way to reuse things in that patch series, situation is
> > > > different, in that patch it needs to get RSDP in very early boot stage
> > > > so it did everything from scratch, in this patch kexec_file_load need
> > > > to get RSDP too, but everything is well setup so things are a lot
> > > > easier, just read from current boot_prams, efi and fallback to
> > > > acpi_find_root_pointer should be good.
> > >
> > > No no. Early code should find out that venerable RSDP thing once and
> > > will save it somewhere for further use. No gazillion parsings of it.
> > > Just once and share it with the rest of the code that needs it.
> > >
> >
> > How about we refill the boot_params.acpi_rsdp_addr if it is not valid
> > in early code, so it could be used as a reliable RSDP address source?
> > That should make things easier.
> >
> > But if early code should parse it and store it should be done in
> > Chao's patch, or I can post another patch to do it if Chao's patch is
> > merged.
> >
> > For now I think good to have something like this in this patch series
> > to always keep storing acpi_rsdp in late code,
> > acpi_os_get_root_pointer_late (maybe comeup with a better name later)
> > could be used anytime to get RSDP and no extra parsing:
> >
> > --- a/drivers/acpi/osl.c
> > +++ b/drivers/acpi/osl.c
> > @@ -180,8 +180,8 @@ void acpi_os_vprintf(const char *fmt, va_list args)
> > #endif
> > }
> >
> > -#ifdef CONFIG_KEXEC
> > static unsigned long acpi_rsdp;
> > +#ifdef CONFIG_KEXEC
> > static int __init setup_acpi_rsdp(char *arg)
> > {
> > return kstrtoul(arg, 16, &acpi_rsdp);
> > @@ -189,28 +189,38 @@ static int __init setup_acpi_rsdp(char *arg)
> > early_param("acpi_rsdp", setup_acpi_rsdp);
> > #endif
> >
> > +acpi_physical_address acpi_os_get_root_pointer_late(void) {
> > + return acpi_rsdp;
> > +}
> > +
> > acpi_physical_address __init acpi_os_get_root_pointer(void)
> > {
> > acpi_physical_address pa;
> >
> > -#ifdef CONFIG_KEXEC
> > if (acpi_rsdp)
> > return acpi_rsdp;
> > -#endif
> > +
> > pa = acpi_arch_get_root_pointer();
> > - if (pa)
> > + if (pa) {
> > + acpi_rsdp = pa;
> > return pa;
> > + }
> >
> > if (efi_enabled(EFI_CONFIG_TABLES)) {
> > - if (efi.acpi20 != EFI_INVALID_TABLE_ADDR)
> > + if (efi.acpi20 != EFI_INVALID_TABLE_ADDR) {
> > + acpi_rsdp = efi.acpi20;
> > return efi.acpi20;
> > - if (efi.acpi != EFI_INVALID_TABLE_ADDR)
> > + }
> > + if (efi.acpi != EFI_INVALID_TABLE_ADDR) {
> > + acpi_rsdp = efi.acpi;
> > return efi.acpi;
> > + }
> > pr_err(PREFIX "System description tables not found\n");
> > } else if (IS_ENABLED(CONFIG_ACPI_LEGACY_TABLES_LOOKUP)) {
> > acpi_find_root_pointer(&pa);
> > }
> >
> > + acpi_rsdp = pa;
> > return pa;
> > }
> >
> > > --
> > > Regards/Gruss,
> > > Boris.
> > >
> > > Good mailing practices for 400: avoid top-posting and trim the reply.
> > --
> > Best Regards,
> > Kairui Song
On Thu, Jan 17, 2019 at 5:40 PM Rafael J. Wysocki <[email protected]> wrote:
>
> On Thu, Jan 17, 2019 at 9:53 AM Dave Young <[email protected]> wrote:
> >
> > Add linux-acpi list
>
> Well, thanks, but please resend the patches with a CC to linux-acpi.
>
Hi, sure will do.
Any thought on adding an acpi_os_get_root_pointer_late and store rsdp
pointer as mentioned? Will updat the patch and post V2, and cc
linux-acpi as well later.
> > On 01/17/19 at 03:41pm, Kairui Song wrote:
> > > On Wed, Jan 16, 2019 at 5:46 PM Borislav Petkov <[email protected]> wrote:
> > > >
> > > > On Wed, Jan 16, 2019 at 03:08:42PM +0800, Kairui Song wrote:
> > > > > I didn't see a way to reuse things in that patch series, situation is
> > > > > different, in that patch it needs to get RSDP in very early boot stage
> > > > > so it did everything from scratch, in this patch kexec_file_load need
> > > > > to get RSDP too, but everything is well setup so things are a lot
> > > > > easier, just read from current boot_prams, efi and fallback to
> > > > > acpi_find_root_pointer should be good.
> > > >
> > > > No no. Early code should find out that venerable RSDP thing once and
> > > > will save it somewhere for further use. No gazillion parsings of it.
> > > > Just once and share it with the rest of the code that needs it.
> > > >
> > >
> > > How about we refill the boot_params.acpi_rsdp_addr if it is not valid
> > > in early code, so it could be used as a reliable RSDP address source?
> > > That should make things easier.
> > >
> > > But if early code should parse it and store it should be done in
> > > Chao's patch, or I can post another patch to do it if Chao's patch is
> > > merged.
> > >
> > > For now I think good to have something like this in this patch series
> > > to always keep storing acpi_rsdp in late code,
> > > acpi_os_get_root_pointer_late (maybe comeup with a better name later)
> > > could be used anytime to get RSDP and no extra parsing:
> > >
> > > --- a/drivers/acpi/osl.c
> > > +++ b/drivers/acpi/osl.c
> > > @@ -180,8 +180,8 @@ void acpi_os_vprintf(const char *fmt, va_list args)
> > > #endif
> > > }
> > >
> > > -#ifdef CONFIG_KEXEC
> > > static unsigned long acpi_rsdp;
> > > +#ifdef CONFIG_KEXEC
> > > static int __init setup_acpi_rsdp(char *arg)
> > > {
> > > return kstrtoul(arg, 16, &acpi_rsdp);
> > > @@ -189,28 +189,38 @@ static int __init setup_acpi_rsdp(char *arg)
> > > early_param("acpi_rsdp", setup_acpi_rsdp);
> > > #endif
> > >
> > > +acpi_physical_address acpi_os_get_root_pointer_late(void) {
> > > + return acpi_rsdp;
> > > +}
> > > +
> > > acpi_physical_address __init acpi_os_get_root_pointer(void)
> > > {
> > > acpi_physical_address pa;
> > >
> > > -#ifdef CONFIG_KEXEC
> > > if (acpi_rsdp)
> > > return acpi_rsdp;
> > > -#endif
> > > +
> > > pa = acpi_arch_get_root_pointer();
> > > - if (pa)
> > > + if (pa) {
> > > + acpi_rsdp = pa;
> > > return pa;
> > > + }
> > >
> > > if (efi_enabled(EFI_CONFIG_TABLES)) {
> > > - if (efi.acpi20 != EFI_INVALID_TABLE_ADDR)
> > > + if (efi.acpi20 != EFI_INVALID_TABLE_ADDR) {
> > > + acpi_rsdp = efi.acpi20;
> > > return efi.acpi20;
> > > - if (efi.acpi != EFI_INVALID_TABLE_ADDR)
> > > + }
> > > + if (efi.acpi != EFI_INVALID_TABLE_ADDR) {
> > > + acpi_rsdp = efi.acpi;
> > > return efi.acpi;
> > > + }
> > > pr_err(PREFIX "System description tables not found\n");
> > > } else if (IS_ENABLED(CONFIG_ACPI_LEGACY_TABLES_LOOKUP)) {
> > > acpi_find_root_pointer(&pa);
> > > }
> > >
> > > + acpi_rsdp = pa;
> > > return pa;
> > > }
> > >
> > > > --
> > > > Regards/Gruss,
> > > > Boris.
> > > >
> > > > Good mailing practices for 400: avoid top-posting and trim the reply.
> > > --
> > > Best Regards,
> > > Kairui Song
--
Best Regards,
Kairui Song
On Fri, Jan 18, 2019 at 5:47 AM Kairui Song <[email protected]> wrote:
>
> On Thu, Jan 17, 2019 at 5:40 PM Rafael J. Wysocki <[email protected]> wrote:
> >
> > On Thu, Jan 17, 2019 at 9:53 AM Dave Young <[email protected]> wrote:
> > >
> > > Add linux-acpi list
> >
> > Well, thanks, but please resend the patches with a CC to linux-acpi.
> >
>
> Hi, sure will do.
> Any thought on adding an acpi_os_get_root_pointer_late and store rsdp
> pointer as mentioned?
It is a good idea in general IMO, but all depends on how the code
changes will look like.
> Will updat the patch and post V2, and cc linux-acpi as well later.
Thanks!
On Thu, Jan 17, 2019 at 03:41:13PM +0800, Kairui Song wrote:
> How about we refill the boot_params.acpi_rsdp_addr if it is not valid
> in early code, so it could be used as a reliable RSDP address source?
> That should make things easier.
>
> But if early code should parse it and store it should be done in
> Chao's patch, or I can post another patch to do it if Chao's patch is
> merged.
Chao's stuff does the as early as possible parsing of RDSP. Then, it
should be saved into boot_params and everything else should read it from
there. Simple.
--
Regards/Gruss,
Boris.
Good mailing practices for 400: avoid top-posting and trim the reply.
On Fri, Jan 18, 2019 at 11:26:36AM +0100, Borislav Petkov wrote:
>On Thu, Jan 17, 2019 at 03:41:13PM +0800, Kairui Song wrote:
>> How about we refill the boot_params.acpi_rsdp_addr if it is not valid
>> in early code, so it could be used as a reliable RSDP address source?
>> That should make things easier.
>>
>> But if early code should parse it and store it should be done in
>> Chao's patch, or I can post another patch to do it if Chao's patch is
>> merged.
>
>Chao's stuff does the as early as possible parsing of RDSP. Then, it
>should be saved into boot_params and everything else should read it from
>there. Simple.
Hi Boris,
So I have changed as this method and put in my mail thread, you may not
notice, so I put here for my function if I need to fill the
boot_parameters:
static inline acpi_physical_address get_boot_params_rsdp(void)
{
return boot_params->acpi_rsdp_addr;
}
static acpi_physical_address get_rsdp_addr(void)
{
bool boot_params_rsdp_exist;
acpi_physical_address pa;
pa = get_acpi_rsdp();
if (!pa)
pa = get_boot_params_rsdp();
if (!pa) {
pa = efi_get_rsdp_addr();
boot_params_rsdp_exist = false;
}
else
boot_params_rsdp_exist = true;
if (!pa)
pa = bios_get_rsdp_addr();
if (pa && !boot_params_rsdp_exist)
boot_params.acpi_rsdp_addr = pa;
return pa;
}
At the same time, I notice kernel only parses it when
"#ifdef CONFIG_ACPI", we should keep sync with kernel, but I think
we are parsing SRAT, CONFIG_ACPI is needed sure, so I am going to
update the define of EARLY_SRAT_PARSE:
config EARLY_SRAT_PARSE
bool "EARLY SRAT parsing"
def_bool y
depends on RANDOMIZE_BASE && MEMORY_HOTREMOVE && ACPI
If I miss something, please let me know.
Or in my PATCHSET, I don't need to fill boot_parameters, just
leave the job another PATCH?
Thanks,
Chao Fan
>
>--
>Regards/Gruss,
> Boris.
>
>Good mailing practices for 400: avoid top-posting and trim the reply.
>
>
On Mon, Jan 21, 2019 at 09:18:30AM +0800, Chao Fan wrote:
> So I have changed as this method and put in my mail thread, you may not
> notice, so I put here for my function if I need to fill the
> boot_parameters:
>
> static inline acpi_physical_address get_boot_params_rsdp(void)
> {
> return boot_params->acpi_rsdp_addr;
> }
Why do you need that silly wrapper?
> static acpi_physical_address get_rsdp_addr(void)
> {
> bool boot_params_rsdp_exist;
What's that bool supposed to do?
> acpi_physical_address pa;
>
> pa = get_acpi_rsdp();
>
> if (!pa)
> pa = get_boot_params_rsdp();
>
> if (!pa) {
> pa = efi_get_rsdp_addr();
> boot_params_rsdp_exist = false;
> }
> else
> boot_params_rsdp_exist = true;
>
> if (!pa)
> pa = bios_get_rsdp_addr();
>
> if (pa && !boot_params_rsdp_exist)
> boot_params.acpi_rsdp_addr = pa;
>
> return pa;
> }
>
> At the same time, I notice kernel only parses it when
> "#ifdef CONFIG_ACPI", we should keep sync with kernel, but I think
> we are parsing SRAT, CONFIG_ACPI is needed sure, so I am going to
> update the define of EARLY_SRAT_PARSE:
>
> config EARLY_SRAT_PARSE
> bool "EARLY SRAT parsing"
> def_bool y
> depends on RANDOMIZE_BASE && MEMORY_HOTREMOVE && ACPI
Actually, you don't need that anymore - make it unconditionally
built-in. Because there are a bunch of users which need this and instead
of complicating this config option with a bunch of dependencies, we can
just as well have it always on.
Thx.
--
Regards/Gruss,
Boris.
Good mailing practices for 400: avoid top-posting and trim the reply.
On Mon, Jan 21, 2019 at 09:29:32AM +0100, Borislav Petkov wrote:
>On Mon, Jan 21, 2019 at 09:18:30AM +0800, Chao Fan wrote:
>> So I have changed as this method and put in my mail thread, you may not
>> notice, so I put here for my function if I need to fill the
>> boot_parameters:
>>
>> static inline acpi_physical_address get_boot_params_rsdp(void)
>> {
>> return boot_params->acpi_rsdp_addr;
>> }
>
>Why do you need that silly wrapper?
Will drop it.
>
>> static acpi_physical_address get_rsdp_addr(void)
>> {
>> bool boot_params_rsdp_exist;
>
>What's that bool supposed to do?
Since I didn't see where Xen to fill the value, if
boot_params->acpi_rsdp_addr is filled before my code, I just need to
read it. If when I try to read it but not found, then parse RSDP and
fill the RSDP address to boot_params->acpi_rsdp_addr.
>
>> acpi_physical_address pa;
>>
>> pa = get_acpi_rsdp();
>>
>> if (!pa)
>> pa = get_boot_params_rsdp();
>>
>> if (!pa) {
>> pa = efi_get_rsdp_addr();
>> boot_params_rsdp_exist = false;
>> }
>> else
>> boot_params_rsdp_exist = true;
>>
>> if (!pa)
>> pa = bios_get_rsdp_addr();
>>
>> if (pa && !boot_params_rsdp_exist)
>> boot_params.acpi_rsdp_addr = pa;
>>
>> return pa;
>> }
>>
>> At the same time, I notice kernel only parses it when
>> "#ifdef CONFIG_ACPI", we should keep sync with kernel, but I think
>> we are parsing SRAT, CONFIG_ACPI is needed sure, so I am going to
>> update the define of EARLY_SRAT_PARSE:
>>
>> config EARLY_SRAT_PARSE
>> bool "EARLY SRAT parsing"
>> def_bool y
>> depends on RANDOMIZE_BASE && MEMORY_HOTREMOVE && ACPI
>
>Actually, you don't need that anymore - make it unconditionally
>built-in. Because there are a bunch of users which need this and instead
>of complicating this config option with a bunch of dependencies, we can
>just as well have it always on.
If I need to fill boot_params->acpi_rsdp_addr, I think we can make it
unconditionally built-in.
Thanks,
Chao Fan
>
>Thx.
>
>--
>Regards/Gruss,
> Boris.
>
>Good mailing practices for 400: avoid top-posting and trim the reply.
>
>
On Mon, Jan 21, 2019 at 04:43:52PM +0800, Chao Fan wrote:
> Since I didn't see where Xen to fill the value, if
> boot_params->acpi_rsdp_addr is filled before my code, I just need to
> read it. If when I try to read it but not found, then parse RSDP and
> fill the RSDP address to boot_params->acpi_rsdp_addr.
And you can't simply do:
if (boot_params.acpi_rsdp_addr)
return;
at the beginning of that function?
--
Regards/Gruss,
Boris.
Good mailing practices for 400: avoid top-posting and trim the reply.
On Mon, Jan 21, 2019 at 09:29:32AM +0100, Borislav Petkov wrote:
>On Mon, Jan 21, 2019 at 09:18:30AM +0800, Chao Fan wrote:
>> So I have changed as this method and put in my mail thread, you may not
>> notice, so I put here for my function if I need to fill the
>> boot_parameters:
>>
>> static inline acpi_physical_address get_boot_params_rsdp(void)
>> {
>> return boot_params->acpi_rsdp_addr;
>> }
>
>Why do you need that silly wrapper?
>
>> static acpi_physical_address get_rsdp_addr(void)
>> {
>> bool boot_params_rsdp_exist;
>
>What's that bool supposed to do?
>
>> acpi_physical_address pa;
>>
>> pa = get_acpi_rsdp();
>>
>> if (!pa)
>> pa = get_boot_params_rsdp();
>>
>> if (!pa) {
>> pa = efi_get_rsdp_addr();
>> boot_params_rsdp_exist = false;
>> }
>> else
>> boot_params_rsdp_exist = true;
>>
>> if (!pa)
>> pa = bios_get_rsdp_addr();
>>
>> if (pa && !boot_params_rsdp_exist)
>> boot_params.acpi_rsdp_addr = pa;
>>
>> return pa;
>> }
>>
>> At the same time, I notice kernel only parses it when
>> "#ifdef CONFIG_ACPI", we should keep sync with kernel, but I think
>> we are parsing SRAT, CONFIG_ACPI is needed sure, so I am going to
>> update the define of EARLY_SRAT_PARSE:
>>
>> config EARLY_SRAT_PARSE
>> bool "EARLY SRAT parsing"
>> def_bool y
>> depends on RANDOMIZE_BASE && MEMORY_HOTREMOVE && ACPI
>
>Actually, you don't need that anymore - make it unconditionally
>built-in. Because there are a bunch of users which need this and instead
>of complicating this config option with a bunch of dependencies, we can
>just as well have it always on.
According to your reply, I change it as:
vmlinux-objs-y += $(obj)/acpi.o
But I notice the only function call entry is in kaslr.c which needs
RANDOMIZE_BASE, so do I need change it as:
vmlinux-objs-$(CONFIG_RANDOMIZE_BASE) += $(obj)/acpi.o
Thanks,
Chao Fan
>
>Thx.
>
>--
>Regards/Gruss,
> Boris.
>
>Good mailing practices for 400: avoid top-posting and trim the reply.
>
>
On Tue, Jan 22, 2019 at 11:32:41AM +0800, Chao Fan wrote:
> But I notice the only function call entry is in kaslr.c which needs
> RANDOMIZE_BASE, so do I need change it as:
> vmlinux-objs-$(CONFIG_RANDOMIZE_BASE) += $(obj)/acpi.o
Well, the very first patch in this thread doesn't have anything to do
with CONFIG_RANDOMIZE_BASE but wants RDSP too.
--
Regards/Gruss,
Boris.
Good mailing practices for 400: avoid top-posting and trim the reply.