Re: [PATCH 11/13] kexec-bzImage: Support for loading bzImage using 64bit entry
On Tue, Jun 24, 2014 at 01:31:25PM -0400, Vivek Goyal wrote: > I think problem is that we shift 1 by 32 bits in this case (31 - 0 + > 1) and that overflows the size of unsigned. So there is this corner > case where it does not seem to work (or atleast outputs warning). Right, that is a corner case which overflows the shift. The only thing I can think of right now is: #define GENMASK(h, l) ((u32)GENMASK_ULL(h, l)) u32 because we're implicitly assuming we're dealing with 32-bit unsigned quantities. There might be a better solution though... -- Regards/Gruss, Boris. Sent from a fat crate under my desk. Formatting is fine. -- -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 11/13] kexec-bzImage: Support for loading bzImage using 64bit entry
On Sun, Jun 15, 2014 at 06:35:15PM +0200, Borislav Petkov wrote:
[..]
> > +int kexec_setup_initrd(struct boot_params *params,
> > + unsigned long initrd_load_addr, unsigned long initrd_len)
> > +{
> > + params->hdr.ramdisk_image = initrd_load_addr & 0xUL;
> > + params->hdr.ramdisk_size = initrd_len & 0xUL;
>
> We have more readable GENMASK* macros for contiguous masks. This one
> will then look like:
>
> params->hdr.ramdisk_image = initrd_load_addr & GENMASK(31, 0);
> params->hdr.ramdisk_size = initrd_len & GENMASK(31, 0);
>
> and this way we know exactly about which bits are we talking about. :)
[ CC gong.c...@linux.intel.com ]
GENMASK(31,0) outputs compilation warning.
arch/x86/kernel/machine_kexec.c: In function ‘kexec_setup_initrd’:
arch/x86/kernel/machine_kexec.c:30:2: warning: left shift count >= width
of type [enabled by default]
params->hdr.ramdisk_image = initrd_load_addr & GENMASK(31, 0);
^
arch/x86/kernel/machine_kexec.c:31:2: warning: left shift count >= width
of type [enabled by default]
params->hdr.ramdisk_size = initrd_len & GENMASK(31, 0);
^
arch/x86/kernel/machine_kexec.c: In function ‘kexec_setup_cmdline’:
arch/x86/kernel/machine_kexec.c:52:2: warning: left shift count >= width
of type [enabled by default]
cmdline_low_32 = cmdline_ptr_phys & GENMASK(31, 0);
I think problem is that we shift 1 by 32 bits in this case (31 - 0 + 1) and
that overflows the size of unsigned. So there is this corner case where
it does not seem to work (or atleast outputs warning).
Thanks
Vivek
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 11/13] kexec-bzImage: Support for loading bzImage using 64bit entry
On Sun, Jun 15, 2014 at 06:35:15PM +0200, Borislav Petkov wrote:
[..]
+int kexec_setup_initrd(struct boot_params *params,
+ unsigned long initrd_load_addr, unsigned long initrd_len)
+{
+ params-hdr.ramdisk_image = initrd_load_addr 0xUL;
+ params-hdr.ramdisk_size = initrd_len 0xUL;
We have more readable GENMASK* macros for contiguous masks. This one
will then look like:
params-hdr.ramdisk_image = initrd_load_addr GENMASK(31, 0);
params-hdr.ramdisk_size = initrd_len GENMASK(31, 0);
and this way we know exactly about which bits are we talking about. :)
[ CC gong.c...@linux.intel.com ]
GENMASK(31,0) outputs compilation warning.
arch/x86/kernel/machine_kexec.c: In function ‘kexec_setup_initrd’:
arch/x86/kernel/machine_kexec.c:30:2: warning: left shift count = width
of type [enabled by default]
params-hdr.ramdisk_image = initrd_load_addr GENMASK(31, 0);
^
arch/x86/kernel/machine_kexec.c:31:2: warning: left shift count = width
of type [enabled by default]
params-hdr.ramdisk_size = initrd_len GENMASK(31, 0);
^
arch/x86/kernel/machine_kexec.c: In function ‘kexec_setup_cmdline’:
arch/x86/kernel/machine_kexec.c:52:2: warning: left shift count = width
of type [enabled by default]
cmdline_low_32 = cmdline_ptr_phys GENMASK(31, 0);
I think problem is that we shift 1 by 32 bits in this case (31 - 0 + 1) and
that overflows the size of unsigned. So there is this corner case where
it does not seem to work (or atleast outputs warning).
Thanks
Vivek
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 11/13] kexec-bzImage: Support for loading bzImage using 64bit entry
On Tue, Jun 24, 2014 at 01:31:25PM -0400, Vivek Goyal wrote: I think problem is that we shift 1 by 32 bits in this case (31 - 0 + 1) and that overflows the size of unsigned. So there is this corner case where it does not seem to work (or atleast outputs warning). Right, that is a corner case which overflows the shift. The only thing I can think of right now is: #define GENMASK(h, l) ((u32)GENMASK_ULL(h, l)) u32 because we're implicitly assuming we're dealing with 32-bit unsigned quantities. There might be a better solution though... -- Regards/Gruss, Boris. Sent from a fat crate under my desk. Formatting is fine. -- -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 11/13] kexec-bzImage: Support for loading bzImage using 64bit entry
On Mon, Jun 16, 2014 at 11:27:20PM +0200, Borislav Petkov wrote: > On Mon, Jun 16, 2014 at 05:15:00PM -0400, Vivek Goyal wrote: > > Do we want to show all the rejection messages from bzImage64 and > > bzImage32 loaders. It might be too verbose to show users that before > > vmlinux loader accepted the image other loaders on this arches rejcted > > the image. > > I get all that. But, if people want to get feedback from the system > about *why* their image didn't load, they absolutely have to enable > dynamic debug. And this is not optimal IMO because they will have to > look at the code first to see what they need to do. > > Or is kexec-tools going to be taught to interpret return values from the > syscall? In most of the cases return code is -ENOEXEC so kexec-tools can't figure out what's wrong. > > In any case, we want information about why an image fails loading to > reach the user in the easiest way possible. And why should the user need > to enable dynamic debug if he can get the info without doing so? > > Oh, and not everyone knows about dynamic debug so... > > And I don't think it'll be too much info - only the line which fails > the check will be printed before the image loader fails so that's > practically one error reason per failed image. > Ok, there will be one line of error and that's not too bad. I will convert these pr_debug() statements in bzImage_probe() to pr_err(). Thanks Vivek -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 11/13] kexec-bzImage: Support for loading bzImage using 64bit entry
On Mon, Jun 16, 2014 at 05:15:00PM -0400, Vivek Goyal wrote: > Do we want to show all the rejection messages from bzImage64 and > bzImage32 loaders. It might be too verbose to show users that before > vmlinux loader accepted the image other loaders on this arches rejcted > the image. I get all that. But, if people want to get feedback from the system about *why* their image didn't load, they absolutely have to enable dynamic debug. And this is not optimal IMO because they will have to look at the code first to see what they need to do. Or is kexec-tools going to be taught to interpret return values from the syscall? In any case, we want information about why an image fails loading to reach the user in the easiest way possible. And why should the user need to enable dynamic debug if he can get the info without doing so? Oh, and not everyone knows about dynamic debug so... And I don't think it'll be too much info - only the line which fails the check will be printed before the image loader fails so that's practically one error reason per failed image. Ok? -- Regards/Gruss, Boris. Sent from a fat crate under my desk. Formatting is fine. -- -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 11/13] kexec-bzImage: Support for loading bzImage using 64bit entry
On Mon, Jun 16, 2014 at 10:57:43PM +0200, Borislav Petkov wrote:
> On Mon, Jun 16, 2014 at 04:06:08PM -0400, Vivek Goyal wrote:
> > There can be more than one loader and the one which claims first
> > to recognize the image will get to load the image. So once 32 bit
> > loader support comes in, it might happen that we ask 64bit loader
> > first and it rejects the image and then we ask 32bit loader.
>
> What does that have to do with anything??
Say down the line you support 3 types of kernel images. 64bit bzImage,
32bit bzImage and ELF vmlinux. And there are 3 different loader
implementations in kernel. Now assume user us trying to load an ELF vmlinux
image.
Generic code will call.
arch_kexec_kernel_image_probe {
for (i = 0; i < nr_file_types; i++) {
if (!kexec_file_type[i].probe)
continue;
ret = kexec_file_type[i].probe(buf, buf_len);
if (!ret) {
image->file_handler_idx = i;
return ret;
}
}
return ret;
}
This code calls into very registered loader and if nobody is ready to
load the image it returns error. Say first bzImage64 and bzImage32 bit
loader are called. They both will reject the image and vmlinux loader
will accept it.
Do we want to show all the rejection messages from bzImage64 and bzImage32
loaders. It might be too verbose to show users that before vmlinux loader
accepted the image other loaders on this arches rejcted the image.
This is very similar to binary file loaing. Looks at load_elf_binary(). If
it does not find elf header it knows it is not an ELF binary and
returns error -ENOEXEC without outputing any messages.
>
> > So these message are really debug message which tells why loader
> > is not accepting an image. It might not be image destined for that
> > loader at all.
> >
> > pr_debug() allows being verbose if user wants to for debugging purposes.
> > You just have to make sure that CONFIG_DYNAMIC_DEBUG=y and enable verbosity
> > in individual file.
> >
> > echo 'file kexec-bzimage.c +p' > /sys/kernel/debug/dynamic_debug/control
>
> So people are supposed to enable dynamic_debug just so that they see
> *why* their image doesn't load.
>
> Doesn't sound optimal to me.
This is one way of doing it. I can change it if you think that displaying
messages from all the loaders is fine.
>
> > Same here. We will potentially be trying multiple loaders and if every
> > loader prints messages for rejection by default, it is too much of
> > info, IMO.
>
> For max two loaders on one architecture? I don't think so. Now you're
> just arguing for the sake of it.
Well, we have 3 loaders in user space currently for x86_64. bzImage64,
bzImage32 and ELF vmlinux. So one would think that somebody might
go about implementing these in kernel space too.
>
> > I like doing memory allocations early in the functions (as far as
> > possible) and error out if need be. If memory is available to begin
> > with for all the data structures needed by this function, it is kind
> > of pointless to do rest of the processing.
>
> We're talking about memory for a single void * which is ridiculous. And
> I think simplifying the error paths is a much higher win than doing some
> minor allocation.
Ok, I will change it.
>
> > Hmm..., If you feel strongly about it, I can make this change. I
> > thought I just made it easier to share the code between 32bit and
> > 64bit by this.
>
> Someone later can do that - right now this code is 64-bit only as far as
> we're concerned and if it can be made to work on 32-bit, then people are
> free to do so.
Ok.
Thanks
Vivek
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 11/13] kexec-bzImage: Support for loading bzImage using 64bit entry
On Mon, Jun 16, 2014 at 04:06:08PM -0400, Vivek Goyal wrote: > There can be more than one loader and the one which claims first > to recognize the image will get to load the image. So once 32 bit > loader support comes in, it might happen that we ask 64bit loader > first and it rejects the image and then we ask 32bit loader. What does that have to do with anything?? > So these message are really debug message which tells why loader > is not accepting an image. It might not be image destined for that > loader at all. > > pr_debug() allows being verbose if user wants to for debugging purposes. > You just have to make sure that CONFIG_DYNAMIC_DEBUG=y and enable verbosity > in individual file. > > echo 'file kexec-bzimage.c +p' > /sys/kernel/debug/dynamic_debug/control So people are supposed to enable dynamic_debug just so that they see *why* their image doesn't load. Doesn't sound optimal to me. > Same here. We will potentially be trying multiple loaders and if every > loader prints messages for rejection by default, it is too much of > info, IMO. For max two loaders on one architecture? I don't think so. Now you're just arguing for the sake of it. > I like doing memory allocations early in the functions (as far as > possible) and error out if need be. If memory is available to begin > with for all the data structures needed by this function, it is kind > of pointless to do rest of the processing. We're talking about memory for a single void * which is ridiculous. And I think simplifying the error paths is a much higher win than doing some minor allocation. > Hmm..., If you feel strongly about it, I can make this change. I > thought I just made it easier to share the code between 32bit and > 64bit by this. Someone later can do that - right now this code is 64-bit only as far as we're concerned and if it can be made to work on 32-bit, then people are free to do so. > I think it just makes it safer that we don't try to copy more than > size of destination, in case ->eddbuf_entries is not right or corrupted. > > I see copy_edd() does similar thing. > > memcpy(edd.edd_info, boot_params.eddbuf, sizeof(edd.edd_info)); > edd.edd_info_nr = boot_params.eddbuf_entries; > > So may be it is not a bad idea to copy based on max size of data > structures. Ok, makes sense. Thanks. -- Regards/Gruss, Boris. Sent from a fat crate under my desk. Formatting is fine. -- -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 11/13] kexec-bzImage: Support for loading bzImage using 64bit entry
On Sun, Jun 15, 2014 at 06:35:15PM +0200, Borislav Petkov wrote:
[..]
> > +int bzImage64_probe(const char *buf, unsigned long len)
> > +{
> > + int ret = -ENOEXEC;
> > + struct setup_header *header;
> > +
> > + /* kernel should be atleast two sector long */
>
> two sectors
>
> > + if (len < 2 * 512) {
> > + pr_debug("File is too short to be a bzImage\n");
>
> Those error messages are all pr_debug. Now, wouldn't we want to tell
> userspace what the problem is, *when* there is one?
>
> I.e., pr_err or pr_info is much more helpful than pr_debug IMO.
There can be more than one loader and the one which claims first
to recognize the image will get to load the image. So once 32 bit
loader support comes in, it might happen that we ask 64bit loader
first and it rejects the image and then we ask 32bit loader.
So these message are really debug message which tells why loader
is not accepting an image. It might not be image destined for that
loader at all.
pr_debug() allows being verbose if user wants to for debugging purposes.
You just have to make sure that CONFIG_DYNAMIC_DEBUG=y and enable verbosity
in individual file.
echo 'file kexec-bzimage.c +p' > /sys/kernel/debug/dynamic_debug/control
>
> > + return ret;
> > + }
> > +
> > + header = (struct setup_header *)(buf + offsetof(struct boot_params,
> > + hdr));
>
> Just let that stick out. The 80 cols limit is not a hard one anyway,
> especially if it impairs readability.
Will do.
>
> > + if (memcmp((char *)>header, "HdrS", 4) != 0) {
>
> Not strncmp? "HdrS" is a string...
As peter said, this is not string. So I will retain it.
>
> > + pr_debug("Not a bzImage\n");
> > + return ret;
> > + }
> > +
> > + if (header->boot_flag != 0xAA55) {
> > + pr_debug("No x86 boot sector present\n");
> > + return ret;
> > + }
> > +
> > + if (header->version < 0x020C) {
> > + pr_debug("Must be at least protocol version 2.12\n");
> > + return ret;
> > + }
> > +
> > + if ((header->loadflags & LOADED_HIGH) == 0) {
>
> if (!(header->loadflags.. ))
Will do.
>
> > + pr_debug("zImage not a bzImage\n");
> > + return ret;
> > + }
> > +
> > + if (!(header->xloadflags & XLF_KERNEL_64)) {
> > + pr_debug("Not a bzImage64. XLF_KERNEL_64 is not set.\n");
> > + return ret;
> > + }
> > +
> > + if (!(header->xloadflags & XLF_CAN_BE_LOADED_ABOVE_4G)) {
> > + pr_debug("XLF_CAN_BE_LOADED_ABOVE_4G is not set.\n");
> > + return ret;
> > + }
>
> Just merge the two checks:
>
> if ((header->xloadflags & (XLF_KERNEL_64 | XLF_CAN_BE_LOADED_ABOVE_4G))
> !=
> (XLF_KERNEL_64 |
> XLF_CAN_BE_LOADED_ABOVE_4G)) {
> pr_err("Not a bzImage, xloadflags: 0x%x\n",
> header->xloadflags);
> return ret;
> }
I think I like separate checks better. That way I can output much better
debug message. Just saying xloadflags=0x%x does not tell anything about
what flags the loader is looking for (without looking at the code).
>
> > +
> > + /* I've got a bzImage */
> > + pr_debug("It's a relocatable bzImage64\n");
> > + ret = 0;
> > +
> > + return ret;
> > +}
> > +
> > +void *bzImage64_load(struct kimage *image, char *kernel,
> > + unsigned long kernel_len,
> > + char *initrd, unsigned long initrd_len,
> > + char *cmdline, unsigned long cmdline_len)
>
> Arg alignment.
Will do.
[..]
> > + header = (struct setup_header *)(kernel + setup_hdr_offset);
> > + setup_sects = header->setup_sects;
> > + if (setup_sects == 0)
> > + setup_sects = 4;
> > +
> > + kern16_size = (setup_sects + 1) * 512;
> > + if (kernel_len < kern16_size) {
> > + pr_debug("bzImage truncated\n");
>
> Ditto for all those pr_debug's in here - I think we want to know why the
> bzImage load fails and pr_debug is not suitable for that.
Same here. We will potentially be trying multiple loaders and if every
loader prints messages for rejection by default, it is too much of info,
IMO.
>
> > + return ERR_PTR(-ENOEXEC);
> > + }
> > +
> > + if (cmdline_len > header->cmdline_size) {
>
> As we talked, I think COMMAND_LINE_SIZE is perfectly fine and safe for
> all intents and purposes.
I still have concerns about using COMMAND_LINE_SIZE. If header information
is useful for a bootloader, then kernel is just a bootloader in this case
and if we really want to limit the size, it should be based on information
present in the header and not based on currently running kernel's limit.
>
> > + pr_debug("Kernel command line too long\n");
> > + return ERR_PTR(-EINVAL);
> > + }
> > +
> > + /* Allocate loader specific data */
> > + ldata = kzalloc(sizeof(struct bzimage64_data), GFP_KERNEL);
> > + if
Re: [PATCH 11/13] kexec-bzImage: Support for loading bzImage using 64bit entry
On Sun, Jun 15, 2014 at 06:35:15PM +0200, Borislav Petkov wrote:
[..]
+int bzImage64_probe(const char *buf, unsigned long len)
+{
+ int ret = -ENOEXEC;
+ struct setup_header *header;
+
+ /* kernel should be atleast two sector long */
two sectors
+ if (len 2 * 512) {
+ pr_debug(File is too short to be a bzImage\n);
Those error messages are all pr_debug. Now, wouldn't we want to tell
userspace what the problem is, *when* there is one?
I.e., pr_err or pr_info is much more helpful than pr_debug IMO.
There can be more than one loader and the one which claims first
to recognize the image will get to load the image. So once 32 bit
loader support comes in, it might happen that we ask 64bit loader
first and it rejects the image and then we ask 32bit loader.
So these message are really debug message which tells why loader
is not accepting an image. It might not be image destined for that
loader at all.
pr_debug() allows being verbose if user wants to for debugging purposes.
You just have to make sure that CONFIG_DYNAMIC_DEBUG=y and enable verbosity
in individual file.
echo 'file kexec-bzimage.c +p' /sys/kernel/debug/dynamic_debug/control
+ return ret;
+ }
+
+ header = (struct setup_header *)(buf + offsetof(struct boot_params,
+ hdr));
Just let that stick out. The 80 cols limit is not a hard one anyway,
especially if it impairs readability.
Will do.
+ if (memcmp((char *)header-header, HdrS, 4) != 0) {
Not strncmp? HdrS is a string...
As peter said, this is not string. So I will retain it.
+ pr_debug(Not a bzImage\n);
+ return ret;
+ }
+
+ if (header-boot_flag != 0xAA55) {
+ pr_debug(No x86 boot sector present\n);
+ return ret;
+ }
+
+ if (header-version 0x020C) {
+ pr_debug(Must be at least protocol version 2.12\n);
+ return ret;
+ }
+
+ if ((header-loadflags LOADED_HIGH) == 0) {
if (!(header-loadflags.. ))
Will do.
+ pr_debug(zImage not a bzImage\n);
+ return ret;
+ }
+
+ if (!(header-xloadflags XLF_KERNEL_64)) {
+ pr_debug(Not a bzImage64. XLF_KERNEL_64 is not set.\n);
+ return ret;
+ }
+
+ if (!(header-xloadflags XLF_CAN_BE_LOADED_ABOVE_4G)) {
+ pr_debug(XLF_CAN_BE_LOADED_ABOVE_4G is not set.\n);
+ return ret;
+ }
Just merge the two checks:
if ((header-xloadflags (XLF_KERNEL_64 | XLF_CAN_BE_LOADED_ABOVE_4G))
!=
(XLF_KERNEL_64 |
XLF_CAN_BE_LOADED_ABOVE_4G)) {
pr_err(Not a bzImage, xloadflags: 0x%x\n,
header-xloadflags);
return ret;
}
I think I like separate checks better. That way I can output much better
debug message. Just saying xloadflags=0x%x does not tell anything about
what flags the loader is looking for (without looking at the code).
+
+ /* I've got a bzImage */
+ pr_debug(It's a relocatable bzImage64\n);
+ ret = 0;
+
+ return ret;
+}
+
+void *bzImage64_load(struct kimage *image, char *kernel,
+ unsigned long kernel_len,
+ char *initrd, unsigned long initrd_len,
+ char *cmdline, unsigned long cmdline_len)
Arg alignment.
Will do.
[..]
+ header = (struct setup_header *)(kernel + setup_hdr_offset);
+ setup_sects = header-setup_sects;
+ if (setup_sects == 0)
+ setup_sects = 4;
+
+ kern16_size = (setup_sects + 1) * 512;
+ if (kernel_len kern16_size) {
+ pr_debug(bzImage truncated\n);
Ditto for all those pr_debug's in here - I think we want to know why the
bzImage load fails and pr_debug is not suitable for that.
Same here. We will potentially be trying multiple loaders and if every
loader prints messages for rejection by default, it is too much of info,
IMO.
+ return ERR_PTR(-ENOEXEC);
+ }
+
+ if (cmdline_len header-cmdline_size) {
As we talked, I think COMMAND_LINE_SIZE is perfectly fine and safe for
all intents and purposes.
I still have concerns about using COMMAND_LINE_SIZE. If header information
is useful for a bootloader, then kernel is just a bootloader in this case
and if we really want to limit the size, it should be based on information
present in the header and not based on currently running kernel's limit.
+ pr_debug(Kernel command line too long\n);
+ return ERR_PTR(-EINVAL);
+ }
+
+ /* Allocate loader specific data */
+ ldata = kzalloc(sizeof(struct bzimage64_data), GFP_KERNEL);
+ if (!ldata)
+ return ERR_PTR(-ENOMEM);
Why don't you move that allocation to the place right before it is being
assigned to it? I.e., to the ldata-bootparams_buf = params line.
I like doing memory allocations early
Re: [PATCH 11/13] kexec-bzImage: Support for loading bzImage using 64bit entry
On Mon, Jun 16, 2014 at 04:06:08PM -0400, Vivek Goyal wrote: There can be more than one loader and the one which claims first to recognize the image will get to load the image. So once 32 bit loader support comes in, it might happen that we ask 64bit loader first and it rejects the image and then we ask 32bit loader. What does that have to do with anything?? So these message are really debug message which tells why loader is not accepting an image. It might not be image destined for that loader at all. pr_debug() allows being verbose if user wants to for debugging purposes. You just have to make sure that CONFIG_DYNAMIC_DEBUG=y and enable verbosity in individual file. echo 'file kexec-bzimage.c +p' /sys/kernel/debug/dynamic_debug/control So people are supposed to enable dynamic_debug just so that they see *why* their image doesn't load. Doesn't sound optimal to me. Same here. We will potentially be trying multiple loaders and if every loader prints messages for rejection by default, it is too much of info, IMO. For max two loaders on one architecture? I don't think so. Now you're just arguing for the sake of it. I like doing memory allocations early in the functions (as far as possible) and error out if need be. If memory is available to begin with for all the data structures needed by this function, it is kind of pointless to do rest of the processing. We're talking about memory for a single void * which is ridiculous. And I think simplifying the error paths is a much higher win than doing some minor allocation. Hmm..., If you feel strongly about it, I can make this change. I thought I just made it easier to share the code between 32bit and 64bit by this. Someone later can do that - right now this code is 64-bit only as far as we're concerned and if it can be made to work on 32-bit, then people are free to do so. I think it just makes it safer that we don't try to copy more than size of destination, in case -eddbuf_entries is not right or corrupted. I see copy_edd() does similar thing. memcpy(edd.edd_info, boot_params.eddbuf, sizeof(edd.edd_info)); edd.edd_info_nr = boot_params.eddbuf_entries; So may be it is not a bad idea to copy based on max size of data structures. Ok, makes sense. Thanks. -- Regards/Gruss, Boris. Sent from a fat crate under my desk. Formatting is fine. -- -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 11/13] kexec-bzImage: Support for loading bzImage using 64bit entry
On Mon, Jun 16, 2014 at 10:57:43PM +0200, Borislav Petkov wrote:
On Mon, Jun 16, 2014 at 04:06:08PM -0400, Vivek Goyal wrote:
There can be more than one loader and the one which claims first
to recognize the image will get to load the image. So once 32 bit
loader support comes in, it might happen that we ask 64bit loader
first and it rejects the image and then we ask 32bit loader.
What does that have to do with anything??
Say down the line you support 3 types of kernel images. 64bit bzImage,
32bit bzImage and ELF vmlinux. And there are 3 different loader
implementations in kernel. Now assume user us trying to load an ELF vmlinux
image.
Generic code will call.
arch_kexec_kernel_image_probe {
for (i = 0; i nr_file_types; i++) {
if (!kexec_file_type[i].probe)
continue;
ret = kexec_file_type[i].probe(buf, buf_len);
if (!ret) {
image-file_handler_idx = i;
return ret;
}
}
return ret;
}
This code calls into very registered loader and if nobody is ready to
load the image it returns error. Say first bzImage64 and bzImage32 bit
loader are called. They both will reject the image and vmlinux loader
will accept it.
Do we want to show all the rejection messages from bzImage64 and bzImage32
loaders. It might be too verbose to show users that before vmlinux loader
accepted the image other loaders on this arches rejcted the image.
This is very similar to binary file loaing. Looks at load_elf_binary(). If
it does not find elf header it knows it is not an ELF binary and
returns error -ENOEXEC without outputing any messages.
So these message are really debug message which tells why loader
is not accepting an image. It might not be image destined for that
loader at all.
pr_debug() allows being verbose if user wants to for debugging purposes.
You just have to make sure that CONFIG_DYNAMIC_DEBUG=y and enable verbosity
in individual file.
echo 'file kexec-bzimage.c +p' /sys/kernel/debug/dynamic_debug/control
So people are supposed to enable dynamic_debug just so that they see
*why* their image doesn't load.
Doesn't sound optimal to me.
This is one way of doing it. I can change it if you think that displaying
messages from all the loaders is fine.
Same here. We will potentially be trying multiple loaders and if every
loader prints messages for rejection by default, it is too much of
info, IMO.
For max two loaders on one architecture? I don't think so. Now you're
just arguing for the sake of it.
Well, we have 3 loaders in user space currently for x86_64. bzImage64,
bzImage32 and ELF vmlinux. So one would think that somebody might
go about implementing these in kernel space too.
I like doing memory allocations early in the functions (as far as
possible) and error out if need be. If memory is available to begin
with for all the data structures needed by this function, it is kind
of pointless to do rest of the processing.
We're talking about memory for a single void * which is ridiculous. And
I think simplifying the error paths is a much higher win than doing some
minor allocation.
Ok, I will change it.
Hmm..., If you feel strongly about it, I can make this change. I
thought I just made it easier to share the code between 32bit and
64bit by this.
Someone later can do that - right now this code is 64-bit only as far as
we're concerned and if it can be made to work on 32-bit, then people are
free to do so.
Ok.
Thanks
Vivek
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 11/13] kexec-bzImage: Support for loading bzImage using 64bit entry
On Mon, Jun 16, 2014 at 05:15:00PM -0400, Vivek Goyal wrote: Do we want to show all the rejection messages from bzImage64 and bzImage32 loaders. It might be too verbose to show users that before vmlinux loader accepted the image other loaders on this arches rejcted the image. I get all that. But, if people want to get feedback from the system about *why* their image didn't load, they absolutely have to enable dynamic debug. And this is not optimal IMO because they will have to look at the code first to see what they need to do. Or is kexec-tools going to be taught to interpret return values from the syscall? In any case, we want information about why an image fails loading to reach the user in the easiest way possible. And why should the user need to enable dynamic debug if he can get the info without doing so? Oh, and not everyone knows about dynamic debug so... And I don't think it'll be too much info - only the line which fails the check will be printed before the image loader fails so that's practically one error reason per failed image. Ok? -- Regards/Gruss, Boris. Sent from a fat crate under my desk. Formatting is fine. -- -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 11/13] kexec-bzImage: Support for loading bzImage using 64bit entry
On Mon, Jun 16, 2014 at 11:27:20PM +0200, Borislav Petkov wrote: On Mon, Jun 16, 2014 at 05:15:00PM -0400, Vivek Goyal wrote: Do we want to show all the rejection messages from bzImage64 and bzImage32 loaders. It might be too verbose to show users that before vmlinux loader accepted the image other loaders on this arches rejcted the image. I get all that. But, if people want to get feedback from the system about *why* their image didn't load, they absolutely have to enable dynamic debug. And this is not optimal IMO because they will have to look at the code first to see what they need to do. Or is kexec-tools going to be taught to interpret return values from the syscall? In most of the cases return code is -ENOEXEC so kexec-tools can't figure out what's wrong. In any case, we want information about why an image fails loading to reach the user in the easiest way possible. And why should the user need to enable dynamic debug if he can get the info without doing so? Oh, and not everyone knows about dynamic debug so... And I don't think it'll be too much info - only the line which fails the check will be printed before the image loader fails so that's practically one error reason per failed image. Ok, there will be one line of error and that's not too bad. I will convert these pr_debug() statements in bzImage_probe() to pr_err(). Thanks Vivek -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 11/13] kexec-bzImage: Support for loading bzImage using 64bit entry
On 06/15/2014 09:35 AM, Borislav Petkov wrote:
>
>> +if (memcmp((char *)>header, "HdrS", 4) != 0) {
>
> Not strncmp? "HdrS" is a string...
>
No, memcmp() is more appropriate. It is really more of a byte sequence
than a string.
It could just as easily be done as:
header->header == 0x53726448
-hpa
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 11/13] kexec-bzImage: Support for loading bzImage using 64bit entry
On Tue, Jun 03, 2014 at 09:07:00AM -0400, Vivek Goyal wrote:
> This is loader specific code which can load bzImage and set it up for
> 64bit entry. This does not take care of 32bit entry or real mode entry.
>
> 32bit mode entry can be implemented if somebody needs it.
>
> Signed-off-by: Vivek Goyal
> ---
...
> diff --git a/arch/x86/kernel/kexec-bzimage.c b/arch/x86/kernel/kexec-bzimage.c
> new file mode 100644
> index 000..0750784
> --- /dev/null
> +++ b/arch/x86/kernel/kexec-bzimage.c
> @@ -0,0 +1,269 @@
> +/*
> + * Kexec bzImage loader
> + *
> + * Copyright (C) 2014 Red Hat Inc.
> + * Authors:
> + * Vivek Goyal
> + *
> + * This source code is licensed under the GNU General Public License,
> + * Version 2. See the file COPYING for more details.
> + */
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +
> +#include
> +#include
> +
> +/*
> + * Defines lowest physical address for various segments. Not sure where
> + * exactly these limits came from. Current bzimage64 loader in kexec-tools
> + * uses these so I am retaining it. It can be changed over time as we gain
> + * more insight.
> + */
> +#define MIN_PURGATORY_ADDR 0x3000
> +#define MIN_BOOTPARAM_ADDR 0x3000
> +#define MIN_KERNEL_LOAD_ADDR 0x10
> +#define MIN_INITRD_LOAD_ADDR 0x100
> +
> +#ifdef CONFIG_X86_64
> +
> +/*
> + * This is a place holder for all boot loader specific data structure which
> + * gets allocated in one call but gets freed much later during cleanup
> + * time. Right now there is only one field but it can grow as need be.
> + */
> +struct bzimage64_data {
> + /*
> + * Temporary buffer to hold bootparams buffer. This should be
> + * freed once the bootparam segment has been loaded.
> + */
> + void *bootparams_buf;
> +};
> +
> +int bzImage64_probe(const char *buf, unsigned long len)
> +{
> + int ret = -ENOEXEC;
> + struct setup_header *header;
> +
> + /* kernel should be atleast two sector long */
two sectors
> + if (len < 2 * 512) {
> + pr_debug("File is too short to be a bzImage\n");
Those error messages are all pr_debug. Now, wouldn't we want to tell
userspace what the problem is, *when* there is one?
I.e., pr_err or pr_info is much more helpful than pr_debug IMO.
> + return ret;
> + }
> +
> + header = (struct setup_header *)(buf + offsetof(struct boot_params,
> + hdr));
Just let that stick out. The 80 cols limit is not a hard one anyway,
especially if it impairs readability.
> + if (memcmp((char *)>header, "HdrS", 4) != 0) {
Not strncmp? "HdrS" is a string...
> + pr_debug("Not a bzImage\n");
> + return ret;
> + }
> +
> + if (header->boot_flag != 0xAA55) {
> + pr_debug("No x86 boot sector present\n");
> + return ret;
> + }
> +
> + if (header->version < 0x020C) {
> + pr_debug("Must be at least protocol version 2.12\n");
> + return ret;
> + }
> +
> + if ((header->loadflags & LOADED_HIGH) == 0) {
if (!(header->loadflags.. ))
> + pr_debug("zImage not a bzImage\n");
> + return ret;
> + }
> +
> + if (!(header->xloadflags & XLF_KERNEL_64)) {
> + pr_debug("Not a bzImage64. XLF_KERNEL_64 is not set.\n");
> + return ret;
> + }
> +
> + if (!(header->xloadflags & XLF_CAN_BE_LOADED_ABOVE_4G)) {
> + pr_debug("XLF_CAN_BE_LOADED_ABOVE_4G is not set.\n");
> + return ret;
> + }
Just merge the two checks:
if ((header->xloadflags & (XLF_KERNEL_64 | XLF_CAN_BE_LOADED_ABOVE_4G))
!=
(XLF_KERNEL_64 | XLF_CAN_BE_LOADED_ABOVE_4G))
{
pr_err("Not a bzImage, xloadflags: 0x%x\n", header->xloadflags);
return ret;
}
> +
> + /* I've got a bzImage */
> + pr_debug("It's a relocatable bzImage64\n");
> + ret = 0;
> +
> + return ret;
> +}
> +
> +void *bzImage64_load(struct kimage *image, char *kernel,
> + unsigned long kernel_len,
> + char *initrd, unsigned long initrd_len,
> + char *cmdline, unsigned long cmdline_len)
Arg alignment.
> +{
> +
> + struct setup_header *header;
> + int setup_sects, kern16_size, ret = 0;
> + unsigned long setup_header_size, params_cmdline_sz;
> + struct boot_params *params;
> + unsigned long bootparam_load_addr, kernel_load_addr, initrd_load_addr;
> + unsigned long purgatory_load_addr;
> + unsigned long kernel_bufsz, kernel_memsz, kernel_align;
> + char *kernel_buf;
> + struct bzimage64_data *ldata;
> + struct kexec_entry64_regs regs64;
> + void *stack;
> + unsigned int setup_hdr_offset = offsetof(struct boot_params, hdr);
> +
> + header = (struct setup_header *)(kernel + setup_hdr_offset);
> +
Re: [PATCH 11/13] kexec-bzImage: Support for loading bzImage using 64bit entry
On Tue, Jun 03, 2014 at 09:07:00AM -0400, Vivek Goyal wrote:
This is loader specific code which can load bzImage and set it up for
64bit entry. This does not take care of 32bit entry or real mode entry.
32bit mode entry can be implemented if somebody needs it.
Signed-off-by: Vivek Goyal vgo...@redhat.com
---
...
diff --git a/arch/x86/kernel/kexec-bzimage.c b/arch/x86/kernel/kexec-bzimage.c
new file mode 100644
index 000..0750784
--- /dev/null
+++ b/arch/x86/kernel/kexec-bzimage.c
@@ -0,0 +1,269 @@
+/*
+ * Kexec bzImage loader
+ *
+ * Copyright (C) 2014 Red Hat Inc.
+ * Authors:
+ * Vivek Goyal vgo...@redhat.com
+ *
+ * This source code is licensed under the GNU General Public License,
+ * Version 2. See the file COPYING for more details.
+ */
+#include linux/string.h
+#include linux/printk.h
+#include linux/errno.h
+#include linux/slab.h
+#include linux/kexec.h
+#include linux/kernel.h
+#include linux/mm.h
+
+#include asm/bootparam.h
+#include asm/setup.h
+
+/*
+ * Defines lowest physical address for various segments. Not sure where
+ * exactly these limits came from. Current bzimage64 loader in kexec-tools
+ * uses these so I am retaining it. It can be changed over time as we gain
+ * more insight.
+ */
+#define MIN_PURGATORY_ADDR 0x3000
+#define MIN_BOOTPARAM_ADDR 0x3000
+#define MIN_KERNEL_LOAD_ADDR 0x10
+#define MIN_INITRD_LOAD_ADDR 0x100
+
+#ifdef CONFIG_X86_64
+
+/*
+ * This is a place holder for all boot loader specific data structure which
+ * gets allocated in one call but gets freed much later during cleanup
+ * time. Right now there is only one field but it can grow as need be.
+ */
+struct bzimage64_data {
+ /*
+ * Temporary buffer to hold bootparams buffer. This should be
+ * freed once the bootparam segment has been loaded.
+ */
+ void *bootparams_buf;
+};
+
+int bzImage64_probe(const char *buf, unsigned long len)
+{
+ int ret = -ENOEXEC;
+ struct setup_header *header;
+
+ /* kernel should be atleast two sector long */
two sectors
+ if (len 2 * 512) {
+ pr_debug(File is too short to be a bzImage\n);
Those error messages are all pr_debug. Now, wouldn't we want to tell
userspace what the problem is, *when* there is one?
I.e., pr_err or pr_info is much more helpful than pr_debug IMO.
+ return ret;
+ }
+
+ header = (struct setup_header *)(buf + offsetof(struct boot_params,
+ hdr));
Just let that stick out. The 80 cols limit is not a hard one anyway,
especially if it impairs readability.
+ if (memcmp((char *)header-header, HdrS, 4) != 0) {
Not strncmp? HdrS is a string...
+ pr_debug(Not a bzImage\n);
+ return ret;
+ }
+
+ if (header-boot_flag != 0xAA55) {
+ pr_debug(No x86 boot sector present\n);
+ return ret;
+ }
+
+ if (header-version 0x020C) {
+ pr_debug(Must be at least protocol version 2.12\n);
+ return ret;
+ }
+
+ if ((header-loadflags LOADED_HIGH) == 0) {
if (!(header-loadflags.. ))
+ pr_debug(zImage not a bzImage\n);
+ return ret;
+ }
+
+ if (!(header-xloadflags XLF_KERNEL_64)) {
+ pr_debug(Not a bzImage64. XLF_KERNEL_64 is not set.\n);
+ return ret;
+ }
+
+ if (!(header-xloadflags XLF_CAN_BE_LOADED_ABOVE_4G)) {
+ pr_debug(XLF_CAN_BE_LOADED_ABOVE_4G is not set.\n);
+ return ret;
+ }
Just merge the two checks:
if ((header-xloadflags (XLF_KERNEL_64 | XLF_CAN_BE_LOADED_ABOVE_4G))
!=
(XLF_KERNEL_64 | XLF_CAN_BE_LOADED_ABOVE_4G))
{
pr_err(Not a bzImage, xloadflags: 0x%x\n, header-xloadflags);
return ret;
}
+
+ /* I've got a bzImage */
+ pr_debug(It's a relocatable bzImage64\n);
+ ret = 0;
+
+ return ret;
+}
+
+void *bzImage64_load(struct kimage *image, char *kernel,
+ unsigned long kernel_len,
+ char *initrd, unsigned long initrd_len,
+ char *cmdline, unsigned long cmdline_len)
Arg alignment.
+{
+
+ struct setup_header *header;
+ int setup_sects, kern16_size, ret = 0;
+ unsigned long setup_header_size, params_cmdline_sz;
+ struct boot_params *params;
+ unsigned long bootparam_load_addr, kernel_load_addr, initrd_load_addr;
+ unsigned long purgatory_load_addr;
+ unsigned long kernel_bufsz, kernel_memsz, kernel_align;
+ char *kernel_buf;
+ struct bzimage64_data *ldata;
+ struct kexec_entry64_regs regs64;
+ void *stack;
+ unsigned int setup_hdr_offset = offsetof(struct boot_params, hdr);
+
+ header = (struct setup_header *)(kernel + setup_hdr_offset);
+ setup_sects =
Re: [PATCH 11/13] kexec-bzImage: Support for loading bzImage using 64bit entry
On 06/15/2014 09:35 AM, Borislav Petkov wrote:
+if (memcmp((char *)header-header, HdrS, 4) != 0) {
Not strncmp? HdrS is a string...
No, memcmp() is more appropriate. It is really more of a byte sequence
than a string.
It could just as easily be done as:
header-header == 0x53726448
-hpa
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
