[PATCH 2/2] efi: Capsule update support

[PATCH 2/2] efi: Capsule update support

[Matt Fleming]

From: Matt Fleming <matt.fleming-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>

The EFI capsule mechanism allows data blobs to be passed to the EFI
firmware. This patch just introduces the main infrastruture for
interacting with the firmware.

Once a capsule has been passed to the firmware, the next reboot will
always be performed using the ResetSystem() EFI runtime service, which
may involve overriding the reboot type specified by reboot=. This
ensures the reset value returned by QueryCapsuleCapabilities() is used
to reset the system, which is required for the capsule to be processed.

Cc: Leif Lindholm <leif.lindholm-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
Cc: "Kweh, Hock Leong" <hock.leong.kweh-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
Signed-off-by: Matt Fleming <matt.fleming-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
---
 arch/x86/kernel/reboot.c       |   7 ++
 drivers/firmware/efi/Makefile  |   2 +-
 drivers/firmware/efi/capsule.c | 239 +++++++++++++++++++++++++++++++++++++++++
 drivers/firmware/efi/reboot.c  |  12 ++-
 include/linux/efi.h            |  20 ++++
 5 files changed, 278 insertions(+), 2 deletions(-)
 create mode 100644 drivers/firmware/efi/capsule.c

diff --git a/arch/x86/kernel/reboot.c b/arch/x86/kernel/reboot.c
index 17962e667a91..59fe1c03c71a 100644
--- a/arch/x86/kernel/reboot.c
+++ b/arch/x86/kernel/reboot.c
@@ -516,6 +516,13 @@ static void native_machine_emergency_restart(void)
 	mode = reboot_mode == REBOOT_WARM ? 0x1234 : 0;
 	*((unsigned short *)__va(0x472)) = mode;
 
+	/*
+	 * If an EFI capsule has been registered with the firmware then
+	 * override the reboot= parameter.
+	 */
+	if (efi_capsule_pending(NULL))
+		reboot_type = BOOT_EFI;
+
 	for (;;) {
 		/* Could also try the reset bit in the Hammer NB */
 		switch (reboot_type) {
diff --git a/drivers/firmware/efi/Makefile b/drivers/firmware/efi/Makefile
index d8be608a9f3b..698846e67b09 100644
--- a/drivers/firmware/efi/Makefile
+++ b/drivers/firmware/efi/Makefile
@@ -1,7 +1,7 @@
 #
 # Makefile for linux kernel
 #
-obj-$(CONFIG_EFI)			+= efi.o vars.o reboot.o
+obj-$(CONFIG_EFI)			+= efi.o vars.o reboot.o capsule.o
 obj-$(CONFIG_EFI_VARS)			+= efivars.o
 obj-$(CONFIG_EFI_VARS_PSTORE)		+= efi-pstore.o
 obj-$(CONFIG_UEFI_CPER)			+= cper.o
diff --git a/drivers/firmware/efi/capsule.c b/drivers/firmware/efi/capsule.c
new file mode 100644
index 000000000000..475643d66258
--- /dev/null
+++ b/drivers/firmware/efi/capsule.c
@@ -0,0 +1,239 @@
+/*
+ * EFI capsule support.
+ *
+ * Copyright 2013 Intel Corporation <matt.fleming-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
+ *
+ * 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) "efi-capsule: " fmt
+
+#include <linux/slab.h>
+#include <linux/mutex.h>
+#include <linux/highmem.h>
+#include <linux/efi.h>
+#include <linux/vmalloc.h>
+#include <asm/io.h>
+
+typedef struct {
+	u64 length;
+	u64 data;
+} efi_capsule_block_desc_t;
+
+static bool capsule_pending;
+static int efi_reset_type = -1;
+
+/*
+ * capsule_mutex serialises access to both 'capsule_pending' and
+ * 'efi_reset_type'.
+ *
+ * This mutex must be held across calls to efi_capsule_supported() and
+ * efi_update_capsule() so that the operation is atomic. This ensures
+ * that efi_update_capsule() isn't called with a capsule that requires a
+ * different reset type to the registered 'efi_reset_type'.
+ */
+static DEFINE_MUTEX(capsule_mutex);
+
+static int efi_update_capsule(efi_capsule_header_t *capsule,
+			      struct page **pages, size_t size, int reset);
+
+/**
+ * efi_capsule_pending - has a capsule been passed to the firmware?
+ * @reset_type: store the type of EFI reset if capsule is pending
+ *
+ * To ensure that the registered capsule is processed correctly by the
+ * firmware we need to perform a specific type of reset. If a capsule is
+ * pending return the reset type in @reset_type.
+ */
+bool efi_capsule_pending(int *reset_type)
+{
+	bool rv = false;
+
+	mutex_lock(&capsule_mutex);
+	if (!capsule_pending)
+		goto out;
+
+	if (reset_type)
+		*reset_type = efi_reset_type;
+	rv = true;
+
+out:
+	mutex_unlock(&capsule_mutex);
+	return rv;
+}
+
+/**
+ * efi_capsule_supported - does the firmware support the capsule?
+ * @guid: vendor guid of capsule
+ * @flags: capsule flags
+ * @size: size of capsule data
+ * @reset: the reset type required for this capsule
+ *
+ * Check whether a capsule with @flags is supported and that @size
+ * doesn't exceed the maximum size for a capsule.
+ */
+int efi_capsule_supported(efi_guid_t guid, u32 flags, size_t size, int *reset)
+{
+	efi_capsule_header_t *capsule;
+	efi_status_t status;
+	u64 max_size;
+	int rv = 0;
+
+	lockdep_assert_held(&capsule_mutex);
+
+	capsule = kmalloc(sizeof(*capsule), GFP_KERNEL);
+	if (!capsule)
+		return -ENOMEM;
+
+	capsule->headersize = capsule->imagesize = sizeof(*capsule);
+	memcpy(&capsule->guid, &guid, sizeof(efi_guid_t));
+	capsule->flags = flags;
+
+	status = efi.query_capsule_caps(&capsule, 1, &max_size, reset);
+	if (status != EFI_SUCCESS) {
+		rv = efi_status_to_err(status);
+		goto out;
+	}
+
+	if (size > max_size)
+		rv = -ENOSPC;
+out:
+	kfree(capsule);
+	return rv;
+}
+
+/**
+ * efi_capsule_update - send a capsule to the firmware
+ * @capsule: capsule to send to firmware
+ * @pages: an array of capsule data
+ *
+ * Check that @capsule is supported by the firmware and that it doesn't
+ * conflict with any previously registered capsule.
+ */
+int efi_capsule_update(efi_capsule_header_t *capsule, struct page **pages)
+{
+	efi_guid_t guid = capsule->guid;
+	size_t size = capsule->imagesize;
+	u32 flags = capsule->flags;
+	int rv, reset_type;
+
+	mutex_lock(&capsule_mutex);
+	rv = efi_capsule_supported(guid, flags, size, &reset_type);
+	if (rv)
+		goto out;
+
+	if (efi_reset_type >= 0 && efi_reset_type != reset_type) {
+		pr_err("Incompatible capsule reset type %d\n", reset_type);
+		rv = -EINVAL;
+		goto out;
+	}
+
+	rv = efi_update_capsule(capsule, pages, size, reset_type);
+out:
+	mutex_unlock(&capsule_mutex);
+	return rv;
+}
+EXPORT_SYMBOL_GPL(efi_capsule_update);
+
+#define BLOCKS_PER_PAGE	(PAGE_SIZE / sizeof(efi_capsule_block_desc_t))
+
+/*
+ * How many pages of block descriptors do we need to map 'nr_pages'?
+ *
+ * Every list of block descriptors in a page must end with a
+ * continuation pointer. The last continuation pointer of the lage page
+ * must be zero to mark the end of the chain.
+ */
+static inline unsigned int num_block_pages(unsigned int nr_pages)
+{
+	return DIV_ROUND_UP(nr_pages, BLOCKS_PER_PAGE - 1);
+}
+
+/**
+ * efi_update_capsule - pass a single capsule to the firmware.
+ * @capsule: capsule to send to the firmware.
+ * @pages: an array of capsule data.
+ * @size: total size of capsule data + headers in @capsule.
+ * @reset: the reset type required for @capsule
+ *
+ * Map @capsule with EFI capsule block descriptors in PAGE_SIZE chunks.
+ * @size needn't necessarily be a multiple of PAGE_SIZE - we can handle
+ * a trailing chunk that is smaller than PAGE_SIZE.
+ *
+ * @capsule MUST be virtually contiguous.
+ *
+ * Return 0 on success.
+ */
+static int efi_update_capsule(efi_capsule_header_t *capsule,
+			      struct page **pages, size_t size, int reset)
+{
+	efi_capsule_block_desc_t *block = NULL;
+	struct page **block_pgs;
+	efi_status_t status;
+	unsigned int nr_data_pgs, nr_block_pgs;
+	int i, j, err = -ENOMEM;
+
+	lockdep_assert_held(&capsule_mutex);
+
+	nr_data_pgs = DIV_ROUND_UP(size, PAGE_SIZE);
+	nr_block_pgs = num_block_pages(nr_data_pgs);
+
+	block_pgs = kzalloc(nr_block_pgs * sizeof(*block_pgs), GFP_KERNEL);
+	if (!block_pgs)
+		return -ENOMEM;
+
+	for (i = 0; i < nr_block_pgs; i++) {
+		block_pgs[i] = alloc_page(GFP_KERNEL);
+		if (!block_pgs[i])
+			goto fail;
+	}
+
+	for (i = 0; i < nr_block_pgs; i++) {
+		block = kmap(block_pgs[i]);
+		if (!block)
+			goto fail;
+
+		for (j = 0; j < BLOCKS_PER_PAGE - 1 && nr_data_pgs > 0; j++) {
+			u64 sz = min_t(u64, size, PAGE_SIZE);
+
+			block[j].length = sz;
+			block[j].data = page_to_phys(*pages++);
+
+			size -= sz;
+			nr_data_pgs--;
+		}
+
+		/* Continuation pointer */
+		block[j].length = 0;
+
+		if (i + 1 == nr_block_pgs)
+			block[j].data = 0;
+		else
+			block[j].data = page_to_phys(block_pgs[i + 1]);
+
+		kunmap(block_pgs[i]);
+	}
+
+	status = efi.update_capsule(&capsule, 1, page_to_phys(block_pgs[0]));
+	if (status != EFI_SUCCESS) {
+		pr_err("update_capsule fail: 0x%lx\n", status);
+		err = efi_status_to_err(status);
+		goto fail;
+	}
+
+	capsule_pending = true;
+	efi_reset_type = reset;
+
+	kfree(block_pgs);
+	return 0;
+
+fail:
+	for (i = 0; i < nr_block_pgs; i++) {
+		if (block_pgs[i])
+			__free_page(block_pgs[i]);
+	}
+
+	kfree(block_pgs);
+	return err;
+}
diff --git a/drivers/firmware/efi/reboot.c b/drivers/firmware/efi/reboot.c
index 9c59d1c795d1..1afb3e932cd1 100644
--- a/drivers/firmware/efi/reboot.c
+++ b/drivers/firmware/efi/reboot.c
@@ -9,7 +9,8 @@ int efi_reboot_quirk_mode = -1;
 
 void efi_reboot(enum reboot_mode reboot_mode, const char *__unused)
 {
-	int efi_mode;
+	const char *str[] = { "cold", "warm", "shutdown", "platform" };
+	int efi_mode, cap_reset_mode;
 
 	if (!efi_enabled(EFI_RUNTIME_SERVICES))
 		return;
@@ -30,6 +31,15 @@ void efi_reboot(enum reboot_mode reboot_mode, const char *__unused)
 	if (efi_reboot_quirk_mode != -1)
 		efi_mode = efi_reboot_quirk_mode;
 
+	if (efi_capsule_pending(&cap_reset_mode)) {
+		if (efi_mode != cap_reset_mode)
+			printk("efi: %s reset requested but pending capsule "
+			       "update requires %s reset... Performing "
+			       "%s reset\n", str[efi_mode], str[cap_reset_mode],
+			       str[cap_reset_mode]);
+		efi_mode = cap_reset_mode;
+	}
+
 	efi.reset_system(efi_mode, EFI_SUCCESS, 0, NULL);
 }
 
diff --git a/include/linux/efi.h b/include/linux/efi.h
index 48d936cf17d3..3730cb071e4e 100644
--- a/include/linux/efi.h
+++ b/include/linux/efi.h
@@ -119,6 +119,13 @@ typedef struct {
 } efi_capsule_header_t;
 
 /*
+ * EFI capsule flags
+ */
+#define EFI_CAPSULE_PERSIST_ACROSS_RESET	0x00010000
+#define EFI_CAPSULE_POPULATE_SYSTEM_TABLE	0x00020000
+#define EFI_CAPSULE_INITIATE_RESET		0x00040000
+
+/*
  * Allocation types for calls to boottime->allocate_pages.
  */
 #define EFI_ALLOCATE_ANY_PAGES		0
@@ -953,6 +960,12 @@ static inline bool efi_enabled(int feature)
 }
 static inline void
 efi_reboot(enum reboot_mode reboot_mode, const char *__unused) {}
+
+static inline bool
+efi_capsule_pending(int *reset_type)
+{
+	return false;
+}
 #endif
 
 /*
@@ -1199,6 +1212,10 @@ int efivars_sysfs_init(void);
 #define EFIVARS_DATA_SIZE_MAX 1024
 
 #endif /* CONFIG_EFI_VARS */
+extern bool efi_capsule_pending(int *reset_type);
+
+extern int efi_capsule_supported(efi_guid_t guid, u32 flags,
+				 size_t size, int *reset);
 
 #ifdef CONFIG_EFI_RUNTIME_MAP
 int efi_runtime_map_init(struct kobject *);
@@ -1277,4 +1294,7 @@ efi_status_t handle_cmdline_files(efi_system_table_t *sys_table_arg,
 efi_status_t efi_parse_options(char *cmdline);
 
 bool efi_runtime_disabled(void);
+
+extern int efi_capsule_update(efi_capsule_header_t *capsule,
+			      struct page **pages);
 #endif /* _LINUX_EFI_H */
-- 
1.9.3

Re: [PATCH 2/2] efi: Capsule update support

[Sam Protsenko]

Hi Matt,

1. Why x86 code isn't separated to another patch?
2. drivers/firmware/efi/reboot.c: efi_reboot():
    One shouldn't use "printk()" with no KERN_* stuff passed into it.
I'd recommend to use "pr_info()" macro or something like that.

Re: [PATCH 2/2] efi: Capsule update support

[Borislav Petkov]

On Tue, Oct 07, 2014 at 03:42:31PM +0100, Matt Fleming wrote:
> From: Matt Fleming <matt.fleming-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> 
> The EFI capsule mechanism allows data blobs to be passed to the EFI
> firmware. This patch just introduces the main infrastruture for
> interacting with the firmware.
> 
> Once a capsule has been passed to the firmware, the next reboot will
> always be performed using the ResetSystem() EFI runtime service, which
> may involve overriding the reboot type specified by reboot=. This
> ensures the reset value returned by QueryCapsuleCapabilities() is used
> to reset the system, which is required for the capsule to be processed.
> 
> Cc: Leif Lindholm <leif.lindholm-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> Cc: "Kweh, Hock Leong" <hock.leong.kweh-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> Signed-off-by: Matt Fleming <matt.fleming-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>

Just a couple of quick thoughts which might or might not make sense...

> ---
>  arch/x86/kernel/reboot.c       |   7 ++
>  drivers/firmware/efi/Makefile  |   2 +-
>  drivers/firmware/efi/capsule.c | 239 +++++++++++++++++++++++++++++++++++++++++
>  drivers/firmware/efi/reboot.c  |  12 ++-
>  include/linux/efi.h            |  20 ++++
>  5 files changed, 278 insertions(+), 2 deletions(-)
>  create mode 100644 drivers/firmware/efi/capsule.c
> 
> diff --git a/arch/x86/kernel/reboot.c b/arch/x86/kernel/reboot.c
> index 17962e667a91..59fe1c03c71a 100644
> --- a/arch/x86/kernel/reboot.c
> +++ b/arch/x86/kernel/reboot.c
> @@ -516,6 +516,13 @@ static void native_machine_emergency_restart(void)
>  	mode = reboot_mode == REBOOT_WARM ? 0x1234 : 0;
>  	*((unsigned short *)__va(0x472)) = mode;
>  
> +	/*
> +	 * If an EFI capsule has been registered with the firmware then
> +	 * override the reboot= parameter.
> +	 */
> +	if (efi_capsule_pending(NULL))
> +		reboot_type = BOOT_EFI;
> +
>  	for (;;) {
>  		/* Could also try the reset bit in the Hammer NB */
>  		switch (reboot_type) {
> diff --git a/drivers/firmware/efi/Makefile b/drivers/firmware/efi/Makefile
> index d8be608a9f3b..698846e67b09 100644
> --- a/drivers/firmware/efi/Makefile
> +++ b/drivers/firmware/efi/Makefile
> @@ -1,7 +1,7 @@
>  #
>  # Makefile for linux kernel
>  #
> -obj-$(CONFIG_EFI)			+= efi.o vars.o reboot.o
> +obj-$(CONFIG_EFI)			+= efi.o vars.o reboot.o capsule.o
>  obj-$(CONFIG_EFI_VARS)			+= efivars.o
>  obj-$(CONFIG_EFI_VARS_PSTORE)		+= efi-pstore.o
>  obj-$(CONFIG_UEFI_CPER)			+= cper.o
> diff --git a/drivers/firmware/efi/capsule.c b/drivers/firmware/efi/capsule.c
> new file mode 100644
> index 000000000000..475643d66258
> --- /dev/null
> +++ b/drivers/firmware/efi/capsule.c
> @@ -0,0 +1,239 @@
> +/*
> + * EFI capsule support.
> + *
> + * Copyright 2013 Intel Corporation <matt.fleming-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> + *
> + * 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) "efi-capsule: " fmt
> +
> +#include <linux/slab.h>
> +#include <linux/mutex.h>
> +#include <linux/highmem.h>
> +#include <linux/efi.h>
> +#include <linux/vmalloc.h>
> +#include <asm/io.h>
> +
> +typedef struct {
> +	u64 length;
> +	u64 data;
> +} efi_capsule_block_desc_t;
> +
> +static bool capsule_pending;
> +static int efi_reset_type = -1;
> +
> +/*
> + * capsule_mutex serialises access to both 'capsule_pending' and
> + * 'efi_reset_type'.
> + *
> + * This mutex must be held across calls to efi_capsule_supported() and
> + * efi_update_capsule() so that the operation is atomic. This ensures
> + * that efi_update_capsule() isn't called with a capsule that requires a
> + * different reset type to the registered 'efi_reset_type'.
> + */
> +static DEFINE_MUTEX(capsule_mutex);
> +
> +static int efi_update_capsule(efi_capsule_header_t *capsule,
> +			      struct page **pages, size_t size, int reset);
> +
> +/**
> + * efi_capsule_pending - has a capsule been passed to the firmware?
> + * @reset_type: store the type of EFI reset if capsule is pending
> + *
> + * To ensure that the registered capsule is processed correctly by the
> + * firmware we need to perform a specific type of reset. If a capsule is
> + * pending return the reset type in @reset_type.
> + */
> +bool efi_capsule_pending(int *reset_type)
> +{
> +	bool rv = false;
> +
> +	mutex_lock(&capsule_mutex);
> +	if (!capsule_pending)
> +		goto out;
> +
> +	if (reset_type)
> +		*reset_type = efi_reset_type;
> +	rv = true;
> +
> +out:
> +	mutex_unlock(&capsule_mutex);
> +	return rv;
> +}
> +
> +/**
> + * efi_capsule_supported - does the firmware support the capsule?
> + * @guid: vendor guid of capsule
> + * @flags: capsule flags
> + * @size: size of capsule data
> + * @reset: the reset type required for this capsule
> + *
> + * Check whether a capsule with @flags is supported and that @size
> + * doesn't exceed the maximum size for a capsule.
> + */
> +int efi_capsule_supported(efi_guid_t guid, u32 flags, size_t size, int *reset)
> +{
> +	efi_capsule_header_t *capsule;
> +	efi_status_t status;
> +	u64 max_size;
> +	int rv = 0;
> +
> +	lockdep_assert_held(&capsule_mutex);
> +
> +	capsule = kmalloc(sizeof(*capsule), GFP_KERNEL);
> +	if (!capsule)
> +		return -ENOMEM;
> +
> +	capsule->headersize = capsule->imagesize = sizeof(*capsule);
> +	memcpy(&capsule->guid, &guid, sizeof(efi_guid_t));
> +	capsule->flags = flags;
> +
> +	status = efi.query_capsule_caps(&capsule, 1, &max_size, reset);
> +	if (status != EFI_SUCCESS) {
> +		rv = efi_status_to_err(status);
> +		goto out;
> +	}
> +
> +	if (size > max_size)
> +		rv = -ENOSPC;
> +out:
> +	kfree(capsule);
> +	return rv;
> +}
> +
> +/**
> + * efi_capsule_update - send a capsule to the firmware
> + * @capsule: capsule to send to firmware
> + * @pages: an array of capsule data
> + *
> + * Check that @capsule is supported by the firmware and that it doesn't
> + * conflict with any previously registered capsule.
> + */
> +int efi_capsule_update(efi_capsule_header_t *capsule, struct page **pages)

You have efi_capsule_update() vs efi_update_capsule(). Maybe change the
names a bit more for differentiation. Or prepend the workhorse doing all
the work with "__" or so...

> +{
> +	efi_guid_t guid = capsule->guid;
> +	size_t size = capsule->imagesize;
> +	u32 flags = capsule->flags;
> +	int rv, reset_type;
> +
> +	mutex_lock(&capsule_mutex);
> +	rv = efi_capsule_supported(guid, flags, size, &reset_type);
> +	if (rv)
> +		goto out;
> +
> +	if (efi_reset_type >= 0 && efi_reset_type != reset_type) {
> +		pr_err("Incompatible capsule reset type %d\n", reset_type);
> +		rv = -EINVAL;
> +		goto out;
> +	}
> +
> +	rv = efi_update_capsule(capsule, pages, size, reset_type);
> +out:
> +	mutex_unlock(&capsule_mutex);
> +	return rv;
> +}
> +EXPORT_SYMBOL_GPL(efi_capsule_update);
> +
> +#define BLOCKS_PER_PAGE	(PAGE_SIZE / sizeof(efi_capsule_block_desc_t))
> +
> +/*
> + * How many pages of block descriptors do we need to map 'nr_pages'?
> + *
> + * Every list of block descriptors in a page must end with a
> + * continuation pointer. The last continuation pointer of the lage page
> + * must be zero to mark the end of the chain.
> + */
> +static inline unsigned int num_block_pages(unsigned int nr_pages)
> +{
> +	return DIV_ROUND_UP(nr_pages, BLOCKS_PER_PAGE - 1);
> +}
> +
> +/**
> + * efi_update_capsule - pass a single capsule to the firmware.
> + * @capsule: capsule to send to the firmware.
> + * @pages: an array of capsule data.
> + * @size: total size of capsule data + headers in @capsule.
> + * @reset: the reset type required for @capsule
> + *
> + * Map @capsule with EFI capsule block descriptors in PAGE_SIZE chunks.
> + * @size needn't necessarily be a multiple of PAGE_SIZE - we can handle
> + * a trailing chunk that is smaller than PAGE_SIZE.
> + *
> + * @capsule MUST be virtually contiguous.
> + *
> + * Return 0 on success.
> + */
> +static int efi_update_capsule(efi_capsule_header_t *capsule,
> +			      struct page **pages, size_t size, int reset)
> +{
> +	efi_capsule_block_desc_t *block = NULL;
> +	struct page **block_pgs;
> +	efi_status_t status;
> +	unsigned int nr_data_pgs, nr_block_pgs;
> +	int i, j, err = -ENOMEM;
> +
> +	lockdep_assert_held(&capsule_mutex);
> +
> +	nr_data_pgs = DIV_ROUND_UP(size, PAGE_SIZE);
> +	nr_block_pgs = num_block_pages(nr_data_pgs);
> +
> +	block_pgs = kzalloc(nr_block_pgs * sizeof(*block_pgs), GFP_KERNEL);
> +	if (!block_pgs)
> +		return -ENOMEM;
> +
> +	for (i = 0; i < nr_block_pgs; i++) {
> +		block_pgs[i] = alloc_page(GFP_KERNEL);

Maybe alloc_pages() once we verify that it actually gives phys. contig.
memory and maybe also try to do it outside of the locked region. I don't
know if it would matter to drop the locks though as capsule updating is
not something you do pretty often. I'd hope!

-- 
Regards/Gruss,
    Boris.

Sent from a fat crate under my desk. Formatting is fine.
--

Re: [PATCH 2/2] efi: Capsule update support

[Matt Fleming]

On Fri, 10 Oct, at 06:55:49PM, Sam Protsenko wrote:
> Hi Matt,
> 
> 1. Why x86 code isn't separated to another patch?

When I originally wrote this patch in 2013 arm64 support didn't exist,
and ia64 isn't going to be using capsule support. I can separate that
out into a separate patch though, no problem.

> 2. drivers/firmware/efi/reboot.c: efi_reboot():
>     One shouldn't use "printk()" with no KERN_* stuff passed into it.
> I'd recommend to use "pr_info()" macro or something like that.

Oops, I missed that, good catch.

Next time, could you please quote the part of the patch you're
commenting on inline? That would have saved me searching through the
original email.

-- 
Matt Fleming, Intel Open Source Technology Center

Re: [PATCH 2/2] efi: Capsule update support

[Sam Protsenko]

> When I originally wrote this patch in 2013 arm64 support didn't exist,
> and ia64 isn't going to be using capsule support. I can separate that
> out into a separate patch though, no problem.

For me it's just the matter of good VCS practices. In this case I call
this "patch atomicity" (one patch per feature). It's not about your
patch particularly, it's just policy. In the end it boils down to next
two things:
 1. Separating common code from platform code makes it easier to use
"git bisect" in case of regressions.
 2. This way if we want to revert patch, we can revert only stuff we
want, not touching another part (e.g. you want to revert platform
code, you can keep common code in place).

> Next time, could you please quote the part of the patch you're
> commenting on inline? That would have saved me searching through the
> original email.

Sure, my bad. I know it's general approach in mailing lists to review
patch, just forgot it.


On 13 October 2014 12:53, Matt Fleming <matt-HNK1S37rvNbeXh+fF434Mdi2O/JbrIOy@public.gmane.org> wrote:
> On Fri, 10 Oct, at 06:55:49PM, Sam Protsenko wrote:
>> Hi Matt,
>>
>> 1. Why x86 code isn't separated to another patch?
>
> When I originally wrote this patch in 2013 arm64 support didn't exist,
> and ia64 isn't going to be using capsule support. I can separate that
> out into a separate patch though, no problem.
>
>> 2. drivers/firmware/efi/reboot.c: efi_reboot():
>>     One shouldn't use "printk()" with no KERN_* stuff passed into it.
>> I'd recommend to use "pr_info()" macro or something like that.
>
> Oops, I missed that, good catch.
>
> Next time, could you please quote the part of the patch you're
> commenting on inline? That would have saved me searching through the
> original email.
>
> --
> Matt Fleming, Intel Open Source Technology Center

Re: [PATCH 2/2] efi: Capsule update support

[Sam Protsenko]

Matt,

I tried to play with your code and now I have some extra notes about this patch:

1. As it was proposed earlier, I support thought that it would be nice to
   rename function names in next way:

     efi_update_capsule -> __efi_update_capsule
     efi_capsule_update -> efi_update_capsule

   because it's quite confusing to have both efi_update_capsule() and
   efi_capsule_update(). Besides, EFI function called UpdateCapsule, so it's
   good idea to stick to this name in kernel API (I mean exporting
   efi_update_capsule() instead of efi_capsule_update()).

2. UEFI's UpdateCapsule() runtime service supports passing more than one
   capsule to it (we can pass CapsuleCount argument to it for this purpose).
   But your particular kernel implementation allows us only to provide one
   capsule at a time. Is that was done for a reason? Can it be consider as
   shortcoming?

3. I noticed that you dropped efi_capsule_build() in this patch (w.r.t.
   https://git.kernel.org/cgit/linux/kernel/git/mfleming/efi.git/
   implementation). BTW, it should be declared in header there.
   Anyway, how do we suppose to build capsule to pass to efi_capsule_update()?
   I mean, it can take a quite large code to build a capsule (allocating pages
   etc). Wouldn't it be easier to user to use your API if it has something
   ready to use? Anyway, if it should be done like this, it would be nice
   to have a decent example code (use-case) how to use this API (maybe in
   Documentation/, idk), because it looks quite non-intuitive (for me at least).

4. Tedious stuff: I checked your patch with "checkpatch.pl" and it shows
    some warnings, please fix them if possible.

I will try to test and verify this patch further, will notify you if
notice any issues.


On 13 October 2014 18:43, Sam Protsenko <semen.protsenko-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> wrote:
>> When I originally wrote this patch in 2013 arm64 support didn't exist,
>> and ia64 isn't going to be using capsule support. I can separate that
>> out into a separate patch though, no problem.
>
> For me it's just the matter of good VCS practices. In this case I call
> this "patch atomicity" (one patch per feature). It's not about your
> patch particularly, it's just policy. In the end it boils down to next
> two things:
>  1. Separating common code from platform code makes it easier to use
> "git bisect" in case of regressions.
>  2. This way if we want to revert patch, we can revert only stuff we
> want, not touching another part (e.g. you want to revert platform
> code, you can keep common code in place).
>
>> Next time, could you please quote the part of the patch you're
>> commenting on inline? That would have saved me searching through the
>> original email.
>
> Sure, my bad. I know it's general approach in mailing lists to review
> patch, just forgot it.
>
>
> On 13 October 2014 12:53, Matt Fleming <matt-HNK1S37rvNbeXh+fF434Mdi2O/JbrIOy@public.gmane.org> wrote:
>> On Fri, 10 Oct, at 06:55:49PM, Sam Protsenko wrote:
>>> Hi Matt,
>>>
>>> 1. Why x86 code isn't separated to another patch?
>>
>> When I originally wrote this patch in 2013 arm64 support didn't exist,
>> and ia64 isn't going to be using capsule support. I can separate that
>> out into a separate patch though, no problem.
>>
>>> 2. drivers/firmware/efi/reboot.c: efi_reboot():
>>>     One shouldn't use "printk()" with no KERN_* stuff passed into it.
>>> I'd recommend to use "pr_info()" macro or something like that.
>>
>> Oops, I missed that, good catch.
>>
>> Next time, could you please quote the part of the patch you're
>> commenting on inline? That would have saved me searching through the
>> original email.
>>
>> --
>> Matt Fleming, Intel Open Source Technology Center

Re: [PATCH 2/2] efi: Capsule update support

[Matt Fleming]

On Fri, 10 Oct, at 08:28:47PM, Borislav Petkov wrote:
> 
> You have efi_capsule_update() vs efi_update_capsule(). Maybe change the
> names a bit more for differentiation. Or prepend the workhorse doing all
> the work with "__" or so...
 
Yeah, I really didn't come up with a great naming scheme here. I'll fix
that.

> > +
> > +	for (i = 0; i < nr_block_pgs; i++) {
> > +		block_pgs[i] = alloc_page(GFP_KERNEL);
> 
> Maybe alloc_pages() once we verify that it actually gives phys. contig.
> memory and maybe also try to do it outside of the locked region. I don't
> know if it would matter to drop the locks though as capsule updating is
> not something you do pretty often. I'd hope!

Actually, I'm not bothered about getting physically contiguous memory
because we pass a scatter gather list to the firmware anyway. What I was
looking for was to avoid doing high order allocations when we don't
really need them (lots of low order allocs are fine).

Right, allocating under the lock isn't a great idea. I'll take a look at
reworking this to do the allocation up front.

-- 
Matt Fleming, Intel Open Source Technology Center

Re: [PATCH 2/2] efi: Capsule update support

[Matt Fleming]

On Tue, 14 Oct, at 06:30:22PM, Sam Protsenko wrote:
> Matt,
> 
> I tried to play with your code and now I have some extra notes about this patch:
> 
> 1. As it was proposed earlier, I support thought that it would be nice to
>    rename function names in next way:
> 
>      efi_update_capsule -> __efi_update_capsule
>      efi_capsule_update -> efi_update_capsule
> 
>    because it's quite confusing to have both efi_update_capsule() and
>    efi_capsule_update(). Besides, EFI function called UpdateCapsule, so it's
>    good idea to stick to this name in kernel API (I mean exporting
>    efi_update_capsule() instead of efi_capsule_update()).
 
I'm not so convinced by that argument. Remember, we're building a kernel
API here, so we've got functions like,

  efi_capsule_supported()
  efi_capsule_pending()

I've stuck with efi_capsule_update() and __efi_capsule_update() for now,
to continue the efi_capsule* theme (avoiding both efi_capsule_update()
and efi_update_capsule() was a good point though).

> 2. UEFI's UpdateCapsule() runtime service supports passing more than one
>    capsule to it (we can pass CapsuleCount argument to it for this purpose).
>    But your particular kernel implementation allows us only to provide one
>    capsule at a time. Is that was done for a reason? Can it be consider as
>    shortcoming?
 
Yeah, the reason is simply that it makes the capsule management more
complicated if you have more than one capsule, and when testing the
patches (and experimenting with the features in the capsule-* branches
in my git tree) I didn't come across a scenario where sending multiple
capsules at one time was required.

Doesn't mean we couldn't extend the kernel API in the future, though.
We'd just need an in-kernel user first.

> 3. I noticed that you dropped efi_capsule_build() in this patch (w.r.t.
>    https://git.kernel.org/cgit/linux/kernel/git/mfleming/efi.git/
>    implementation). BTW, it should be declared in header there.
>    Anyway, how do we suppose to build capsule to pass to efi_capsule_update()?
>    I mean, it can take a quite large code to build a capsule (allocating pages
>    etc). Wouldn't it be easier to user to use your API if it has something
>    ready to use? Anyway, if it should be done like this, it would be nice
>    to have a decent example code (use-case) how to use this API (maybe in
>    Documentation/, idk), because it looks quite non-intuitive (for me at least).
 
The two patches that I sent are only preparatory patches for EFI capsule
support, and Kweh (Cc'd) is working on patches that implement a userland
interface.

Wilson, do you think you could post your patches by the beginning of
next week? They just need to give an idea of how we can use this API. 

> 4. Tedious stuff: I checked your patch with "checkpatch.pl" and it shows
>     some warnings, please fix them if possible.

Will do.

> I will try to test and verify this patch further, will notify you if
> notice any issues.

Great, thanks.

-- 
Matt Fleming, Intel Open Source Technology Center

Re: [PATCH 2/2] efi: Capsule update support

[Sam Protsenko]

Matt,

I've tested your patch with zero image size (no image passed, only headers)
and it crashes because there is no check for image size there.
This case (zero image size) seems to be legit according to specification
and also can be useful in real life. So I developed a little fix for your patch:

<<<<<<<<<<<<<<<<<<<<<<< cut here >>>>>>>>>>>>>>>>>>>>>
diff --git a/drivers/firmware/efi/capsule.c b/drivers/firmware/efi/capsule.c
index ca29bad..597b363 100644
--- a/drivers/firmware/efi/capsule.c
+++ b/drivers/firmware/efi/capsule.c
@@ -169,13 +169,17 @@ static int
efi_update_capsule(efi_capsule_header_t *capsule,
                   struct page **pages, size_t size, int reset)
 {
     efi_capsule_block_desc_t *block = NULL;
-    struct page **block_pgs;
+    struct page **block_pgs = NULL;
     efi_status_t status;
-    unsigned int nr_data_pgs, nr_block_pgs;
+    unsigned int nr_data_pgs = 0, nr_block_pgs = 0;
+    unsigned long sg_list = 0;
     int i, j, err = -ENOMEM;

     lockdep_assert_held(&capsule_mutex);

+    if (size == 0)
+        goto update_caps;
+
     nr_data_pgs = DIV_ROUND_UP(size, PAGE_SIZE);
     nr_block_pgs = num_block_pages(nr_data_pgs);

@@ -215,7 +219,10 @@ static int
efi_update_capsule(efi_capsule_header_t *capsule,
         kunmap(block_pgs[i]);
     }

-    status = efi.update_capsule(&capsule, 1, page_to_phys(block_pgs[0]));
+    sg_list = page_to_phys(block_pgs[0]);
+
+update_caps:
+    status = efi.update_capsule(&capsule, 1, sg_list);
     if (status != EFI_SUCCESS) {
         pr_err("update_capsule fail: 0x%lx\n", status);
         err = efi_status_to_err(status);
-- 
2.1.1
<<<<<<<<<<<<<<<<<<<<<<< cut here >>>>>>>>>>>>>>>>>>>>>

I'm planning to use your API for our UpdateCapsule test module so
it would be really helpful if you can include this fix to your patch.


On 16 October 2014 19:15, Matt Fleming <matt-HNK1S37rvNbeXh+fF434Mdi2O/JbrIOy@public.gmane.org> wrote:
> On Tue, 14 Oct, at 06:30:22PM, Sam Protsenko wrote:
>> Matt,
>>
>> I tried to play with your code and now I have some extra notes about this patch:
>>
>> 1. As it was proposed earlier, I support thought that it would be nice to
>>    rename function names in next way:
>>
>>      efi_update_capsule -> __efi_update_capsule
>>      efi_capsule_update -> efi_update_capsule
>>
>>    because it's quite confusing to have both efi_update_capsule() and
>>    efi_capsule_update(). Besides, EFI function called UpdateCapsule, so it's
>>    good idea to stick to this name in kernel API (I mean exporting
>>    efi_update_capsule() instead of efi_capsule_update()).
>
> I'm not so convinced by that argument. Remember, we're building a kernel
> API here, so we've got functions like,
>
>   efi_capsule_supported()
>   efi_capsule_pending()
>
> I've stuck with efi_capsule_update() and __efi_capsule_update() for now,
> to continue the efi_capsule* theme (avoiding both efi_capsule_update()
> and efi_update_capsule() was a good point though).
>
>> 2. UEFI's UpdateCapsule() runtime service supports passing more than one
>>    capsule to it (we can pass CapsuleCount argument to it for this purpose).
>>    But your particular kernel implementation allows us only to provide one
>>    capsule at a time. Is that was done for a reason? Can it be consider as
>>    shortcoming?
>
> Yeah, the reason is simply that it makes the capsule management more
> complicated if you have more than one capsule, and when testing the
> patches (and experimenting with the features in the capsule-* branches
> in my git tree) I didn't come across a scenario where sending multiple
> capsules at one time was required.
>
> Doesn't mean we couldn't extend the kernel API in the future, though.
> We'd just need an in-kernel user first.
>
>> 3. I noticed that you dropped efi_capsule_build() in this patch (w.r.t.
>>    https://git.kernel.org/cgit/linux/kernel/git/mfleming/efi.git/
>>    implementation). BTW, it should be declared in header there.
>>    Anyway, how do we suppose to build capsule to pass to efi_capsule_update()?
>>    I mean, it can take a quite large code to build a capsule (allocating pages
>>    etc). Wouldn't it be easier to user to use your API if it has something
>>    ready to use? Anyway, if it should be done like this, it would be nice
>>    to have a decent example code (use-case) how to use this API (maybe in
>>    Documentation/, idk), because it looks quite non-intuitive (for me at least).
>
> The two patches that I sent are only preparatory patches for EFI capsule
> support, and Kweh (Cc'd) is working on patches that implement a userland
> interface.
>
> Wilson, do you think you could post your patches by the beginning of
> next week? They just need to give an idea of how we can use this API.
>
>> 4. Tedious stuff: I checked your patch with "checkpatch.pl" and it shows
>>     some warnings, please fix them if possible.
>
> Will do.
>
>> I will try to test and verify this patch further, will notify you if
>> notice any issues.
>
> Great, thanks.
>
> --
> Matt Fleming, Intel Open Source Technology Center

Re: [PATCH 2/2] efi: Capsule update support

[Matt Fleming]

On Tue, 04 Nov, at 03:56:22PM, Sam Protsenko wrote:
> Matt,
> 
> I've tested your patch with zero image size (no image passed, only headers)
> and it crashes because there is no check for image size there.
> This case (zero image size) seems to be legit according to specification
> and also can be useful in real life. So I developed a little fix for your patch:
 
[...]

> I'm planning to use your API for our UpdateCapsule test module so
> it would be really helpful if you can include this fix to your patch.

Sure, I'll include that snippet and post fixed up code next week.

Thanks Sam.

-- 
Matt Fleming, Intel Open Source Technology Center