2015-12-21 06:09:53

by Xunlei Pang

[permalink] [raw]
Subject: [PATCH] kexec: Move some memembers and definitions within the scope of CONFIG_KEXEC_FILE

Move the stuff currently only used by the kexec file code within
CONFIG_KEXEC_FILE (and CONFIG_KEXEC_VERIFY_SIG).

Also move internal "struct kexec_sha_region" and "struct kexec_buf"
into "kexec_internal.h".

Signed-off-by: Xunlei Pang <[email protected]>
---
arch/x86/kernel/machine_kexec_64.c | 2 ++
include/linux/kexec.h | 62 +++++++++++++++-----------------------
kernel/kexec_file.c | 2 ++
kernel/kexec_internal.h | 21 +++++++++++++
4 files changed, 50 insertions(+), 37 deletions(-)

diff --git a/arch/x86/kernel/machine_kexec_64.c b/arch/x86/kernel/machine_kexec_64.c
index 819ab3f..ba7fbba 100644
--- a/arch/x86/kernel/machine_kexec_64.c
+++ b/arch/x86/kernel/machine_kexec_64.c
@@ -385,6 +385,7 @@ int arch_kimage_file_post_load_cleanup(struct kimage *image)
return image->fops->cleanup(image->image_loader_data);
}

+#ifdef CONFIG_KEXEC_VERIFY_SIG
int arch_kexec_kernel_verify_sig(struct kimage *image, void *kernel,
unsigned long kernel_len)
{
@@ -395,6 +396,7 @@ int arch_kexec_kernel_verify_sig(struct kimage *image, void *kernel,

return image->fops->verify_sig(kernel, kernel_len);
}
+#endif

/*
* Apply purgatory relocations.
diff --git a/include/linux/kexec.h b/include/linux/kexec.h
index 7b68d27..2cc643c 100644
--- a/include/linux/kexec.h
+++ b/include/linux/kexec.h
@@ -109,11 +109,7 @@ struct compat_kexec_segment {
};
#endif

-struct kexec_sha_region {
- unsigned long start;
- unsigned long len;
-};
-
+#ifdef CONFIG_KEXEC_FILE
struct purgatory_info {
/* Pointer to elf header of read only purgatory */
Elf_Ehdr *ehdr;
@@ -130,6 +126,28 @@ struct purgatory_info {
unsigned long purgatory_load_addr;
};

+typedef int (kexec_probe_t)(const char *kernel_buf, unsigned long kernel_size);
+typedef void *(kexec_load_t)(struct kimage *image, char *kernel_buf,
+ unsigned long kernel_len, char *initrd,
+ unsigned long initrd_len, char *cmdline,
+ unsigned long cmdline_len);
+typedef int (kexec_cleanup_t)(void *loader_data);
+
+#ifdef CONFIG_KEXEC_VERIFY_SIG
+typedef int (kexec_verify_sig_t)(const char *kernel_buf,
+ unsigned long kernel_len);
+#endif
+
+struct kexec_file_ops {
+ kexec_probe_t *probe;
+ kexec_load_t *load;
+ kexec_cleanup_t *cleanup;
+#ifdef CONFIG_KEXEC_VERIFY_SIG
+ kexec_verify_sig_t *verify_sig;
+#endif
+};
+#endif
+
struct kimage {
kimage_entry_t head;
kimage_entry_t *entry;
@@ -161,6 +179,7 @@ struct kimage {
struct kimage_arch arch;
#endif

+#ifdef CONFIG_KEXEC_FILE
/* Additional fields for file based kexec syscall */
void *kernel_buf;
unsigned long kernel_buf_len;
@@ -179,38 +198,7 @@ struct kimage {

/* Information for loading purgatory */
struct purgatory_info purgatory_info;
-};
-
-/*
- * Keeps track of buffer parameters as provided by caller for requesting
- * memory placement of buffer.
- */
-struct kexec_buf {
- struct kimage *image;
- char *buffer;
- unsigned long bufsz;
- unsigned long mem;
- unsigned long memsz;
- unsigned long buf_align;
- unsigned long buf_min;
- unsigned long buf_max;
- bool top_down; /* allocate from top of memory hole */
-};
-
-typedef int (kexec_probe_t)(const char *kernel_buf, unsigned long kernel_size);
-typedef void *(kexec_load_t)(struct kimage *image, char *kernel_buf,
- unsigned long kernel_len, char *initrd,
- unsigned long initrd_len, char *cmdline,
- unsigned long cmdline_len);
-typedef int (kexec_cleanup_t)(void *loader_data);
-typedef int (kexec_verify_sig_t)(const char *kernel_buf,
- unsigned long kernel_len);
-
-struct kexec_file_ops {
- kexec_probe_t *probe;
- kexec_load_t *load;
- kexec_cleanup_t *cleanup;
- kexec_verify_sig_t *verify_sig;
+#endif
};

/* kexec interface functions */
diff --git a/kernel/kexec_file.c b/kernel/kexec_file.c
index b70ada0..007b791 100644
--- a/kernel/kexec_file.c
+++ b/kernel/kexec_file.c
@@ -109,11 +109,13 @@ int __weak arch_kimage_file_post_load_cleanup(struct kimage *image)
return -EINVAL;
}

+#ifdef CONFIG_KEXEC_VERIFY_SIG
int __weak arch_kexec_kernel_verify_sig(struct kimage *image, void *buf,
unsigned long buf_len)
{
return -EKEYREJECTED;
}
+#endif

/* Apply relocations of type RELA */
int __weak
diff --git a/kernel/kexec_internal.h b/kernel/kexec_internal.h
index e4392a6..0a52315 100644
--- a/kernel/kexec_internal.h
+++ b/kernel/kexec_internal.h
@@ -15,6 +15,27 @@ int kimage_is_destination_range(struct kimage *image,
extern struct mutex kexec_mutex;

#ifdef CONFIG_KEXEC_FILE
+struct kexec_sha_region {
+ unsigned long start;
+ unsigned long len;
+};
+
+/*
+ * Keeps track of buffer parameters as provided by caller for requesting
+ * memory placement of buffer.
+ */
+struct kexec_buf {
+ struct kimage *image;
+ char *buffer;
+ unsigned long bufsz;
+ unsigned long mem;
+ unsigned long memsz;
+ unsigned long buf_align;
+ unsigned long buf_min;
+ unsigned long buf_max;
+ bool top_down; /* allocate from top of memory hole */
+};
+
void kimage_file_post_load_cleanup(struct kimage *image);
#else /* CONFIG_KEXEC_FILE */
static inline void kimage_file_post_load_cleanup(struct kimage *image) { }
--
2.5.0


2015-12-22 10:44:45

by Minfei Huang

[permalink] [raw]
Subject: Re: [PATCH] kexec: Move some memembers and definitions within the scope of CONFIG_KEXEC_FILE

On 12/21/15 at 02:09pm, Xunlei Pang wrote:
> diff --git a/include/linux/kexec.h b/include/linux/kexec.h
> index 7b68d27..2cc643c 100644
> --- a/include/linux/kexec.h
> +++ b/include/linux/kexec.h
> @@ -109,11 +109,7 @@ struct compat_kexec_segment {
> };
> #endif
>
> -struct kexec_sha_region {
> - unsigned long start;
> - unsigned long len;
> -};
> -
> +#ifdef CONFIG_KEXEC_FILE
> struct purgatory_info {
> /* Pointer to elf header of read only purgatory */
> Elf_Ehdr *ehdr;
> @@ -130,6 +126,28 @@ struct purgatory_info {
> unsigned long purgatory_load_addr;
> };
>
> +typedef int (kexec_probe_t)(const char *kernel_buf, unsigned long kernel_size);
> +typedef void *(kexec_load_t)(struct kimage *image, char *kernel_buf,
> + unsigned long kernel_len, char *initrd,
> + unsigned long initrd_len, char *cmdline,
> + unsigned long cmdline_len);
> +typedef int (kexec_cleanup_t)(void *loader_data);
> +
> +#ifdef CONFIG_KEXEC_VERIFY_SIG
> +typedef int (kexec_verify_sig_t)(const char *kernel_buf,
> + unsigned long kernel_len);
> +#endif
> +
> +struct kexec_file_ops {
> + kexec_probe_t *probe;
> + kexec_load_t *load;
> + kexec_cleanup_t *cleanup;
> +#ifdef CONFIG_KEXEC_VERIFY_SIG
> + kexec_verify_sig_t *verify_sig;
> +#endif
> +};
> +#endif
> +
> struct kimage {
> kimage_entry_t head;
> kimage_entry_t *entry;
> @@ -161,6 +179,7 @@ struct kimage {
> struct kimage_arch arch;
> #endif
>
> +#ifdef CONFIG_KEXEC_FILE
> /* Additional fields for file based kexec syscall */
> void *kernel_buf;
> unsigned long kernel_buf_len;
> @@ -179,38 +198,7 @@ struct kimage {
>
> /* Information for loading purgatory */
> struct purgatory_info purgatory_info;
> -};
> -
> -/*
> - * Keeps track of buffer parameters as provided by caller for requesting
> - * memory placement of buffer.
> - */
> -struct kexec_buf {
> - struct kimage *image;
> - char *buffer;
> - unsigned long bufsz;
> - unsigned long mem;
> - unsigned long memsz;
> - unsigned long buf_align;
> - unsigned long buf_min;
> - unsigned long buf_max;
> - bool top_down; /* allocate from top of memory hole */
> -};
> -
> -typedef int (kexec_probe_t)(const char *kernel_buf, unsigned long kernel_size);
> -typedef void *(kexec_load_t)(struct kimage *image, char *kernel_buf,
> - unsigned long kernel_len, char *initrd,
> - unsigned long initrd_len, char *cmdline,
> - unsigned long cmdline_len);
> -typedef int (kexec_cleanup_t)(void *loader_data);
> -typedef int (kexec_verify_sig_t)(const char *kernel_buf,
> - unsigned long kernel_len);
> -
> -struct kexec_file_ops {
> - kexec_probe_t *probe;
> - kexec_load_t *load;
> - kexec_cleanup_t *cleanup;
> - kexec_verify_sig_t *verify_sig;
> +#endif
> };
>
> /* kexec interface functions */

Hi, Xunlei.

Following functions will be used only in kexec_file. Please wrap them in
CONFIG_KEXEC_FILE.

int __weak arch_kexec_kernel_image_probe(struct kimage *image, void *buf,
unsigned long buf_len);
void * __weak arch_kexec_kernel_image_load(struct kimage *image);
int __weak arch_kimage_file_post_load_cleanup(struct kimage *image);
int __weak arch_kexec_kernel_verify_sig(struct kimage *image, void *buf,
unsigned long buf_len);
int __weak arch_kexec_apply_relocations_add(const Elf_Ehdr *ehdr,
Elf_Shdr *sechdrs, unsigned int relsec);
int __weak arch_kexec_apply_relocations(const Elf_Ehdr *ehdr, Elf_Shdr *sechdrs,
unsigned int relsec);

Thanks
Minfei

2015-12-22 11:41:10

by Xunlei Pang

[permalink] [raw]
Subject: Re: [PATCH] kexec: Move some memembers and definitions within the scope of CONFIG_KEXEC_FILE

On 12/22/2015 at 06:47 PM, Minfei Huang wrote:
> On 12/21/15 at 02:09pm, Xunlei Pang wrote:
>> diff --git a/include/linux/kexec.h b/include/linux/kexec.h
>> index 7b68d27..2cc643c 100644
>> --- a/include/linux/kexec.h
>> +++ b/include/linux/kexec.h
>> @@ -109,11 +109,7 @@ struct compat_kexec_segment {
>> };
>> #endif
>>
>> -struct kexec_sha_region {
>> - unsigned long start;
>> - unsigned long len;
>> -};
>> -
>> +#ifdef CONFIG_KEXEC_FILE
>> struct purgatory_info {
>> /* Pointer to elf header of read only purgatory */
>> Elf_Ehdr *ehdr;
>> @@ -130,6 +126,28 @@ struct purgatory_info {
>> unsigned long purgatory_load_addr;
>> };
>>
>> +typedef int (kexec_probe_t)(const char *kernel_buf, unsigned long kernel_size);
>> +typedef void *(kexec_load_t)(struct kimage *image, char *kernel_buf,
>> + unsigned long kernel_len, char *initrd,
>> + unsigned long initrd_len, char *cmdline,
>> + unsigned long cmdline_len);
>> +typedef int (kexec_cleanup_t)(void *loader_data);
>> +
>> +#ifdef CONFIG_KEXEC_VERIFY_SIG
>> +typedef int (kexec_verify_sig_t)(const char *kernel_buf,
>> + unsigned long kernel_len);
>> +#endif
>> +
>> +struct kexec_file_ops {
>> + kexec_probe_t *probe;
>> + kexec_load_t *load;
>> + kexec_cleanup_t *cleanup;
>> +#ifdef CONFIG_KEXEC_VERIFY_SIG
>> + kexec_verify_sig_t *verify_sig;
>> +#endif
>> +};
>> +#endif
>> +
>> struct kimage {
>> kimage_entry_t head;
>> kimage_entry_t *entry;
>> @@ -161,6 +179,7 @@ struct kimage {
>> struct kimage_arch arch;
>> #endif
>>
>> +#ifdef CONFIG_KEXEC_FILE
>> /* Additional fields for file based kexec syscall */
>> void *kernel_buf;
>> unsigned long kernel_buf_len;
>> @@ -179,38 +198,7 @@ struct kimage {
>>
>> /* Information for loading purgatory */
>> struct purgatory_info purgatory_info;
>> -};
>> -
>> -/*
>> - * Keeps track of buffer parameters as provided by caller for requesting
>> - * memory placement of buffer.
>> - */
>> -struct kexec_buf {
>> - struct kimage *image;
>> - char *buffer;
>> - unsigned long bufsz;
>> - unsigned long mem;
>> - unsigned long memsz;
>> - unsigned long buf_align;
>> - unsigned long buf_min;
>> - unsigned long buf_max;
>> - bool top_down; /* allocate from top of memory hole */
>> -};
>> -
>> -typedef int (kexec_probe_t)(const char *kernel_buf, unsigned long kernel_size);
>> -typedef void *(kexec_load_t)(struct kimage *image, char *kernel_buf,
>> - unsigned long kernel_len, char *initrd,
>> - unsigned long initrd_len, char *cmdline,
>> - unsigned long cmdline_len);
>> -typedef int (kexec_cleanup_t)(void *loader_data);
>> -typedef int (kexec_verify_sig_t)(const char *kernel_buf,
>> - unsigned long kernel_len);
>> -
>> -struct kexec_file_ops {
>> - kexec_probe_t *probe;
>> - kexec_load_t *load;
>> - kexec_cleanup_t *cleanup;
>> - kexec_verify_sig_t *verify_sig;
>> +#endif
>> };
>>
>> /* kexec interface functions */
> Hi, Xunlei.
>
> Following functions will be used only in kexec_file. Please wrap them in
> CONFIG_KEXEC_FILE.
>
> int __weak arch_kexec_kernel_image_probe(struct kimage *image, void *buf,
> unsigned long buf_len);
> void * __weak arch_kexec_kernel_image_load(struct kimage *image);
> int __weak arch_kimage_file_post_load_cleanup(struct kimage *image);
> int __weak arch_kexec_kernel_verify_sig(struct kimage *image, void *buf,
> unsigned long buf_len);
> int __weak arch_kexec_apply_relocations_add(const Elf_Ehdr *ehdr,
> Elf_Shdr *sechdrs, unsigned int relsec);
> int __weak arch_kexec_apply_relocations(const Elf_Ehdr *ehdr, Elf_Shdr *sechdrs,
> unsigned int relsec);

Thanks for the comment.

I noticed this as well, but seems for the function declarations we don't need do this,
since they don't consume the actual space.

For example, in the include/linux/timekeeping.h
/*
* RTC specific
*/
extern bool timekeeping_rtc_skipsuspend(void);
extern bool timekeeping_rtc_skipresume(void);

extern void timekeeping_inject_sleeptime64(struct timespec64 *delta);

also not embraced by the corresponding macros.

Regards,
Xunlei

>
> Thanks
> Minfei

2015-12-22 12:03:18

by Minfei Huang

[permalink] [raw]
Subject: Re: [PATCH] kexec: Move some memembers and definitions within the scope of CONFIG_KEXEC_FILE

On 12/22/15 at 07:40pm, Xunlei Pang wrote:
> On 12/22/2015 at 06:47 PM, Minfei Huang wrote:
> > On 12/21/15 at 02:09pm, Xunlei Pang wrote:
> >> diff --git a/include/linux/kexec.h b/include/linux/kexec.h
> > Hi, Xunlei.
> >
> > Following functions will be used only in kexec_file. Please wrap them in
> > CONFIG_KEXEC_FILE.
> >
> > int __weak arch_kexec_kernel_image_probe(struct kimage *image, void *buf,
> > unsigned long buf_len);
> > void * __weak arch_kexec_kernel_image_load(struct kimage *image);
> > int __weak arch_kimage_file_post_load_cleanup(struct kimage *image);
> > int __weak arch_kexec_kernel_verify_sig(struct kimage *image, void *buf,
> > unsigned long buf_len);
> > int __weak arch_kexec_apply_relocations_add(const Elf_Ehdr *ehdr,
> > Elf_Shdr *sechdrs, unsigned int relsec);
> > int __weak arch_kexec_apply_relocations(const Elf_Ehdr *ehdr, Elf_Shdr *sechdrs,
> > unsigned int relsec);
>
> Thanks for the comment.
>
> I noticed this as well, but seems for the function declarations we don't need do this,
> since they don't consume the actual space.

Yeah. I think it is better to fold above code as well, since there is
no side effect.

>
> For example, in the include/linux/timekeeping.h
> /*
> * RTC specific
> */
> extern bool timekeeping_rtc_skipsuspend(void);
> extern bool timekeeping_rtc_skipresume(void);
>
> extern void timekeeping_inject_sleeptime64(struct timespec64 *delta);
>
> also not embraced by the corresponding macros.

I think it is consistent of the construction to make code more clear.

Thanks
Minfei

2015-12-22 21:14:55

by Andrew Morton

[permalink] [raw]
Subject: Re: [PATCH] kexec: Move some memembers and definitions within the scope of CONFIG_KEXEC_FILE

On Tue, 22 Dec 2015 19:40:39 +0800 Xunlei Pang <[email protected]> wrote:

> > Following functions will be used only in kexec_file. Please wrap them in
> > CONFIG_KEXEC_FILE.
> >
> > int __weak arch_kexec_kernel_image_probe(struct kimage *image, void *buf,
> > unsigned long buf_len);
> > void * __weak arch_kexec_kernel_image_load(struct kimage *image);
> > int __weak arch_kimage_file_post_load_cleanup(struct kimage *image);
> > int __weak arch_kexec_kernel_verify_sig(struct kimage *image, void *buf,
> > unsigned long buf_len);
> > int __weak arch_kexec_apply_relocations_add(const Elf_Ehdr *ehdr,
> > Elf_Shdr *sechdrs, unsigned int relsec);
> > int __weak arch_kexec_apply_relocations(const Elf_Ehdr *ehdr, Elf_Shdr *sechdrs,
> > unsigned int relsec);
>
> Thanks for the comment.
>
> I noticed this as well, but seems for the function declarations we don't need do this,
> since they don't consume the actual space.
>
> For example, in the include/linux/timekeeping.h
> /*
> * RTC specific
> */
> extern bool timekeeping_rtc_skipsuspend(void);
> extern bool timekeeping_rtc_skipresume(void);
>
> extern void timekeeping_inject_sleeptime64(struct timespec64 *delta);
>
> also not embraced by the corresponding macros.

Yes. If we add the ifdefs then a programming error will be detected at
compile time. If we don't add the ifdefs then that error will be
detected at link time. So the ifdefs provide a quite small advantage,
while making the code harder to read and harder to maintain. I believe
that "no ifdefs" is the better side of this tradeoff.