RE: Re: [PATCH v2 3/3] efi: Capsule update with user helper interface

RE: Re: [PATCH v2 3/3] efi: Capsule update with user helper interface

[Kweh, Hock Leong]

[-- Attachment #1: Type: text/plain, Size: 5650 bytes --]

> -----Original Message-----
> From: Kweh, Hock Leong
> Sent: Tuesday, February 24, 2015 6:54 PM
> 
> In callbackfn_efi_capsule, you call request_firmware_nowait.  When that
> callback is invoked, I think that the /sys/class/firmware/efi-capsule-file
> directory doesn't exist at all.
> If the callback takes longer than it takes your script to make it through a full
> iteration, then it will try uploading the second capsule before the firmware
> class directory is there, so it will fail.
> 
> But I just realized that your script has a loop below to handle that.
> It's this:
> 
>                  oldtime=$(date +%S)
>                  oldtime=$(((time + 2) % 60))
>                  until [ -f /sys/class/firmware/efi-capsule-file/loading ]
>                  do
>                          newtime=$(date +%S)
>                          if [ $newtime -eq $oldtime ]
>                          then
>                                  break
>                          fi
>                  done
> 
> Aside from the fact that this loop itself is racy (it may loop forever if
> something goes wrong in the kernel, since $newtime -eq $oldtime may
> never happen), it should help, if you're lucky.  But there's another bug.
> 
> 
> Here's the race:
> 
> User:
> echo 1 > /sys/class/firmware/efi-capsule-file/loading
> cat capsule1 > /sys/class/firmware/efi-capsule-file/data
> echo 0 > /sys/class/firmware/efi-capsule-file/loading
> Kernel: Be a little slow here due to preemption or whatever.
> 
> User:
> -f /sys/class/firmware/efi-capsule-file/loading returns true capsules_loaded
> == 0 Assume failure, incorrectly
> 
> Kernel: catch up and increment capsules_loaded.
> 
> If these patches get applied, then I think that the protocol needs to be
> documented in Documentation/ABI.  It should say something like:
> 
> To upload an EFI capsule, do this:
> 
> Write 1 to /sys/class/firmware/efi-capsule-file/loading
> Write the capsule to /sys/class/firmware/efi-capsule-file/data
> Write 0 to /sys/class/firmware/efi-capsule-file/loading
> 
> Make sure that /sys/class/firmware/efi-capsule-file disappears and comes
> back, perhaps by cd-ing there and waiting for all the files in the directory to
> go away.
> 
> Then, and only then, read capsules_loaded to detect success.
> 
> 
> Once you've written that doc, please seriously consider whether this
> interface is justifiable.  I think it sucks.
> 
> --Andy

Hi All,

After some internal discussion and re-design prototyping & testing on
this efi capsule interface kernel module, I would like to start a discussion
here on the new idea and wish to get input for the implementation and
eventually upstream.

This new idea will expose 2 read-only file notes from the efi capsule kernel
module - "capsule_ticket" and "capsule_report". The "capsule_ticket" will
let user to obtain a NON-ZERO number and then perform a mutex lock. The
obtained number will be used later for "capsule_report" status checking.
Unlock mutex is done after "echo 0 > loading" at the end of callback function.

So the process steps basically look like this:
1.) cat capsule_ticket    =======> acquire a number and lock mutex then
                                                                     expose firmware_class user helper
                                                                     interface as well as start timer for timeout
                                                                     counting
2.) repeat step 1 if obtained a "0" number
3.) echo 1 > loading
4.) cat bin > data
5.) echo 0 > loading        =======> stop the timeout counting  then unlock
                                                                      mutex at the end of callback routine 
6.) cat capsule_report   =======> grep the number acquired from beginning
                                                                      for checking succeeded/failed

The ticket numbering is starting from 1 to 999 and then rolls over from
1 to 999 again. The "capsule_report" will show the whole list of the acquired
entries and will be limited to 128 entries which is capped within one page buffer.

The format of this "capsule_report" output will look like:
"Capsule $num uploads successful/failed/aborted"

There is a 2mins time out (internal) for each of the number acquired.
After that, the interface will be closed/disappeared.

I have attached a SAMPLE script and kernel module code in case you are not
able to understand my description above.

I believe this would really take care of the multi-capsule update concurrently
issue and also asynchronous status reporting issue. Besides, this will indirectly
take care the module unload issue with "rmmod" and not "rmmod -f".
What do you guys think?

----------------------------------------------------------------------------------------------

There is another idea during internal discussion. The 2nd idea would require
some changes to drivers/base/firmware_class.c user helper interface for the
mutex locking as well as status return. Mutex lock will be performed while
doing the 'echo 1 > loading' session and status return will be performed after
the 'echo 0 > loading'. The mutex unlock will be done at 'echo 0 > loading' too
or may be at the status reading session for avoid asynchronous reporting.

This will likely changed the original firmware class user helper interface design
behavior. But this approach could save the 2 file notes from the 1st approach.


Which of the approach do you guys prefer? Thanks.


Regards,
Wilson


[-- Attachment #2: newcapsule.txt --]
[-- Type: text/plain, Size: 947 bytes --]

#!/bin/sh

for arg in "$@"
do
	if [ -f $arg ]
	then
		retry=0
		until [ ! $num = '0' ]
		do
			sleep 1
			num=$(cat /sys/devices/platform/efi_capsule_user_helper/capsule_ticket)
			if [ $retry -le 130 ]
			then
				retry=$((retry + 1))
			else
				echo "Failed to acquire capsule ticket"
				exit 1
			fi
		done

		oldtime=$(date +%S)
		oldtime=$(((time + 2) % 60))
		until [ -f /sys/class/firmware/efi-capsule-file/loading ]
		do
			newtime=$(date +%S)
			if [ $newtime -eq $oldtime ]
			then
				echo "Failed to expose user helper interface"
				exit 1
			fi
		done

		echo 1 > /sys/class/firmware/efi-capsule-file/loading
		cat $arg > /sys/class/firmware/efi-capsule-file/data
		echo 0 > /sys/class/firmware/efi-capsule-file/loading

		until [ -n "$result" ]
		do
			result=$(cat /sys/devices/platform/efi_capsule_user_helper/capsule_report | grep $num)
		done
		echo $result " (" $arg ")"
	else
		echo "File $arg not found !!"
	fi
done
exit 0

[-- Attachment #3: 0002-efi-Capsule-update-with-user-helper-interface.patch --]
[-- Type: application/octet-stream, Size: 12646 bytes --]

From 7395877e6895231255baab6838d8b92e44f0342b Mon Sep 17 00:00:00 2001
Message-Id: <7395877e6895231255baab6838d8b92e44f0342b.1424244086.git.hock.leong.kweh@intel.com>
In-Reply-To: <90701184a919bb58c8c71cd5d9d2e28cc722e5d1.1424244086.git.hock.leong.kweh@intel.com>
References: <90701184a919bb58c8c71cd5d9d2e28cc722e5d1.1424244086.git.hock.leong.kweh@intel.com>
From: "Kweh, Hock Leong" <hock.leong.kweh@intel.com>
Date: Tue, 2 Sep 2014 22:10:42 +0800
Subject: [PATCH 2/2] efi: Capsule update with user helper interface

Introducing a kernel module to expose user helper interface for
user to upload capsule binaries. This module leverage the
request_firmware_nowait() to expose an interface to user.

Example steps to load the capsule binary:
1.) num=$(cat /sys/devices/platform/efi_capsule_user_helper/capsule_ticket)
2.) continue to acquire the number if the number is '0'
3.) echo 1 > /sys/class/firmware/efi-capsule-file/loading
4.) cat capsule.bin > /sys/class/firmware/efi-capsule-file/data
5.) echo 0 > /sys/class/firmware/efi-capsule-file/loading
6.) cat /sys/devices/platform/efi_capsule_user_helper/capsule_report | grep $num

Whereas, this module does not allow the capsule binaries to be
obtained from the request_firmware library search path. If any
capsule binary loaded from firmware seach path, the module will
abort the capsule storing function.

Besides the request_firmware user helper interface, this module
also exposes two read only file notes to interact with user.

- 'capsule_ticket' allows user to acquire a number and enabling
firmware_class user helper interface for uploading the capsule
binary. The ticket number is starting from 001 to 999. If
obtained a number of '0' means someone is performing the loading.
Please wait until the loading is done and obtained the number in
the correct range then perform to the next steps. A 2 minutes
time out is implemented for each loading activity.

- 'capsule_report' allows user to verify the status of the
capsule loading. User can verify by using the number acquired at
the 1st step with 'capsule_ticket'.

This design cater the preventing of multi capsules concurrent
loading problem as well as the status reporting synchronization.

Cc: Matt Fleming <matt.fleming@intel.com>
Signed-off-by: Kweh, Hock Leong <hock.leong.kweh@intel.com>
---
 drivers/firmware/efi/Kconfig                   |   13 +
 drivers/firmware/efi/Makefile                  |    1 +
 drivers/firmware/efi/efi-capsule-user-helper.c |  316 ++++++++++++++++++++++++
 3 files changed, 330 insertions(+)
 create mode 100644 drivers/firmware/efi/efi-capsule-user-helper.c

diff --git a/drivers/firmware/efi/Kconfig b/drivers/firmware/efi/Kconfig
index f712d47..7dc814e 100644
--- a/drivers/firmware/efi/Kconfig
+++ b/drivers/firmware/efi/Kconfig
@@ -60,6 +60,19 @@ config EFI_RUNTIME_WRAPPERS
 config EFI_ARMSTUB
 	bool
 
+config EFI_CAPSULE_USER_HELPER
+	tristate "EFI capsule user mode helper"
+	depends on EFI
+	select FW_LOADER
+	select FW_LOADER_USER_HELPER
+	help
+	  This option exposes the user mode helper interface for user to load
+	  EFI capsule binary and update the EFI firmware after system reboot.
+	  This feature does not support auto locating capsule binaries at the
+	  firmware lib search path.
+
+	  If unsure, say N.
+
 endmenu
 
 config UEFI_CPER
diff --git a/drivers/firmware/efi/Makefile b/drivers/firmware/efi/Makefile
index 698846e..63f6910 100644
--- a/drivers/firmware/efi/Makefile
+++ b/drivers/firmware/efi/Makefile
@@ -8,3 +8,4 @@ obj-$(CONFIG_UEFI_CPER)			+= cper.o
 obj-$(CONFIG_EFI_RUNTIME_MAP)		+= runtime-map.o
 obj-$(CONFIG_EFI_RUNTIME_WRAPPERS)	+= runtime-wrappers.o
 obj-$(CONFIG_EFI_STUB)			+= libstub/
+obj-$(CONFIG_EFI_CAPSULE_USER_HELPER)	+= efi-capsule-user-helper.o
diff --git a/drivers/firmware/efi/efi-capsule-user-helper.c b/drivers/firmware/efi/efi-capsule-user-helper.c
new file mode 100644
index 0000000..05838f5
--- /dev/null
+++ b/drivers/firmware/efi/efi-capsule-user-helper.c
@@ -0,0 +1,316 @@
+/*
+ * EFI capsule user mode helper interface driver.
+ *
+ * Copyright 2015 Intel Corporation
+ *
+ * This file is part of the Linux kernel, and is made available under
+ * the terms of the GNU General Public License version 2.
+ */
+
+#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
+
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/delay.h>
+#include <linux/highmem.h>
+#include <linux/slab.h>
+#include <linux/mutex.h>
+#include <linux/reboot.h>
+#include <linux/efi.h>
+#include <linux/firmware.h>
+#include <linux/workqueue.h>
+
+#define CAPSULE_NAME "efi-capsule-file"
+#define DEV_NAME "efi_capsule_user_helper"
+#define STRING_INTEGER_MAX_LENGTH 13
+#define MAX_SYSFS_BUF_LENGTH 4096
+#define CAPSULE_USER_HELPER_TIMEOUT 120
+#define CAPSULE_REPORT_MAX_STR_SIZE 32
+#define CAPSULE_RESULT_PASS "successful"
+#define CAPSULE_RESULT_FAIL "failed    "
+#define CAPSULE_RESULT_EXIT "aborted   "
+
+static DEFINE_MUTEX(user_helper_lock);
+static int capsule_count;
+static struct platform_device *efi_capsule_pdev;
+static struct delayed_work timeout_work;
+static unsigned char *report_buf;
+static int report_buf_head_off, report_buf_write_off;
+
+/*
+ * This function will store the capsule binary and pass it to
+ * efi_capsule_update() API in capsule.c
+ */
+static int efi_capsule_store(const struct firmware *fw)
+{
+	int i;
+	int ret;
+	int count = fw->size;
+	int copy_size = (fw->size > PAGE_SIZE) ? PAGE_SIZE : fw->size;
+	int pages_needed = ALIGN(fw->size, PAGE_SIZE) >> PAGE_SHIFT;
+	struct page **pages;
+	void *page_data;
+	efi_capsule_header_t *capsule = NULL;
+
+	pages = kmalloc_array(pages_needed, sizeof(void *), GFP_KERNEL);
+	if (!pages) {
+		pr_err("%s: kmalloc_array() failed\n", __func__);
+		return -ENOMEM;
+	}
+
+	for (i = 0; i < pages_needed; i++) {
+		pages[i] = alloc_page(GFP_KERNEL);
+		if (!pages[i]) {
+			pr_err("%s: alloc_page() failed\n", __func__);
+			--i;
+			ret = -ENOMEM;
+			goto failed;
+		}
+	}
+
+	for (i = 0; i < pages_needed; i++) {
+		page_data = kmap(pages[i]);
+		memcpy(page_data, fw->data + (i * PAGE_SIZE), copy_size);
+
+		if (i == 0)
+			capsule = (efi_capsule_header_t *)page_data;
+		else
+			kunmap(pages[i]);
+
+		count -= copy_size;
+		if (count < PAGE_SIZE)
+			copy_size = count;
+	}
+
+	ret = efi_capsule_update(capsule, pages);
+	if (ret) {
+		pr_err("%s: efi_capsule_update() failed\n", __func__);
+		--i;
+		goto failed;
+	}
+	kunmap(pages[0]);
+
+	/*
+	 * we cannot free the pages here due to reboot need that data
+	 * maintained.
+	 */
+	return 0;
+
+failed:
+	while (i >= 0)
+		__free_page(pages[i--]);
+	kfree(pages);
+	return ret;
+}
+
+/*
+ * This callback function will be called by request_firmware_nowait() when
+ * user has loaded the capsule binary or aborted user helper interface
+ */
+static void callbackfn_efi_capsule(const struct firmware *fw, void *context)
+{
+	int ret;
+
+	if (fw) {
+		/*
+		 * Binary which is not getting from user helper interface,
+		 * the fw->pages is equal to NULL
+		 */
+		if (!fw->pages) {
+			pr_err("%s: ERROR: Capsule binary '%s' at %s\n",
+			       __func__, CAPSULE_NAME,
+			       "firmware lib search path are not supported");
+			pr_err("user helper interface disabled\n");
+			snprintf(report_buf + report_buf_write_off,
+				 CAPSULE_REPORT_MAX_STR_SIZE,
+				 "Capsule %03d uploads %s\n",
+				 capsule_count,
+				 CAPSULE_RESULT_FAIL);
+		} else {
+			ret = efi_capsule_store(fw);
+			if (!ret) {
+				snprintf(report_buf + report_buf_write_off,
+					 CAPSULE_REPORT_MAX_STR_SIZE,
+					 "Capsule %03d uploads %s\n",
+					 capsule_count,
+					 CAPSULE_RESULT_PASS);
+			} else {
+				pr_err("%s: %s %03d, return error code = %d\n",
+				       __func__,
+				       "Failed to store capsule binary",
+				       capsule_count, ret);
+				snprintf(report_buf + report_buf_write_off,
+					 CAPSULE_REPORT_MAX_STR_SIZE,
+					 "Capsule %03d uploads %s\n",
+					 capsule_count,
+					 CAPSULE_RESULT_FAIL);
+			}
+		}
+		release_firmware(fw);
+	} else {
+		snprintf(report_buf + report_buf_write_off,
+			 CAPSULE_REPORT_MAX_STR_SIZE,
+			 "Capsule %03d uploads %s\n",
+			 capsule_count,
+			 CAPSULE_RESULT_EXIT);
+	}
+
+	report_buf_write_off += CAPSULE_REPORT_MAX_STR_SIZE;
+	report_buf_write_off %= PAGE_SIZE;
+
+	cancel_delayed_work_sync(&timeout_work);
+	mutex_unlock(&user_helper_lock);
+}
+
+static void capsule_user_helper_timeout_work(struct work_struct *work)
+{
+	request_firmware_abort(CAPSULE_NAME);
+}
+
+static ssize_t capsule_ticket_show(struct device *dev,
+				   struct device_attribute *attr, char *buf)
+{
+	int ret;
+
+	if (mutex_trylock(&user_helper_lock)) {
+		/*
+		 * Use request_firmware_nowait() to expose an user helper
+		 * interface for obtaining the capsule binary from user space
+		 */
+		ret = request_firmware_nowait(THIS_MODULE, FW_ACTION_NOHOTPLUG,
+					      CAPSULE_NAME,
+					      &efi_capsule_pdev->dev,
+					      GFP_KERNEL, NULL,
+					      callbackfn_efi_capsule);
+		if (ret) {
+			pr_err("%s: request_firmware_nowait() failed\n",
+			       __func__);
+			mutex_unlock(&user_helper_lock);
+			return snprintf(buf, STRING_INTEGER_MAX_LENGTH, "0\n");
+		}
+
+		queue_delayed_work(system_power_efficient_wq,
+				   &timeout_work,
+				   CAPSULE_USER_HELPER_TIMEOUT * HZ);
+
+		++capsule_count;
+		capsule_count %= 1000 ? : ++capsule_count;
+
+		return snprintf(buf, STRING_INTEGER_MAX_LENGTH, "%03d\n",
+				capsule_count);
+	} else {
+		return snprintf(buf, STRING_INTEGER_MAX_LENGTH, "0\n");
+	}
+}
+
+static DEVICE_ATTR_RO(capsule_ticket);
+
+static ssize_t capsule_report_show(struct device *dev,
+				   struct device_attribute *attr, char *buf)
+{
+	int length = capsule_count * CAPSULE_REPORT_MAX_STR_SIZE;
+
+	length = length > PAGE_SIZE ? PAGE_SIZE : length;
+	memcpy(buf, report_buf + report_buf_head_off, length);
+	return length;
+}
+
+static DEVICE_ATTR_RO(capsule_report);
+
+/* reboot notifier for avoid deadlock with usermode_lock */
+static int capsule_shutdown_notify(struct notifier_block *nb,
+				   unsigned long sys_state,
+				   void *reboot_cmd)
+{
+	request_firmware_abort(CAPSULE_NAME);
+	return NOTIFY_DONE;
+}
+
+static struct notifier_block capsule_shutdown_nb = {
+	.notifier_call = capsule_shutdown_notify,
+	/*
+	 * In order to reboot properly, it is required to ensure the priority
+	 * here is higher than firmware_class fw_shutdown_nb priority
+	 */
+	.priority = 1,
+};
+
+static int __init efi_capsule_user_helper_init(void)
+{
+	int ret;
+
+	efi_capsule_pdev = platform_device_register_simple(DEV_NAME,
+							   PLATFORM_DEVID_NONE,
+							   NULL, 0);
+	if (IS_ERR(efi_capsule_pdev)) {
+		pr_err("%s: platform_device_register_simple() failed\n",
+		       __func__);
+		return PTR_ERR(efi_capsule_pdev);
+	}
+
+	report_buf = devm_kzalloc(&efi_capsule_pdev->dev, PAGE_SIZE,
+				  GFP_KERNEL);
+	if (!report_buf) {
+		ret = -ENOMEM;
+		pr_err("%s: devm_kzalloc() failed\n", __func__);
+		goto out;
+	}
+
+	/*
+	 * create this file node for user to acquire a ticket number and get
+	 * ready to upload capsule binaries
+	 */
+	ret = device_create_file(&efi_capsule_pdev->dev,
+				 &dev_attr_capsule_ticket);
+	if (ret) {
+		pr_err("%s: device_create_file() failed\n", __func__);
+		goto out;
+	}
+
+	/*
+	 * create this file node for user to check the capsule binaries upload
+	 * status
+	 */
+	ret = device_create_file(&efi_capsule_pdev->dev,
+				 &dev_attr_capsule_report);
+	if (ret) {
+		pr_err("%s: device_create_file() failed\n", __func__);
+		goto out;
+	}
+
+	INIT_DELAYED_WORK(&timeout_work, capsule_user_helper_timeout_work);
+	register_reboot_notifier(&capsule_shutdown_nb);
+	return 0;
+
+out:
+	platform_device_unregister(efi_capsule_pdev);
+	return ret;
+}
+module_init(efi_capsule_user_helper_init);
+
+/*
+ * To remove this kernel module, just perform:
+ * rmmod efi_capsule_user_helper.ko
+ *
+ * rmmod -f efi_capsule_user_helper.ko is NOT recommended and NOT supported
+ * by this module design. Any force unload may cause the system crash.
+ */
+static void __exit efi_capsule_user_helper_exit(void)
+{
+	unregister_reboot_notifier(&capsule_shutdown_nb);
+
+	// request_firmware_abort(CAPSULE_NAME);
+	// /*
+	 // * synchronization is needed to make sure request_firmware is fully
+	 // * aborted
+	 // */
+	// while (efi_capsule_pdev->dev.kobj.kref.refcount.counter > 3)
+		// msleep(20); /* avoid busy waiting for cooperative kernel */
+
+	platform_device_unregister(efi_capsule_pdev);
+}
+module_exit(efi_capsule_user_helper_exit);
+
+MODULE_DESCRIPTION("EFI Capsule user helper binary load utility");
+MODULE_LICENSE("GPL v2");
-- 
1.7.9.5

Re: Re: [PATCH v2 3/3] efi: Capsule update with user helper interface

[Borislav Petkov]

On Tue, Feb 24, 2015 at 12:49:09PM +0000, Kweh, Hock Leong wrote:
> So the process steps basically look like this:
> 1.) cat capsule_ticket    =======> acquire a number and lock mutex then
>                                                                      expose firmware_class user helper
>                                                                      interface as well as start timer for timeout
>                                                                      counting
> 2.) repeat step 1 if obtained a "0" number
> 3.) echo 1 > loading
> 4.) cat bin > data
> 5.) echo 0 > loading        =======> stop the timeout counting  then unlock
>                                                                       mutex at the end of callback routine 
> 6.) cat capsule_report   =======> grep the number acquired from beginning
>                                                                       for checking succeeded/failed

So this sounds pretty overengineered for no reason, or maybe I'm missing
the reason.

If I had to give an example from the microcode loader, what we do there
is put the microcode in /lib/firmware/... and do

echo 1 > /sys/devices/system/cpu/microcode/reload

which goes and calls reload_store() in
arch/x86/kernel/cpu/microcode/core.c which grabs a mutex, disables CPU
hotplug, etc, etc...

And this mechanism is as simple as it can get. Maybe capsules can be
loaded like that too?

Error code can be propagated too, if needed, of course.

-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.
--

RE: Re: [PATCH v2 3/3] efi: Capsule update with user helper interface

[Kweh, Hock Leong]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 1201 bytes --]

> -----Original Message-----
> From: Borislav Petkov [mailto:bp@alien8.de]
> Sent: Wednesday, February 25, 2015 7:48 PM
> 
> On Tue, Feb 24, 2015 at 12:49:09PM +0000, Kweh, Hock Leong wrote:
> 
> So this sounds pretty overengineered for no reason, or maybe I'm missing
> the reason.
> 
> If I had to give an example from the microcode loader, what we do there
> is put the microcode in /lib/firmware/... and do
> 
> echo 1 > /sys/devices/system/cpu/microcode/reload
> 
> which goes and calls reload_store() in
> arch/x86/kernel/cpu/microcode/core.c which grabs a mutex, disables CPU
> hotplug, etc, etc...
> 
> And this mechanism is as simple as it can get. Maybe capsules can be
> loaded like that too?
> 
> Error code can be propagated too, if needed, of course.
> 
> --
> Regards/Gruss,
>     Boris.
> 
> ECO tip #101: Trim your mails when you reply.
> --

Hi Boris,

The reason we use this interface for efi capsule is that efi capsule
support multi binaries to be uploaded and each binary file name
can be different.


Regards,
Wilson

ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

Re: Re: [PATCH v2 3/3] efi: Capsule update with user helper interface

[Borislav Petkov]

On Wed, Feb 25, 2015 at 12:38:20PM +0000, Kweh, Hock Leong wrote:
> The reason we use this interface for efi capsule is that efi capsule
> support multi binaries to be uploaded and each binary file name
> can be different.

So you can write the file path to a second file and reload then, once
per file.

Alternatively, you can combine all the files into a cpio archive like
we do with the microcode loader too, and let the kernel parse the cpio
archive.

-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.
--

Re: Re: [PATCH v2 3/3] efi: Capsule update with user helper interface

[Andy Lutomirski]

On Wed, Feb 25, 2015 at 3:47 AM, Borislav Petkov <bp@alien8.de> wrote:
> On Tue, Feb 24, 2015 at 12:49:09PM +0000, Kweh, Hock Leong wrote:
>> So the process steps basically look like this:
>> 1.) cat capsule_ticket    =======> acquire a number and lock mutex then
>>                                                                      expose firmware_class user helper
>>                                                                      interface as well as start timer for timeout
>>                                                                      counting
>> 2.) repeat step 1 if obtained a "0" number
>> 3.) echo 1 > loading
>> 4.) cat bin > data
>> 5.) echo 0 > loading        =======> stop the timeout counting  then unlock
>>                                                                       mutex at the end of callback routine
>> 6.) cat capsule_report   =======> grep the number acquired from beginning
>>                                                                       for checking succeeded/failed
>
> So this sounds pretty overengineered for no reason, or maybe I'm missing
> the reason.
>
> If I had to give an example from the microcode loader, what we do there
> is put the microcode in /lib/firmware/... and do
>
> echo 1 > /sys/devices/system/cpu/microcode/reload
>
> which goes and calls reload_store() in
> arch/x86/kernel/cpu/microcode/core.c which grabs a mutex, disables CPU
> hotplug, etc, etc...
>
> And this mechanism is as simple as it can get. Maybe capsules can be
> loaded like that too?
>
> Error code can be propagated too, if needed, of course.

How can the error code be propagated?  Would that echo command fail in
case of error?

--Andy

Re: Re: [PATCH v2 3/3] efi: Capsule update with user helper interface

[Borislav Petkov]

On Thu, Feb 26, 2015 at 07:30:54AM -0800, Andy Lutomirski wrote:
> How can the error code be propagated?  Would that echo command fail in
> case of error?

Yeah, either that or we can put the error code in the sysfs file which
userspace can cat.

-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.
--

Re: Re: [PATCH v2 3/3] efi: Capsule update with user helper interface

[Matt Fleming]

On Thu, 26 Feb, at 04:54:58PM, Borislav Petkov wrote:
> On Thu, Feb 26, 2015 at 07:30:54AM -0800, Andy Lutomirski wrote:
> > How can the error code be propagated?  Would that echo command fail in
> > case of error?
> 
> Yeah, either that or we can put the error code in the sysfs file which
> userspace can cat.

FWIW, I'd prefer to make the echo command fail in case of capsule error.

-- 
Matt Fleming, Intel Open Source Technology Center

Re: Re: [PATCH v2 3/3] efi: Capsule update with user helper interface

[Peter Jones]

On Tue, Feb 24, 2015 at 12:49:09PM +0000, Kweh, Hock Leong wrote:
> Hi All,
> 
> After some internal discussion and re-design prototyping & testing on
> this efi capsule interface kernel module, I would like to start a discussion
> here on the new idea and wish to get input for the implementation and
> eventually upstream.

So do we actually *need* a kernel interface to UpdateCapsule?  We've
already got an implementation in progress where we use my ESRT patch to
figure out what firmwares we can update, and we schedule them and do the
update during boot up before the normal bootloader is run.  That means
never having to call UpdateCapsule() after ExitBootServices() or
SetVirtualAddressMap() have been called, and only doing it across
reboots that UEFI is performing itself.  It also means never having to
do it with multiple CPUs running.

I've posted several revisions of the ESRT patch to linux-efi , and right
now we're just waiting on the UEFI 2.5 spec to be finalized before I
send a final copy to Matt.  The command line tool and the code to apply
the firmwares during boot are at https://github.com/rhinstaller/fwupdate
and there's a GNOME-based UI in progress at
https://github.com/hughsie/fwupd (yes we apparently stink at naming
things.)

I'm not sure how making this go through the kernel will make things
better - it introduces a lot more things that can go wrong, and the only
benefit is one reboot where BootNext is set - which actually is
relatively fast even slow-POSTing machines.  After that the
UpdateCapsule+reboot to apply is *extremely* fast, because
applying capsule updates have to happen very very early in the boot-up
sequence.  (That early processing is /effectively/ a requirement,
since it has to happen before the block write locking on the SPI part is
done.)  So even on the machine that takes quite some time to POST, the
time that would be saved saved is less than 1% or so of the total update
time.

An early version of my code worked to update a machine I got from your
employer, FWIW - right now we're adding API and fixing up implementation
bits I made specifically to that one proof-of-concept scenario, and
there's some API parts that are in UEFI 2.5 draft releases that we're
not quite handling yet.  However, there's code there that's already been
checked in which has successfully applied system firmware updates
without having a kernel interface to UpdateCapsule().

So again: do we really need or want to do this?

-- 
        Peter

Re: Re: [PATCH v2 3/3] efi: Capsule update with user helper interface

[Roy Franz]

On Fri, Mar 6, 2015 at 1:39 PM, Peter Jones <pjones@redhat.com> wrote:
> On Tue, Feb 24, 2015 at 12:49:09PM +0000, Kweh, Hock Leong wrote:
>> Hi All,
>>
>> After some internal discussion and re-design prototyping & testing on
>> this efi capsule interface kernel module, I would like to start a discussion
>> here on the new idea and wish to get input for the implementation and
>> eventually upstream.
>
> So do we actually *need* a kernel interface to UpdateCapsule?  We've
> already got an implementation in progress where we use my ESRT patch to
> figure out what firmwares we can update, and we schedule them and do the
> update during boot up before the normal bootloader is run.  That means
> never having to call UpdateCapsule() after ExitBootServices() or
> SetVirtualAddressMap() have been called, and only doing it across
> reboots that UEFI is performing itself.  It also means never having to
> do it with multiple CPUs running.

So this does it by writing the capsules to the EFI system partition, and having
them processed from there during the next boot?
How would this work on diskless systems? (or boot-disk-less systems)

What are the use cases for capsules that don't require a reboot?  I'm not
really clear uses for those, but the kernel interface could be better for those,
as it would allow processing without a reboot.

Roy

>
> I've posted several revisions of the ESRT patch to linux-efi , and right
> now we're just waiting on the UEFI 2.5 spec to be finalized before I
> send a final copy to Matt.  The command line tool and the code to apply
> the firmwares during boot are at https://github.com/rhinstaller/fwupdate
> and there's a GNOME-based UI in progress at
> https://github.com/hughsie/fwupd (yes we apparently stink at naming
> things.)
>
> I'm not sure how making this go through the kernel will make things
> better - it introduces a lot more things that can go wrong, and the only
> benefit is one reboot where BootNext is set - which actually is
> relatively fast even slow-POSTing machines.  After that the
> UpdateCapsule+reboot to apply is *extremely* fast, because
> applying capsule updates have to happen very very early in the boot-up
> sequence.  (That early processing is /effectively/ a requirement,
> since it has to happen before the block write locking on the SPI part is
> done.)  So even on the machine that takes quite some time to POST, the
> time that would be saved saved is less than 1% or so of the total update
> time.
>
> An early version of my code worked to update a machine I got from your
> employer, FWIW - right now we're adding API and fixing up implementation
> bits I made specifically to that one proof-of-concept scenario, and
> there's some API parts that are in UEFI 2.5 draft releases that we're
> not quite handling yet.  However, there's code there that's already been
> checked in which has successfully applied system firmware updates
> without having a kernel interface to UpdateCapsule().
>
> So again: do we really need or want to do this?
>
> --
>         Peter
> --
> To unsubscribe from this list: send the line "unsubscribe linux-efi" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Re: [PATCH v2 3/3] efi: Capsule update with user helper interface

[Peter Jones]

On Fri, Mar 06, 2015 at 01:49:20PM -0800, Roy Franz wrote:
> On Fri, Mar 6, 2015 at 1:39 PM, Peter Jones <pjones@redhat.com> wrote:
> > On Tue, Feb 24, 2015 at 12:49:09PM +0000, Kweh, Hock Leong wrote:
> >> Hi All,
> >>
> >> After some internal discussion and re-design prototyping & testing on
> >> this efi capsule interface kernel module, I would like to start a discussion
> >> here on the new idea and wish to get input for the implementation and
> >> eventually upstream.
> >
> > So do we actually *need* a kernel interface to UpdateCapsule?  We've
> > already got an implementation in progress where we use my ESRT patch to
> > figure out what firmwares we can update, and we schedule them and do the
> > update during boot up before the normal bootloader is run.  That means
> > never having to call UpdateCapsule() after ExitBootServices() or
> > SetVirtualAddressMap() have been called, and only doing it across
> > reboots that UEFI is performing itself.  It also means never having to
> > do it with multiple CPUs running.
> 
> So this does it by writing the capsules to the EFI system partition, and having
> them processed from there during the next boot?
> How would this work on diskless systems? (or boot-disk-less systems)

One of the things I'm currently writing is making the file we load be
specified correctly by UEFI device paths - and that'll include the
ability to get it from devices presented by the network drivers.  On
currently extant test machines that includes tftp support, just like
netbooting.  On UEFI 2.5 machines, where ESRT is introduced so that we
actually know something about the capsules we can apply updates for, it
also includes http support.  Admittedly that means when you're doing
deployment you'll have to have some process for putting the images
someplace, but it can be the same device you're net-booting from.
Just one example.

If we're talking about systems that are booting entirely out of their
own firmware volume, there's definitely some legitimate argument that
doing this without an ESP could be valuable - so yeah, a kernel API in
that case might be worthwhile.

That said, the extra complexity combined with the fact that it's running
after ExitBootServices() and SetVirtualAddressMap() means I'll probably
avoid using it from the userland code except on machines where there are
no other options.  My faith that we're going to see a lot of machines
where that's well tested without our address maps looking exactly like
Windows's isn't very high, and we've repeatedly run into a lot of pain
with that in the past.  That's not the only thing that has to be exactly
right, either - for instance there's no guarantee it'll work if you use
the ACPI reset vector instead of the EFI runtime services
ResetSystem(EfiResetWarm) call.  Right now we use the ACPI method
preferentially because of SetVirtualAddressMap() not working as
documented on so *many* platforms.  That means what happens upon reboot
when we do UpdateCapsule() is undefined behavior.

Of course I'm likely not the only person who will ever implement tools
in userland to orchestrate firmware updates, either :)

> What are the use cases for capsules that don't require a reboot?  I'm
> not really clear uses for those, but the kernel interface could be
> better for those, as it would allow processing without a reboot.

I'm really not sure if we know the answer to that yet.  Things like USB
devices most often won't ever be registered with firmware's FMP anyway,
so they won't have capsules exposed, and they'll use more traditional
USB commands to do firmware updates - stuff like hughsie's ColorHug
device are in that category, and he's already supporting that with
fwupd.

Things like peripheral card option ROMs are potentially in that
category, though I'm a little skeptical of the idea that I actually want
to update the firmware on my video or network card with the kernel
already running, and that when I do I'm not going to want to reboot
immediately to make sure it worked right anyway.  There are almost
certainly lots of cases I haven't thought of, though.

If we want this interface for those cases, I think we should also be
enumerating the cases we think it's supporting, as well, even if only in
broad strokes.  I don't think we need to say "support this specific
device's updates", but keeping track of the basic model of the update
we're supporting would go a long way to establishing the value of
supporting all the complexity.

-- 
        Peter

Re: Re: [PATCH v2 3/3] efi: Capsule update with user helper interface

[Matt Fleming]

On Fri, 06 Mar, at 04:39:12PM, Peter Jones wrote:
> 
> So again: do we really need or want to do this?

One thing that we totally lose the ability to do is use the capsule
interface for things *other* than firmware updates, e.g.

 https://lkml.org/lkml/2013/10/16/327 

Also, requiring embedded or custom OS to include fwupdate into their
existing boot solutions is a bit heavy handed when literally all they
want to do is cat a binary blob to a sysfs file.

I don't see why we can't have both solutions.

Another thing is that ESRT isn't going to be supported by every
platform.

So, for the sysfs interface, let's not allow loading from /lib. Let's
not require a userland tool. Let's just do,

  # echo /path/to/my/awesome/capsule.bin > /sys/../capsule

and be done with it.

Hmmm?

-- 
Matt Fleming, Intel Open Source Technology Center

Re: Re: [PATCH v2 3/3] efi: Capsule update with user helper interface

[Peter Jones]

On Tue, Mar 10, 2015 at 12:26:52PM +0000, Matt Fleming wrote:
> On Fri, 06 Mar, at 04:39:12PM, Peter Jones wrote:
> > 
> > So again: do we really need or want to do this?
> 
> One thing that we totally lose the ability to do is use the capsule
> interface for things *other* than firmware updates, e.g.
> 
>  https://lkml.org/lkml/2013/10/16/327 
> 
> Also, requiring embedded or custom OS to include fwupdate into their
> existing boot solutions is a bit heavy handed when literally all they
> want to do is cat a binary blob to a sysfs file.
> 
> I don't see why we can't have both solutions.

Yeah - we clearly need a kernel interface for some embedded devices, and
it'd be better for every vendor to not implement it themselves.

> Another thing is that ESRT isn't going to be supported by every
> platform.

Yeah - though I think you're *mostly* talking about the same platforms
as above.

> So, for the sysfs interface, let's not allow loading from /lib. Let's
> not require a userland tool. Let's just do,
> 
>   # echo /path/to/my/awesome/capsule.bin > /sys/../capsule

> 
> and be done with it.
> 
> Hmmm?

I assume you're implying a) the capsule header with the guid is embedded
in the .bin there already, and b) one contiguous write(2) with error
reporting coming through something like vars.c's efi_status_to_err()?

If so, yes, I prefer this API.

-- 
        Peter

Re: Re: [PATCH v2 3/3] efi: Capsule update with user helper interface

[Andy Lutomirski]

On Tue, Mar 10, 2015 at 8:21 AM, Peter Jones <pjones@redhat.com> wrote:
> On Tue, Mar 10, 2015 at 12:26:52PM +0000, Matt Fleming wrote:
>> On Fri, 06 Mar, at 04:39:12PM, Peter Jones wrote:
>> >
>> > So again: do we really need or want to do this?
>>
>> One thing that we totally lose the ability to do is use the capsule
>> interface for things *other* than firmware updates, e.g.
>>
>>  https://lkml.org/lkml/2013/10/16/327
>>
>> Also, requiring embedded or custom OS to include fwupdate into their
>> existing boot solutions is a bit heavy handed when literally all they
>> want to do is cat a binary blob to a sysfs file.
>>
>> I don't see why we can't have both solutions.
>
> Yeah - we clearly need a kernel interface for some embedded devices, and
> it'd be better for every vendor to not implement it themselves.
>
>> Another thing is that ESRT isn't going to be supported by every
>> platform.
>
> Yeah - though I think you're *mostly* talking about the same platforms
> as above.
>
>> So, for the sysfs interface, let's not allow loading from /lib. Let's
>> not require a userland tool. Let's just do,
>>
>>   # echo /path/to/my/awesome/capsule.bin > /sys/../capsule
>
>>
>> and be done with it.
>>
>> Hmmm?
>
> I assume you're implying a) the capsule header with the guid is embedded
> in the .bin there already, and b) one contiguous write(2) with error
> reporting coming through something like vars.c's efi_status_to_err()?
>
> If so, yes, I prefer this API.
>

Is using a char device really so bad?  I have a "simple_char" that
makes this really easy that's pending review.

--Andy

> --
>         Peter



-- 
Andy Lutomirski
AMA Capital Management, LLC

Re: Re: [PATCH v2 3/3] efi: Capsule update with user helper interface

[Peter Jones]

> >> So, for the sysfs interface, let's not allow loading from /lib. Let's
> >> not require a userland tool. Let's just do,
> >>
> >>   # echo /path/to/my/awesome/capsule.bin > /sys/../capsule
> >
> >>
> >> and be done with it.
> >>
> >> Hmmm?
> >
> > I assume you're implying a) the capsule header with the guid is embedded
> > in the .bin there already, and b) one contiguous write(2) with error
> > reporting coming through something like vars.c's efi_status_to_err()?
> >
> > If so, yes, I prefer this API.
> >
> 
> Is using a char device really so bad?  I have a "simple_char" that
> makes this really easy that's pending review.

As long as there's straightforward propagation of the EFI_STATUS return
from UpdateCapsule() back, sysfs file vs char device makes very little
difference to me.  Either way it's open(), write(), close().  Using the
runtime firmware upload interface designed for wifi and scsi devices is
the part I don't really like.

-- 
        Peter

Re: Re: [PATCH v2 3/3] efi: Capsule update with user helper interface

[Andy Lutomirski]

On Tue, Mar 10, 2015 at 8:40 AM, Peter Jones <pjones@redhat.com> wrote:
>
>> >> So, for the sysfs interface, let's not allow loading from /lib. Let's
>> >> not require a userland tool. Let's just do,
>> >>
>> >>   # echo /path/to/my/awesome/capsule.bin > /sys/../capsule
>> >
>> >>
>> >> and be done with it.
>> >>
>> >> Hmmm?
>> >
>> > I assume you're implying a) the capsule header with the guid is embedded
>> > in the .bin there already, and b) one contiguous write(2) with error
>> > reporting coming through something like vars.c's efi_status_to_err()?
>> >
>> > If so, yes, I prefer this API.
>> >
>>
>> Is using a char device really so bad?  I have a "simple_char" that
>> makes this really easy that's pending review.
>
> As long as there's straightforward propagation of the EFI_STATUS return
> from UpdateCapsule() back, sysfs file vs char device makes very little
> difference to me.  Either way it's open(), write(), close().  Using the
> runtime firmware upload interface designed for wifi and scsi devices is
> the part I don't really like.
>

I'm not 100% happy with write(2) (which is all we have in sysfs) for
two reasons:

1. If we write a file name, eww.  That's more complicated, requires
temporary files, has annoying mount namespace issues, etc.

2. If we write the full contents, we need to do it in a single call to
write.  That means that we can't use cat, which mostly defeats the
purpose.  In fact, using cat could be actively harmful.


--Andy

Re: Re: [PATCH v2 3/3] efi: Capsule update with user helper interface

[Peter Jones]

On Tue, Mar 10, 2015 at 08:51:59AM -0700, Andy Lutomirski wrote:
> On Tue, Mar 10, 2015 at 8:40 AM, Peter Jones <pjones@redhat.com> wrote:
> >
> >> >> So, for the sysfs interface, let's not allow loading from /lib. Let's
> >> >> not require a userland tool. Let's just do,
> >> >>
> >> >>   # echo /path/to/my/awesome/capsule.bin > /sys/../capsule
> >> >
> >> >>
> >> >> and be done with it.
> >> >>
> >> >> Hmmm?
> >> >
> >> > I assume you're implying a) the capsule header with the guid is embedded
> >> > in the .bin there already, and b) one contiguous write(2) with error
> >> > reporting coming through something like vars.c's efi_status_to_err()?
> >> >
> >> > If so, yes, I prefer this API.
> >> >
> >>
> >> Is using a char device really so bad?  I have a "simple_char" that
> >> makes this really easy that's pending review.
> >
> > As long as there's straightforward propagation of the EFI_STATUS return
> > from UpdateCapsule() back, sysfs file vs char device makes very little
> > difference to me.  Either way it's open(), write(), close().  Using the
> > runtime firmware upload interface designed for wifi and scsi devices is
> > the part I don't really like.
> >
> 
> I'm not 100% happy with write(2) (which is all we have in sysfs) for
> two reasons:
> 
> 1. If we write a file name, eww.  That's more complicated, requires
> temporary files, has annoying mount namespace issues, etc.
> 
> 2. If we write the full contents, we need to do it in a single call to
> write.  That means that we can't use cat, which mostly defeats the
> purpose.  In fact, using cat could be actively harmful.

So if what we wind up with is:

fd = open("/sys/.../capsule", O_RDWR);
write(fd, buf, size/N);
...
write(fd, buf + M*size/N, size/N);
close(fd);

You're suggesting the error code would post on close()?  My worry about
that is that I imagine a lot less code in the wild checks the error code
on close() than on write() - though gnu cat does do so on both.  But
there are other questions still - will it post on fdatasync()?  On
fsync()?

-- 
        Peter

Re: Re: [PATCH v2 3/3] efi: Capsule update with user helper interface

[Andy Lutomirski]

On Tue, Mar 10, 2015 at 10:26 AM, Peter Jones <pjones@redhat.com> wrote:
> On Tue, Mar 10, 2015 at 08:51:59AM -0700, Andy Lutomirski wrote:
>> On Tue, Mar 10, 2015 at 8:40 AM, Peter Jones <pjones@redhat.com> wrote:
>> >
>> >> >> So, for the sysfs interface, let's not allow loading from /lib. Let's
>> >> >> not require a userland tool. Let's just do,
>> >> >>
>> >> >>   # echo /path/to/my/awesome/capsule.bin > /sys/../capsule
>> >> >
>> >> >>
>> >> >> and be done with it.
>> >> >>
>> >> >> Hmmm?
>> >> >
>> >> > I assume you're implying a) the capsule header with the guid is embedded
>> >> > in the .bin there already, and b) one contiguous write(2) with error
>> >> > reporting coming through something like vars.c's efi_status_to_err()?
>> >> >
>> >> > If so, yes, I prefer this API.
>> >> >
>> >>
>> >> Is using a char device really so bad?  I have a "simple_char" that
>> >> makes this really easy that's pending review.
>> >
>> > As long as there's straightforward propagation of the EFI_STATUS return
>> > from UpdateCapsule() back, sysfs file vs char device makes very little
>> > difference to me.  Either way it's open(), write(), close().  Using the
>> > runtime firmware upload interface designed for wifi and scsi devices is
>> > the part I don't really like.
>> >
>>
>> I'm not 100% happy with write(2) (which is all we have in sysfs) for
>> two reasons:
>>
>> 1. If we write a file name, eww.  That's more complicated, requires
>> temporary files, has annoying mount namespace issues, etc.
>>
>> 2. If we write the full contents, we need to do it in a single call to
>> write.  That means that we can't use cat, which mostly defeats the
>> purpose.  In fact, using cat could be actively harmful.
>
> So if what we wind up with is:
>
> fd = open("/sys/.../capsule", O_RDWR);
> write(fd, buf, size/N);
> ...
> write(fd, buf + M*size/N, size/N);
> close(fd);
>
> You're suggesting the error code would post on close()?  My worry about
> that is that I imagine a lot less code in the wild checks the error code
> on close() than on write() - though gnu cat does do so on both.  But
> there are other questions still - will it post on fdatasync()?  On
> fsync()?

Cat, for example, doesn't check any of the above, which is why I don't
like this approach.

--Andy

Re: Re: [PATCH v2 3/3] efi: Capsule update with user helper interface

[Matt Fleming]

On Tue, 10 Mar, at 08:51:59AM, Andy Lutomirski wrote:
> 
> I'm not 100% happy with write(2) (which is all we have in sysfs) for
> two reasons:
> 
> 1. If we write a file name, eww.  That's more complicated, requires
> temporary files, has annoying mount namespace issues, etc.
> 
> 2. If we write the full contents, we need to do it in a single call to
> write.  That means that we can't use cat, which mostly defeats the
> purpose.  In fact, using cat could be actively harmful.

At this point I'd really like Greg to chime in.

In principal, I'm not stricly opposed to using a simple char device
provided that it's not essentially a copy and paste of code from
drivers/base/firmware_class.c.

Greg?

-- 
Matt Fleming, Intel Open Source Technology Center

Re: Re: [PATCH v2 3/3] efi: Capsule update with user helper interface

[Greg Kroah-Hartman]

On Thu, Mar 12, 2015 at 10:47:54PM +0000, Matt Fleming wrote:
> On Tue, 10 Mar, at 08:51:59AM, Andy Lutomirski wrote:
> > 
> > I'm not 100% happy with write(2) (which is all we have in sysfs) for
> > two reasons:
> > 
> > 1. If we write a file name, eww.  That's more complicated, requires
> > temporary files, has annoying mount namespace issues, etc.
> > 
> > 2. If we write the full contents, we need to do it in a single call to
> > write.  That means that we can't use cat, which mostly defeats the
> > purpose.  In fact, using cat could be actively harmful.
> 
> At this point I'd really like Greg to chime in.
> 
> In principal, I'm not stricly opposed to using a simple char device
> provided that it's not essentially a copy and paste of code from
> drivers/base/firmware_class.c.
> 
> Greg?

Yes, I don't want a character driver here for this if at all possible.
Just stick with the firmware download code, it's there and should work
"as-is" for your stuff.

thanks,

greg k-h

Re: Re: [PATCH v2 3/3] efi: Capsule update with user helper interface

[Andy Lutomirski]

On Mar 13, 2015 7:42 AM, "Greg Kroah-Hartman"
<gregkh@linuxfoundation.org> wrote:
>
> On Thu, Mar 12, 2015 at 10:47:54PM +0000, Matt Fleming wrote:
> > On Tue, 10 Mar, at 08:51:59AM, Andy Lutomirski wrote:
> > >
> > > I'm not 100% happy with write(2) (which is all we have in sysfs) for
> > > two reasons:
> > >
> > > 1. If we write a file name, eww.  That's more complicated, requires
> > > temporary files, has annoying mount namespace issues, etc.
> > >
> > > 2. If we write the full contents, we need to do it in a single call to
> > > write.  That means that we can't use cat, which mostly defeats the
> > > purpose.  In fact, using cat could be actively harmful.
> >
> > At this point I'd really like Greg to chime in.
> >
> > In principal, I'm not stricly opposed to using a simple char device
> > provided that it's not essentially a copy and paste of code from
> > drivers/base/firmware_class.c.
> >
> > Greg?
>
> Yes, I don't want a character driver here for this if at all possible.
> Just stick with the firmware download code, it's there and should work
> "as-is" for your stuff.

Given the rest of this interminable discussion, it seems pretty clear
to me that the firmware download code doesn't work as is for this use
case.  It will sort of work with lots of changes (to locking,
synchronicity, error reporting, enumeration, etc), but I think that
the total complexity of doing that will far exceed the complexity if
either a new chardev or some straightforward sysfs interface.

We don't have ioctls in sysfs, though, and adding that sounds worse
than a new character driver, so...

--Andy

>
> thanks,
>
> greg k-h

RE: Re: [PATCH v2 3/3] efi: Capsule update with user helper interface

[Andy Lutomirski]

On Mar 6, 2015 4:20 AM, "Kweh, Hock Leong" <hock.leong.kweh@intel.com> wrote:
>
> > -----Original Message-----
> > From: Andy Lutomirski [mailto:luto@amacapital.net]
> > Sent: Friday, March 06, 2015 7:09 AM
> >
> > On Mar 5, 2015 1:19 AM, "Kweh, Hock Leong" <hock.leong.kweh@intel.com>
> > wrote:
> > >
> > > > > This really is not a big deal. User should cope with it.
> > > >
> > > > No, it's a big deal, and the user should not cope.
> > > >
> > > > The user *should not* be required to have write access to anything in
> > > > /lib to install a UEFI capsule that they download from their
> > > > motherboard vendor's website.  /lib belongs to the distro, and UEFI
> > > > capsules do not belong to the distro.  In this regard, UEFI capsules
> > > > are completely unlike your wireless card firmware, your cpu microcode,
> > > > etc.
> > > >
> > > > Imagine systems using NFS root, Atomic-style systems (e.g. ostree),
> > > > systems that boot off squashfs, etc.  They should still be able to
> > > > load capsules.  The basic user interface that should work is:
> > > >
> > > > # uefi-load-capsule /path/to/capsule
> > > >
> > > > or:
> > > >
> > > > # uefi-load-capsule - </path/to/capsule
> > > >
> > > > I don't really care how uefi-load-capsule is implemented, as long as
> > > > it's straightforward, because people will screw it up if it isn't
> > > > straightforward.
> > > >
> > > > Why is it so hard to have a file in sysfs that you write the capsule
> > > > to using *cat* (not echo) and that will return an error code if cat
> > > > fails?  Is it because you don't know where the end of the capsule is?
> > > > if so, ioctl is designed for exactly this purpose.
> > > >
> > > > TBH, I find this thread kind of ridiculous.  The problem that you're
> > > > trying to solve is extremely simple, the functionality that userspace
> > > > needs is trivial, and all of these complex proposals for how it should
> > > > work are an artifact of the fact that the kernel-internal interfaces
> > > > you're using for it are not well suited to the problem at hand.
> > > >
> > > > --Andy
> > >
> > > Sorry, I may not catch your point correctly. Are you trying to tell that
> > > a "normal" user can perform efi capsule update. But a "normal" user
> > > does not have the right to install or copy the capsule binary into
> > > "/lib/firmware/". So, there is a need to make this capsule module to
> > > allow uploading the capsule binary at any path or location other than
> > > "/lib/firmware/".
> > >
> > > Is this what you mean?
> >
> > No.  Only root should be able to load capsules, but even root may not
> > be able to write to /lib.
> >
> > --Andy
> >
>
> Okay, I accept that only root user can perform the load capsule. It is new
> to me that root user may not have the access right to "/lib/firmware".
>
> But I remember you do mention that CPU micro code and wifi firmware
> they are different and able to write in /lib/firmware. I am curious why
> efi capsule binary have such a restriction.  What is preventing it being
> write to that location?
>

It has to do with where the firmware comes from.  When someone
prepares Linux fs image, they put user code, a kernel (usually), all
modules that might be needed, and all firmware that's likely to be
needed into /.  This might come from the distro or a company-wide
image.

If a normal firmware firmware file needs an update, it's just like
updating a driver or a library -- / will be updated by whatever
mechanism is in use.

Nonvolatile firmware is different.  The update isn't needed on
subsequent boots, and it could be much less generic than a driver.
For example, a capsule could contain a boot splash screen image that
says "this is computer #27."  Updating the system image to do this
makes little sense.  Instead it'll be a one-time thing done by root.

--Andy

> I even went to check out the FHS:
> http://en.wikipedia.org/wiki/Filesystem_Hierarchy_Standard
> I do not find any restriction calling out for that.
>
> Would you mind to educate me on that?
> Thanks.
>
>
> Regards,
> Wilson
>

Re: Re: [PATCH v2 3/3] efi: Capsule update with user helper interface

[Borislav Petkov]

On Fri, Mar 06, 2015 at 11:41:57AM +0000, Kweh, Hock Leong wrote:
> # cat /any/path/capsule.bin > /sys/devices/platform/efi_capsule/capsule_load

This is straight-forward and clean.

> or doing:
> # echo "/any/path/capsule.bin" > /sys/devices/platform/efi_capsule/capsule_load

This is strange and clumsy. So do the first one please.

-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.
--

RE: Re: [PATCH v2 3/3] efi: Capsule update with user helper interface

[Kweh, Hock Leong]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 3223 bytes --]

> -----Original Message-----
> From: Andy Lutomirski [mailto:luto@amacapital.net]
> Sent: Friday, March 06, 2015 7:09 AM
> 
> On Mar 5, 2015 1:19 AM, "Kweh, Hock Leong" <hock.leong.kweh@intel.com>
> wrote:
> >
> > > > This really is not a big deal. User should cope with it.
> > >
> > > No, it's a big deal, and the user should not cope.
> > >
> > > The user *should not* be required to have write access to anything in
> > > /lib to install a UEFI capsule that they download from their
> > > motherboard vendor's website.  /lib belongs to the distro, and UEFI
> > > capsules do not belong to the distro.  In this regard, UEFI capsules
> > > are completely unlike your wireless card firmware, your cpu microcode,
> > > etc.
> > >
> > > Imagine systems using NFS root, Atomic-style systems (e.g. ostree),
> > > systems that boot off squashfs, etc.  They should still be able to
> > > load capsules.  The basic user interface that should work is:
> > >
> > > # uefi-load-capsule /path/to/capsule
> > >
> > > or:
> > >
> > > # uefi-load-capsule - </path/to/capsule
> > >
> > > I don't really care how uefi-load-capsule is implemented, as long as
> > > it's straightforward, because people will screw it up if it isn't
> > > straightforward.
> > >
> > > Why is it so hard to have a file in sysfs that you write the capsule
> > > to using *cat* (not echo) and that will return an error code if cat
> > > fails?  Is it because you don't know where the end of the capsule is?
> > > if so, ioctl is designed for exactly this purpose.
> > >
> > > TBH, I find this thread kind of ridiculous.  The problem that you're
> > > trying to solve is extremely simple, the functionality that userspace
> > > needs is trivial, and all of these complex proposals for how it should
> > > work are an artifact of the fact that the kernel-internal interfaces
> > > you're using for it are not well suited to the problem at hand.
> > >
> > > --Andy
> >
> > Sorry, I may not catch your point correctly. Are you trying to tell that
> > a "normal" user can perform efi capsule update. But a "normal" user
> > does not have the right to install or copy the capsule binary into
> > "/lib/firmware/". So, there is a need to make this capsule module to
> > allow uploading the capsule binary at any path or location other than
> > "/lib/firmware/".
> >
> > Is this what you mean?
> 
> No.  Only root should be able to load capsules, but even root may not
> be able to write to /lib.
> 
> --Andy
> 

Okay, I accept that only root user can perform the load capsule. It is new
to me that root user may not have the access right to "/lib/firmware".

But I remember you do mention that CPU micro code and wifi firmware
they are different and able to write in /lib/firmware. I am curious why
efi capsule binary have such a restriction.  What is preventing it being
write to that location?

I even went to check out the FHS:
http://en.wikipedia.org/wiki/Filesystem_Hierarchy_Standard
I do not find any restriction calling out for that.

Would you mind to educate me on that?
Thanks.


Regards,
Wilson

ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

RE: Re: [PATCH v2 3/3] efi: Capsule update with user helper interface

[Kweh, Hock Leong]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 1151 bytes --]

> -----Original Message-----
> From: Borislav Petkov [mailto:bp@alien8.de]
> Sent: Friday, March 06, 2015 4:14 PM
> 
> On Thu, Mar 05, 2015 at 03:08:42PM -0800, Andy Lutomirski wrote:
> > No.  Only root should be able to load capsules, but even root may not
> > be able to write to /lib.
> 
> So basically what we want to do is:
> 
> # cat /any/path/to/efi/capsule/accessible/to/root/efi_capsule.img >
> /sys/firmware/efi/update
> 
> Now it can't get any simpler than that and you get error codes too by
> failing the cat if the update fails.
> 
> Mind you, I'm using '#' and not '$' as a shell prompt :-)
> 
> --
> Regards/Gruss,
>     Boris.
> 
> ECO tip #101: Trim your mails when you reply.
> --

I believe you guys are more paying attention to the PATH and
whether doing:
# cat /any/path/capsule.bin > /sys/devices/platform/efi_capsule/capsule_load

or doing:
# echo "/any/path/capsule.bin" > /sys/devices/platform/efi_capsule/capsule_load

is not a big issue right?


Regards,
Wilson

ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

Re: Re: [PATCH v2 3/3] efi: Capsule update with user helper interface

[Borislav Petkov]

On Thu, Mar 05, 2015 at 03:08:42PM -0800, Andy Lutomirski wrote:
> No.  Only root should be able to load capsules, but even root may not
> be able to write to /lib.

So basically what we want to do is:

# cat /any/path/to/efi/capsule/accessible/to/root/efi_capsule.img > /sys/firmware/efi/update

Now it can't get any simpler than that and you get error codes too by
failing the cat if the update fails.

Mind you, I'm using '#' and not '$' as a shell prompt :-)

-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.
--

RE: Re: [PATCH v2 3/3] efi: Capsule update with user helper interface

[Andy Lutomirski]

On Mar 5, 2015 1:19 AM, "Kweh, Hock Leong" <hock.leong.kweh@intel.com> wrote:
>
> > -----Original Message-----
> > From: Andy Lutomirski [mailto:luto@amacapital.net]
> > Sent: Wednesday, March 04, 2015 4:38 AM
> >
> > On Mon, Mar 2, 2015 at 9:56 PM, Kweh, Hock Leong
> > <hock.leong.kweh@intel.com> wrote:
> > >
> > > Just to call out that using firmware class auto locate binary feature is limited
> > > to locations:
> > > - "/lib/firmware/updates/" UTS_RELEASE,
> > > - "/lib/firmware/updates",
> > > - "/lib/firmware/" UTS_RELEASE,
> > > - "/lib/firmware"
> > > and one custom path which inputted during firmware_class module load
> > > time or kernel boot up time.
> > >
> > > It is just not like the user helper interface which allow load the binary at
> > > any path/location.
> > >
> > > This really is not a big deal. User should cope with it.
> >
> > No, it's a big deal, and the user should not cope.
> >
> > The user *should not* be required to have write access to anything in
> > /lib to install a UEFI capsule that they download from their
> > motherboard vendor's website.  /lib belongs to the distro, and UEFI
> > capsules do not belong to the distro.  In this regard, UEFI capsules
> > are completely unlike your wireless card firmware, your cpu microcode,
> > etc.
> >
> > Imagine systems using NFS root, Atomic-style systems (e.g. ostree),
> > systems that boot off squashfs, etc.  They should still be able to
> > load capsules.  The basic user interface that should work is:
> >
> > # uefi-load-capsule /path/to/capsule
> >
> > or:
> >
> > # uefi-load-capsule - </path/to/capsule
> >
> > I don't really care how uefi-load-capsule is implemented, as long as
> > it's straightforward, because people will screw it up if it isn't
> > straightforward.
> >
> > Why is it so hard to have a file in sysfs that you write the capsule
> > to using *cat* (not echo) and that will return an error code if cat
> > fails?  Is it because you don't know where the end of the capsule is?
> > if so, ioctl is designed for exactly this purpose.
> >
> > TBH, I find this thread kind of ridiculous.  The problem that you're
> > trying to solve is extremely simple, the functionality that userspace
> > needs is trivial, and all of these complex proposals for how it should
> > work are an artifact of the fact that the kernel-internal interfaces
> > you're using for it are not well suited to the problem at hand.
> >
> > --Andy
>
> Sorry, I may not catch your point correctly. Are you trying to tell that
> a "normal" user can perform efi capsule update. But a "normal" user
> does not have the right to install or copy the capsule binary into
> "/lib/firmware/". So, there is a need to make this capsule module to
> allow uploading the capsule binary at any path or location other than
> "/lib/firmware/".
>
> Is this what you mean?

No.  Only root should be able to load capsules, but even root may not
be able to write to /lib.

--Andy

>
>
> Regards,
> Wilson

RE: Re: [PATCH v2 3/3] efi: Capsule update with user helper interface

[Kweh, Hock Leong]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 2817 bytes --]

> -----Original Message-----
> From: Andy Lutomirski [mailto:luto@amacapital.net]
> Sent: Wednesday, March 04, 2015 4:38 AM
> 
> On Mon, Mar 2, 2015 at 9:56 PM, Kweh, Hock Leong
> <hock.leong.kweh@intel.com> wrote:
> >
> > Just to call out that using firmware class auto locate binary feature is limited
> > to locations:
> > - "/lib/firmware/updates/" UTS_RELEASE,
> > - "/lib/firmware/updates",
> > - "/lib/firmware/" UTS_RELEASE,
> > - "/lib/firmware"
> > and one custom path which inputted during firmware_class module load
> > time or kernel boot up time.
> >
> > It is just not like the user helper interface which allow load the binary at
> > any path/location.
> >
> > This really is not a big deal. User should cope with it.
> 
> No, it's a big deal, and the user should not cope.
> 
> The user *should not* be required to have write access to anything in
> /lib to install a UEFI capsule that they download from their
> motherboard vendor's website.  /lib belongs to the distro, and UEFI
> capsules do not belong to the distro.  In this regard, UEFI capsules
> are completely unlike your wireless card firmware, your cpu microcode,
> etc.
> 
> Imagine systems using NFS root, Atomic-style systems (e.g. ostree),
> systems that boot off squashfs, etc.  They should still be able to
> load capsules.  The basic user interface that should work is:
> 
> # uefi-load-capsule /path/to/capsule
> 
> or:
> 
> # uefi-load-capsule - </path/to/capsule
> 
> I don't really care how uefi-load-capsule is implemented, as long as
> it's straightforward, because people will screw it up if it isn't
> straightforward.
> 
> Why is it so hard to have a file in sysfs that you write the capsule
> to using *cat* (not echo) and that will return an error code if cat
> fails?  Is it because you don't know where the end of the capsule is?
> if so, ioctl is designed for exactly this purpose.
> 
> TBH, I find this thread kind of ridiculous.  The problem that you're
> trying to solve is extremely simple, the functionality that userspace
> needs is trivial, and all of these complex proposals for how it should
> work are an artifact of the fact that the kernel-internal interfaces
> you're using for it are not well suited to the problem at hand.
> 
> --Andy

Sorry, I may not catch your point correctly. Are you trying to tell that
a "normal" user can perform efi capsule update. But a "normal" user
does not have the right to install or copy the capsule binary into
"/lib/firmware/". So, there is a need to make this capsule module to
allow uploading the capsule binary at any path or location other than
"/lib/firmware/".

Is this what you mean?


Regards,
Wilson
ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

Re: Re: [PATCH v2 3/3] efi: Capsule update with user helper interface

[Andy Lutomirski]

On Mar 3, 2015 12:51 PM, "Borislav Petkov" <bp@alien8.de> wrote:
>
> On Tue, Mar 03, 2015 at 12:37:54PM -0800, Andy Lutomirski wrote:
> > The user *should not* be required to have write access to anything in
> > /lib to install a UEFI capsule that they download from their
> > motherboard vendor's website.  /lib belongs to the distro, and UEFI
> > capsules do not belong to the distro.  In this regard, UEFI capsules
> > are completely unlike your wireless card firmware, your cpu microcode,
> > etc.
>
> Oh oh but but, if an UEFI capsule can brick the system, a normal user
> would be able to brick that system then. I think we should forbid that.

Absolutely.  That's why I said

# uefi-load-capsule

and not

$ uefi-load-capsule

:)

>
> I agree with the rest of your note that a simple
>
> cat <fw_blob> > /sys/...
>
> should be enough.
>
> --
> Regards/Gruss,
>     Boris.
>
> ECO tip #101: Trim your mails when you reply.
> --

Re: Re: [PATCH v2 3/3] efi: Capsule update with user helper interface

[Borislav Petkov]

On Tue, Mar 03, 2015 at 12:37:54PM -0800, Andy Lutomirski wrote:
> The user *should not* be required to have write access to anything in
> /lib to install a UEFI capsule that they download from their
> motherboard vendor's website.  /lib belongs to the distro, and UEFI
> capsules do not belong to the distro.  In this regard, UEFI capsules
> are completely unlike your wireless card firmware, your cpu microcode,
> etc.

Oh oh but but, if an UEFI capsule can brick the system, a normal user
would be able to brick that system then. I think we should forbid that.

I agree with the rest of your note that a simple

cat <fw_blob> > /sys/...

should be enough.

-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.
--

Re: Re: [PATCH v2 3/3] efi: Capsule update with user helper interface

[Andy Lutomirski]

On Mon, Mar 2, 2015 at 9:56 PM, Kweh, Hock Leong
<hock.leong.kweh@intel.com> wrote:
>> -----Original Message-----
>> From: Matt Fleming [mailto:matt@console-pimps.org]
>> Sent: Monday, March 02, 2015 8:30 PM
>>
>> On Mon, 02 Mar, at 10:59:00AM, Kweh Hock Leong wrote:
>> > > -----Original Message-----
>> > > From: Borislav Petkov [mailto:bp@alien8.de]
>> > > Sent: Wednesday, February 25, 2015 8:49 PM
>> > >
>> > > On Wed, Feb 25, 2015 at 12:38:20PM +0000, Kweh, Hock Leong wrote:
>> > > > The reason we use this interface for efi capsule is that efi capsule
>> > > > support multi binaries to be uploaded and each binary file name
>> > > > can be different.
>> > >
>> > > So you can write the file path to a second file and reload then, once
>> > > per file.
>> >
>> > Thanks for the suggestion. Did some exploration on this approach and
>> > would like to continue the discussion together with this suggested design.
>> >
>> > Ideal condition:
>> > - one file note is enough for load binary and status return (capsule_load)
>> > - load steps would be as simple as just:
>> >    echo [binary file name] > capsule_load
>> > - status return in the same command action. If failed, the echo will fail
>> >    with the failing status code.
>> >
>> > Trade off:
>> > - does not have the run time flexibility to load any firmware binaries at
>> >    different different path as firmware class  only support one custom
>> >    path inputted during boot time/load time. Unless to add new export
>> >    function for firmware class.
>>
>> Could you elaborate here? I don't understand this point.
>
> Just to call out that using firmware class auto locate binary feature is limited
> to locations:
> - "/lib/firmware/updates/" UTS_RELEASE,
> - "/lib/firmware/updates",
> - "/lib/firmware/" UTS_RELEASE,
> - "/lib/firmware"
> and one custom path which inputted during firmware_class module load
> time or kernel boot up time.
>
> It is just not like the user helper interface which allow load the binary at
> any path/location.
>
> This really is not a big deal. User should cope with it.

No, it's a big deal, and the user should not cope.

The user *should not* be required to have write access to anything in
/lib to install a UEFI capsule that they download from their
motherboard vendor's website.  /lib belongs to the distro, and UEFI
capsules do not belong to the distro.  In this regard, UEFI capsules
are completely unlike your wireless card firmware, your cpu microcode,
etc.

Imagine systems using NFS root, Atomic-style systems (e.g. ostree),
systems that boot off squashfs, etc.  They should still be able to
load capsules.  The basic user interface that should work is:

# uefi-load-capsule /path/to/capsule

or:

# uefi-load-capsule - </path/to/capsule

I don't really care how uefi-load-capsule is implemented, as long as
it's straightforward, because people will screw it up if it isn't
straightforward.

Why is it so hard to have a file in sysfs that you write the capsule
to using *cat* (not echo) and that will return an error code if cat
fails?  Is it because you don't know where the end of the capsule is?
if so, ioctl is designed for exactly this purpose.

TBH, I find this thread kind of ridiculous.  The problem that you're
trying to solve is extremely simple, the functionality that userspace
needs is trivial, and all of these complex proposals for how it should
work are an artifact of the fact that the kernel-internal interfaces
you're using for it are not well suited to the problem at hand.

--Andy

RE: Re: [PATCH v2 3/3] efi: Capsule update with user helper interface

[Kweh, Hock Leong]

> -----Original Message-----
> From: Matt Fleming [mailto:matt@console-pimps.org]
> Sent: Monday, March 02, 2015 8:30 PM
> 
> On Mon, 02 Mar, at 10:59:00AM, Kweh Hock Leong wrote:
> > > -----Original Message-----
> > > From: Borislav Petkov [mailto:bp@alien8.de]
> > > Sent: Wednesday, February 25, 2015 8:49 PM
> > >
> > > On Wed, Feb 25, 2015 at 12:38:20PM +0000, Kweh, Hock Leong wrote:
> > > > The reason we use this interface for efi capsule is that efi capsule
> > > > support multi binaries to be uploaded and each binary file name
> > > > can be different.
> > >
> > > So you can write the file path to a second file and reload then, once
> > > per file.
> >
> > Thanks for the suggestion. Did some exploration on this approach and
> > would like to continue the discussion together with this suggested design.
> >
> > Ideal condition:
> > - one file note is enough for load binary and status return (capsule_load)
> > - load steps would be as simple as just:
> >    echo [binary file name] > capsule_load
> > - status return in the same command action. If failed, the echo will fail
> >    with the failing status code.
> >
> > Trade off:
> > - does not have the run time flexibility to load any firmware binaries at
> >    different different path as firmware class  only support one custom
> >    path inputted during boot time/load time. Unless to add new export
> >    function for firmware class.
> 
> Could you elaborate here? I don't understand this point.

Just to call out that using firmware class auto locate binary feature is limited
to locations:
- "/lib/firmware/updates/" UTS_RELEASE,
- "/lib/firmware/updates",
- "/lib/firmware/" UTS_RELEASE,
- "/lib/firmware"
and one custom path which inputted during firmware_class module load
time or kernel boot up time.

It is just not like the user helper interface which allow load the binary at
any path/location.

This really is not a big deal. User should cope with it.

> 
> > - if any typo on user level or file path not found, firmware class falls back
> >    to user helper interface. User is required to open another terminal to
> >    performs:
> >    -> echo 1 > loading
> >    -> cat capsule.bin > data
> >    -> echo 0 > loading
> >    Or wait for timeout.
> 
> Not if you use request_firmware_direct(), right?
> request_firmware_direct() explicitly does not invoke the user helper.
> 
> > - User has the capability to configure the firmware_class time out value.
> >    If the infinite value is set, the only method to break the infinite waiting
> >    by manually perform "echo -1 > loading" on the other terminal.
> 
> I'm not sure this is a problem if we use request_firmware_direct().

Oops... I miss out this function. Will rework using this function.

> 
> > Are you guys okay with these trade off?
> > Or you guys still okay with the previous 2 design idea?
> 
> I quite like the design that Borislav is proposing. Things become a lot
> simpler when we stop using request_firmware_nowait().
> 
> The next question is whether we want to batch passing capsules to the
> firmware. The UpdateCapsule() EFI runtime service allows you to chain
> capsule blobs together and pass a scatter-gather list.
> 
> If we do want to batch uploading then we need the equivalent of the
> 'reload' file from the microcode loader to signal to the kernel that the
> userland process has finished uploading capsules, and the kernel can now
> send them to the firmware as one chunk.
> 
> Also, Andy previously raised the point about multiple userland processes
> passing capsules to the firmware simultaneously and the races that
> occur, so we'd need a way to make that atomic.
> 
> I'd much prefer to send one capsule at a time to the firmware, because
> it just makes this whole thing simpler.
> 
> Wilson, I'm really looking for your input here with how capsule
> uploading works on Quark. If we can just repeatedly call UpdateCapsule()
> with one capsule file at a time, that's fine. If we absolutely must
> bundle multiple capsule files together then we probably need to provide
> some atomicity.
> 
> Thoughts?

Quark does not need batch uploading. Even I tested multiple capsules on
Quark by doing repeatedly calling UpdateCapsule() is still working for Quark.
So, Quark does not require this bundling.

> 
> > > Alternatively, you can combine all the files into a cpio archive like
> > > we do with the microcode loader too, and let the kernel parse the cpio
> > > archive.
> >
> > I believe this will make the implementation complicated compare to above.
> 
> Agreed. The capsule files we're sending to the firmware (via this
> interface) already contain all the information we need to stitch
> multiple binaries together - there's really no reason to bundle things
> any further.
> 
> --
> Matt Fleming, Intel Open Source Technology Center

Hi Greg, Ming Lei, Sam & Andy,

Do you guys have any others concern on Borislav proposed approach?
Thanks.


Regards,
Wilson

Re: Re: [PATCH v2 3/3] efi: Capsule update with user helper interface

[Matt Fleming]

On Mon, 02 Mar, at 10:59:00AM, Kweh Hock Leong wrote:
> > -----Original Message-----
> > From: Borislav Petkov [mailto:bp@alien8.de]
> > Sent: Wednesday, February 25, 2015 8:49 PM
> > 
> > On Wed, Feb 25, 2015 at 12:38:20PM +0000, Kweh, Hock Leong wrote:
> > > The reason we use this interface for efi capsule is that efi capsule
> > > support multi binaries to be uploaded and each binary file name
> > > can be different.
> > 
> > So you can write the file path to a second file and reload then, once
> > per file.
> 
> Thanks for the suggestion. Did some exploration on this approach and
> would like to continue the discussion together with this suggested design.
> 
> Ideal condition:
> - one file note is enough for load binary and status return (capsule_load)
> - load steps would be as simple as just:
>    echo [binary file name] > capsule_load
> - status return in the same command action. If failed, the echo will fail
>    with the failing status code.
> 
> Trade off:
> - does not have the run time flexibility to load any firmware binaries at
>    different different path as firmware class  only support one custom
>    path inputted during boot time/load time. Unless to add new export
>    function for firmware class.

Could you elaborate here? I don't understand this point.

> - if any typo on user level or file path not found, firmware class falls back
>    to user helper interface. User is required to open another terminal to
>    performs:
>    -> echo 1 > loading
>    -> cat capsule.bin > data
>    -> echo 0 > loading
>    Or wait for timeout.

Not if you use request_firmware_direct(), right?
request_firmware_direct() explicitly does not invoke the user helper.

> - User has the capability to configure the firmware_class time out value.
>    If the infinite value is set, the only method to break the infinite waiting
>    by manually perform "echo -1 > loading" on the other terminal.
 
I'm not sure this is a problem if we use request_firmware_direct().

> Are you guys okay with these trade off?
> Or you guys still okay with the previous 2 design idea?
 
I quite like the design that Borislav is proposing. Things become a lot
simpler when we stop using request_firmware_nowait().

The next question is whether we want to batch passing capsules to the
firmware. The UpdateCapsule() EFI runtime service allows you to chain
capsule blobs together and pass a scatter-gather list.

If we do want to batch uploading then we need the equivalent of the
'reload' file from the microcode loader to signal to the kernel that the
userland process has finished uploading capsules, and the kernel can now
send them to the firmware as one chunk.

Also, Andy previously raised the point about multiple userland processes
passing capsules to the firmware simultaneously and the races that
occur, so we'd need a way to make that atomic.

I'd much prefer to send one capsule at a time to the firmware, because
it just makes this whole thing simpler.

Wilson, I'm really looking for your input here with how capsule
uploading works on Quark. If we can just repeatedly call UpdateCapsule()
with one capsule file at a time, that's fine. If we absolutely must
bundle multiple capsule files together then we probably need to provide
some atomicity.

Thoughts?

> > Alternatively, you can combine all the files into a cpio archive like
> > we do with the microcode loader too, and let the kernel parse the cpio
> > archive.
> 
> I believe this will make the implementation complicated compare to above.

Agreed. The capsule files we're sending to the firmware (via this
interface) already contain all the information we need to stitch
multiple binaries together - there's really no reason to bundle things
any further.

-- 
Matt Fleming, Intel Open Source Technology Center

RE: Re: [PATCH v2 3/3] efi: Capsule update with user helper interface

[Kweh, Hock Leong]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 2121 bytes --]

> -----Original Message-----
> From: Borislav Petkov [mailto:bp@alien8.de]
> Sent: Wednesday, February 25, 2015 8:49 PM
> 
> On Wed, Feb 25, 2015 at 12:38:20PM +0000, Kweh, Hock Leong wrote:
> > The reason we use this interface for efi capsule is that efi capsule
> > support multi binaries to be uploaded and each binary file name
> > can be different.
> 
> So you can write the file path to a second file and reload then, once
> per file.

Thanks for the suggestion. Did some exploration on this approach and
would like to continue the discussion together with this suggested design.

Ideal condition:
- one file note is enough for load binary and status return (capsule_load)
- load steps would be as simple as just:
   echo [binary file name] > capsule_load
- status return in the same command action. If failed, the echo will fail
   with the failing status code.

Trade off:
- does not have the run time flexibility to load any firmware binaries at
   different different path as firmware class  only support one custom
   path inputted during boot time/load time. Unless to add new export
   function for firmware class.
- if any typo on user level or file path not found, firmware class falls back
   to user helper interface. User is required to open another terminal to
   performs:
   -> echo 1 > loading
   -> cat capsule.bin > data
   -> echo 0 > loading
   Or wait for timeout.
- User has the capability to configure the firmware_class time out value.
   If the infinite value is set, the only method to break the infinite waiting
   by manually perform "echo -1 > loading" on the other terminal.

Are you guys okay with these trade off?
Or you guys still okay with the previous 2 design idea?

> 
> Alternatively, you can combine all the files into a cpio archive like
> we do with the microcode loader too, and let the kernel parse the cpio
> archive.

I believe this will make the implementation complicated compare to above.


Regards,
Wilson
ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥