make kexec work with efi=noruntime or efi=old_map

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(-)

arch/x86/kernel/kexec-bzimage64.c +11 -0

index 53917a3ebf94..0a90dcbd041f 100644
@@ -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;

Quoted 49 lines · Kairui Song

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(-)

arch/x86/kernel/kexec-bzimage64.c +11 -0

index 53917a3ebf94..0a90dcbd041f 100644
@@ -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;

arch/x86/kernel/kexec-bzimage64.c setup_boot_parameters(struct kimage *image, struct boot_params *params, · line 278

Quoted 10 lines

arch/x86/kernel/kexec-bzimage64.c +9 -0

+				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(&params->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.

Signature · 4 lines

Regards/Gruss,
    Boris.

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

Quoted 64 lines · Borislav Petkov

Quoted 49 lines · Kairui Song

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(-)

arch/x86/kernel/kexec-bzimage64.c +11 -0

index 53917a3ebf94..0a90dcbd041f 100644
@@ -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;

arch/x86/kernel/kexec-bzimage64.c setup_boot_parameters(struct kimage *image, struct boot_params *params, · line 278

Quoted 10 lines

arch/x86/kernel/kexec-bzimage64.c +9 -0

+                           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(&params->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...

Quoted 6 lines

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?

Quoted 4 lines

Thx.

Quoted 5 lines

--
Regards/Gruss,
Boris.

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

Thanks
Dave

Quoted 73 lines · Dave Young

Quoted 64 lines · Borislav Petkov

Quoted 49 lines · Kairui Song

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(-)

arch/x86/kernel/kexec-bzimage64.c +11 -0

index 53917a3ebf94..0a90dcbd041f 100644
@@ -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;

arch/x86/kernel/kexec-bzimage64.c setup_boot_parameters(struct kimage *image, struct boot_params *params, · line 278

Quoted 10 lines

arch/x86/kernel/kexec-bzimage64.c +9 -0

+                           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(&params->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...

Quoted 6 lines

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.

Quoted 7 lines

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.

Quoted 86 lines · Kairui Song

Quoted 70 lines · Dave Young

Quoted 63 lines · Borislav Petkov

Quoted 49 lines · Kairui Song

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(-)

arch/x86/kernel/kexec-bzimage64.c +11 -0

index 53917a3ebf94..0a90dcbd041f 100644
@@ -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;

arch/x86/kernel/kexec-bzimage64.c setup_boot_parameters(struct kimage *image, struct boot_params *params, · line 278

Quoted 10 lines

arch/x86/kernel/kexec-bzimage64.c +9 -0

+                           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(&params->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...

Quoted 6 lines

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.

Quoted 7 lines

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.

Quoted 4 lines

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.

Quoted 9 lines

arch/x86/kernel/kexec-bzimage64.c +9 -0

+#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;

arch/x86/kernel/kexec-bzimage64.c setup_boot_parameters(struct kimage *image, struct boot_params *params, · line 278

Quoted 10 lines

arch/x86/kernel/kexec-bzimage64.c +9 -0

+                             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(&params->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.

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.

Quoted 6 lines · Kairui Song

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.

Quoted 9 lines · Borislav Petkov

Quoted 6 lines · Kairui Song

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.

Quoted 4 lines

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:

drivers/acpi/osl.c +16 -6

@@ -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;
 }
 

Quoted 4 lines

--
Regards/Gruss,
Boris.

Quoted 1 line

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

--
Best Regards,
Kairui Song

Quoted 10 lines · Kairui Song

Quoted 9 lines · Borislav Petkov

Quoted 6 lines · Kairui Song

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.

Quoted 8 lines

Quoted 4 lines

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

Quoted 10 lines · Kairui Song

Quoted 9 lines · Borislav Petkov

Quoted 6 lines · Kairui Song

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.

Quoted 8 lines

Quoted 4 lines

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

Quoted 76 lines

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:

drivers/acpi/osl.c +16 -6

@@ -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;
 }
 

Quoted 5 lines

--
Regards/Gruss,
Boris.

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

--
Best Regards,
Kairui Song

Quoted 9 lines · Borislav Petkov

Quoted 6 lines · Kairui Song

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.

Quoted 4 lines

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:

drivers/acpi/osl.c +15 -6

@@ -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;
 }

Quoted 6 lines

--
Regards/Gruss,
Boris.

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

--
Best Regards,
Kairui Song

Add linux-acpi list

Quoted 92 lines · Borislav Petkov

Quoted 9 lines · Kairui Song

Quoted 6 lines

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.

Quoted 4 lines

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:

drivers/acpi/osl.c +15 -6

@@ -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;
 }

Quoted 6 lines

--
Regards/Gruss,
Boris.

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

--
Best Regards,
Kairui Song

Quoted 2 lines · Dave Young

Add linux-acpi list

Well, thanks, but please resend the patches with a CC to linux-acpi.

Quoted 93 lines · Kairui Song

Quoted 92 lines · Borislav Petkov

Quoted 9 lines · Kairui Song

Quoted 6 lines

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.

Quoted 4 lines

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:

drivers/acpi/osl.c +15 -6

@@ -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;
 }

Quoted 6 lines

--
Regards/Gruss,
Boris.

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

--
Best Regards,
Kairui Song

Quoted 3 lines · Rafael J. Wysocki

Quoted 1 line · Dave Young

Quoted 4 lines

Quoted 1 line

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.

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.

Quoted 4 lines · Kairui Song

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.

Quoted 3 lines

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.

Signature · 5 lines

-- 
Regards/Gruss,
   Boris.

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

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

Quoted 8 lines · Chao Fan

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?

Quoted 4 lines

static acpi_physical_address get_rsdp_addr(void)

{
        bool boot_params_rsdp_exist;

What's that bool supposed to do?

Quoted 29 lines

        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:

Quoted 4 lines

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.

Signature · 4 lines

Regards/Gruss,
   Boris.

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

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.

Quoted 8 lines · Chao Fan

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?

Quoted 3 lines

static acpi_physical_address get_rsdp_addr(void)

{
        bool boot_params_rsdp_exist;

What's that bool supposed to do?

Quoted 28 lines

        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:

Quoted 4 lines

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.

Signature · 4 lines

Regards/Gruss,
   Boris.

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

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