* [patch 1/4] Add function get_bootparam
2013-10-27 4:04 [patch 0/4] kexec-tools: efi runtime support on kexec kernel dyoung
@ 2013-10-27 4:04 ` dyoung
2013-10-28 0:38 ` Simon Horman
2013-10-27 4:04 ` [patch 2/4] remove extra acpi_rsdp command line for efi dyoung
` (2 subsequent siblings)
3 siblings, 1 reply; 38+ messages in thread
From: dyoung @ 2013-10-27 4:04 UTC (permalink / raw)
To: kexec
Cc: mjg59, James.Bottomley, horms, kexec, ebiederm, hpa, bp,
Dave Young, vgoyal
[-- Attachment #1: 01-kexec-add-function-get-boot-param.patch --]
[-- Type: text/plain, Size: 1809 bytes --]
Not only setup_subarch will get data from debugfs file
boot_params/data, later code for adding efi_info will
also need do same thing. Thus add a common function here
for later use.
Signed-off-by: Dave Young <dyoung@redhat.com>
---
kexec/arch/i386/x86-linux-setup.c | 14 ++++++++++----
1 file changed, 10 insertions(+), 4 deletions(-)
--- kexec-tools.orig/kexec/arch/i386/x86-linux-setup.c
+++ kexec-tools/kexec/arch/i386/x86-linux-setup.c
@@ -436,10 +436,9 @@ char *find_mnt_by_fsname(char *fsname)
return mntdir;
}
-void setup_subarch(struct x86_linux_param_header *real_mode)
+void get_bootparam(void *buf, off_t offset, size_t size)
{
int data_file;
- const off_t offset = offsetof(typeof(*real_mode), hardware_subarch);
char *debugfs_mnt;
char filename[PATH_MAX];
@@ -447,7 +446,7 @@ void setup_subarch(struct x86_linux_para
if (!debugfs_mnt)
return;
snprintf(filename, PATH_MAX, "%s/%s", debugfs_mnt, "boot_params/data");
- filename[PATH_MAX-1] = 0;
+ filename[PATH_MAX - 1] = 0;
free(debugfs_mnt);
data_file = open(filename, O_RDONLY);
@@ -455,11 +454,18 @@ void setup_subarch(struct x86_linux_para
return;
if (lseek(data_file, offset, SEEK_SET) < 0)
goto close;
- read(data_file, &real_mode->hardware_subarch, sizeof(uint32_t));
+ read(data_file, buf, size);
close:
close(data_file);
}
+void setup_subarch(struct x86_linux_param_header *real_mode)
+{
+ off_t offset = offsetof(typeof(*real_mode), hardware_subarch);
+
+ get_bootparam(&real_mode->hardware_subarch, offset, sizeof(uint32_t));
+}
+
void setup_linux_system_parameters(struct kexec_info *info,
struct x86_linux_param_header *real_mode)
{
_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec
^ permalink raw reply [flat|nested] 38+ messages in thread* Re: [patch 1/4] Add function get_bootparam
2013-10-27 4:04 ` [patch 1/4] Add function get_bootparam dyoung
@ 2013-10-28 0:38 ` Simon Horman
2013-10-28 1:13 ` Dave Young
0 siblings, 1 reply; 38+ messages in thread
From: Simon Horman @ 2013-10-28 0:38 UTC (permalink / raw)
To: dyoung; +Cc: mjg59, kexec, vgoyal, James.Bottomley, bp, ebiederm, hpa, kexec
On Sun, Oct 27, 2013 at 12:04:38PM +0800, dyoung@redhat.com wrote:
> Not only setup_subarch will get data from debugfs file
> boot_params/data, later code for adding efi_info will
> also need do same thing. Thus add a common function here
> for later use.
>
> Signed-off-by: Dave Young <dyoung@redhat.com>
> ---
> kexec/arch/i386/x86-linux-setup.c | 14 ++++++++++----
> 1 file changed, 10 insertions(+), 4 deletions(-)
>
> --- kexec-tools.orig/kexec/arch/i386/x86-linux-setup.c
> +++ kexec-tools/kexec/arch/i386/x86-linux-setup.c
> @@ -436,10 +436,9 @@ char *find_mnt_by_fsname(char *fsname)
> return mntdir;
> }
>
> -void setup_subarch(struct x86_linux_param_header *real_mode)
> +void get_bootparam(void *buf, off_t offset, size_t size)
> {
> int data_file;
> - const off_t offset = offsetof(typeof(*real_mode), hardware_subarch);
> char *debugfs_mnt;
> char filename[PATH_MAX];
>
> @@ -447,7 +446,7 @@ void setup_subarch(struct x86_linux_para
> if (!debugfs_mnt)
> return;
> snprintf(filename, PATH_MAX, "%s/%s", debugfs_mnt, "boot_params/data");
> - filename[PATH_MAX-1] = 0;
> + filename[PATH_MAX - 1] = 0;
> free(debugfs_mnt);
This change appears to be unrelated to the rest of the patch.
>
> data_file = open(filename, O_RDONLY);
> @@ -455,11 +454,18 @@ void setup_subarch(struct x86_linux_para
> return;
> if (lseek(data_file, offset, SEEK_SET) < 0)
> goto close;
> - read(data_file, &real_mode->hardware_subarch, sizeof(uint32_t));
> + read(data_file, buf, size);
> close:
> close(data_file);
> }
>
> +void setup_subarch(struct x86_linux_param_header *real_mode)
> +{
> + off_t offset = offsetof(typeof(*real_mode), hardware_subarch);
> +
> + get_bootparam(&real_mode->hardware_subarch, offset, sizeof(uint32_t));
> +}
> +
> void setup_linux_system_parameters(struct kexec_info *info,
> struct x86_linux_param_header *real_mode)
> {
>
>
> _______________________________________________
> kexec mailing list
> kexec@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/kexec
>
_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec
^ permalink raw reply [flat|nested] 38+ messages in thread* Re: [patch 1/4] Add function get_bootparam
2013-10-28 0:38 ` Simon Horman
@ 2013-10-28 1:13 ` Dave Young
2013-10-28 2:30 ` Dave Young
0 siblings, 1 reply; 38+ messages in thread
From: Dave Young @ 2013-10-28 1:13 UTC (permalink / raw)
To: Simon Horman
Cc: mjg59, kexec, vgoyal, James.Bottomley, bp, ebiederm, hpa, kexec
> > @@ -447,7 +446,7 @@ void setup_subarch(struct x86_linux_para
> > if (!debugfs_mnt)
> > return;
> > snprintf(filename, PATH_MAX, "%s/%s", debugfs_mnt, "boot_params/data");
> > - filename[PATH_MAX-1] = 0;
> > + filename[PATH_MAX - 1] = 0;
> > free(debugfs_mnt);
>
> This change appears to be unrelated to the rest of the patch.
>
Will remove the change from the patch.
_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [patch 1/4] Add function get_bootparam
2013-10-28 1:13 ` Dave Young
@ 2013-10-28 2:30 ` Dave Young
2013-10-28 4:20 ` Simon Horman
0 siblings, 1 reply; 38+ messages in thread
From: Dave Young @ 2013-10-28 2:30 UTC (permalink / raw)
To: Simon Horman
Cc: mjg59, kexec, vgoyal, James.Bottomley, bp, ebiederm, hpa, kexec
On 10/28/13 at 09:13am, Dave Young wrote:
> > > @@ -447,7 +446,7 @@ void setup_subarch(struct x86_linux_para
> > > if (!debugfs_mnt)
> > > return;
> > > snprintf(filename, PATH_MAX, "%s/%s", debugfs_mnt, "boot_params/data");
> > > - filename[PATH_MAX-1] = 0;
> > > + filename[PATH_MAX - 1] = 0;
> > > free(debugfs_mnt);
> >
> > This change appears to be unrelated to the rest of the patch.
> >
>
> Will remove the change from the patch.
Relooking it, with this patch the line "filename[PATH_MAX - 1] = 0;"
is in the share function get_bootparam, it's not in setup_subarch
any more so I think it's ok, what do you think?
--
Thanks
Dave
_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [patch 1/4] Add function get_bootparam
2013-10-28 2:30 ` Dave Young
@ 2013-10-28 4:20 ` Simon Horman
2013-10-28 5:06 ` Dave Young
0 siblings, 1 reply; 38+ messages in thread
From: Simon Horman @ 2013-10-28 4:20 UTC (permalink / raw)
To: Dave Young
Cc: mjg59, kexec, vgoyal, James.Bottomley, bp, ebiederm, hpa, kexec
On Mon, Oct 28, 2013 at 10:30:32AM +0800, Dave Young wrote:
> On 10/28/13 at 09:13am, Dave Young wrote:
> > > > @@ -447,7 +446,7 @@ void setup_subarch(struct x86_linux_para
> > > > if (!debugfs_mnt)
> > > > return;
> > > > snprintf(filename, PATH_MAX, "%s/%s", debugfs_mnt, "boot_params/data");
> > > > - filename[PATH_MAX-1] = 0;
> > > > + filename[PATH_MAX - 1] = 0;
> > > > free(debugfs_mnt);
> > >
> > > This change appears to be unrelated to the rest of the patch.
> > >
> >
> > Will remove the change from the patch.
>
> Relooking it, with this patch the line "filename[PATH_MAX - 1] = 0;"
> is in the share function get_bootparam, it's not in setup_subarch
> any more so I think it's ok, what do you think?
I'm a bit confused.
My reading of the hunk above is that it is a whitespace-only change.
If so it is not a change that I object to but also not one that
I feel belongs in this patch. If not I am somehow mistaken.
_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [patch 1/4] Add function get_bootparam
2013-10-28 4:20 ` Simon Horman
@ 2013-10-28 5:06 ` Dave Young
2013-10-28 6:08 ` Simon Horman
0 siblings, 1 reply; 38+ messages in thread
From: Dave Young @ 2013-10-28 5:06 UTC (permalink / raw)
To: Simon Horman
Cc: mjg59, kexec, vgoyal, James.Bottomley, bp, ebiederm, hpa, kexec
On 10/28/13 at 01:20pm, Simon Horman wrote:
> On Mon, Oct 28, 2013 at 10:30:32AM +0800, Dave Young wrote:
> > On 10/28/13 at 09:13am, Dave Young wrote:
> > > > > @@ -447,7 +446,7 @@ void setup_subarch(struct x86_linux_para
> > > > > if (!debugfs_mnt)
> > > > > return;
> > > > > snprintf(filename, PATH_MAX, "%s/%s", debugfs_mnt, "boot_params/data");
> > > > > - filename[PATH_MAX-1] = 0;
> > > > > + filename[PATH_MAX - 1] = 0;
> > > > > free(debugfs_mnt);
> > > >
> > > > This change appears to be unrelated to the rest of the patch.
> > > >
> > >
> > > Will remove the change from the patch.
> >
> > Relooking it, with this patch the line "filename[PATH_MAX - 1] = 0;"
> > is in the share function get_bootparam, it's not in setup_subarch
> > any more so I think it's ok, what do you think?
>
> I'm a bit confused.
>
> My reading of the hunk above is that it is a whitespace-only change.
> If so it is not a change that I object to but also not one that
> I feel belongs in this patch. If not I am somehow mistaken.
The diff context is cheating us, it might because of the context (+- 3 lines)
is same with original setup_arch, but it is really moving to a new function.
_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [patch 1/4] Add function get_bootparam
2013-10-28 5:06 ` Dave Young
@ 2013-10-28 6:08 ` Simon Horman
0 siblings, 0 replies; 38+ messages in thread
From: Simon Horman @ 2013-10-28 6:08 UTC (permalink / raw)
To: Dave Young
Cc: mjg59, kexec, James.Bottomley, bp, ebiederm, hpa, kexec, vgoyal
On Mon, Oct 28, 2013 at 01:06:45PM +0800, Dave Young wrote:
> On 10/28/13 at 01:20pm, Simon Horman wrote:
> > On Mon, Oct 28, 2013 at 10:30:32AM +0800, Dave Young wrote:
> > > On 10/28/13 at 09:13am, Dave Young wrote:
> > > > > > @@ -447,7 +446,7 @@ void setup_subarch(struct x86_linux_para
> > > > > > if (!debugfs_mnt)
> > > > > > return;
> > > > > > snprintf(filename, PATH_MAX, "%s/%s", debugfs_mnt, "boot_params/data");
> > > > > > - filename[PATH_MAX-1] = 0;
> > > > > > + filename[PATH_MAX - 1] = 0;
> > > > > > free(debugfs_mnt);
> > > > >
> > > > > This change appears to be unrelated to the rest of the patch.
> > > > >
> > > >
> > > > Will remove the change from the patch.
> > >
> > > Relooking it, with this patch the line "filename[PATH_MAX - 1] = 0;"
> > > is in the share function get_bootparam, it's not in setup_subarch
> > > any more so I think it's ok, what do you think?
> >
> > I'm a bit confused.
> >
> > My reading of the hunk above is that it is a whitespace-only change.
> > If so it is not a change that I object to but also not one that
> > I feel belongs in this patch. If not I am somehow mistaken.
>
> The diff context is cheating us, it might because of the context (+- 3 lines)
> is same with original setup_arch, but it is really moving to a new function.
Ok, in that case I have no objections.
_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec
^ permalink raw reply [flat|nested] 38+ messages in thread
* [patch 2/4] remove extra acpi_rsdp command line for efi
2013-10-27 4:04 [patch 0/4] kexec-tools: efi runtime support on kexec kernel dyoung
2013-10-27 4:04 ` [patch 1/4] Add function get_bootparam dyoung
@ 2013-10-27 4:04 ` dyoung
2013-10-28 0:39 ` Simon Horman
2013-10-28 9:42 ` Matthew Garrett
2013-10-27 4:04 ` [patch 3/4] Add efi_info in x86 setup header dyoung
2013-10-27 4:04 ` [patch 4/4] Passing efi related data via setup_data dyoung
3 siblings, 2 replies; 38+ messages in thread
From: dyoung @ 2013-10-27 4:04 UTC (permalink / raw)
To: kexec
Cc: mjg59, James.Bottomley, horms, kexec, ebiederm, hpa, bp,
Dave Young, vgoyal
[-- Attachment #1: 02-kexec-remove-cmdline-add-efi.patch --]
[-- Type: text/plain, Size: 2036 bytes --]
Removing code to pass acpi_rsdp because this
patch series will support efi runtime, it's not
necessary any more. EFI initialization code will
take the functionality.
Signed-off-by: Dave Young <dyoung@redhat.com>
---
include/x86/x86-linux.h | 3 ++-
kexec/arch/i386/crashdump-x86.c | 35 -----------------------------------
kexec/arch/i386/x86-linux-setup.c | 8 ++++++++
3 files changed, 10 insertions(+), 36 deletions(-)
--- kexec-tools.orig/kexec/arch/i386/crashdump-x86.c
+++ kexec-tools/kexec/arch/i386/crashdump-x86.c
@@ -848,40 +848,6 @@ static int cmdline_add_memmap_acpi(char
return 0;
}
-/* Appends 'acpi_rsdp=' commandline for efi boot crash dump */
-static void cmdline_add_efi(char *cmdline)
-{
- FILE *fp;
- int cmdlen, len;
- char line[MAX_LINE], *s;
- const char *acpis = " acpi_rsdp=";
-
- fp = fopen("/sys/firmware/efi/systab", "r");
- if (!fp)
- return;
-
- while(fgets(line, sizeof(line), fp) != 0) {
- /* ACPI20= always goes before ACPI= */
- if ((strstr(line, "ACPI20=")) || (strstr(line, "ACPI="))) {
- line[strlen(line) - 1] = '\0';
- s = strchr(line, '=');
- s += 1;
- len = strlen(s) + strlen(acpis);
- cmdlen = strlen(cmdline) + len;
- if (cmdlen > (COMMAND_LINE_SIZE - 1))
- die("Command line overflow\n");
- strcat(cmdline, acpis);
- strcat(cmdline, s);
- dbgprintf("Command line after adding efi\n");
- dbgprintf("%s\n", cmdline);
-
- break;
- }
- }
-
- fclose(fp);
-}
-
static void get_backup_area(struct kexec_info *info,
struct memory_range *range, int ranges)
{
@@ -1046,7 +1012,6 @@ int load_crashdump_segments(struct kexec
if (delete_memmap(memmap_p, elfcorehdr, memsz) < 0)
return -1;
cmdline_add_memmap(mod_cmdline, memmap_p);
- cmdline_add_efi(mod_cmdline);
cmdline_add_elfcorehdr(mod_cmdline, elfcorehdr);
/* Inform second kernel about the presence of ACPI tables. */
_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec
^ permalink raw reply [flat|nested] 38+ messages in thread* Re: [patch 2/4] remove extra acpi_rsdp command line for efi
2013-10-27 4:04 ` [patch 2/4] remove extra acpi_rsdp command line for efi dyoung
@ 2013-10-28 0:39 ` Simon Horman
2013-10-28 1:39 ` Dave Young
2013-10-28 9:42 ` Matthew Garrett
1 sibling, 1 reply; 38+ messages in thread
From: Simon Horman @ 2013-10-28 0:39 UTC (permalink / raw)
To: dyoung; +Cc: mjg59, kexec, James.Bottomley, kexec, ebiederm, hpa, bp, vgoyal
On Sun, Oct 27, 2013 at 12:04:39PM +0800, dyoung@redhat.com wrote:
> Removing code to pass acpi_rsdp because this
> patch series will support efi runtime, it's not
> necessary any more. EFI initialization code will
> take the functionality.
I wonder if it would make more sense to make this the
last patch in the series or squash it into the last patch
of the series.
>
> Signed-off-by: Dave Young <dyoung@redhat.com>
> ---
> include/x86/x86-linux.h | 3 ++-
> kexec/arch/i386/crashdump-x86.c | 35 -----------------------------------
> kexec/arch/i386/x86-linux-setup.c | 8 ++++++++
> 3 files changed, 10 insertions(+), 36 deletions(-)
>
> --- kexec-tools.orig/kexec/arch/i386/crashdump-x86.c
> +++ kexec-tools/kexec/arch/i386/crashdump-x86.c
> @@ -848,40 +848,6 @@ static int cmdline_add_memmap_acpi(char
> return 0;
> }
>
> -/* Appends 'acpi_rsdp=' commandline for efi boot crash dump */
> -static void cmdline_add_efi(char *cmdline)
> -{
> - FILE *fp;
> - int cmdlen, len;
> - char line[MAX_LINE], *s;
> - const char *acpis = " acpi_rsdp=";
> -
> - fp = fopen("/sys/firmware/efi/systab", "r");
> - if (!fp)
> - return;
> -
> - while(fgets(line, sizeof(line), fp) != 0) {
> - /* ACPI20= always goes before ACPI= */
> - if ((strstr(line, "ACPI20=")) || (strstr(line, "ACPI="))) {
> - line[strlen(line) - 1] = '\0';
> - s = strchr(line, '=');
> - s += 1;
> - len = strlen(s) + strlen(acpis);
> - cmdlen = strlen(cmdline) + len;
> - if (cmdlen > (COMMAND_LINE_SIZE - 1))
> - die("Command line overflow\n");
> - strcat(cmdline, acpis);
> - strcat(cmdline, s);
> - dbgprintf("Command line after adding efi\n");
> - dbgprintf("%s\n", cmdline);
> -
> - break;
> - }
> - }
> -
> - fclose(fp);
> -}
> -
> static void get_backup_area(struct kexec_info *info,
> struct memory_range *range, int ranges)
> {
> @@ -1046,7 +1012,6 @@ int load_crashdump_segments(struct kexec
> if (delete_memmap(memmap_p, elfcorehdr, memsz) < 0)
> return -1;
> cmdline_add_memmap(mod_cmdline, memmap_p);
> - cmdline_add_efi(mod_cmdline);
> cmdline_add_elfcorehdr(mod_cmdline, elfcorehdr);
>
> /* Inform second kernel about the presence of ACPI tables. */
>
_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec
^ permalink raw reply [flat|nested] 38+ messages in thread* Re: [patch 2/4] remove extra acpi_rsdp command line for efi
2013-10-28 0:39 ` Simon Horman
@ 2013-10-28 1:39 ` Dave Young
0 siblings, 0 replies; 38+ messages in thread
From: Dave Young @ 2013-10-28 1:39 UTC (permalink / raw)
To: Simon Horman
Cc: mjg59, kexec, James.Bottomley, kexec, ebiederm, hpa, bp, vgoyal
On 10/28/13 at 09:39am, Simon Horman wrote:
> On Sun, Oct 27, 2013 at 12:04:39PM +0800, dyoung@redhat.com wrote:
> > Removing code to pass acpi_rsdp because this
> > patch series will support efi runtime, it's not
> > necessary any more. EFI initialization code will
> > take the functionality.
>
> I wonder if it would make more sense to make this the
> last patch in the series or squash it into the last patch
> of the series.
>
Sure, logiclly it should be the last one.
--
Thanks
Dave
_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [patch 2/4] remove extra acpi_rsdp command line for efi
2013-10-27 4:04 ` [patch 2/4] remove extra acpi_rsdp command line for efi dyoung
2013-10-28 0:39 ` Simon Horman
@ 2013-10-28 9:42 ` Matthew Garrett
2013-10-28 9:54 ` Dave Young
1 sibling, 1 reply; 38+ messages in thread
From: Matthew Garrett @ 2013-10-28 9:42 UTC (permalink / raw)
To: dyoung; +Cc: kexec, James.Bottomley, horms, kexec, ebiederm, hpa, bp, vgoyal
On Sun, Oct 27, 2013 at 12:04:39PM +0800, dyoung@redhat.com wrote:
> Removing code to pass acpi_rsdp because this
> patch series will support efi runtime, it's not
> necessary any more. EFI initialization code will
> take the functionality.
Won't this break kexec of old kernels that don't have the new UEFI setup
code?
--
Matthew Garrett | mjg59@srcf.ucam.org
_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [patch 2/4] remove extra acpi_rsdp command line for efi
2013-10-28 9:42 ` Matthew Garrett
@ 2013-10-28 9:54 ` Dave Young
2013-10-28 10:12 ` Matthew Garrett
0 siblings, 1 reply; 38+ messages in thread
From: Dave Young @ 2013-10-28 9:54 UTC (permalink / raw)
To: Matthew Garrett
Cc: kexec, James.Bottomley, horms, kexec, ebiederm, hpa, bp, vgoyal
Hi,
Thanks for review.
On 10/28/13 at 09:42am, Matthew Garrett wrote:
> On Sun, Oct 27, 2013 at 12:04:39PM +0800, dyoung@redhat.com wrote:
> > Removing code to pass acpi_rsdp because this
> > patch series will support efi runtime, it's not
> > necessary any more. EFI initialization code will
> > take the functionality.
>
> Won't this break kexec of old kernels that don't have the new UEFI setup
> code?
For old kernels the efi setup_data esdata will be NULL, it wil switch to
old logic. Also for old kernel user need to pass acpi_rsdp in cmdline
Anyway I will do test with old/new kernel/userspace test results.
--
Thanks
Dave
_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [patch 2/4] remove extra acpi_rsdp command line for efi
2013-10-28 9:54 ` Dave Young
@ 2013-10-28 10:12 ` Matthew Garrett
2013-10-28 10:34 ` Dave Young
0 siblings, 1 reply; 38+ messages in thread
From: Matthew Garrett @ 2013-10-28 10:12 UTC (permalink / raw)
To: Dave Young
Cc: kexec, James.Bottomley, horms, kexec, ebiederm, hpa, bp, vgoyal
On Mon, Oct 28, 2013 at 05:54:59PM +0800, Dave Young wrote:
> Hi,
> Thanks for review.
>
> On 10/28/13 at 09:42am, Matthew Garrett wrote:
> > On Sun, Oct 27, 2013 at 12:04:39PM +0800, dyoung@redhat.com wrote:
> > > Removing code to pass acpi_rsdp because this
> > > patch series will support efi runtime, it's not
> > > necessary any more. EFI initialization code will
> > > take the functionality.
> >
> > Won't this break kexec of old kernels that don't have the new UEFI setup
> > code?
>
> For old kernels the efi setup_data esdata will be NULL, it wil switch to
> old logic. Also for old kernel user need to pass acpi_rsdp in cmdline
Right, but previously acpi_rsdp was passed automatically and now it
won't be?
--
Matthew Garrett | mjg59@srcf.ucam.org
_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [patch 2/4] remove extra acpi_rsdp command line for efi
2013-10-28 10:12 ` Matthew Garrett
@ 2013-10-28 10:34 ` Dave Young
2013-10-28 10:39 ` Matthew Garrett
0 siblings, 1 reply; 38+ messages in thread
From: Dave Young @ 2013-10-28 10:34 UTC (permalink / raw)
To: Matthew Garrett
Cc: kexec, James.Bottomley, horms, kexec, ebiederm, hpa, bp, vgoyal
On 10/28/13 at 10:12am, Matthew Garrett wrote:
> On Mon, Oct 28, 2013 at 05:54:59PM +0800, Dave Young wrote:
> > Hi,
> > Thanks for review.
> >
> > On 10/28/13 at 09:42am, Matthew Garrett wrote:
> > > On Sun, Oct 27, 2013 at 12:04:39PM +0800, dyoung@redhat.com wrote:
> > > > Removing code to pass acpi_rsdp because this
> > > > patch series will support efi runtime, it's not
> > > > necessary any more. EFI initialization code will
> > > > take the functionality.
> > >
> > > Won't this break kexec of old kernels that don't have the new UEFI setup
> > > code?
> >
> > For old kernels the efi setup_data esdata will be NULL, it wil switch to
> > old logic. Also for old kernel user need to pass acpi_rsdp in cmdline
>
> Right, but previously acpi_rsdp was passed automatically and now it
> won't be?
Yes, it was. I'm removing them in kexec-tools patches for efi runtime support.
Correct myself about esdata, for old kernels there's no setup_data type for
efi so parse_efi_setup will not be called, thus there will be no effect with
passing new setup_data because nobody use it?
_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [patch 2/4] remove extra acpi_rsdp command line for efi
2013-10-28 10:34 ` Dave Young
@ 2013-10-28 10:39 ` Matthew Garrett
2013-10-28 10:45 ` Dave Young
0 siblings, 1 reply; 38+ messages in thread
From: Matthew Garrett @ 2013-10-28 10:39 UTC (permalink / raw)
To: Dave Young
Cc: kexec, James.Bottomley, horms, kexec, ebiederm, hpa, bp, vgoyal
On Mon, Oct 28, 2013 at 06:34:12PM +0800, Dave Young wrote:
> On 10/28/13 at 10:12am, Matthew Garrett wrote:
> > Right, but previously acpi_rsdp was passed automatically and now it
> > won't be?
>
> Yes, it was. I'm removing them in kexec-tools patches for efi runtime support.
If I upgrade kexec-tools and try to launch an old kernel, I now need to
add an extra parameter?
--
Matthew Garrett | mjg59@srcf.ucam.org
_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [patch 2/4] remove extra acpi_rsdp command line for efi
2013-10-28 10:39 ` Matthew Garrett
@ 2013-10-28 10:45 ` Dave Young
2013-10-28 10:51 ` Matthew Garrett
0 siblings, 1 reply; 38+ messages in thread
From: Dave Young @ 2013-10-28 10:45 UTC (permalink / raw)
To: Matthew Garrett
Cc: kexec, James.Bottomley, horms, kexec, ebiederm, hpa, bp, vgoyal
On 10/28/13 at 10:39am, Matthew Garrett wrote:
> On Mon, Oct 28, 2013 at 06:34:12PM +0800, Dave Young wrote:
> > On 10/28/13 at 10:12am, Matthew Garrett wrote:
> > > Right, but previously acpi_rsdp was passed automatically and now it
> > > won't be?
> >
> > Yes, it was. I'm removing them in kexec-tools patches for efi runtime support.
>
> If I upgrade kexec-tools and try to launch an old kernel, I now need to
> add an extra parameter?
Yes, it should work by passing the acpi_rsdp= via --append
_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [patch 2/4] remove extra acpi_rsdp command line for efi
2013-10-28 10:45 ` Dave Young
@ 2013-10-28 10:51 ` Matthew Garrett
2013-10-28 10:53 ` Dave Young
2013-10-28 16:10 ` Vivek Goyal
0 siblings, 2 replies; 38+ messages in thread
From: Matthew Garrett @ 2013-10-28 10:51 UTC (permalink / raw)
To: Dave Young
Cc: kexec, James.Bottomley, horms, kexec, ebiederm, hpa, bp, vgoyal
On Mon, Oct 28, 2013 at 06:45:32PM +0800, Dave Young wrote:
> On 10/28/13 at 10:39am, Matthew Garrett wrote:
> > On Mon, Oct 28, 2013 at 06:34:12PM +0800, Dave Young wrote:
> > > On 10/28/13 at 10:12am, Matthew Garrett wrote:
> > > > Right, but previously acpi_rsdp was passed automatically and now it
> > > > won't be?
> > >
> > > Yes, it was. I'm removing them in kexec-tools patches for efi runtime support.
> >
> > If I upgrade kexec-tools and try to launch an old kernel, I now need to
> > add an extra parameter?
>
> Yes, it should work by passing the acpi_rsdp= via --append
Yes, that's my point. You're breaking old configurations by requiring
the user to pass an additional argument.
--
Matthew Garrett | mjg59@srcf.ucam.org
_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [patch 2/4] remove extra acpi_rsdp command line for efi
2013-10-28 10:51 ` Matthew Garrett
@ 2013-10-28 10:53 ` Dave Young
2013-10-29 3:05 ` Dave Young
2013-10-28 16:10 ` Vivek Goyal
1 sibling, 1 reply; 38+ messages in thread
From: Dave Young @ 2013-10-28 10:53 UTC (permalink / raw)
To: Matthew Garrett
Cc: kexec, James.Bottomley, horms, kexec, ebiederm, hpa, bp, vgoyal
On 10/28/13 at 10:51am, Matthew Garrett wrote:
> On Mon, Oct 28, 2013 at 06:45:32PM +0800, Dave Young wrote:
> > On 10/28/13 at 10:39am, Matthew Garrett wrote:
> > > On Mon, Oct 28, 2013 at 06:34:12PM +0800, Dave Young wrote:
> > > > On 10/28/13 at 10:12am, Matthew Garrett wrote:
> > > > > Right, but previously acpi_rsdp was passed automatically and now it
> > > > > won't be?
> > > >
> > > > Yes, it was. I'm removing them in kexec-tools patches for efi runtime support.
> > >
> > > If I upgrade kexec-tools and try to launch an old kernel, I now need to
> > > add an extra parameter?
> >
> > Yes, it should work by passing the acpi_rsdp= via --append
>
> Yes, that's my point. You're breaking old configurations by requiring
> the user to pass an additional argument.
Hmm, I will do some test without this patch, if it works well it's not
harmful to drop this one.
_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [patch 2/4] remove extra acpi_rsdp command line for efi
2013-10-28 10:53 ` Dave Young
@ 2013-10-29 3:05 ` Dave Young
2013-10-29 3:34 ` Dave Young
0 siblings, 1 reply; 38+ messages in thread
From: Dave Young @ 2013-10-29 3:05 UTC (permalink / raw)
To: Matthew Garrett
Cc: kexec, James.Bottomley, horms, kexec, ebiederm, hpa, bp, vgoyal
On 10/28/13 at 06:53pm, Dave Young wrote:
> On 10/28/13 at 10:51am, Matthew Garrett wrote:
> > On Mon, Oct 28, 2013 at 06:45:32PM +0800, Dave Young wrote:
> > > On 10/28/13 at 10:39am, Matthew Garrett wrote:
> > > > On Mon, Oct 28, 2013 at 06:34:12PM +0800, Dave Young wrote:
> > > > > On 10/28/13 at 10:12am, Matthew Garrett wrote:
> > > > > > Right, but previously acpi_rsdp was passed automatically and now it
> > > > > > won't be?
> > > > >
> > > > > Yes, it was. I'm removing them in kexec-tools patches for efi runtime support.
> > > >
> > > > If I upgrade kexec-tools and try to launch an old kernel, I now need to
> > > > add an extra parameter?
> > >
> > > Yes, it should work by passing the acpi_rsdp= via --append
> >
> > Yes, that's my point. You're breaking old configurations by requiring
> > the user to pass an additional argument.
>
> Hmm, I will do some test without this patch, if it works well it's not
> harmful to drop this one.
Removing this patch does not work because in this series efi_info is passed into
2nd kernel thus 2nd kernel will initilize efi but it does not have related code
to handle the converted talbe addresses and it will endter virtual mode again.
I will think about how to solve this problem.
_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [patch 2/4] remove extra acpi_rsdp command line for efi
2013-10-29 3:05 ` Dave Young
@ 2013-10-29 3:34 ` Dave Young
2013-10-29 8:50 ` Dave Young
0 siblings, 1 reply; 38+ messages in thread
From: Dave Young @ 2013-10-29 3:34 UTC (permalink / raw)
To: Matthew Garrett
Cc: kexec, James.Bottomley, horms, kexec, ebiederm, hpa, bp, vgoyal
On 10/29/13 at 11:05am, Dave Young wrote:
> On 10/28/13 at 06:53pm, Dave Young wrote:
> > On 10/28/13 at 10:51am, Matthew Garrett wrote:
> > > On Mon, Oct 28, 2013 at 06:45:32PM +0800, Dave Young wrote:
> > > > On 10/28/13 at 10:39am, Matthew Garrett wrote:
> > > > > On Mon, Oct 28, 2013 at 06:34:12PM +0800, Dave Young wrote:
> > > > > > On 10/28/13 at 10:12am, Matthew Garrett wrote:
> > > > > > > Right, but previously acpi_rsdp was passed automatically and now it
> > > > > > > won't be?
> > > > > >
> > > > > > Yes, it was. I'm removing them in kexec-tools patches for efi runtime support.
> > > > >
> > > > > If I upgrade kexec-tools and try to launch an old kernel, I now need to
> > > > > add an extra parameter?
> > > >
> > > > Yes, it should work by passing the acpi_rsdp= via --append
> > >
> > > Yes, that's my point. You're breaking old configurations by requiring
> > > the user to pass an additional argument.
> >
> > Hmm, I will do some test without this patch, if it works well it's not
> > harmful to drop this one.
>
> Removing this patch does not work because in this series efi_info is passed into
> 2nd kernel thus 2nd kernel will initilize efi but it does not have related code
> to handle the converted talbe addresses and it will endter virtual mode again.
>
> I will think about how to solve this problem.
>
The only way what I can think out is introduce a flag in x86 setup header.
Will try this way, the userspace code will use old logic if the kernel is old.
_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [patch 2/4] remove extra acpi_rsdp command line for efi
2013-10-29 3:34 ` Dave Young
@ 2013-10-29 8:50 ` Dave Young
2013-10-29 14:57 ` H. Peter Anvin
0 siblings, 1 reply; 38+ messages in thread
From: Dave Young @ 2013-10-29 8:50 UTC (permalink / raw)
To: Matthew Garrett
Cc: kexec, James.Bottomley, horms, kexec, ebiederm, hpa, bp, vgoyal
On 10/29/13 at 11:34am, Dave Young wrote:
> On 10/29/13 at 11:05am, Dave Young wrote:
> > On 10/28/13 at 06:53pm, Dave Young wrote:
> > > On 10/28/13 at 10:51am, Matthew Garrett wrote:
> > > > On Mon, Oct 28, 2013 at 06:45:32PM +0800, Dave Young wrote:
> > > > > On 10/28/13 at 10:39am, Matthew Garrett wrote:
> > > > > > On Mon, Oct 28, 2013 at 06:34:12PM +0800, Dave Young wrote:
> > > > > > > On 10/28/13 at 10:12am, Matthew Garrett wrote:
> > > > > > > > Right, but previously acpi_rsdp was passed automatically and now it
> > > > > > > > won't be?
> > > > > > >
> > > > > > > Yes, it was. I'm removing them in kexec-tools patches for efi runtime support.
> > > > > >
> > > > > > If I upgrade kexec-tools and try to launch an old kernel, I now need to
> > > > > > add an extra parameter?
> > > > >
> > > > > Yes, it should work by passing the acpi_rsdp= via --append
> > > >
> > > > Yes, that's my point. You're breaking old configurations by requiring
> > > > the user to pass an additional argument.
> > >
> > > Hmm, I will do some test without this patch, if it works well it's not
> > > harmful to drop this one.
> >
> > Removing this patch does not work because in this series efi_info is passed into
> > 2nd kernel thus 2nd kernel will initilize efi but it does not have related code
> > to handle the converted talbe addresses and it will endter virtual mode again.
> >
> > I will think about how to solve this problem.
> >
>
> The only way what I can think out is introduce a flag in x86 setup header.
> Will try this way, the userspace code will use old logic if the kernel is old.
Firstly I want to ask opinion from HPA, what do you think about adding a flag to
xloadflags for this purpose?
_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [patch 2/4] remove extra acpi_rsdp command line for efi
2013-10-29 8:50 ` Dave Young
@ 2013-10-29 14:57 ` H. Peter Anvin
0 siblings, 0 replies; 38+ messages in thread
From: H. Peter Anvin @ 2013-10-29 14:57 UTC (permalink / raw)
To: Dave Young, Matthew Garrett
Cc: kexec, vgoyal, James.Bottomley, horms, bp, ebiederm, kexec
Seems reasonable to me.
Dave Young <dyoung@redhat.com> wrote:
>On 10/29/13 at 11:34am, Dave Young wrote:
>> On 10/29/13 at 11:05am, Dave Young wrote:
>> > On 10/28/13 at 06:53pm, Dave Young wrote:
>> > > On 10/28/13 at 10:51am, Matthew Garrett wrote:
>> > > > On Mon, Oct 28, 2013 at 06:45:32PM +0800, Dave Young wrote:
>> > > > > On 10/28/13 at 10:39am, Matthew Garrett wrote:
>> > > > > > On Mon, Oct 28, 2013 at 06:34:12PM +0800, Dave Young wrote:
>> > > > > > > On 10/28/13 at 10:12am, Matthew Garrett wrote:
>> > > > > > > > Right, but previously acpi_rsdp was passed
>automatically and now it
>> > > > > > > > won't be?
>> > > > > > >
>> > > > > > > Yes, it was. I'm removing them in kexec-tools patches for
>efi runtime support.
>> > > > > >
>> > > > > > If I upgrade kexec-tools and try to launch an old kernel, I
>now need to
>> > > > > > add an extra parameter?
>> > > > >
>> > > > > Yes, it should work by passing the acpi_rsdp= via --append
>> > > >
>> > > > Yes, that's my point. You're breaking old configurations by
>requiring
>> > > > the user to pass an additional argument.
>> > >
>> > > Hmm, I will do some test without this patch, if it works well
>it's not
>> > > harmful to drop this one.
>> >
>> > Removing this patch does not work because in this series efi_info
>is passed into
>> > 2nd kernel thus 2nd kernel will initilize efi but it does not have
>related code
>> > to handle the converted talbe addresses and it will endter virtual
>mode again.
>> >
>> > I will think about how to solve this problem.
>> >
>>
>> The only way what I can think out is introduce a flag in x86 setup
>header.
>> Will try this way, the userspace code will use old logic if the
>kernel is old.
>
>Firstly I want to ask opinion from HPA, what do you think about adding
>a flag to
>xloadflags for this purpose?
--
Sent from my mobile phone. Please pardon brevity and lack of formatting.
_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [patch 2/4] remove extra acpi_rsdp command line for efi
2013-10-28 10:51 ` Matthew Garrett
2013-10-28 10:53 ` Dave Young
@ 2013-10-28 16:10 ` Vivek Goyal
2013-10-29 2:04 ` Simon Horman
1 sibling, 1 reply; 38+ messages in thread
From: Vivek Goyal @ 2013-10-28 16:10 UTC (permalink / raw)
To: Matthew Garrett
Cc: kexec, James.Bottomley, horms, bp, ebiederm, hpa, Dave Young,
kexec
On Mon, Oct 28, 2013 at 10:51:19AM +0000, Matthew Garrett wrote:
> On Mon, Oct 28, 2013 at 06:45:32PM +0800, Dave Young wrote:
> > On 10/28/13 at 10:39am, Matthew Garrett wrote:
> > > On Mon, Oct 28, 2013 at 06:34:12PM +0800, Dave Young wrote:
> > > > On 10/28/13 at 10:12am, Matthew Garrett wrote:
> > > > > Right, but previously acpi_rsdp was passed automatically and now it
> > > > > won't be?
> > > >
> > > > Yes, it was. I'm removing them in kexec-tools patches for efi runtime support.
> > >
> > > If I upgrade kexec-tools and try to launch an old kernel, I now need to
> > > add an extra parameter?
> >
> > Yes, it should work by passing the acpi_rsdp= via --append
>
> Yes, that's my point. You're breaking old configurations by requiring
> the user to pass an additional argument.
Yes this is a problem. Of course solution is easy by always passing
acpi_rsdp on command line. But in long term this is a problem. In the
sense, I am not sure how to cleanup the kexec-tools code as things improve.
Now we will support the EFI properly and still pass acpi_rsdp always in
an effort to matain backward compatibility.
For a very long time kexec-tools were not automatically appending
acpi_rsdp and user were supposed to add it on command line. We were
carrying this change in kdump scripts and pushed this change into
kexec-tools. In hindsight, it looks like that hardcoding parameters
in kexec-tools is a bad idea. It is hard to get rid of them in future.
Thanks
Vivek
_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [patch 2/4] remove extra acpi_rsdp command line for efi
2013-10-28 16:10 ` Vivek Goyal
@ 2013-10-29 2:04 ` Simon Horman
2013-10-29 2:36 ` Dave Young
2013-10-29 13:27 ` Vivek Goyal
0 siblings, 2 replies; 38+ messages in thread
From: Simon Horman @ 2013-10-29 2:04 UTC (permalink / raw)
To: Vivek Goyal
Cc: Matthew Garrett, kexec, James.Bottomley, bp, ebiederm, hpa,
Dave Young, kexec
On Mon, Oct 28, 2013 at 12:10:40PM -0400, Vivek Goyal wrote:
> On Mon, Oct 28, 2013 at 10:51:19AM +0000, Matthew Garrett wrote:
> > On Mon, Oct 28, 2013 at 06:45:32PM +0800, Dave Young wrote:
> > > On 10/28/13 at 10:39am, Matthew Garrett wrote:
> > > > On Mon, Oct 28, 2013 at 06:34:12PM +0800, Dave Young wrote:
> > > > > On 10/28/13 at 10:12am, Matthew Garrett wrote:
> > > > > > Right, but previously acpi_rsdp was passed automatically and now it
> > > > > > won't be?
> > > > >
> > > > > Yes, it was. I'm removing them in kexec-tools patches for efi runtime support.
> > > >
> > > > If I upgrade kexec-tools and try to launch an old kernel, I now need to
> > > > add an extra parameter?
> > >
> > > Yes, it should work by passing the acpi_rsdp= via --append
> >
> > Yes, that's my point. You're breaking old configurations by requiring
> > the user to pass an additional argument.
>
> Yes this is a problem. Of course solution is easy by always passing
> acpi_rsdp on command line. But in long term this is a problem. In the
> sense, I am not sure how to cleanup the kexec-tools code as things improve.
> Now we will support the EFI properly and still pass acpi_rsdp always in
> an effort to matain backward compatibility.
>
> For a very long time kexec-tools were not automatically appending
> acpi_rsdp and user were supposed to add it on command line. We were
> carrying this change in kdump scripts and pushed this change into
> kexec-tools. In hindsight, it looks like that hardcoding parameters
> in kexec-tools is a bad idea. It is hard to get rid of them in future.
I tend to agree. However, if the parameters end up being hard coded
elsewhere, for example in widely used wrapper scripts, then I think that
the same problem still exists. Just outside of kexec-tools itself.
_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [patch 2/4] remove extra acpi_rsdp command line for efi
2013-10-29 2:04 ` Simon Horman
@ 2013-10-29 2:36 ` Dave Young
2013-10-29 3:18 ` Dave Young
2013-10-30 0:45 ` Simon Horman
2013-10-29 13:27 ` Vivek Goyal
1 sibling, 2 replies; 38+ messages in thread
From: Dave Young @ 2013-10-29 2:36 UTC (permalink / raw)
To: Simon Horman
Cc: Matthew Garrett, kexec, James.Bottomley, kexec, ebiederm, hpa, bp,
Vivek Goyal
On 10/29/13 at 11:04am, Simon Horman wrote:
> On Mon, Oct 28, 2013 at 12:10:40PM -0400, Vivek Goyal wrote:
> > On Mon, Oct 28, 2013 at 10:51:19AM +0000, Matthew Garrett wrote:
> > > On Mon, Oct 28, 2013 at 06:45:32PM +0800, Dave Young wrote:
> > > > On 10/28/13 at 10:39am, Matthew Garrett wrote:
> > > > > On Mon, Oct 28, 2013 at 06:34:12PM +0800, Dave Young wrote:
> > > > > > On 10/28/13 at 10:12am, Matthew Garrett wrote:
> > > > > > > Right, but previously acpi_rsdp was passed automatically and now it
> > > > > > > won't be?
> > > > > >
> > > > > > Yes, it was. I'm removing them in kexec-tools patches for efi runtime support.
> > > > >
> > > > > If I upgrade kexec-tools and try to launch an old kernel, I now need to
> > > > > add an extra parameter?
> > > >
> > > > Yes, it should work by passing the acpi_rsdp= via --append
> > >
> > > Yes, that's my point. You're breaking old configurations by requiring
> > > the user to pass an additional argument.
> >
> > Yes this is a problem. Of course solution is easy by always passing
> > acpi_rsdp on command line. But in long term this is a problem. In the
> > sense, I am not sure how to cleanup the kexec-tools code as things improve.
> > Now we will support the EFI properly and still pass acpi_rsdp always in
> > an effort to matain backward compatibility.
> >
> > For a very long time kexec-tools were not automatically appending
> > acpi_rsdp and user were supposed to add it on command line. We were
> > carrying this change in kdump scripts and pushed this change into
> > kexec-tools. In hindsight, it looks like that hardcoding parameters
> > in kexec-tools is a bad idea. It is hard to get rid of them in future.
>
> I tend to agree. However, if the parameters end up being hard coded
> elsewhere, for example in widely used wrapper scripts, then I think that
> the same problem still exists. Just outside of kexec-tools itself.
Kernel has a rule of not breaking userspace, does kexec-tools also have the
policy? Maybe user or distribution carry that will be slight better so upstream
kexec code will be cleaner..
_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [patch 2/4] remove extra acpi_rsdp command line for efi
2013-10-29 2:36 ` Dave Young
@ 2013-10-29 3:18 ` Dave Young
2013-10-30 0:45 ` Simon Horman
1 sibling, 0 replies; 38+ messages in thread
From: Dave Young @ 2013-10-29 3:18 UTC (permalink / raw)
To: Simon Horman
Cc: Matthew Garrett, kexec, James.Bottomley, kexec, ebiederm, hpa, bp,
Vivek Goyal
On 10/29/13 at 10:36am, Dave Young wrote:
> On 10/29/13 at 11:04am, Simon Horman wrote:
> > On Mon, Oct 28, 2013 at 12:10:40PM -0400, Vivek Goyal wrote:
> > > On Mon, Oct 28, 2013 at 10:51:19AM +0000, Matthew Garrett wrote:
> > > > On Mon, Oct 28, 2013 at 06:45:32PM +0800, Dave Young wrote:
> > > > > On 10/28/13 at 10:39am, Matthew Garrett wrote:
> > > > > > On Mon, Oct 28, 2013 at 06:34:12PM +0800, Dave Young wrote:
> > > > > > > On 10/28/13 at 10:12am, Matthew Garrett wrote:
> > > > > > > > Right, but previously acpi_rsdp was passed automatically and now it
> > > > > > > > won't be?
> > > > > > >
> > > > > > > Yes, it was. I'm removing them in kexec-tools patches for efi runtime support.
> > > > > >
> > > > > > If I upgrade kexec-tools and try to launch an old kernel, I now need to
> > > > > > add an extra parameter?
> > > > >
> > > > > Yes, it should work by passing the acpi_rsdp= via --append
> > > >
> > > > Yes, that's my point. You're breaking old configurations by requiring
> > > > the user to pass an additional argument.
> > >
> > > Yes this is a problem. Of course solution is easy by always passing
> > > acpi_rsdp on command line. But in long term this is a problem. In the
> > > sense, I am not sure how to cleanup the kexec-tools code as things improve.
> > > Now we will support the EFI properly and still pass acpi_rsdp always in
> > > an effort to matain backward compatibility.
> > >
> > > For a very long time kexec-tools were not automatically appending
> > > acpi_rsdp and user were supposed to add it on command line. We were
> > > carrying this change in kdump scripts and pushed this change into
> > > kexec-tools. In hindsight, it looks like that hardcoding parameters
> > > in kexec-tools is a bad idea. It is hard to get rid of them in future.
> >
> > I tend to agree. However, if the parameters end up being hard coded
> > elsewhere, for example in widely used wrapper scripts, then I think that
> > the same problem still exists. Just outside of kexec-tools itself.
>
> Kernel has a rule of not breaking userspace, does kexec-tools also have the
> policy? Maybe user or distribution carry that will be slight better so upstream
> kexec code will be cleaner..
Forgot what I said above about the policy, they are different things.
I just replied mjg that even carry this acpi_rsdp, old kernel can not be used
because there's efi info in setup_header.
_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [patch 2/4] remove extra acpi_rsdp command line for efi
2013-10-29 2:36 ` Dave Young
2013-10-29 3:18 ` Dave Young
@ 2013-10-30 0:45 ` Simon Horman
1 sibling, 0 replies; 38+ messages in thread
From: Simon Horman @ 2013-10-30 0:45 UTC (permalink / raw)
To: Dave Young
Cc: Matthew Garrett, kexec, James.Bottomley, kexec, ebiederm, hpa, bp,
Vivek Goyal
On Tue, Oct 29, 2013 at 10:36:36AM +0800, Dave Young wrote:
> On 10/29/13 at 11:04am, Simon Horman wrote:
> > On Mon, Oct 28, 2013 at 12:10:40PM -0400, Vivek Goyal wrote:
> > > On Mon, Oct 28, 2013 at 10:51:19AM +0000, Matthew Garrett wrote:
> > > > On Mon, Oct 28, 2013 at 06:45:32PM +0800, Dave Young wrote:
> > > > > On 10/28/13 at 10:39am, Matthew Garrett wrote:
> > > > > > On Mon, Oct 28, 2013 at 06:34:12PM +0800, Dave Young wrote:
> > > > > > > On 10/28/13 at 10:12am, Matthew Garrett wrote:
> > > > > > > > Right, but previously acpi_rsdp was passed automatically and now it
> > > > > > > > won't be?
> > > > > > >
> > > > > > > Yes, it was. I'm removing them in kexec-tools patches for efi runtime support.
> > > > > >
> > > > > > If I upgrade kexec-tools and try to launch an old kernel, I now need to
> > > > > > add an extra parameter?
> > > > >
> > > > > Yes, it should work by passing the acpi_rsdp= via --append
> > > >
> > > > Yes, that's my point. You're breaking old configurations by requiring
> > > > the user to pass an additional argument.
> > >
> > > Yes this is a problem. Of course solution is easy by always passing
> > > acpi_rsdp on command line. But in long term this is a problem. In the
> > > sense, I am not sure how to cleanup the kexec-tools code as things improve.
> > > Now we will support the EFI properly and still pass acpi_rsdp always in
> > > an effort to matain backward compatibility.
> > >
> > > For a very long time kexec-tools were not automatically appending
> > > acpi_rsdp and user were supposed to add it on command line. We were
> > > carrying this change in kdump scripts and pushed this change into
> > > kexec-tools. In hindsight, it looks like that hardcoding parameters
> > > in kexec-tools is a bad idea. It is hard to get rid of them in future.
> >
> > I tend to agree. However, if the parameters end up being hard coded
> > elsewhere, for example in widely used wrapper scripts, then I think that
> > the same problem still exists. Just outside of kexec-tools itself.
>
> Kernel has a rule of not breaking userspace, does kexec-tools also have
> the policy? Maybe user or distribution carry that will be slight better
> so upstream kexec code will be cleaner..
I'm not sure that we have had to make a policy decision with regards
to this. At least not recently. But yes, I think it should be a goal
of kexec-tools not to break its users.
_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [patch 2/4] remove extra acpi_rsdp command line for efi
2013-10-29 2:04 ` Simon Horman
2013-10-29 2:36 ` Dave Young
@ 2013-10-29 13:27 ` Vivek Goyal
1 sibling, 0 replies; 38+ messages in thread
From: Vivek Goyal @ 2013-10-29 13:27 UTC (permalink / raw)
To: Simon Horman
Cc: Matthew Garrett, kexec, James.Bottomley, bp, ebiederm, hpa,
Dave Young, kexec
On Tue, Oct 29, 2013 at 11:04:28AM +0900, Simon Horman wrote:
[..]
> > > Yes, that's my point. You're breaking old configurations by requiring
> > > the user to pass an additional argument.
> >
> > Yes this is a problem. Of course solution is easy by always passing
> > acpi_rsdp on command line. But in long term this is a problem. In the
> > sense, I am not sure how to cleanup the kexec-tools code as things improve.
> > Now we will support the EFI properly and still pass acpi_rsdp always in
> > an effort to matain backward compatibility.
> >
> > For a very long time kexec-tools were not automatically appending
> > acpi_rsdp and user were supposed to add it on command line. We were
> > carrying this change in kdump scripts and pushed this change into
> > kexec-tools. In hindsight, it looks like that hardcoding parameters
> > in kexec-tools is a bad idea. It is hard to get rid of them in future.
>
> I tend to agree. However, if the parameters end up being hard coded
> elsewhere, for example in widely used wrapper scripts, then I think that
> the same problem still exists. Just outside of kexec-tools itself.
I think getting rid of them in vendor scripts is easier beacause control
the range of kexec-tools and kernel version which will be used in a
particular release life cycle. So when we know that we are not going to
support other kernels than vendor shipped kernel, we can remove parameters
from scripts.
Thanks
Vivek
_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec
^ permalink raw reply [flat|nested] 38+ messages in thread
* [patch 3/4] Add efi_info in x86 setup header
2013-10-27 4:04 [patch 0/4] kexec-tools: efi runtime support on kexec kernel dyoung
2013-10-27 4:04 ` [patch 1/4] Add function get_bootparam dyoung
2013-10-27 4:04 ` [patch 2/4] remove extra acpi_rsdp command line for efi dyoung
@ 2013-10-27 4:04 ` dyoung
2013-10-27 4:04 ` [patch 4/4] Passing efi related data via setup_data dyoung
3 siblings, 0 replies; 38+ messages in thread
From: dyoung @ 2013-10-27 4:04 UTC (permalink / raw)
To: kexec
Cc: mjg59, James.Bottomley, horms, kexec, ebiederm, hpa, bp,
Dave Young, vgoyal
[-- Attachment #1: 03-kexec-padd-efi_info.patch --]
[-- Type: text/plain, Size: 1624 bytes --]
For supporting efi runtime on kexec kernel we need to
fill the efi_info struct in setup_header. I just get
the info in kernel exported boot_params data in debugfs.
Signed-off-by: Dave Young <dyoung@redhat.com>
---
--- kexec-tools.orig/include/x86/x86-linux.h
+++ kexec-tools/include/x86/x86-linux.h
@@ -115,7 +115,8 @@ struct x86_linux_param_header {
uint32_t ext_ramdisk_image; /* 0xc0 */
uint32_t ext_ramdisk_size; /* 0xc4 */
uint32_t ext_cmd_line_ptr; /* 0xc8 */
- uint8_t reserved4_1[0x1e0 - 0xcc]; /* 0xcc */
+ uint8_t reserved4_1[0x1c0 - 0xcc]; /* 0xcc */
+ uint8_t efi_info[32]; /* 0x1c0 */
uint32_t alt_mem_k; /* 0x1e0 */
uint8_t reserved5[4]; /* 0x1e4 */
uint8_t e820_map_nr; /* 0x1e8 */
--- kexec-tools.orig/kexec/arch/i386/x86-linux-setup.c
+++ kexec-tools/kexec/arch/i386/x86-linux-setup.c
@@ -466,6 +466,13 @@ void setup_subarch(struct x86_linux_para
get_bootparam(&real_mode->hardware_subarch, offset, sizeof(uint32_t));
}
+void setup_efi_info(struct x86_linux_param_header *real_mode)
+{
+ off_t offset = offsetof(typeof(*real_mode), efi_info);
+
+ get_bootparam(&real_mode->efi_info, offset, 32);
+}
+
void setup_linux_system_parameters(struct kexec_info *info,
struct x86_linux_param_header *real_mode)
{
@@ -475,6 +482,7 @@ void setup_linux_system_parameters(struc
/* get subarch from running kernel */
setup_subarch(real_mode);
+ setup_efi_info(real_mode);
/* Default screen size */
real_mode->orig_x = 0;
_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec
^ permalink raw reply [flat|nested] 38+ messages in thread* [patch 4/4] Passing efi related data via setup_data
2013-10-27 4:04 [patch 0/4] kexec-tools: efi runtime support on kexec kernel dyoung
` (2 preceding siblings ...)
2013-10-27 4:04 ` [patch 3/4] Add efi_info in x86 setup header dyoung
@ 2013-10-27 4:04 ` dyoung
2013-10-28 0:37 ` Simon Horman
` (2 more replies)
3 siblings, 3 replies; 38+ messages in thread
From: dyoung @ 2013-10-27 4:04 UTC (permalink / raw)
To: kexec
Cc: mjg59, James.Bottomley, horms, kexec, ebiederm, hpa, bp,
Dave Young, vgoyal
[-- Attachment #1: 05-kexec-add-efi-setup-data.patch --]
[-- Type: text/plain, Size: 5122 bytes --]
For supporting efi runtime, several efi physical addresses
fw_vendor, runtime, config tables, smbios and the whole runtime
mapping info need to be used in kexec kernel. Thus introduce
setup_data struct for passing these data.
collect the varialbes from /sys/firmware/efi/systab and
/sys/firmware/efi/efi-runtime-map
Tested on qemu+ovmf, dell laptop, lenovo laptop and HP workstation.
Signed-off-by: Dave Young <dyoung@redhat.com>
---
include/x86/x86-linux.h | 2
kexec/arch/i386/x86-linux-setup.c | 120 +++++++++++++++++++++++++++++++++++++-
2 files changed, 120 insertions(+), 2 deletions(-)
--- kexec-tools.orig/include/x86/x86-linux.h
+++ kexec-tools/include/x86/x86-linux.h
@@ -115,7 +115,7 @@ struct x86_linux_param_header {
uint32_t ext_ramdisk_image; /* 0xc0 */
uint32_t ext_ramdisk_size; /* 0xc4 */
uint32_t ext_cmd_line_ptr; /* 0xc8 */
- uint8_t reserved4_1[0x1c0 - 0xcc]; /* 0xcc */
+ uint8_t reserved4_1[0x1c0 - 0xcc]; /* 0xe4 */
uint8_t efi_info[32]; /* 0x1c0 */
uint32_t alt_mem_k; /* 0x1e0 */
uint8_t reserved5[4]; /* 0x1e4 */
--- kexec-tools.orig/kexec/arch/i386/x86-linux-setup.c
+++ kexec-tools/kexec/arch/i386/x86-linux-setup.c
@@ -473,6 +473,124 @@ void setup_efi_info(struct x86_linux_par
get_bootparam(&real_mode->efi_info, offset, 32);
}
+struct efi_mem_descriptor {
+ uint32_t type;
+ uint32_t pad;
+ uint64_t phys_addr;
+ uint64_t virt_addr;
+ uint64_t num_pages;
+ uint64_t attribute;
+};
+
+struct efi_setup_data {
+ uint64_t fw_vendor;
+ uint64_t runtime;
+ uint64_t tables;
+ uint64_t smbios;
+ uint64_t reserved[8];
+ struct efi_mem_descriptor map[0];
+};
+
+struct setup_data {
+ __uint64_t next;
+ __u32 type;
+ __u32 len;
+ __u8 data[0];
+}__attribute__ ((packed));
+
+static void _get_efi_value(char *line, const char *pattern, __uint64_t *val)
+{
+ char *s, *end;
+ s = strstr(line, pattern);
+ if (s)
+ *val = strtoull(s + strlen(pattern), &end, 16);
+}
+
+static void get_efi_value(struct efi_setup_data *esd)
+{
+ FILE *fp;
+ char line[1024];
+
+ fp = fopen("/sys/firmware/efi/systab", "r");
+ if (!fp)
+ return;
+
+ while(fgets(line, sizeof(line), fp) != 0) {
+ _get_efi_value(line, "fw_vendor=0x", &esd->fw_vendor);
+ _get_efi_value(line, "runtime=0x", &esd->runtime);
+ _get_efi_value(line, "config_tables=0x", &esd->tables);
+ _get_efi_value(line, "SMBIOS=0x", &esd->smbios);
+ }
+
+ fclose(fp);
+}
+
+static int get_efi_runtime_map(struct efi_setup_data **esd)
+{
+ DIR * dirp;
+ struct dirent * entry;
+ char filename[1024];
+ struct efi_mem_descriptor md;
+ int nr_maps = 0;
+
+ dirp = opendir("/sys/firmware/efi/efi-runtime-map");
+ while ((entry = readdir(dirp)) != NULL) {
+ sprintf(filename, "/sys/firmware/efi/efi-runtime-map/%s", (char *)entry->d_name);
+ if (*entry->d_name == '.' )
+ continue;
+ file_scanf(filename, "type", "0x%x", (unsigned int *)&md.type);
+ file_scanf(filename, "phys_addr", "0x%llx", (unsigned long long *)&md.phys_addr);
+ file_scanf(filename, "virt_addr", "0x%llx", (unsigned long long *)&md.virt_addr);
+ file_scanf(filename, "num_pages", "0x%llx", (unsigned long long *)&md.num_pages);
+ file_scanf(filename, "attribute", "0x%llx", (unsigned long long *)&md.attribute);
+ *esd = realloc(*esd, sizeof(struct efi_setup_data) + (nr_maps + 1) * sizeof(struct efi_mem_descriptor));
+ *((*esd)->map + nr_maps) = md;
+ nr_maps++;
+ }
+
+ closedir(dirp);
+ return nr_maps;
+}
+
+static void setup_efi_setup_data(struct kexec_info *info,
+ struct x86_linux_param_header *real_mode)
+{
+ int nr_maps;
+ int64_t setup_data_paddr;
+ struct setup_data *sd;
+ struct efi_setup_data *esd;
+ int size, sdsize;
+ int has_efi = 0;
+
+ has_efi = access("/sys/firmware/efi/systab", F_OK);
+ if (has_efi < 0)
+ return;
+
+ esd = malloc(sizeof(struct efi_setup_data));
+ if (!esd)
+ return;
+ memset(esd, 0, sizeof(struct efi_setup_data));
+ get_efi_value(esd);
+ nr_maps = get_efi_runtime_map(&esd);
+ size = nr_maps * sizeof(struct efi_mem_descriptor) + sizeof(struct efi_setup_data);
+ sd = malloc(sizeof(struct setup_data) + size);
+ if (!sd) {
+ free(esd);
+ return;
+ }
+
+ memset(sd, 0, sizeof(struct setup_data) + size);
+ sd->next = 0;
+ sd->type = 4;
+ sd->len = size;
+ memcpy(sd->data, esd, size);
+ sdsize = sd->len + sizeof(struct setup_data);
+ setup_data_paddr = add_buffer(info, sd, sdsize, sdsize, getpagesize(), 0x100000, ULONG_MAX, INT_MAX);
+
+ real_mode->setup_data = setup_data_paddr;
+}
+
+
void setup_linux_system_parameters(struct kexec_info *info,
struct x86_linux_param_header *real_mode)
{
@@ -483,7 +601,7 @@ void setup_linux_system_parameters(struc
/* get subarch from running kernel */
setup_subarch(real_mode);
setup_efi_info(real_mode);
-
+ setup_efi_setup_data(info, real_mode);
/* Default screen size */
real_mode->orig_x = 0;
real_mode->orig_y = 0;
_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec
^ permalink raw reply [flat|nested] 38+ messages in thread* Re: [patch 4/4] Passing efi related data via setup_data
2013-10-27 4:04 ` [patch 4/4] Passing efi related data via setup_data dyoung
@ 2013-10-28 0:37 ` Simon Horman
2013-10-28 1:12 ` Dave Young
2013-10-28 1:28 ` Dave Young
2013-10-28 10:41 ` H. Peter Anvin
2 siblings, 1 reply; 38+ messages in thread
From: Simon Horman @ 2013-10-28 0:37 UTC (permalink / raw)
To: dyoung; +Cc: mjg59, kexec, James.Bottomley, kexec, ebiederm, hpa, bp, vgoyal
On Sun, Oct 27, 2013 at 12:04:41PM +0800, dyoung@redhat.com wrote:
> For supporting efi runtime, several efi physical addresses
> fw_vendor, runtime, config tables, smbios and the whole runtime
> mapping info need to be used in kexec kernel. Thus introduce
> setup_data struct for passing these data.
>
> collect the varialbes from /sys/firmware/efi/systab and
> /sys/firmware/efi/efi-runtime-map
>
> Tested on qemu+ovmf, dell laptop, lenovo laptop and HP workstation.
>
> Signed-off-by: Dave Young <dyoung@redhat.com>
> ---
> include/x86/x86-linux.h | 2
> kexec/arch/i386/x86-linux-setup.c | 120 +++++++++++++++++++++++++++++++++++++-
> 2 files changed, 120 insertions(+), 2 deletions(-)
>
> --- kexec-tools.orig/include/x86/x86-linux.h
> +++ kexec-tools/include/x86/x86-linux.h
> @@ -115,7 +115,7 @@ struct x86_linux_param_header {
> uint32_t ext_ramdisk_image; /* 0xc0 */
> uint32_t ext_ramdisk_size; /* 0xc4 */
> uint32_t ext_cmd_line_ptr; /* 0xc8 */
> - uint8_t reserved4_1[0x1c0 - 0xcc]; /* 0xcc */
> + uint8_t reserved4_1[0x1c0 - 0xcc]; /* 0xe4 */
> uint8_t efi_info[32]; /* 0x1c0 */
> uint32_t alt_mem_k; /* 0x1e0 */
> uint8_t reserved5[4]; /* 0x1e4 */
> --- kexec-tools.orig/kexec/arch/i386/x86-linux-setup.c
> +++ kexec-tools/kexec/arch/i386/x86-linux-setup.c
> @@ -473,6 +473,124 @@ void setup_efi_info(struct x86_linux_par
> get_bootparam(&real_mode->efi_info, offset, 32);
> }
>
> +struct efi_mem_descriptor {
> + uint32_t type;
> + uint32_t pad;
> + uint64_t phys_addr;
> + uint64_t virt_addr;
> + uint64_t num_pages;
> + uint64_t attribute;
> +};
> +
> +struct efi_setup_data {
> + uint64_t fw_vendor;
> + uint64_t runtime;
> + uint64_t tables;
> + uint64_t smbios;
> + uint64_t reserved[8];
> + struct efi_mem_descriptor map[0];
> +};
> +
> +struct setup_data {
> + __uint64_t next;
> + __u32 type;
> + __u32 len;
> + __u8 data[0];
> +}__attribute__ ((packed));
> +
> +static void _get_efi_value(char *line, const char *pattern, __uint64_t *val)
> +{
> + char *s, *end;
> + s = strstr(line, pattern);
> + if (s)
> + *val = strtoull(s + strlen(pattern), &end, 16);
> +}
> +
> +static void get_efi_value(struct efi_setup_data *esd)
> +{
> + FILE *fp;
> + char line[1024];
> +
> + fp = fopen("/sys/firmware/efi/systab", "r");
> + if (!fp)
> + return;
> +
> + while(fgets(line, sizeof(line), fp) != 0) {
> + _get_efi_value(line, "fw_vendor=0x", &esd->fw_vendor);
> + _get_efi_value(line, "runtime=0x", &esd->runtime);
> + _get_efi_value(line, "config_tables=0x", &esd->tables);
> + _get_efi_value(line, "SMBIOS=0x", &esd->smbios);
> + }
> +
> + fclose(fp);
> +}
> +
> +static int get_efi_runtime_map(struct efi_setup_data **esd)
> +{
> + DIR * dirp;
> + struct dirent * entry;
> + char filename[1024];
> + struct efi_mem_descriptor md;
> + int nr_maps = 0;
> +
> + dirp = opendir("/sys/firmware/efi/efi-runtime-map");
> + while ((entry = readdir(dirp)) != NULL) {
> + sprintf(filename, "/sys/firmware/efi/efi-runtime-map/%s", (char *)entry->d_name);
> + if (*entry->d_name == '.' )
> + continue;
> + file_scanf(filename, "type", "0x%x", (unsigned int *)&md.type);
> + file_scanf(filename, "phys_addr", "0x%llx", (unsigned long long *)&md.phys_addr);
> + file_scanf(filename, "virt_addr", "0x%llx", (unsigned long long *)&md.virt_addr);
> + file_scanf(filename, "num_pages", "0x%llx", (unsigned long long *)&md.num_pages);
> + file_scanf(filename, "attribute", "0x%llx", (unsigned long long *)&md.attribute);
> + *esd = realloc(*esd, sizeof(struct efi_setup_data) + (nr_maps + 1) * sizeof(struct efi_mem_descriptor));
> + *((*esd)->map + nr_maps) = md;
> + nr_maps++;
> + }
> +
> + closedir(dirp);
> + return nr_maps;
> +}
> +
> +static void setup_efi_setup_data(struct kexec_info *info,
> + struct x86_linux_param_header *real_mode)
> +{
> + int nr_maps;
> + int64_t setup_data_paddr;
> + struct setup_data *sd;
> + struct efi_setup_data *esd;
> + int size, sdsize;
> + int has_efi = 0;
The indentation of the line below is inconsistent with the line above.
> +
> + has_efi = access("/sys/firmware/efi/systab", F_OK);
> + if (has_efi < 0)
> + return;
> +
> + esd = malloc(sizeof(struct efi_setup_data));
> + if (!esd)
> + return;
This function appears to leak esd.
> + memset(esd, 0, sizeof(struct efi_setup_data));
> + get_efi_value(esd);
> + nr_maps = get_efi_runtime_map(&esd);
> + size = nr_maps * sizeof(struct efi_mem_descriptor) + sizeof(struct efi_setup_data);
> + sd = malloc(sizeof(struct setup_data) + size);
> + if (!sd) {
> + free(esd);
> + return;
> + }
This function appears to leak sd.
> +
> + memset(sd, 0, sizeof(struct setup_data) + size);
> + sd->next = 0;
> + sd->type = 4;
> + sd->len = size;
> + memcpy(sd->data, esd, size);
> + sdsize = sd->len + sizeof(struct setup_data);
> + setup_data_paddr = add_buffer(info, sd, sdsize, sdsize, getpagesize(), 0x100000, ULONG_MAX, INT_MAX);
> +
> + real_mode->setup_data = setup_data_paddr;
> +}
> +
> +
> void setup_linux_system_parameters(struct kexec_info *info,
> struct x86_linux_param_header *real_mode)
> {
> @@ -483,7 +601,7 @@ void setup_linux_system_parameters(struc
> /* get subarch from running kernel */
> setup_subarch(real_mode);
> setup_efi_info(real_mode);
> -
> + setup_efi_setup_data(info, real_mode);
> /* Default screen size */
> real_mode->orig_x = 0;
> real_mode->orig_y = 0;
>
_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec
^ permalink raw reply [flat|nested] 38+ messages in thread* Re: [patch 4/4] Passing efi related data via setup_data
2013-10-28 0:37 ` Simon Horman
@ 2013-10-28 1:12 ` Dave Young
2013-10-28 3:02 ` Dave Young
0 siblings, 1 reply; 38+ messages in thread
From: Dave Young @ 2013-10-28 1:12 UTC (permalink / raw)
To: Simon Horman
Cc: mjg59, kexec, James.Bottomley, kexec, ebiederm, hpa, bp, vgoyal
Hi, Simon
Thanks for review.
> > + int has_efi = 0;
>
> The indentation of the line below is inconsistent with the line above.
Will fix
>
> > +
> > + has_efi = access("/sys/firmware/efi/systab", F_OK);
> > + if (has_efi < 0)
> > + return;
> > +
> > + esd = malloc(sizeof(struct efi_setup_data));
> > + if (!esd)
> > + return;
>
> This function appears to leak esd.
I left the error handling after testing but later forgot to add them.
Will fix
>
> > + memset(esd, 0, sizeof(struct efi_setup_data));
> > + get_efi_value(esd);
> > + nr_maps = get_efi_runtime_map(&esd);
> > + size = nr_maps * sizeof(struct efi_mem_descriptor) + sizeof(struct efi_setup_data);
> > + sd = malloc(sizeof(struct setup_data) + size);
> > + if (!sd) {
> > + free(esd);
> > + return;
> > + }
>
> This function appears to leak sd.
Will fix
--
Thanks
Dave
_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec
^ permalink raw reply [flat|nested] 38+ messages in thread* Re: [patch 4/4] Passing efi related data via setup_data
2013-10-28 1:12 ` Dave Young
@ 2013-10-28 3:02 ` Dave Young
2013-10-28 4:16 ` Simon Horman
0 siblings, 1 reply; 38+ messages in thread
From: Dave Young @ 2013-10-28 3:02 UTC (permalink / raw)
To: Simon Horman
Cc: mjg59, kexec, James.Bottomley, kexec, ebiederm, hpa, bp, vgoyal
On 10/28/13 at 09:12am, Dave Young wrote:
> > > + size = nr_maps * sizeof(struct efi_mem_descriptor) + sizeof(struct efi_setup_data);
> > > + sd = malloc(sizeof(struct setup_data) + size);
> > > + if (!sd) {
> > > + free(esd);
> > > + return;
> > > + }
> >
> > This function appears to leak sd.
>
> Will fix
sd will be passed to add_buffer, it should be freed after kexec_load,
is it really necessary, or I misunderstand something?
>
> --
> Thanks
> Dave
_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec
^ permalink raw reply [flat|nested] 38+ messages in thread* Re: [patch 4/4] Passing efi related data via setup_data
2013-10-28 3:02 ` Dave Young
@ 2013-10-28 4:16 ` Simon Horman
0 siblings, 0 replies; 38+ messages in thread
From: Simon Horman @ 2013-10-28 4:16 UTC (permalink / raw)
To: Dave Young
Cc: mjg59, kexec, James.Bottomley, kexec, ebiederm, hpa, bp, vgoyal
On Mon, Oct 28, 2013 at 11:02:59AM +0800, Dave Young wrote:
> On 10/28/13 at 09:12am, Dave Young wrote:
> > > > + size = nr_maps * sizeof(struct efi_mem_descriptor) + sizeof(struct efi_setup_data);
> > > > + sd = malloc(sizeof(struct setup_data) + size);
> > > > + if (!sd) {
> > > > + free(esd);
> > > > + return;
> > > > + }
> > >
> > > This function appears to leak sd.
> >
> > Will fix
>
> sd will be passed to add_buffer, it should be freed after kexec_load,
> is it really necessary, or I misunderstand something?
Thanks and sorry for missing that.
I now think the current handling of sd in this patch seems fine.
_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [patch 4/4] Passing efi related data via setup_data
2013-10-27 4:04 ` [patch 4/4] Passing efi related data via setup_data dyoung
2013-10-28 0:37 ` Simon Horman
@ 2013-10-28 1:28 ` Dave Young
2013-10-28 10:41 ` H. Peter Anvin
2 siblings, 0 replies; 38+ messages in thread
From: Dave Young @ 2013-10-28 1:28 UTC (permalink / raw)
To: kexec; +Cc: mjg59, James.Bottomley, horms, kexec, ebiederm, hpa, bp, vgoyal
On 10/27/13 at 12:04pm, Dave Young wrote:
> For supporting efi runtime, several efi physical addresses
> fw_vendor, runtime, config tables, smbios and the whole runtime
> mapping info need to be used in kexec kernel. Thus introduce
> setup_data struct for passing these data.
>
> collect the varialbes from /sys/firmware/efi/systab and
> /sys/firmware/efi/efi-runtime-map
>
> Tested on qemu+ovmf, dell laptop, lenovo laptop and HP workstation.
>
> Signed-off-by: Dave Young <dyoung@redhat.com>
> ---
> include/x86/x86-linux.h | 2
> kexec/arch/i386/x86-linux-setup.c | 120 +++++++++++++++++++++++++++++++++++++-
> 2 files changed, 120 insertions(+), 2 deletions(-)
>
> --- kexec-tools.orig/include/x86/x86-linux.h
> +++ kexec-tools/include/x86/x86-linux.h
> @@ -115,7 +115,7 @@ struct x86_linux_param_header {
> uint32_t ext_ramdisk_image; /* 0xc0 */
> uint32_t ext_ramdisk_size; /* 0xc4 */
> uint32_t ext_cmd_line_ptr; /* 0xc8 */
> - uint8_t reserved4_1[0x1c0 - 0xcc]; /* 0xcc */
> + uint8_t reserved4_1[0x1c0 - 0xcc]; /* 0xe4 */
Above change should be in previous patch. Will update in next version.
_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec
^ permalink raw reply [flat|nested] 38+ messages in thread* Re: [patch 4/4] Passing efi related data via setup_data
2013-10-27 4:04 ` [patch 4/4] Passing efi related data via setup_data dyoung
2013-10-28 0:37 ` Simon Horman
2013-10-28 1:28 ` Dave Young
@ 2013-10-28 10:41 ` H. Peter Anvin
2013-10-28 11:21 ` Dave Young
2 siblings, 1 reply; 38+ messages in thread
From: H. Peter Anvin @ 2013-10-28 10:41 UTC (permalink / raw)
To: dyoung, kexec; +Cc: mjg59, vgoyal, James.Bottomley, horms, bp, ebiederm, kexec
On 10/26/2013 09:04 PM, dyoung@redhat.com wrote:
> + __uint64_t next;
WTF is __uint64_t?
-hpa
_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [patch 4/4] Passing efi related data via setup_data
2013-10-28 10:41 ` H. Peter Anvin
@ 2013-10-28 11:21 ` Dave Young
0 siblings, 0 replies; 38+ messages in thread
From: Dave Young @ 2013-10-28 11:21 UTC (permalink / raw)
To: H. Peter Anvin
Cc: mjg59, kexec, James.Bottomley, horms, kexec, ebiederm, bp, vgoyal
On 10/28/13 at 03:41am, H. Peter Anvin wrote:
> On 10/26/2013 09:04 PM, dyoung@redhat.com wrote:
>
> > + __uint64_t next;
>
> WTF is __uint64_t?
bits/types.h:typedef unsigned long int __uint64_t;
I meant to use uint64_t instead of __unit64_t
Will fix.
_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec
^ permalink raw reply [flat|nested] 38+ messages in thread