Re: [PATCH v4 1/2] firmware_loader: introduce new API - request_firmware_direct_full_path()
On Tue, Apr 14, 2015 at 11:56:26AM -0400, Andy Lutomirski wrote: On Tue, Apr 14, 2015 at 10:08 AM, Greg Kroah-Hartman gre...@linuxfoundation.org wrote: On Tue, Apr 14, 2015 at 05:44:55PM +0800, Kweh, Hock Leong wrote: From: Kweh, Hock Leong hock.leong.k...@intel.com Introduce this new API for loading firmware from a specific location instead of /lib/firmware/ by providing a full path to the firmware file. Ick, why would we want this? Because this mechanism should still work even if /lib is unwriteable (e.g it's on squashfs or a read-only NFS root). Why would a filesystem need to be writable to read a firmware blob from? In this regard, UEFI capsules are very much unlike firmware_class firmware. firmware_class firmwise is kind of like device drivers; it generally comes from the same vendor as your kernel image and /lib/modules, and it can be updated by the same mechanism. UEFI capsules, on the other hand, are one-time things that should be loaded at the explicit request of the admin. Just like BIOS updates, which use the firmware interface. There is no reason whatsoever that they should exist on persistent storage, and, in fact, there's a very good reason that they should not. On little embedded devices, which will apparently be the initial users of this code, keeping the capsules around is a waste of valuable space. This is why I think that the right approach would be to avoid using firmware_class entirely for this. IMO a simple_char device would be the way to go (hint hint...) but other simple approaches are certainly possible. A char device would be present all the time, like a sysfs file to write the firmware to, so I don't see the difference here. For a char device, you would just do the normal open/write/close, just like for the firmware interface, what is the difference? thanks, greg k-h -- To unsubscribe from this list: send the line unsubscribe linux-efi in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4 2/2] efi: an sysfs interface for user to update efi firmware
On Tue, Apr 14, 2015 at 11:52:48AM -0400, Andy Lutomirski wrote: On Tue, Apr 14, 2015 at 10:09 AM, Greg Kroah-Hartman gre...@linuxfoundation.org wrote: On Tue, Apr 14, 2015 at 05:44:56PM +0800, Kweh, Hock Leong wrote: From: Kweh, Hock Leong hock.leong.k...@intel.com Introducing a kernel module to expose capsule loader interface for user to upload capsule binaries. This module leverage the request_firmware_direct_full_path() to obtain the binary at a specific path input by user. Example method to load the capsule binary: echo -n /path/to/capsule/binary /sys/devices/platform/efi_capsule_loader/capsule_loader Ick, why not just have the firmware file location present, and copy it to the sysfs file directly from userspace, instead of this two-step process? Because it's not at all obvious how error handling should work in that case. I don't understand how the error handling is any different. The kernel ends up copying the data in through the firmware interface both ways, we just aren't creating yet-another-firmware-path in the system. thanks, greg k-h -- To unsubscribe from this list: send the line unsubscribe linux-efi in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4 2/2] efi: an sysfs interface for user to update efi firmware
On Apr 15, 2015 6:20 AM, Greg Kroah-Hartman gre...@linuxfoundation.org wrote: On Tue, Apr 14, 2015 at 11:52:48AM -0400, Andy Lutomirski wrote: On Tue, Apr 14, 2015 at 10:09 AM, Greg Kroah-Hartman gre...@linuxfoundation.org wrote: On Tue, Apr 14, 2015 at 05:44:56PM +0800, Kweh, Hock Leong wrote: From: Kweh, Hock Leong hock.leong.k...@intel.com Introducing a kernel module to expose capsule loader interface for user to upload capsule binaries. This module leverage the request_firmware_direct_full_path() to obtain the binary at a specific path input by user. Example method to load the capsule binary: echo -n /path/to/capsule/binary /sys/devices/platform/efi_capsule_loader/capsule_loader Ick, why not just have the firmware file location present, and copy it to the sysfs file directly from userspace, instead of this two-step process? Because it's not at all obvious how error handling should work in that case. I don't understand how the error handling is any different. The kernel ends up copying the data in through the firmware interface both ways, we just aren't creating yet-another-firmware-path in the system. If I run uefi-update-capsule foo.bin, I want it to complain if the UEFI call fails. In contrast, if I boot and my ath10k firmware is bad, there's no explicit user interaction that should fail; instead I have no network device and I get to read the logs and figure out why. IOW bad volatile device firmware is just like a bad device driver, whereas bad UEFI capsules are problems that should be reported to whatever tried to send them to UEFI. --Andy thanks, greg k-h -- To unsubscribe from this list: send the line unsubscribe linux-efi in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4 1/2] firmware_loader: introduce new API - request_firmware_direct_full_path()
[Bah, I'm really bad at email today. Trying again.] On Apr 15, 2015 6:15 AM, Greg Kroah-Hartman gre...@linuxfoundation.org wrote: On Tue, Apr 14, 2015 at 11:56:26AM -0400, Andy Lutomirski wrote: On Tue, Apr 14, 2015 at 10:08 AM, Greg Kroah-Hartman gre...@linuxfoundation.org wrote: On Tue, Apr 14, 2015 at 05:44:55PM +0800, Kweh, Hock Leong wrote: From: Kweh, Hock Leong hock.leong.k...@intel.com Introduce this new API for loading firmware from a specific location instead of /lib/firmware/ by providing a full path to the firmware file. Ick, why would we want this? Because this mechanism should still work even if /lib is unwriteable (e.g it's on squashfs or a read-only NFS root). Why would a filesystem need to be writable to read a firmware blob from? Because someone would need to temporarily put the image there. In practice, these blobs will come from vendors, signed, online using ESRT magic. Imagine a CoreOS system. When a UEFI update needed on 1% of a deployment's metal is published, no one is going to want to push out a new core CoreOS image. Instead they'll want to run the update on that 1% of nodes and be done with it. To be fair, and for those that didn't follow all the various discussions, it's unclear that this mechanism will ever be useful in the x86 server space. There's some reason to believe that MS will only issue UpdateCapsule before ExitBootServices and that firmware vendors will therefore disallow UpdateCapsule after ExitBootServices. The fwupd crowd is (I think) planning on bypassing this entirely and using the boot loader to update firmware. Regardless, those things aren't going to live in /lib, but they'll have to write *something* to a FAT filesystem because they have no choice. More sensible firmwares will support the runtime stuff, and atomic systems (RHEL Atomic, OSTree, CoreOS, whatever Docker's working on, whatever Sandstorm is working on (?), etc.) should probably be as well supported in the kernel as we can manage. In this regard, UEFI capsules are very much unlike firmware_class firmware. firmware_class firmwise is kind of like device drivers; it generally comes from the same vendor as your kernel image and /lib/modules, and it can be updated by the same mechanism. UEFI capsules, on the other hand, are one-time things that should be loaded at the explicit request of the admin. Just like BIOS updates, which use the firmware interface. What BIOS updates? There's flashrom, which quite sensibly reads its input in user space. The other example I found is dell_rbu, which does a complicated packetized update thing and explicitly says in the docs: This method makes sure that all the packets get to the driver in a single operation. The mechanism seems quite awkward to me. There is no reason whatsoever that they should exist on persistent storage, and, in fact, there's a very good reason that they should not. On little embedded devices, which will apparently be the initial users of this code, keeping the capsules around is a waste of valuable space. This is why I think that the right approach would be to avoid using firmware_class entirely for this. IMO a simple_char device would be the way to go (hint hint...) but other simple approaches are certainly possible. A char device would be present all the time, like a sysfs file to write the firmware to, so I don't see the difference here. For a char device, you would just do the normal open/write/close, just like for the firmware interface, what is the difference? You wouldn't use open/write/close; you'd do it atomically with a single ioctl. That gives userspace an error code. It would also be possible to require a single write(2) call, but that seems to defeat most of the purpose of using open/write/close (namely the ability to script it with cat). --Andy -- To unsubscribe from this list: send the line unsubscribe linux-efi in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] x86_64/efi: enforce 32 bit address for command line buffer
On Wed, 15 Apr, at 11:56:05AM, Roy Franz wrote: Yeah, I guess it shouldn't surprise me that there is support for 64 bit addresses there :) I'l spin another patch that sets boot_params-ext_cmd_line_ptr with the upper 32 bits of the address. Should I conditionalize this with #ifdef CONFIG_X86_64, or just do it unconditionally, with it being a NOP on 32 bit? (I guess I may end up with an extra cast for the 32 bit case) Unconditionally is best, I'm loathe to introduce #ifdefs unless absolutely necessary. Thanks! -- Matt Fleming, Intel Open Source Technology Center -- To unsubscribe from this list: send the line unsubscribe linux-efi in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] x86_64/efi: enforce 32 bit address for command line buffer
On Wed, Apr 15, 2015 at 6:18 AM, Matt Fleming m...@codeblueprint.co.uk wrote:
On Tue, 14 Apr, at 05:45:52PM, Roy Franz wrote:
The boot_params structure has a 32 bit field for storing the address of
the kernel command line. When the EFI stub allocates memory for the command
line, it allocates at as low and address as possible, but does not ensure
that the address of memory allocated is below 4G.
This patch enforces this limit, and the stub now returns an error if the
command line buffer is allocated at too high of an address.
For 32 bit systems, the EFI mandated 1-1 memory mapping ensures
that all memory is 32 bit addressable, so we don't have a problem.
Also, mixed-mode booting on EFI platforms does not use the stub
code, so we don't need to handle the case of booting a 32 bit
kernel on a 64 bit EFI platform.
Signed-off-by: Roy Franz roy.fr...@linaro.org
---
arch/x86/boot/compressed/eboot.c | 13 +
1 file changed, 13 insertions(+)
diff --git a/arch/x86/boot/compressed/eboot.c
b/arch/x86/boot/compressed/eboot.c
index ef17683..82dbe27 100644
--- a/arch/x86/boot/compressed/eboot.c
+++ b/arch/x86/boot/compressed/eboot.c
@@ -1108,6 +1108,19 @@ struct boot_params *make_boot_params(struct
efi_config *c)
cmdline_ptr = efi_convert_cmdline(sys_table, image, options_size);
if (!cmdline_ptr)
goto fail;
+
+#ifdef CONFIG_X86_64
+ /*
+ * hdr-cmd_line_ptr is a 32 bit field, so on 64 bit systems we need
+ * to ensure that the allocated buffer for the commandline is 32 bit
+ * addressable.
+ */
+ if ((u64)(cmdline_ptr) + options_size (u64)U32_MAX) {
+ efi_printk(sys_table, Failed to alloc lowmem for command
line\n);
+ efi_free(sys_table, options_size, (unsigned long)cmdline_ptr);
+ goto fail;
+ }
+#endif /* CONFIG_X86_64 */
hdr-cmd_line_ptr = (unsigned long)cmdline_ptr;
hdr-ramdisk_image = 0;
Good catch. But actually, we have boot_params-ext_cmd_line_ptr for
exactly this problem.
So yes, that's a valid bug, but I don't think this is how we should fix
it.
Yeah, I guess it shouldn't surprise me that there is support for 64
bit addresses there :)
I'l spin another patch that sets boot_params-ext_cmd_line_ptr with
the upper 32 bits of the address.
Should I conditionalize this with #ifdef CONFIG_X86_64, or just do it
unconditionally, with it being a NOP
on 32 bit? (I guess I may end up with an extra cast for the 32 bit case)
Roy
--
Matt Fleming, Intel Open Source Technology Center
--
To unsubscribe from this list: send the line unsubscribe linux-efi in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4 2/2] efi: an sysfs interface for user to update efi firmware
On Wed, Apr 15, 2015 at 8:45 AM, Andy Lutomirski l...@amacapital.net wrote: On Apr 15, 2015 6:20 AM, Greg Kroah-Hartman gre...@linuxfoundation.org wrote: On Tue, Apr 14, 2015 at 11:52:48AM -0400, Andy Lutomirski wrote: On Tue, Apr 14, 2015 at 10:09 AM, Greg Kroah-Hartman gre...@linuxfoundation.org wrote: On Tue, Apr 14, 2015 at 05:44:56PM +0800, Kweh, Hock Leong wrote: From: Kweh, Hock Leong hock.leong.k...@intel.com Introducing a kernel module to expose capsule loader interface for user to upload capsule binaries. This module leverage the request_firmware_direct_full_path() to obtain the binary at a specific path input by user. Example method to load the capsule binary: echo -n /path/to/capsule/binary /sys/devices/platform/efi_capsule_loader/capsule_loader Ick, why not just have the firmware file location present, and copy it to the sysfs file directly from userspace, instead of this two-step process? Because it's not at all obvious how error handling should work in that case. I don't understand how the error handling is any different. The kernel ends up copying the data in through the firmware interface both ways, we just aren't creating yet-another-firmware-path in the system. If I run uefi-update-capsule foo.bin, I want it to complain if the UEFI call fails. In contrast, if I boot and my ath10k firmware is bad, there's no explicit user interaction that should fail; instead I have no network device and I get to read the logs and figure out why. IOW bad volatile device firmware is just like a bad device driver, whereas bad UEFI capsules are problems that should be reported to whatever tried to send them to UEFI. --Andy There are several types of errors that can be returned by UpdateCapsule(), allowing us to distinguish between capsules that are not supported by the platform, capsules that must be updated at boot time, and capsule updates that failed due to a device error. I think we really do want this type of information returned to capsule loader. Roy thanks, greg k-h -- To unsubscribe from this list: send the line unsubscribe linux-efi in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line unsubscribe linux-efi in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] efi: stub: use a pool allocation for the cmdline
On 15 April 2015 at 11:55, Matt Fleming m...@codeblueprint.co.uk wrote: On Fri, 10 Apr, at 07:14:34PM, Ard Biesheuvel wrote: It is not such a big deal: the memory is reclaimed anyway, I was just trying to reduce the fragmentation a bit, and trying to avoid efi_xxx_alloc() which are substantially heavier than calling allocate_pool() or allocate_pages() directly. Out of curiosity, do you have any runtime numbers to backup the claim that allocate_*() is substantially more lightweight? No, I do not. But since efi_[high|low]_alloc() call efi_get_memory_map() internally, which itself calls allocate_pool() at least twice [typically], and then iterate over the entire memory map to select a suitable slot which gets allocated using allocate_pages(), calling either of allocate_[pool|pages] directly is arguably more lightweight. But claiming they are 'substantially heavier' is unsubstantiated. -- Ard. -- To unsubscribe from this list: send the line unsubscribe linux-efi in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] efi: stub: use a pool allocation for the cmdline
On Fri, 10 Apr, at 07:14:34PM, Ard Biesheuvel wrote: It is not such a big deal: the memory is reclaimed anyway, I was just trying to reduce the fragmentation a bit, and trying to avoid efi_xxx_alloc() which are substantially heavier than calling allocate_pool() or allocate_pages() directly. Out of curiosity, do you have any runtime numbers to backup the claim that allocate_*() is substantially more lightweight? -- Matt Fleming, Intel Open Source Technology Center -- To unsubscribe from this list: send the line unsubscribe linux-efi in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4 1/2] firmware_loader: introduce new API - request_firmware_direct_full_path()
On Tue, 14 Apr, at 06:18:33PM, Borislav Petkov wrote: On Tue, Apr 14, 2015 at 11:56:26AM -0400, Andy Lutomirski wrote: This is why I think that the right approach would be to avoid using firmware_class entirely for this. IMO a simple_char device would be the way to go (hint hint...) but other simple approaches are certainly possible. Btw, didn't mfleming want to try using capsules for other funky stuff like early logging and pstore-like logging in panic time so that we can read out crash data on the next boot? Yeah, exactly. Being able to pass random data blobs to the capsule internals allows the kernel to support cool things we haven't conceived of yet, and where we want to use the capsule's in-memory persistence across reboot to pass data to the next kernel. The panic code paths is just one example. Which would mean that capsules would need a completely different interface, something new for shuffling lotsa binary data in and out of the kernel and in and out of the UEFI... Which would make the firmware_class thing completely inappropriate for that. Well, I haven't come across a scenario where you need a brand new interface for getting it *out* of the kernel again. And I'm happy enough with these patches for passing data in. -- Matt Fleming, Intel Open Source Technology Center -- To unsubscribe from this list: send the line unsubscribe linux-efi in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4 1/2] firmware_loader: introduce new API - request_firmware_direct_full_path()
On Wed, Apr 15, 2015 at 11:14:55AM +0100, Matt Fleming wrote: Well, I haven't come across a scenario where you need a brand new interface for getting it *out* of the kernel again. Well, how are we going to read crash data on next boot then? EFI var or what? Are we going to have a generic interface like /sys/.../capsule/... or how are we imagining this to look like? Can you give a detailed example please... -- Regards/Gruss, Boris. ECO tip #101: Trim your mails when you reply. -- -- To unsubscribe from this list: send the line unsubscribe linux-efi in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4 1/2] firmware_loader: introduce new API - request_firmware_direct_full_path()
On Wed, 15 Apr, at 12:18:05PM, Borislav Petkov wrote: On Wed, Apr 15, 2015 at 11:14:55AM +0100, Matt Fleming wrote: Well, I haven't come across a scenario where you need a brand new interface for getting it *out* of the kernel again. Well, how are we going to read crash data on next boot then? EFI var or what? Are we going to have a generic interface like /sys/.../capsule/... or how are we imagining this to look like? You can read it out in userspace using the existing pstorefs code. The last thing we need to do is introduce more userspace APIs ;-) It's possible (and desirable) to separate the input interface from output. I've written patches in the past where the EFI kernel subsystem discovers capsules with a specific GUID reserved for crash data, and then hands that data to the pstore subsystem. Things are then exposed via pstorefs. The capsule code would just be another backend to pstore, similar to how the EFI variable code works today. I am in no way advocating for yet another crash API. -- Matt Fleming, Intel Open Source Technology Center -- To unsubscribe from this list: send the line unsubscribe linux-efi in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH v4 2/2] efi: an sysfs interface for user to update efi firmware
-Original Message-
From: Greg Kroah-Hartman [mailto:gre...@linuxfoundation.org]
Sent: Tuesday, April 14, 2015 10:09 PM
On Tue, Apr 14, 2015 at 05:44:56PM +0800, Kweh, Hock Leong wrote:
From: Kweh, Hock Leong hock.leong.k...@intel.com
Introducing a kernel module to expose capsule loader interface
for user to upload capsule binaries. This module leverage the
request_firmware_direct_full_path() to obtain the binary at a
specific path input by user.
Example method to load the capsule binary:
echo -n /path/to/capsule/binary
/sys/devices/platform/efi_capsule_loader/capsule_loader
Ick, why not just have the firmware file location present, and copy it
to the sysfs file directly from userspace, instead of this two-step
process?
Err I may not catch your meaning correctly. Are you trying to say
that you would prefer the user to perform:
cat file.bin /sys/.../capsule_loader
instead of
echo -n /path/to/binary /sys//capsule_laoder
The reason we stick with the firmware_class is because we don't
want to replicate a code which already mature and has open API
for developer to use.
+/*
+ * To remove this kernel module, just perform:
+ * rmmod efi_capsule_loader.ko
This comment is not needed.
Okay, I will remove this.
+ */
+static void __exit efi_capsule_loader_exit(void)
+{
+ platform_device_unregister(efi_capsule_pdev);
This is not a platform device, don't abuse that interface please.
greg k-h
Okay, so you would recommend to use device_register() for this case?
Or you would think that this is more suitable to use class_register()?
Thanks Regards,
Wilson
--
To unsubscribe from this list: send the line unsubscribe linux-efi in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
