[PATCH v2 0/2] capsule pstore backend

[PATCH v2 0/2] capsule pstore backend

[Qiuxu Zhuo]

*** BLURB HERE ***

Qiuxu Zhuo (2):
  efi/capsule: Add 'capsule' lookup support
  efi: capsule pstore backend

 drivers/firmware/efi/Kconfig          |  21 ++
 drivers/firmware/efi/Makefile         |   1 +
 drivers/firmware/efi/capsule-pstore.c | 527 ++++++++++++++++++++++++++++++++++
 drivers/firmware/efi/capsule.c        | 107 +++++++
 drivers/firmware/efi/efi.c            |   4 +
 include/linux/efi.h                   |   4 +
 6 files changed, 664 insertions(+)
 create mode 100644 drivers/firmware/efi/capsule-pstore.c
--

Change Log (v1->v2):
  - Use memremap() instead of ioremap() for firmware tables 
  - Remove "default n" in Kconfig file
  - Move check_capsule_support() up a bit
  - Update SoB chain
-- 
2.9.0.GIT

[PATCH v2 1/2] efi/capsule: Add 'capsule' lookup support

[Qiuxu Zhuo]

Add the interface to get the array of EFI capsules when
parsing the configuration tables.

This code is based on Matt Fleming previous work from:
https://git.kernel.org/cgit/linux/kernel/git/mfleming/efi.git/commit/?h=capsule-pstore&id=99c5f047133555aa0442f64064e85b7da2d4a45f

Signed-off-by: Qiuxu Zhuo <qiuxu.zhuo-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
---
 drivers/firmware/efi/capsule.c | 107 +++++++++++++++++++++++++++++++++++++++++
 drivers/firmware/efi/efi.c     |   4 ++
 include/linux/efi.h            |   4 ++
 3 files changed, 115 insertions(+)

diff --git a/drivers/firmware/efi/capsule.c b/drivers/firmware/efi/capsule.c
index 6eedff4..7bf79fd 100644
--- a/drivers/firmware/efi/capsule.c
+++ b/drivers/firmware/efi/capsule.c
@@ -24,6 +24,11 @@ typedef struct {
 static bool capsule_pending;
 static bool stop_capsules;
 static int efi_reset_type = -1;
+/*
+ * Information about capsules we pulled from the EFI System Table.
+ */
+static efi_capsule_header_t **prev_capsules;
+static u32 efi_capsule_num;
 
 /*
  * capsule_mutex serialises access to both capsule_pending and
@@ -288,6 +293,108 @@ int efi_capsule_update(efi_capsule_header_t *capsule, struct page **pages)
 }
 EXPORT_SYMBOL_GPL(efi_capsule_update);
 
+static int extract_capsules(void)
+{
+	void *capsule;
+	size_t size;
+
+	if (efi.capsule == EFI_INVALID_TABLE_ADDR)
+		return 0;
+
+	capsule = memremap(efi.capsule, sizeof(efi_capsule_num), MEMREMAP_WB);
+	if (!capsule)
+		return -ENOMEM;
+
+	/*
+	 * The array of capsules is prefixed with the number of
+	 * capsule entries in the array.
+	 */
+	efi_capsule_num = *(uint32_t *)capsule;
+	iounmap(capsule);
+
+	if (!efi_capsule_num) {
+		pr_info("No capsules on extraction\n");
+		return 0;
+	}
+
+	size = efi_capsule_num * sizeof(*capsule);
+	capsule = memremap(efi.capsule, size, MEMREMAP_WB);
+	if (!capsule)
+		return -ENOMEM;
+
+	capsule += sizeof(uint32_t);
+	prev_capsules = (efi_capsule_header_t **)capsule;
+	if (!*prev_capsules)
+		pr_err("Capsule array has no entries\n");
+
+	return 0;
+}
+
+/**
+ * efi_capsule_lookup - search capsule array for entries.
+ * @guid: the guid to search for.
+ * @nr_caps: the number of entries found.
+ *
+ * Map each capsule header into the kernel's virtual address space and
+ * inspect the guid. Build an array of capsule headers with every
+ * capsule that is found with @guid. If a match is found the capsule
+ * remains mapped, otherwise it is unmapped.
+ *
+ * Returns an array of capsule headers, each element of which has the
+ * guid @guid. The number of elements in the array is stored in
+ * @nr_caps. Returns %NULL if no capsules were found and stores zero
+ * in @nr_caps.
+ */
+efi_capsule_header_t **efi_capsule_lookup(efi_guid_t guid, uint32_t *nr_caps)
+{
+	efi_capsule_header_t **capsules = NULL, **tmp;
+	size_t capsule_size = 0;
+	int i, rv;
+
+	rv = extract_capsules();
+	if (rv)
+		return ERR_PTR(rv);
+
+	*nr_caps = 0;
+	for (i = 0; i < efi_capsule_num; i++) {
+		efi_capsule_header_t *c;
+		size_t size;
+
+		c = memremap((resource_size_t)prev_capsules[i], sizeof(*c), MEMREMAP_WB);
+		if (!c) {
+			pr_err("Failed to memremap capsule\n");
+			continue;
+		}
+
+		size = c->imagesize;
+		iounmap(c);
+
+		c = memremap((resource_size_t)prev_capsules[i], size, MEMREMAP_WB);
+		if (!c) {
+			pr_err("Failed to memremap header + data\n");
+			continue;
+		}
+
+		if (!efi_guidcmp(c->guid, guid)) {
+			capsule_size += sizeof(*capsules);
+			tmp = krealloc(capsules, capsule_size, GFP_KERNEL);
+			if (!tmp) {
+				kfree(capsules);
+				return ERR_PTR(-ENOMEM);
+			}
+
+			capsules = tmp;
+			capsules[(*nr_caps)++] = c;
+			continue;
+		}
+
+		iounmap(c);
+	}
+
+	return capsules;
+}
+EXPORT_SYMBOL_GPL(efi_capsule_lookup);
+
 static int capsule_reboot_notify(struct notifier_block *nb, unsigned long event, void *cmd)
 {
 	mutex_lock(&capsule_mutex);
diff --git a/drivers/firmware/efi/efi.c b/drivers/firmware/efi/efi.c
index 9291480..03be9b8 100644
--- a/drivers/firmware/efi/efi.c
+++ b/drivers/firmware/efi/efi.c
@@ -52,6 +52,7 @@ struct efi __read_mostly efi = {
 	.properties_table	= EFI_INVALID_TABLE_ADDR,
 	.mem_attr_table		= EFI_INVALID_TABLE_ADDR,
 	.rng_seed		= EFI_INVALID_TABLE_ADDR,
+	.capsule		= EFI_INVALID_TABLE_ADDR,
 };
 EXPORT_SYMBOL(efi);
 
@@ -120,6 +121,8 @@ static ssize_t systab_show(struct kobject *kobj,
 		str += sprintf(str, "BOOTINFO=0x%lx\n", efi.boot_info);
 	if (efi.uga != EFI_INVALID_TABLE_ADDR)
 		str += sprintf(str, "UGA=0x%lx\n", efi.uga);
+	if (efi.capsule != EFI_INVALID_TABLE_ADDR)
+		str += sprintf(str, "CAPSULE=0x%lx\n", efi.capsule);
 
 	return str - buf;
 }
@@ -445,6 +448,7 @@ static __initdata efi_config_table_type_t common_tables[] = {
 	{EFI_PROPERTIES_TABLE_GUID, "PROP", &efi.properties_table},
 	{EFI_MEMORY_ATTRIBUTES_TABLE_GUID, "MEMATTR", &efi.mem_attr_table},
 	{LINUX_EFI_RANDOM_SEED_TABLE_GUID, "RNG", &efi.rng_seed},
+	{LINUX_EFI_CRASH_GUID, "CAPSULE", &efi.capsule},
 	{NULL_GUID, NULL, NULL},
 };
 
diff --git a/include/linux/efi.h b/include/linux/efi.h
index 5b1af30..3296724e 100644
--- a/include/linux/efi.h
+++ b/include/linux/efi.h
@@ -893,6 +893,7 @@ extern struct efi {
 	unsigned long properties_table;	/* properties table */
 	unsigned long mem_attr_table;	/* memory attributes table */
 	unsigned long rng_seed;		/* UEFI firmware random seed */
+	unsigned long capsule;		/* EFI capsule table */
 	efi_get_time_t *get_time;
 	efi_set_time_t *set_time;
 	efi_get_wakeup_time_t *get_wakeup_time;
@@ -1401,6 +1402,9 @@ extern int efi_capsule_supported(efi_guid_t guid, u32 flags,
 extern int efi_capsule_update(efi_capsule_header_t *capsule,
 			      struct page **pages);
 
+extern efi_capsule_header_t **efi_capsule_lookup(efi_guid_t guid,
+			      uint32_t *nr_caps);
+
 #ifdef CONFIG_EFI_RUNTIME_MAP
 int efi_runtime_map_init(struct kobject *);
 int efi_get_runtime_map_size(void);
-- 
2.9.0.GIT

Re: [PATCH v2 1/2] efi/capsule: Add 'capsule' lookup support

[Ard Biesheuvel]

On 1 March 2017 at 17:59, Qiuxu Zhuo <qiuxu.zhuo-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org> wrote:
> Add the interface to get the array of EFI capsules when
> parsing the configuration tables.
>
> This code is based on Matt Fleming previous work from:
> https://git.kernel.org/cgit/linux/kernel/git/mfleming/efi.git/commit/?h=capsule-pstore&id=99c5f047133555aa0442f64064e85b7da2d4a45f
>
> Signed-off-by: Qiuxu Zhuo <qiuxu.zhuo-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> ---
>  drivers/firmware/efi/capsule.c | 107 +++++++++++++++++++++++++++++++++++++++++
>  drivers/firmware/efi/efi.c     |   4 ++
>  include/linux/efi.h            |   4 ++
>  3 files changed, 115 insertions(+)
>
> diff --git a/drivers/firmware/efi/capsule.c b/drivers/firmware/efi/capsule.c
> index 6eedff4..7bf79fd 100644
> --- a/drivers/firmware/efi/capsule.c
> +++ b/drivers/firmware/efi/capsule.c
> @@ -24,6 +24,11 @@ typedef struct {
>  static bool capsule_pending;
>  static bool stop_capsules;
>  static int efi_reset_type = -1;
> +/*
> + * Information about capsules we pulled from the EFI System Table.
> + */
> +static efi_capsule_header_t **prev_capsules;
> +static u32 efi_capsule_num;
>
>  /*
>   * capsule_mutex serialises access to both capsule_pending and
> @@ -288,6 +293,108 @@ int efi_capsule_update(efi_capsule_header_t *capsule, struct page **pages)
>  }
>  EXPORT_SYMBOL_GPL(efi_capsule_update);
>
> +static int extract_capsules(void)
> +{

I would like to understand something: this routine is called
'extract_capsules', but the way it is wired up, we can only use if for
capsules with GUID set to LINUX_CRASH_EFI_GUID. That seems incorrect
to me: the generic capsule code should be able to extract capsules
with any GUID (which the prototype of efi_capsule_lookup suggests).

I think it comes down to removing the global state in prev_capsules
and efi_capsule_num, and folding extract_capsules() into
efi_capsule_lookup().

You could add a routine that iterates over the config table array to
look for your GUID: that way, you don't need to put it in generic EFI
code.

> +       void *capsule;
> +       size_t size;
> +
> +       if (efi.capsule == EFI_INVALID_TABLE_ADDR)
> +               return 0;
> +
> +       capsule = memremap(efi.capsule, sizeof(efi_capsule_num), MEMREMAP_WB);
> +       if (!capsule)
> +               return -ENOMEM;
> +
> +       /*
> +        * The array of capsules is prefixed with the number of
> +        * capsule entries in the array.
> +        */
> +       efi_capsule_num = *(uint32_t *)capsule;
> +       iounmap(capsule);
> +

Please use memunmap here

> +       if (!efi_capsule_num) {
> +               pr_info("No capsules on extraction\n");
> +               return 0;
> +       }
> +
> +       size = efi_capsule_num * sizeof(*capsule);
> +       capsule = memremap(efi.capsule, size, MEMREMAP_WB);

Is 'capsule' ever unmapped? If not, why not?

> +       if (!capsule)
> +               return -ENOMEM;
> +
> +       capsule += sizeof(uint32_t);

This is incorrect for 64-bit. You need to increment by the size of
unsigned long here, regardless of the size of efi_capsule_num.

> +       prev_capsules = (efi_capsule_header_t **)capsule;
> +       if (!*prev_capsules)
> +               pr_err("Capsule array has no entries\n");
> +
> +       return 0;
> +}
> +
> +/**
> + * efi_capsule_lookup - search capsule array for entries.
> + * @guid: the guid to search for.
> + * @nr_caps: the number of entries found.
> + *
> + * Map each capsule header into the kernel's virtual address space and
> + * inspect the guid. Build an array of capsule headers with every
> + * capsule that is found with @guid. If a match is found the capsule
> + * remains mapped, otherwise it is unmapped.
> + *
> + * Returns an array of capsule headers, each element of which has the
> + * guid @guid. The number of elements in the array is stored in
> + * @nr_caps. Returns %NULL if no capsules were found and stores zero
> + * in @nr_caps.
> + */
> +efi_capsule_header_t **efi_capsule_lookup(efi_guid_t guid, uint32_t *nr_caps)
> +{
> +       efi_capsule_header_t **capsules = NULL, **tmp;
> +       size_t capsule_size = 0;
> +       int i, rv;
> +
> +       rv = extract_capsules();
> +       if (rv)
> +               return ERR_PTR(rv);
> +
> +       *nr_caps = 0;
> +       for (i = 0; i < efi_capsule_num; i++) {
> +               efi_capsule_header_t *c;
> +               size_t size;
> +
> +               c = memremap((resource_size_t)prev_capsules[i], sizeof(*c), MEMREMAP_WB);
> +               if (!c) {
> +                       pr_err("Failed to memremap capsule\n");
> +                       continue;
> +               }
> +
> +               size = c->imagesize;
> +               iounmap(c);
> +

memunmap

> +               c = memremap((resource_size_t)prev_capsules[i], size, MEMREMAP_WB);
> +               if (!c) {
> +                       pr_err("Failed to memremap header + data\n");
> +                       continue;
> +               }
> +
> +               if (!efi_guidcmp(c->guid, guid)) {

When can we expect the capsule GUID to deviate from the GUID we looked for?

> +                       capsule_size += sizeof(*capsules);
> +                       tmp = krealloc(capsules, capsule_size, GFP_KERNEL);
> +                       if (!tmp) {
> +                               kfree(capsules);
> +                               return ERR_PTR(-ENOMEM);
> +                       }
> +
> +                       capsules = tmp;
> +                       capsules[(*nr_caps)++] = c;
> +                       continue;
> +               }
> +
> +               iounmap(c);

memunmap

> +       }
> +
> +       return capsules;
> +}
> +EXPORT_SYMBOL_GPL(efi_capsule_lookup);
> +
>  static int capsule_reboot_notify(struct notifier_block *nb, unsigned long event, void *cmd)
>  {
>         mutex_lock(&capsule_mutex);
> diff --git a/drivers/firmware/efi/efi.c b/drivers/firmware/efi/efi.c
> index 9291480..03be9b8 100644
> --- a/drivers/firmware/efi/efi.c
> +++ b/drivers/firmware/efi/efi.c
> @@ -52,6 +52,7 @@ struct efi __read_mostly efi = {
>         .properties_table       = EFI_INVALID_TABLE_ADDR,
>         .mem_attr_table         = EFI_INVALID_TABLE_ADDR,
>         .rng_seed               = EFI_INVALID_TABLE_ADDR,
> +       .capsule                = EFI_INVALID_TABLE_ADDR,
>  };
>  EXPORT_SYMBOL(efi);
>
> @@ -120,6 +121,8 @@ static ssize_t systab_show(struct kobject *kobj,
>                 str += sprintf(str, "BOOTINFO=0x%lx\n", efi.boot_info);
>         if (efi.uga != EFI_INVALID_TABLE_ADDR)
>                 str += sprintf(str, "UGA=0x%lx\n", efi.uga);
> +       if (efi.capsule != EFI_INVALID_TABLE_ADDR)
> +               str += sprintf(str, "CAPSULE=0x%lx\n", efi.capsule);
>
>         return str - buf;
>  }
> @@ -445,6 +448,7 @@ static __initdata efi_config_table_type_t common_tables[] = {
>         {EFI_PROPERTIES_TABLE_GUID, "PROP", &efi.properties_table},
>         {EFI_MEMORY_ATTRIBUTES_TABLE_GUID, "MEMATTR", &efi.mem_attr_table},
>         {LINUX_EFI_RANDOM_SEED_TABLE_GUID, "RNG", &efi.rng_seed},
> +       {LINUX_EFI_CRASH_GUID, "CAPSULE", &efi.capsule},
>         {NULL_GUID, NULL, NULL},
>  };
>
> diff --git a/include/linux/efi.h b/include/linux/efi.h
> index 5b1af30..3296724e 100644
> --- a/include/linux/efi.h
> +++ b/include/linux/efi.h
> @@ -893,6 +893,7 @@ extern struct efi {
>         unsigned long properties_table; /* properties table */
>         unsigned long mem_attr_table;   /* memory attributes table */
>         unsigned long rng_seed;         /* UEFI firmware random seed */
> +       unsigned long capsule;          /* EFI capsule table */

Can we get rid of all this global stuff? If it is too tedious to go
over the config table array, we could keep it I guess (but please
don't call capsule-pstore specific things 'capsule', this is confusing
the hell out of me)

>         efi_get_time_t *get_time;
>         efi_set_time_t *set_time;
>         efi_get_wakeup_time_t *get_wakeup_time;
> @@ -1401,6 +1402,9 @@ extern int efi_capsule_supported(efi_guid_t guid, u32 flags,
>  extern int efi_capsule_update(efi_capsule_header_t *capsule,
>                               struct page **pages);
>
> +extern efi_capsule_header_t **efi_capsule_lookup(efi_guid_t guid,
> +                             uint32_t *nr_caps);
> +
>  #ifdef CONFIG_EFI_RUNTIME_MAP
>  int efi_runtime_map_init(struct kobject *);
>  int efi_get_runtime_map_size(void);
> --
> 2.9.0.GIT
>

Re: [PATCH v2 1/2] efi/capsule: Add 'capsule' lookup support

[Matt Fleming]

On Thu, 02 Mar, at 03:38:51PM, Ard Biesheuvel wrote:
> 
> > +       if (!capsule)
> > +               return -ENOMEM;
> > +
> > +       capsule += sizeof(uint32_t);
> 
> This is incorrect for 64-bit. You need to increment by the size of
> unsigned long here, regardless of the size of efi_capsule_num.

I'm almost positive this is correct, but I can't find the bit in the
spec that says why. We're not trying to step over a pointer here, if
memory serves, it's a capsule count or something and uint32_t is the
right type.

Lemme go dig in the spec.

Re: [PATCH v2 1/2] efi/capsule: Add 'capsule' lookup support

[Ard Biesheuvel]

On 2 March 2017 at 16:44, Matt Fleming <matt-mF/unelCI9GS6iBeEJttW/XRex20P6io@public.gmane.org> wrote:
> On Thu, 02 Mar, at 03:38:51PM, Ard Biesheuvel wrote:
>>
>> > +       if (!capsule)
>> > +               return -ENOMEM;
>> > +
>> > +       capsule += sizeof(uint32_t);
>>
>> This is incorrect for 64-bit. You need to increment by the size of
>> unsigned long here, regardless of the size of efi_capsule_num.
>
> I'm almost positive this is correct, but I can't find the bit in the
> spec that says why. We're not trying to step over a pointer here, if
> memory serves, it's a capsule count or something and uint32_t is the
> right type.
>

Yes, but the next struct member is an array of pointers, so you need
to advance 8 bytes to get to it due to its alignment

Re: [PATCH v2 1/2] efi/capsule: Add 'capsule' lookup support

[Ard Biesheuvel]

On 2 March 2017 at 16:45, Ard Biesheuvel <ard.biesheuvel-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> wrote:
> On 2 March 2017 at 16:44, Matt Fleming <matt-mF/unelCI9GS6iBeEJttW/XRex20P6io@public.gmane.org> wrote:
>> On Thu, 02 Mar, at 03:38:51PM, Ard Biesheuvel wrote:
>>>
>>> > +       if (!capsule)
>>> > +               return -ENOMEM;
>>> > +
>>> > +       capsule += sizeof(uint32_t);
>>>
>>> This is incorrect for 64-bit. You need to increment by the size of
>>> unsigned long here, regardless of the size of efi_capsule_num.
>>
>> I'm almost positive this is correct, but I can't find the bit in the
>> spec that says why. We're not trying to step over a pointer here, if
>> memory serves, it's a capsule count or something and uint32_t is the
>> right type.
>>
>
> Yes, but the next struct member is an array of pointers, so you need
> to advance 8 bytes to get to it due to its alignment

>From EDK2:

typedef struct {
  ///
  /// the size of the array of capsules.
  ///
  UINT32   CapsuleArrayNumber;
  ///
  /// Point to an array of capsules that contain the same CapsuleGuid value.
  ///
  VOID*    CapsulePtr[1];
} EFI_CAPSULE_TABLE;

RE: [PATCH v2 1/2] efi/capsule: Add 'capsule' lookup support

[Zhuo, Qiuxu]

Hi Ard,

     The capsule configuration table entry memory layout here looks like: 
     For capsule guid xxx: |  (u32) capsule number a | (u64)  1st capsule physical addr  | (u64)  2nd capsule physical addr | ...  |  (u64)  a-st capsule physical addr |
     So if I do "capsule += sizeof(*uint32_t);" then the value for each capsule physical address will be shifted 4 bytes backward.

    Do you think the way that we define a '__packed' structure as like below, map it to the configuration table, and then use it to fetch the capsule number and each capsule physical address is ok ?
    typedef struct {
          u32 capsule_array_number;
          void *capsule_addr[];
   } __packed efi_capsule_table_t;
      
Thanks!

BR
qiuxu
                                                  

-----Original Message-----
From: Ard Biesheuvel [mailto:ard.biesheuvel@linaro.org] 
Sent: Friday, March 3, 2017 12:46 AM
To: Matt Fleming <matt@codeblueprint.co.uk>
Cc: Zhuo, Qiuxu <qiuxu.zhuo@intel.com>; linux-efi@vger.kernel.org; Luck, Tony <tony.luck@intel.com>
Subject: Re: [PATCH v2 1/2] efi/capsule: Add 'capsule' lookup support

On 2 March 2017 at 16:45, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
> On 2 March 2017 at 16:44, Matt Fleming <matt@codeblueprint.co.uk> wrote:
>> On Thu, 02 Mar, at 03:38:51PM, Ard Biesheuvel wrote:
>>>
>>> > +       if (!capsule)
>>> > +               return -ENOMEM;
>>> > +
>>> > +       capsule += sizeof(uint32_t);
>>>
>>> This is incorrect for 64-bit. You need to increment by the size of 
>>> unsigned long here, regardless of the size of efi_capsule_num.
>>
>> I'm almost positive this is correct, but I can't find the bit in the 
>> spec that says why. We're not trying to step over a pointer here, if 
>> memory serves, it's a capsule count or something and uint32_t is the 
>> right type.
>>
>
> Yes, but the next struct member is an array of pointers, so you need 
> to advance 8 bytes to get to it due to its alignment

From EDK2:

typedef struct {
  ///
  /// the size of the array of capsules.
  ///
  UINT32   CapsuleArrayNumber;
  ///
  /// Point to an array of capsules that contain the same CapsuleGuid value.
  ///
  VOID*    CapsulePtr[1];
} EFI_CAPSULE_TABLE;

[PATCH v2 2/2] efi: capsule pstore backend

[Qiuxu Zhuo]

The EFI capsule mechanism allows data blobs to be passed to the EFI
firmware. By setting the EFI_CAPSULE_POPULATE_SYSTEM_TABLE and the
EFI_CAPSULE_PERSIST_ACROSS_REBOOT flags, the firmware will place a
pointer to our data blob in the EFI System Table on the next boot.
We can utilise this facility to save crash dumps, call traces, etc
and pick them up after reboot.

Initial cut at this driver by Matt Fleming as below links
https://git.kernel.org/cgit/linux/kernel/git/mfleming/efi.git/commit/?h=capsule-pstore&id=99c5f047133555aa0442f64064e85b7da2d4a45f
https://git.kernel.org/cgit/linux/kernel/git/mfleming/efi.git/commit/?h=capsule-pstore&id=8625c776c9b8bbed7fa4aa023e36542615165240
Extensive cleanup, refactoring, bug fix, and verification by Qiuxu Zhuo

Patch verified on Intel Kabylake client platform + Intel KBL BIOS:(10/24/2016):
- modprobe capsule-pstore
- echo 1 > /sys/module/kernel/parameters/panic
- echo c > /proc/sysrq-trigger
- system reboot...
- ls -l /sys/fs/pstore/
  -r--r--r-- 1 root root 4386 Feb 12 00:31 console-efi-capsule-0
  -r--r--r-- 1 root root 9065 Feb 12 00:29 dmesg-efi-capsule-6250071391647825921
  -r--r--r-- 1 root root 9073 Feb 12 00:29 dmesg-efi-capsule-6250071391647825922
  -r--r--r-- 1 root root 9096 Feb 12 00:29 dmesg-efi-capsule-6250071391647825923
  -r--r--r-- 1 root root 9073 Feb 12 00:29 dmesg-efi-capsule-6250071391647825924
  -r--r--r-- 1 root root 9048 Feb 12 00:29 dmesg-efi-capsule-6250071391647825925

The above files contain the last complete logs.

Signed-off-by: Qiuxu Zhuo <qiuxu.zhuo-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
---
 drivers/firmware/efi/Kconfig          |  21 ++
 drivers/firmware/efi/Makefile         |   1 +
 drivers/firmware/efi/capsule-pstore.c | 527 ++++++++++++++++++++++++++++++++++
 3 files changed, 549 insertions(+)
 create mode 100644 drivers/firmware/efi/capsule-pstore.c

diff --git a/drivers/firmware/efi/Kconfig b/drivers/firmware/efi/Kconfig
index 2e78b0b..aab78a7 100644
--- a/drivers/firmware/efi/Kconfig
+++ b/drivers/firmware/efi/Kconfig
@@ -142,6 +142,27 @@ config APPLE_PROPERTIES
 
 	  If unsure, say Y if you have a Mac.  Otherwise N.
 
+config EFI_CAPSULE_PSTORE
+	tristate "EFI capsule pstore backend"
+	depends on EFI && PSTORE
+	help
+	  Saying Y here enable the EFI capsule mechanism to store crash dumps,
+	  console log, and function tracing data.
+
+	  To compile this driver as a module, choose M here.
+
+	  Not many firmware implementations fully support EFI capsules.
+	  If you plan to rely on this you should test whether yours works by
+	  forcing a crash. Most people should not enable this.
+
+config EFI_CAPSULE_PSTORE_DEFAULT_DISABLE
+	bool "Disable using efi capsule as a pstore backend by default"
+	depends on EFI_CAPSULE_PSTORE
+	help
+	  Saying Y here will disable the use of efi capsule as a storage
+	  backend for pstore by default. This setting can be overridden
+	  using the capsule-pstore module's pstore_disable parameter.
+
 endmenu
 
 config UEFI_CPER
diff --git a/drivers/firmware/efi/Makefile b/drivers/firmware/efi/Makefile
index ad67342..1417e653 100644
--- a/drivers/firmware/efi/Makefile
+++ b/drivers/firmware/efi/Makefile
@@ -14,6 +14,7 @@ obj-$(CONFIG_EFI)			+= capsule.o memmap.o
 obj-$(CONFIG_EFI_VARS)			+= efivars.o
 obj-$(CONFIG_EFI_ESRT)			+= esrt.o
 obj-$(CONFIG_EFI_VARS_PSTORE)		+= efi-pstore.o
+obj-$(CONFIG_EFI_CAPSULE_PSTORE)		+= capsule-pstore.o
 obj-$(CONFIG_UEFI_CPER)			+= cper.o
 obj-$(CONFIG_EFI_RUNTIME_MAP)		+= runtime-map.o
 obj-$(CONFIG_EFI_RUNTIME_WRAPPERS)	+= runtime-wrappers.o
diff --git a/drivers/firmware/efi/capsule-pstore.c b/drivers/firmware/efi/capsule-pstore.c
new file mode 100644
index 0000000..b94cfb9
--- /dev/null
+++ b/drivers/firmware/efi/capsule-pstore.c
@@ -0,0 +1,527 @@
+#define pr_fmt(fmt) "efi-capsule: " fmt
+#include <linux/slab.h>
+#include <linux/vmalloc.h>
+#include <linux/efi.h>
+#include <linux/module.h>
+#include <linux/pstore.h>
+
+#define CAPSULE_SIZE (16 * 1024)
+#define CRASH_SIZE 4096
+#define CAPSULE_MAGIC 0x63617073 /* 'caps' */
+
+static bool efi_capsule_pstore_disable =
+	IS_ENABLED(CONFIG_CAPSULE_PSTORE_DEFAULT_DISABLE);
+
+static int efi_reset_type = -1;
+
+struct efi_capsule_ctx {
+	struct page **pages;
+	unsigned int nr_pages;
+	efi_capsule_header_t *capsule;
+	size_t capsule_size;
+	void *data;
+	size_t data_size;
+};
+
+struct efi_capsule_pstore_buf {
+	void *buf;
+	size_t size;
+	atomic_long_t offset;
+};
+
+struct efi_capsule_pstore {
+	/* Previous records */
+	efi_capsule_header_t **hdrs;
+	uint32_t hdrs_num;
+	off_t hdr_offset;  /* Offset into current header */
+
+	/* New records */
+	struct efi_capsule_pstore_buf console;
+	struct efi_capsule_pstore_buf ftrace;
+	struct efi_capsule_pstore_buf dmesg;
+};
+
+struct efi_capsule_pstore_record {
+	u64 timestamp;
+	u64 id;
+	enum pstore_type_id type;
+	size_t size;
+	bool compressed;
+	u32 magic;
+	char data[];
+} __packed;
+
+static struct pstore_info efi_capsule_pstore_info;
+
+static __init void efi_capsule_destroy(struct efi_capsule_ctx *ctx)
+{
+	if (!ctx)
+		return;
+
+	while (ctx->nr_pages--)
+		__free_page(ctx->pages[ctx->nr_pages]);
+
+	kfree(ctx->pages);
+	kfree(ctx);
+}
+
+/**
+ * efi_capsule_build - alloc data buffer and fill out the header
+ * @guid: vendor's guid
+ * @data_size: size in bytes of the capsule data
+ *
+ * This is a helper function for allocating enough room for user data
+ * + the size of an EFI capsule header.
+ *
+ * Returns a pointer to an allocated capsule on success, an ERR_PTR()
+ * value on error.
+ */
+static __init struct efi_capsule_ctx *
+efi_capsule_build(efi_guid_t guid, size_t data_size)
+{
+	struct efi_capsule_ctx *ctx;
+	size_t capsule_size, needed_pages;
+	u32 flags;
+	int rv;
+
+	flags = EFI_CAPSULE_PERSIST_ACROSS_RESET |
+		EFI_CAPSULE_POPULATE_SYSTEM_TABLE;
+	capsule_size = data_size + sizeof(efi_capsule_header_t);
+	rv = efi_capsule_supported(LINUX_EFI_CRASH_GUID, flags,
+		capsule_size, &efi_reset_type);
+	if (rv)
+		return ERR_PTR(rv);
+
+	ctx = kzalloc(sizeof(*ctx), GFP_KERNEL);
+	if (!ctx)
+		return ERR_PTR(-ENOMEM);
+
+	pr_info("Allocating: %zu\n", capsule_size);
+	needed_pages = ALIGN(capsule_size, PAGE_SIZE) >> PAGE_SHIFT;
+	ctx->pages = kcalloc(needed_pages, sizeof(*ctx->pages), GFP_KERNEL);
+	if (!ctx->pages)
+		goto fail;
+
+	while (needed_pages--) {
+		struct page *page;
+
+		page = alloc_page(GFP_KERNEL | __GFP_HIGHMEM);
+		if (!page)
+			goto fail;
+
+		ctx->pages[ctx->nr_pages++] = page;
+	}
+
+	ctx->capsule = vmap(ctx->pages, ctx->nr_pages, 0, PAGE_KERNEL);
+	if (!ctx->capsule)
+		goto fail;
+
+	ctx->capsule_size = capsule_size;
+	ctx->data = (void *)ctx->capsule + sizeof(efi_capsule_header_t);
+	ctx->data_size = data_size;
+
+	pr_info("Allocated %zd bytes of capsule memory\n", data_size);
+
+	/*
+	 * Setup the EFI capsule header.
+	 */
+	memcpy(&ctx->capsule->guid, &guid, sizeof(guid));
+
+	ctx->capsule->flags = flags;
+	ctx->capsule->headersize = sizeof(*ctx->capsule);
+	ctx->capsule->imagesize = capsule_size;
+
+	return ctx;
+
+fail:
+	efi_capsule_destroy(ctx);
+	return ERR_PTR(-ENOMEM);
+}
+
+/**
+ * We may not be in a position to allocate memory at the time of a
+ * crash, so pre-allocate some space now and register it with the
+ * firmware via efi_capsule_update().
+ *
+ * Also, iterate through the array of capsules pointed to from the EFI
+ * system table and take note of any LINUX_EFI_CRASH_GUID
+ * capsules. They will be parsed by efi_capsule_pstore_read().
+ */
+static __init int efi_capsule_pstore_setup(void)
+{
+	struct efi_capsule_pstore_record *rec;
+	struct efi_capsule_pstore *pctx = NULL;
+	struct efi_capsule_ctx *console_ctx = NULL;
+	struct efi_capsule_ctx *ftrace_ctx = NULL;
+	struct efi_capsule_ctx *dmesg_ctx = NULL;
+	efi_capsule_header_t **hdrs;
+	uint32_t hdrs_num;
+	void *crash_buf = NULL;
+	int rv;
+
+	pctx = kzalloc(sizeof(*pctx), GFP_KERNEL);
+	if (!pctx)
+		return -ENOMEM;
+
+	crash_buf = kmalloc(CRASH_SIZE, GFP_KERNEL);
+	if (!crash_buf) {
+		rv = -ENOMEM;
+		goto fail;
+	}
+
+	/* Allocate all the capsules upfront */
+	dmesg_ctx = efi_capsule_build(LINUX_EFI_CRASH_GUID, CAPSULE_SIZE);
+	if (IS_ERR(dmesg_ctx)) {
+		rv = PTR_ERR(dmesg_ctx);
+		dmesg_ctx = NULL;
+		goto fail2;
+	}
+
+	ftrace_ctx = efi_capsule_build(LINUX_EFI_CRASH_GUID, CAPSULE_SIZE);
+	if (IS_ERR(ftrace_ctx)) {
+		rv = PTR_ERR(ftrace_ctx);
+		ftrace_ctx = NULL;
+		goto fail3;
+	}
+
+	console_ctx = efi_capsule_build(LINUX_EFI_CRASH_GUID, CAPSULE_SIZE);
+	if (IS_ERR(console_ctx)) {
+		rv = PTR_ERR(console_ctx);
+		console_ctx = NULL;
+		goto fail4;
+	}
+
+	/* Register with the firmware */
+	rv = efi_capsule_update(dmesg_ctx->capsule, dmesg_ctx->pages);
+	if (rv)
+		goto fail5;
+	pr_info("Registered dmesg with firmware\n");
+
+	rv = efi_capsule_update(ftrace_ctx->capsule, ftrace_ctx->pages);
+	if (rv)
+		goto fail5;
+	pr_info("Registered ftrace with firmware\n");
+
+	rv = efi_capsule_update(console_ctx->capsule, console_ctx->pages);
+	if (rv)
+		goto fail5;
+	pr_info("Registered console with firmware\n");
+
+	memset(dmesg_ctx->data, 0, dmesg_ctx->data_size);
+	pctx->dmesg.size = dmesg_ctx->data_size;
+	pctx->dmesg.buf = dmesg_ctx->data;
+	atomic_long_set(&pctx->dmesg.offset, 0);
+
+	/* Setup the pstore records for the ring-buffers. */
+	memset(ftrace_ctx->data, 0, ftrace_ctx->data_size);
+	pctx->ftrace.size = ftrace_ctx->data_size - offsetof(typeof(*rec), data);
+	pctx->ftrace.buf = ftrace_ctx->data + offsetof(typeof(*rec), data);
+	atomic_long_set(&pctx->ftrace.offset, 0);
+	rec = ftrace_ctx->data;
+	rec->type = PSTORE_TYPE_FTRACE;
+
+	memset(console_ctx->data, 0, console_ctx->data_size);
+	pctx->console.size = console_ctx->data_size - offsetof(typeof(*rec), data);
+	pctx->console.buf = console_ctx->data + offsetof(typeof(*rec), data);
+	atomic_long_set(&pctx->console.offset, 0);
+	rec = console_ctx->data;
+	rec->type = PSTORE_TYPE_CONSOLE;
+
+	/* Read any pstore entries that were passed across a reboot. */
+	pr_info("Looking up old capsule\n");
+	hdrs = efi_capsule_lookup(LINUX_EFI_CRASH_GUID, &hdrs_num);
+	pctx->hdrs_num = hdrs_num;
+	pctx->hdrs = IS_ERR(hdrs) ? NULL : hdrs;
+
+	if (pctx->hdrs_num)
+		pr_info("Found Linux crash capsule\n");
+
+	/* Register the capsule backend with pstore. */
+	spin_lock_init(&efi_capsule_pstore_info.buf_lock);
+	efi_capsule_pstore_info.buf = crash_buf;
+	efi_capsule_pstore_info.bufsize = CRASH_SIZE;
+	efi_capsule_pstore_info.data = pctx;
+	pr_info("Registering with pstore\n");
+	rv = pstore_register(&efi_capsule_pstore_info);
+	if (rv)
+		pr_err("Capsule support registration failed for pstore: %d\n", rv);
+	else
+		return 0;
+
+fail5:
+	efi_capsule_destroy(console_ctx);
+fail4:
+	efi_capsule_destroy(ftrace_ctx);
+fail3:
+	efi_capsule_destroy(dmesg_ctx);
+fail2:
+	kfree(crash_buf);
+fail:
+	kfree(pctx);
+	return rv;
+}
+
+/**
+ * Return the next pstore record that was passed to us across a reboot
+ * in an EFI capsule.
+ *
+ * This is expected to be called under the pstore
+ * read_mutex. Therefore, no serialisation is done here.
+ */
+static struct efi_capsule_pstore_record *
+get_pstore_read_record(struct efi_capsule_pstore *pctx)
+{
+	struct efi_capsule_pstore_record *rec;
+	efi_capsule_header_t *hdr;
+	off_t remaining;
+
+next:
+	if (!pctx->hdrs_num)
+		return NULL;
+
+	hdr = pctx->hdrs[pctx->hdrs_num - 1];
+	rec = (void *)hdr + hdr->headersize + pctx->hdr_offset;
+
+	remaining = hdr->imagesize - hdr->headersize - pctx->hdr_offset - offsetof(typeof(*rec), data);
+
+	/*
+	 * A single EFI capsule may contain multiple pstore
+	 * records. It may also only be partially filled with pstore
+	 * records, which we can detect by checking for a record with
+	 * zero size.
+	 *
+	 * If there are no more entries in this capsule try the next.
+	 */
+	if (!rec->size) {
+		pctx->hdrs_num--;
+		pctx->hdr_offset = 0;
+		goto next;
+	}
+
+	/*
+	 * If we've finished parsing all records in this capsule, move
+	 * onto the next. Otherwise, increment the offset into the
+	 * current capsule (pctx->hdr_offset).
+	 */
+	if (rec->size == remaining) {
+		pctx->hdrs_num--;
+		pctx->hdr_offset = 0;
+	} else
+		pctx->hdr_offset += rec->size + offsetof(typeof(*rec), data);
+
+	return rec;
+}
+
+static ssize_t efi_capsule_pstore_read(u64 *id, enum pstore_type_id *type,
+				       int *count, struct timespec *time,
+				       char **buf, bool *compressed, ssize_t *ecc_notice_size,
+					   struct pstore_info *psi)
+{
+	struct efi_capsule_pstore_record *rec;
+	struct efi_capsule_pstore *pctx = psi->data;
+	ssize_t size;
+
+	rec = get_pstore_read_record(pctx);
+	if (!rec)
+		return 0;
+
+	if (rec->magic != CAPSULE_MAGIC) {
+		pr_info("%s Invalid capsule record!\n", __func__);
+		return 0;
+	}
+
+	*type = rec->type;
+	time->tv_sec = rec->timestamp;
+	time->tv_nsec = 0;
+	size = rec->size;
+	*id = rec->id;
+	*compressed = rec->compressed;
+	*ecc_notice_size = 0;
+
+	pr_info("Read record type %d size %zu id %llu compressed %d timestamp %llu\n",
+			rec->type, rec->size, rec->id, rec->compressed, rec->timestamp);
+
+	*buf = kmalloc(size, GFP_KERNEL);
+	if (!*buf)
+		return -ENOMEM;
+
+	memcpy(*buf, rec->data, size);
+
+	return size;
+}
+
+/*
+ * We expect to be called with ->buf_lock held, and so don't perform
+ * any serialisation.
+ */
+static struct notrace efi_capsule_pstore_record *
+get_pstore_write_record(struct efi_capsule_pstore_buf *pbuf, size_t *size)
+{
+	struct efi_capsule_pstore_record *rec;
+	long offset = atomic_long_read(&pbuf->offset);
+
+	if (offset + offsetof(typeof(*rec), data) >= pbuf->size)
+		return NULL;
+
+	/* Trim 'size' if there isn't enough remaining space */
+	if (offset + *size + offsetof(typeof(*rec), data) > pbuf->size)
+		*size = pbuf->size - offset - offsetof(typeof(*rec), data);
+
+	rec = pbuf->buf + offset;
+	atomic_long_add(offsetof(typeof(*rec), data) + *size, &pbuf->offset);
+
+	return rec;
+}
+
+static int notrace
+efi_capsule_pstore_write(enum pstore_type_id type,
+			 enum kmsg_dump_reason reason, u64 *id,
+			 unsigned int part, int count, bool compressed,
+			 size_t size, struct pstore_info *psi)
+{
+	struct efi_capsule_pstore_record *rec;
+	struct efi_capsule_pstore *pctx = psi->data;
+	size_t sz = size;
+	static atomic64_t seq;
+
+	/*
+	 * A zero size record would break our detection of
+	 * partially-filled capsules.
+	 */
+	if (!size)
+		return -EINVAL;
+
+	rec = get_pstore_write_record(&pctx->dmesg, &size);
+	if (!rec || (compressed && size < sz))
+		return -ENOSPC;
+
+	rec->type = type;
+	rec->timestamp = get_seconds();
+	rec->size = size;
+	if (!atomic64_read(&seq))
+		atomic64_set(&seq, rec->timestamp << 32);
+	rec->id = (*id) = atomic64_inc_return(&seq);
+	rec->compressed = compressed;
+	rec->magic = CAPSULE_MAGIC;
+	memcpy(rec->data, psi->buf, size);
+
+	return 0;
+}
+
+static notrace void *
+get_pstore_buf(struct efi_capsule_pstore_buf *pbuf, size_t size)
+{
+	long next, curr;
+	struct efi_capsule_pstore_record *rec;
+
+	if (size > pbuf->size)
+		return NULL;
+
+	rec = pbuf->buf - offsetof(typeof(*rec), data);
+	rec->magic = CAPSULE_MAGIC;
+	if (rec->size + size > pbuf->size)
+		rec->size = pbuf->size;
+	else
+		rec->size += size;
+
+	do {
+		curr = atomic_long_read(&pbuf->offset);
+		next = curr + size;
+
+		/* Wrap? */
+		if (next > pbuf->size) {
+			next = size;
+			if (atomic_long_cmpxchg(&pbuf->offset, curr, next)) {
+				curr = 0;
+				break;
+			}
+
+			continue;
+		}
+
+	} while (atomic_long_cmpxchg(&pbuf->offset, curr, next) != curr);
+
+	return pbuf->buf + curr;
+}
+
+static int notrace
+efi_capsule_pstore_write_buf(enum pstore_type_id type,
+			     enum kmsg_dump_reason reason,
+			     u64 *id, unsigned int part,
+			     const char *buf, bool compressed,
+			     size_t size, struct pstore_info *psi)
+{
+	struct efi_capsule_pstore *pctx = psi->data;
+	void *dst;
+
+	if (type == PSTORE_TYPE_FTRACE)
+		dst = get_pstore_buf(&pctx->ftrace, size);
+	else if (type == PSTORE_TYPE_CONSOLE)
+		dst = get_pstore_buf(&pctx->console, size);
+	else
+		return -EINVAL;
+
+	if (!dst)
+		return -ENOSPC;
+
+	memcpy(dst, buf, size);
+
+	return 0;
+}
+
+static struct pstore_info efi_capsule_pstore_info = {
+	.owner     = THIS_MODULE,
+	.name      = "efi-capsule",
+	.flags     = PSTORE_FLAGS_DMESG | PSTORE_FLAGS_CONSOLE | PSTORE_FLAGS_FTRACE,
+	.read      = efi_capsule_pstore_read,
+	.write     = efi_capsule_pstore_write,
+	.write_buf = efi_capsule_pstore_write_buf,
+};
+
+static __init int check_capsule_support(void)
+{
+	int rv;
+
+	efi_guid_t guid = LINUX_EFI_CRASH_GUID;
+	u32 flags = EFI_CAPSULE_PERSIST_ACROSS_RESET |
+		EFI_CAPSULE_POPULATE_SYSTEM_TABLE;
+
+	rv = efi_capsule_supported(guid, flags, CAPSULE_SIZE, &efi_reset_type);
+	if (rv)
+		return rv;
+
+	return 0;
+}
+
+static __init int efi_capsule_pstore_init(void)
+{
+	int rv = 0;
+
+	if (!efi_enabled(EFI_RUNTIME_SERVICES))
+		return -ENODEV;
+
+	if (efi_capsule_pstore_disable)
+		return 0;
+
+	rv = check_capsule_support();
+	if (rv)
+		return rv;
+
+	rv = efi_capsule_pstore_setup();
+	if (rv)
+		return rv;
+
+	return 0;
+}
+
+static __exit void efi_capsule_pstore_exit(void)
+{
+}
+
+module_init(efi_capsule_pstore_init);
+module_exit(efi_capsule_pstore_exit);
+
+module_param_named(pstore_disable, efi_capsule_pstore_disable, bool, 0644);
+
+MODULE_DESCRIPTION("EFI capsule backend for pstore");
+MODULE_LICENSE("GPL v2");
-- 
2.9.0.GIT

Re: [PATCH v2 2/2] efi: capsule pstore backend

[Ard Biesheuvel]

(+ Kees)

On 1 March 2017 at 17:59, Qiuxu Zhuo <qiuxu.zhuo-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org> wrote:
> The EFI capsule mechanism allows data blobs to be passed to the EFI
> firmware. By setting the EFI_CAPSULE_POPULATE_SYSTEM_TABLE and the
> EFI_CAPSULE_PERSIST_ACROSS_REBOOT flags, the firmware will place a
> pointer to our data blob in the EFI System Table on the next boot.
> We can utilise this facility to save crash dumps, call traces, etc
> and pick them up after reboot.
>
> Initial cut at this driver by Matt Fleming as below links
> https://git.kernel.org/cgit/linux/kernel/git/mfleming/efi.git/commit/?h=capsule-pstore&id=99c5f047133555aa0442f64064e85b7da2d4a45f
> https://git.kernel.org/cgit/linux/kernel/git/mfleming/efi.git/commit/?h=capsule-pstore&id=8625c776c9b8bbed7fa4aa023e36542615165240
> Extensive cleanup, refactoring, bug fix, and verification by Qiuxu Zhuo
>
> Patch verified on Intel Kabylake client platform + Intel KBL BIOS:(10/24/2016):
> - modprobe capsule-pstore
> - echo 1 > /sys/module/kernel/parameters/panic
> - echo c > /proc/sysrq-trigger
> - system reboot...
> - ls -l /sys/fs/pstore/
>   -r--r--r-- 1 root root 4386 Feb 12 00:31 console-efi-capsule-0
>   -r--r--r-- 1 root root 9065 Feb 12 00:29 dmesg-efi-capsule-6250071391647825921
>   -r--r--r-- 1 root root 9073 Feb 12 00:29 dmesg-efi-capsule-6250071391647825922
>   -r--r--r-- 1 root root 9096 Feb 12 00:29 dmesg-efi-capsule-6250071391647825923
>   -r--r--r-- 1 root root 9073 Feb 12 00:29 dmesg-efi-capsule-6250071391647825924
>   -r--r--r-- 1 root root 9048 Feb 12 00:29 dmesg-efi-capsule-6250071391647825925
>
> The above files contain the last complete logs.
>

I managed to run this on a 64-bit ARM virtual machine under QEMU.

My kernel spits out the following:

[    1.820650] efi-capsule: Allocating: 16412
[    1.820978] efi-capsule: Allocated 16384 bytes of capsule memory
[    1.821256] efi-capsule: Allocating: 16412
[    1.821447] efi-capsule: Allocated 16384 bytes of capsule memory
[    1.821731] efi-capsule: Allocating: 16412
[    1.821926] efi-capsule: Allocated 16384 bytes of capsule memory
[    1.971762] efi-capsule: Registered dmesg with firmware
[    2.111314] efi-capsule: Registered ftrace with firmware
[    2.241585] efi-capsule: Registered console with firmware
[    2.241907] efi-capsule: Looking up old capsule
[    2.244081] efi-capsule: Found Linux crash capsule
[    2.244502] efi-capsule: Registering with pstore

and

[    6.617506] efi-capsule: Read record type 0 size 3129 id
6392920603753447425 compressed 1 timestamp 1488467819
[    6.620643] efi-capsule: Read record type 0 size 3477 id
6392920603753447426 compressed 1 timestamp 1488467819
[    6.621471] efi-capsule: Read record type 0 size 2466 id
6392920603753447427 compressed 1 timestamp 1488467819
[    6.622116] efi-capsule: Read record type 0 size 1200 id
6392920603753447428 compressed 1 timestamp 1488467819
[    6.622635] efi-capsule: Read record type 0 size 3155 id
6392920603753447429 compressed 1 timestamp 1488467819


Do we really need all that noise? Could we tone it down a bit?

Other than that, this looks ok to me, although I am by no means an expert

Kees: might we have your opinion on this patch, please? Thanks.


> Signed-off-by: Qiuxu Zhuo <qiuxu.zhuo-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> ---
>  drivers/firmware/efi/Kconfig          |  21 ++
>  drivers/firmware/efi/Makefile         |   1 +
>  drivers/firmware/efi/capsule-pstore.c | 527 ++++++++++++++++++++++++++++++++++
>  3 files changed, 549 insertions(+)
>  create mode 100644 drivers/firmware/efi/capsule-pstore.c
>
> diff --git a/drivers/firmware/efi/Kconfig b/drivers/firmware/efi/Kconfig
> index 2e78b0b..aab78a7 100644
> --- a/drivers/firmware/efi/Kconfig
> +++ b/drivers/firmware/efi/Kconfig
> @@ -142,6 +142,27 @@ config APPLE_PROPERTIES
>
>           If unsure, say Y if you have a Mac.  Otherwise N.
>
> +config EFI_CAPSULE_PSTORE
> +       tristate "EFI capsule pstore backend"
> +       depends on EFI && PSTORE
> +       help
> +         Saying Y here enable the EFI capsule mechanism to store crash dumps,
> +         console log, and function tracing data.
> +
> +         To compile this driver as a module, choose M here.
> +
> +         Not many firmware implementations fully support EFI capsules.
> +         If you plan to rely on this you should test whether yours works by
> +         forcing a crash. Most people should not enable this.
> +
> +config EFI_CAPSULE_PSTORE_DEFAULT_DISABLE
> +       bool "Disable using efi capsule as a pstore backend by default"
> +       depends on EFI_CAPSULE_PSTORE
> +       help
> +         Saying Y here will disable the use of efi capsule as a storage
> +         backend for pstore by default. This setting can be overridden
> +         using the capsule-pstore module's pstore_disable parameter.
> +
>  endmenu
>
>  config UEFI_CPER
> diff --git a/drivers/firmware/efi/Makefile b/drivers/firmware/efi/Makefile
> index ad67342..1417e653 100644
> --- a/drivers/firmware/efi/Makefile
> +++ b/drivers/firmware/efi/Makefile
> @@ -14,6 +14,7 @@ obj-$(CONFIG_EFI)                     += capsule.o memmap.o
>  obj-$(CONFIG_EFI_VARS)                 += efivars.o
>  obj-$(CONFIG_EFI_ESRT)                 += esrt.o
>  obj-$(CONFIG_EFI_VARS_PSTORE)          += efi-pstore.o
> +obj-$(CONFIG_EFI_CAPSULE_PSTORE)               += capsule-pstore.o
>  obj-$(CONFIG_UEFI_CPER)                        += cper.o
>  obj-$(CONFIG_EFI_RUNTIME_MAP)          += runtime-map.o
>  obj-$(CONFIG_EFI_RUNTIME_WRAPPERS)     += runtime-wrappers.o
> diff --git a/drivers/firmware/efi/capsule-pstore.c b/drivers/firmware/efi/capsule-pstore.c
> new file mode 100644
> index 0000000..b94cfb9
> --- /dev/null
> +++ b/drivers/firmware/efi/capsule-pstore.c
> @@ -0,0 +1,527 @@
> +#define pr_fmt(fmt) "efi-capsule: " fmt
> +#include <linux/slab.h>
> +#include <linux/vmalloc.h>
> +#include <linux/efi.h>
> +#include <linux/module.h>
> +#include <linux/pstore.h>
> +
> +#define CAPSULE_SIZE (16 * 1024)
> +#define CRASH_SIZE 4096
> +#define CAPSULE_MAGIC 0x63617073 /* 'caps' */
> +
> +static bool efi_capsule_pstore_disable =
> +       IS_ENABLED(CONFIG_CAPSULE_PSTORE_DEFAULT_DISABLE);
> +
> +static int efi_reset_type = -1;
> +
> +struct efi_capsule_ctx {
> +       struct page **pages;
> +       unsigned int nr_pages;
> +       efi_capsule_header_t *capsule;
> +       size_t capsule_size;
> +       void *data;
> +       size_t data_size;
> +};
> +
> +struct efi_capsule_pstore_buf {
> +       void *buf;
> +       size_t size;
> +       atomic_long_t offset;
> +};
> +
> +struct efi_capsule_pstore {
> +       /* Previous records */
> +       efi_capsule_header_t **hdrs;
> +       uint32_t hdrs_num;
> +       off_t hdr_offset;  /* Offset into current header */
> +
> +       /* New records */
> +       struct efi_capsule_pstore_buf console;
> +       struct efi_capsule_pstore_buf ftrace;
> +       struct efi_capsule_pstore_buf dmesg;
> +};
> +
> +struct efi_capsule_pstore_record {
> +       u64 timestamp;
> +       u64 id;
> +       enum pstore_type_id type;
> +       size_t size;
> +       bool compressed;
> +       u32 magic;
> +       char data[];
> +} __packed;
> +
> +static struct pstore_info efi_capsule_pstore_info;
> +
> +static __init void efi_capsule_destroy(struct efi_capsule_ctx *ctx)
> +{
> +       if (!ctx)
> +               return;
> +
> +       while (ctx->nr_pages--)
> +               __free_page(ctx->pages[ctx->nr_pages]);
> +
> +       kfree(ctx->pages);
> +       kfree(ctx);
> +}
> +
> +/**
> + * efi_capsule_build - alloc data buffer and fill out the header
> + * @guid: vendor's guid
> + * @data_size: size in bytes of the capsule data
> + *
> + * This is a helper function for allocating enough room for user data
> + * + the size of an EFI capsule header.
> + *
> + * Returns a pointer to an allocated capsule on success, an ERR_PTR()
> + * value on error.
> + */
> +static __init struct efi_capsule_ctx *
> +efi_capsule_build(efi_guid_t guid, size_t data_size)
> +{
> +       struct efi_capsule_ctx *ctx;
> +       size_t capsule_size, needed_pages;
> +       u32 flags;
> +       int rv;
> +
> +       flags = EFI_CAPSULE_PERSIST_ACROSS_RESET |
> +               EFI_CAPSULE_POPULATE_SYSTEM_TABLE;
> +       capsule_size = data_size + sizeof(efi_capsule_header_t);
> +       rv = efi_capsule_supported(LINUX_EFI_CRASH_GUID, flags,
> +               capsule_size, &efi_reset_type);
> +       if (rv)
> +               return ERR_PTR(rv);
> +
> +       ctx = kzalloc(sizeof(*ctx), GFP_KERNEL);
> +       if (!ctx)
> +               return ERR_PTR(-ENOMEM);
> +
> +       pr_info("Allocating: %zu\n", capsule_size);
> +       needed_pages = ALIGN(capsule_size, PAGE_SIZE) >> PAGE_SHIFT;
> +       ctx->pages = kcalloc(needed_pages, sizeof(*ctx->pages), GFP_KERNEL);
> +       if (!ctx->pages)
> +               goto fail;
> +
> +       while (needed_pages--) {
> +               struct page *page;
> +
> +               page = alloc_page(GFP_KERNEL | __GFP_HIGHMEM);
> +               if (!page)
> +                       goto fail;
> +
> +               ctx->pages[ctx->nr_pages++] = page;
> +       }
> +
> +       ctx->capsule = vmap(ctx->pages, ctx->nr_pages, 0, PAGE_KERNEL);
> +       if (!ctx->capsule)
> +               goto fail;
> +
> +       ctx->capsule_size = capsule_size;
> +       ctx->data = (void *)ctx->capsule + sizeof(efi_capsule_header_t);
> +       ctx->data_size = data_size;
> +
> +       pr_info("Allocated %zd bytes of capsule memory\n", data_size);
> +
> +       /*
> +        * Setup the EFI capsule header.
> +        */
> +       memcpy(&ctx->capsule->guid, &guid, sizeof(guid));
> +
> +       ctx->capsule->flags = flags;
> +       ctx->capsule->headersize = sizeof(*ctx->capsule);
> +       ctx->capsule->imagesize = capsule_size;
> +
> +       return ctx;
> +
> +fail:
> +       efi_capsule_destroy(ctx);
> +       return ERR_PTR(-ENOMEM);
> +}
> +
> +/**
> + * We may not be in a position to allocate memory at the time of a
> + * crash, so pre-allocate some space now and register it with the
> + * firmware via efi_capsule_update().
> + *
> + * Also, iterate through the array of capsules pointed to from the EFI
> + * system table and take note of any LINUX_EFI_CRASH_GUID
> + * capsules. They will be parsed by efi_capsule_pstore_read().
> + */
> +static __init int efi_capsule_pstore_setup(void)
> +{
> +       struct efi_capsule_pstore_record *rec;
> +       struct efi_capsule_pstore *pctx = NULL;
> +       struct efi_capsule_ctx *console_ctx = NULL;
> +       struct efi_capsule_ctx *ftrace_ctx = NULL;
> +       struct efi_capsule_ctx *dmesg_ctx = NULL;
> +       efi_capsule_header_t **hdrs;
> +       uint32_t hdrs_num;
> +       void *crash_buf = NULL;
> +       int rv;
> +
> +       pctx = kzalloc(sizeof(*pctx), GFP_KERNEL);
> +       if (!pctx)
> +               return -ENOMEM;
> +
> +       crash_buf = kmalloc(CRASH_SIZE, GFP_KERNEL);
> +       if (!crash_buf) {
> +               rv = -ENOMEM;
> +               goto fail;
> +       }
> +
> +       /* Allocate all the capsules upfront */
> +       dmesg_ctx = efi_capsule_build(LINUX_EFI_CRASH_GUID, CAPSULE_SIZE);
> +       if (IS_ERR(dmesg_ctx)) {
> +               rv = PTR_ERR(dmesg_ctx);
> +               dmesg_ctx = NULL;
> +               goto fail2;
> +       }
> +
> +       ftrace_ctx = efi_capsule_build(LINUX_EFI_CRASH_GUID, CAPSULE_SIZE);
> +       if (IS_ERR(ftrace_ctx)) {
> +               rv = PTR_ERR(ftrace_ctx);
> +               ftrace_ctx = NULL;
> +               goto fail3;
> +       }
> +
> +       console_ctx = efi_capsule_build(LINUX_EFI_CRASH_GUID, CAPSULE_SIZE);
> +       if (IS_ERR(console_ctx)) {
> +               rv = PTR_ERR(console_ctx);
> +               console_ctx = NULL;
> +               goto fail4;
> +       }
> +
> +       /* Register with the firmware */
> +       rv = efi_capsule_update(dmesg_ctx->capsule, dmesg_ctx->pages);
> +       if (rv)
> +               goto fail5;
> +       pr_info("Registered dmesg with firmware\n");
> +
> +       rv = efi_capsule_update(ftrace_ctx->capsule, ftrace_ctx->pages);
> +       if (rv)
> +               goto fail5;
> +       pr_info("Registered ftrace with firmware\n");
> +
> +       rv = efi_capsule_update(console_ctx->capsule, console_ctx->pages);
> +       if (rv)
> +               goto fail5;
> +       pr_info("Registered console with firmware\n");
> +
> +       memset(dmesg_ctx->data, 0, dmesg_ctx->data_size);
> +       pctx->dmesg.size = dmesg_ctx->data_size;
> +       pctx->dmesg.buf = dmesg_ctx->data;
> +       atomic_long_set(&pctx->dmesg.offset, 0);
> +
> +       /* Setup the pstore records for the ring-buffers. */
> +       memset(ftrace_ctx->data, 0, ftrace_ctx->data_size);
> +       pctx->ftrace.size = ftrace_ctx->data_size - offsetof(typeof(*rec), data);
> +       pctx->ftrace.buf = ftrace_ctx->data + offsetof(typeof(*rec), data);
> +       atomic_long_set(&pctx->ftrace.offset, 0);
> +       rec = ftrace_ctx->data;
> +       rec->type = PSTORE_TYPE_FTRACE;
> +
> +       memset(console_ctx->data, 0, console_ctx->data_size);
> +       pctx->console.size = console_ctx->data_size - offsetof(typeof(*rec), data);
> +       pctx->console.buf = console_ctx->data + offsetof(typeof(*rec), data);
> +       atomic_long_set(&pctx->console.offset, 0);
> +       rec = console_ctx->data;
> +       rec->type = PSTORE_TYPE_CONSOLE;
> +
> +       /* Read any pstore entries that were passed across a reboot. */
> +       pr_info("Looking up old capsule\n");
> +       hdrs = efi_capsule_lookup(LINUX_EFI_CRASH_GUID, &hdrs_num);
> +       pctx->hdrs_num = hdrs_num;
> +       pctx->hdrs = IS_ERR(hdrs) ? NULL : hdrs;
> +
> +       if (pctx->hdrs_num)
> +               pr_info("Found Linux crash capsule\n");
> +
> +       /* Register the capsule backend with pstore. */
> +       spin_lock_init(&efi_capsule_pstore_info.buf_lock);
> +       efi_capsule_pstore_info.buf = crash_buf;
> +       efi_capsule_pstore_info.bufsize = CRASH_SIZE;
> +       efi_capsule_pstore_info.data = pctx;
> +       pr_info("Registering with pstore\n");
> +       rv = pstore_register(&efi_capsule_pstore_info);
> +       if (rv)
> +               pr_err("Capsule support registration failed for pstore: %d\n", rv);
> +       else
> +               return 0;
> +
> +fail5:
> +       efi_capsule_destroy(console_ctx);
> +fail4:
> +       efi_capsule_destroy(ftrace_ctx);
> +fail3:
> +       efi_capsule_destroy(dmesg_ctx);
> +fail2:
> +       kfree(crash_buf);
> +fail:
> +       kfree(pctx);
> +       return rv;
> +}
> +
> +/**
> + * Return the next pstore record that was passed to us across a reboot
> + * in an EFI capsule.
> + *
> + * This is expected to be called under the pstore
> + * read_mutex. Therefore, no serialisation is done here.
> + */
> +static struct efi_capsule_pstore_record *
> +get_pstore_read_record(struct efi_capsule_pstore *pctx)
> +{
> +       struct efi_capsule_pstore_record *rec;
> +       efi_capsule_header_t *hdr;
> +       off_t remaining;
> +
> +next:
> +       if (!pctx->hdrs_num)
> +               return NULL;
> +
> +       hdr = pctx->hdrs[pctx->hdrs_num - 1];
> +       rec = (void *)hdr + hdr->headersize + pctx->hdr_offset;
> +
> +       remaining = hdr->imagesize - hdr->headersize - pctx->hdr_offset - offsetof(typeof(*rec), data);
> +
> +       /*
> +        * A single EFI capsule may contain multiple pstore
> +        * records. It may also only be partially filled with pstore
> +        * records, which we can detect by checking for a record with
> +        * zero size.
> +        *
> +        * If there are no more entries in this capsule try the next.
> +        */
> +       if (!rec->size) {
> +               pctx->hdrs_num--;
> +               pctx->hdr_offset = 0;
> +               goto next;
> +       }
> +
> +       /*
> +        * If we've finished parsing all records in this capsule, move
> +        * onto the next. Otherwise, increment the offset into the
> +        * current capsule (pctx->hdr_offset).
> +        */
> +       if (rec->size == remaining) {
> +               pctx->hdrs_num--;
> +               pctx->hdr_offset = 0;
> +       } else
> +               pctx->hdr_offset += rec->size + offsetof(typeof(*rec), data);
> +
> +       return rec;
> +}
> +
> +static ssize_t efi_capsule_pstore_read(u64 *id, enum pstore_type_id *type,
> +                                      int *count, struct timespec *time,
> +                                      char **buf, bool *compressed, ssize_t *ecc_notice_size,
> +                                          struct pstore_info *psi)
> +{
> +       struct efi_capsule_pstore_record *rec;
> +       struct efi_capsule_pstore *pctx = psi->data;
> +       ssize_t size;
> +
> +       rec = get_pstore_read_record(pctx);
> +       if (!rec)
> +               return 0;
> +
> +       if (rec->magic != CAPSULE_MAGIC) {
> +               pr_info("%s Invalid capsule record!\n", __func__);
> +               return 0;
> +       }
> +
> +       *type = rec->type;
> +       time->tv_sec = rec->timestamp;
> +       time->tv_nsec = 0;
> +       size = rec->size;
> +       *id = rec->id;
> +       *compressed = rec->compressed;
> +       *ecc_notice_size = 0;
> +
> +       pr_info("Read record type %d size %zu id %llu compressed %d timestamp %llu\n",
> +                       rec->type, rec->size, rec->id, rec->compressed, rec->timestamp);
> +
> +       *buf = kmalloc(size, GFP_KERNEL);
> +       if (!*buf)
> +               return -ENOMEM;
> +
> +       memcpy(*buf, rec->data, size);
> +
> +       return size;
> +}
> +
> +/*
> + * We expect to be called with ->buf_lock held, and so don't perform
> + * any serialisation.
> + */
> +static struct notrace efi_capsule_pstore_record *
> +get_pstore_write_record(struct efi_capsule_pstore_buf *pbuf, size_t *size)
> +{
> +       struct efi_capsule_pstore_record *rec;
> +       long offset = atomic_long_read(&pbuf->offset);
> +
> +       if (offset + offsetof(typeof(*rec), data) >= pbuf->size)
> +               return NULL;
> +
> +       /* Trim 'size' if there isn't enough remaining space */
> +       if (offset + *size + offsetof(typeof(*rec), data) > pbuf->size)
> +               *size = pbuf->size - offset - offsetof(typeof(*rec), data);
> +
> +       rec = pbuf->buf + offset;
> +       atomic_long_add(offsetof(typeof(*rec), data) + *size, &pbuf->offset);
> +
> +       return rec;
> +}
> +
> +static int notrace
> +efi_capsule_pstore_write(enum pstore_type_id type,
> +                        enum kmsg_dump_reason reason, u64 *id,
> +                        unsigned int part, int count, bool compressed,
> +                        size_t size, struct pstore_info *psi)
> +{
> +       struct efi_capsule_pstore_record *rec;
> +       struct efi_capsule_pstore *pctx = psi->data;
> +       size_t sz = size;
> +       static atomic64_t seq;
> +
> +       /*
> +        * A zero size record would break our detection of
> +        * partially-filled capsules.
> +        */
> +       if (!size)
> +               return -EINVAL;
> +
> +       rec = get_pstore_write_record(&pctx->dmesg, &size);
> +       if (!rec || (compressed && size < sz))
> +               return -ENOSPC;
> +
> +       rec->type = type;
> +       rec->timestamp = get_seconds();
> +       rec->size = size;
> +       if (!atomic64_read(&seq))
> +               atomic64_set(&seq, rec->timestamp << 32);
> +       rec->id = (*id) = atomic64_inc_return(&seq);
> +       rec->compressed = compressed;
> +       rec->magic = CAPSULE_MAGIC;
> +       memcpy(rec->data, psi->buf, size);
> +
> +       return 0;
> +}
> +
> +static notrace void *
> +get_pstore_buf(struct efi_capsule_pstore_buf *pbuf, size_t size)
> +{
> +       long next, curr;
> +       struct efi_capsule_pstore_record *rec;
> +
> +       if (size > pbuf->size)
> +               return NULL;
> +
> +       rec = pbuf->buf - offsetof(typeof(*rec), data);
> +       rec->magic = CAPSULE_MAGIC;
> +       if (rec->size + size > pbuf->size)
> +               rec->size = pbuf->size;
> +       else
> +               rec->size += size;
> +
> +       do {
> +               curr = atomic_long_read(&pbuf->offset);
> +               next = curr + size;
> +
> +               /* Wrap? */
> +               if (next > pbuf->size) {
> +                       next = size;
> +                       if (atomic_long_cmpxchg(&pbuf->offset, curr, next)) {
> +                               curr = 0;
> +                               break;
> +                       }
> +
> +                       continue;
> +               }
> +
> +       } while (atomic_long_cmpxchg(&pbuf->offset, curr, next) != curr);
> +
> +       return pbuf->buf + curr;
> +}
> +
> +static int notrace
> +efi_capsule_pstore_write_buf(enum pstore_type_id type,
> +                            enum kmsg_dump_reason reason,
> +                            u64 *id, unsigned int part,
> +                            const char *buf, bool compressed,
> +                            size_t size, struct pstore_info *psi)
> +{
> +       struct efi_capsule_pstore *pctx = psi->data;
> +       void *dst;
> +
> +       if (type == PSTORE_TYPE_FTRACE)
> +               dst = get_pstore_buf(&pctx->ftrace, size);
> +       else if (type == PSTORE_TYPE_CONSOLE)
> +               dst = get_pstore_buf(&pctx->console, size);
> +       else
> +               return -EINVAL;
> +
> +       if (!dst)
> +               return -ENOSPC;
> +
> +       memcpy(dst, buf, size);
> +
> +       return 0;
> +}
> +
> +static struct pstore_info efi_capsule_pstore_info = {
> +       .owner     = THIS_MODULE,
> +       .name      = "efi-capsule",
> +       .flags     = PSTORE_FLAGS_DMESG | PSTORE_FLAGS_CONSOLE | PSTORE_FLAGS_FTRACE,
> +       .read      = efi_capsule_pstore_read,
> +       .write     = efi_capsule_pstore_write,
> +       .write_buf = efi_capsule_pstore_write_buf,
> +};
> +
> +static __init int check_capsule_support(void)
> +{
> +       int rv;
> +
> +       efi_guid_t guid = LINUX_EFI_CRASH_GUID;
> +       u32 flags = EFI_CAPSULE_PERSIST_ACROSS_RESET |
> +               EFI_CAPSULE_POPULATE_SYSTEM_TABLE;
> +
> +       rv = efi_capsule_supported(guid, flags, CAPSULE_SIZE, &efi_reset_type);
> +       if (rv)
> +               return rv;
> +
> +       return 0;
> +}
> +
> +static __init int efi_capsule_pstore_init(void)
> +{
> +       int rv = 0;
> +
> +       if (!efi_enabled(EFI_RUNTIME_SERVICES))
> +               return -ENODEV;
> +
> +       if (efi_capsule_pstore_disable)
> +               return 0;
> +
> +       rv = check_capsule_support();
> +       if (rv)
> +               return rv;
> +
> +       rv = efi_capsule_pstore_setup();
> +       if (rv)
> +               return rv;
> +
> +       return 0;
> +}
> +
> +static __exit void efi_capsule_pstore_exit(void)
> +{
> +}
> +
> +module_init(efi_capsule_pstore_init);
> +module_exit(efi_capsule_pstore_exit);
> +
> +module_param_named(pstore_disable, efi_capsule_pstore_disable, bool, 0644);
> +
> +MODULE_DESCRIPTION("EFI capsule backend for pstore");
> +MODULE_LICENSE("GPL v2");
> --
> 2.9.0.GIT
>

Re: [PATCH v2 2/2] efi: capsule pstore backend

[Kees Cook]

On Thu, Mar 2, 2017 at 7:43 AM, Ard Biesheuvel
<ard.biesheuvel-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> wrote:
> (+ Kees)
>
> On 1 March 2017 at 17:59, Qiuxu Zhuo <qiuxu.zhuo-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org> wrote:
>> The EFI capsule mechanism allows data blobs to be passed to the EFI
>> firmware. By setting the EFI_CAPSULE_POPULATE_SYSTEM_TABLE and the
>> EFI_CAPSULE_PERSIST_ACROSS_REBOOT flags, the firmware will place a
>> pointer to our data blob in the EFI System Table on the next boot.
>> We can utilise this facility to save crash dumps, call traces, etc
>> and pick them up after reboot.
>>
>> Initial cut at this driver by Matt Fleming as below links
>> https://git.kernel.org/cgit/linux/kernel/git/mfleming/efi.git/commit/?h=capsule-pstore&id=99c5f047133555aa0442f64064e85b7da2d4a45f
>> https://git.kernel.org/cgit/linux/kernel/git/mfleming/efi.git/commit/?h=capsule-pstore&id=8625c776c9b8bbed7fa4aa023e36542615165240
>> Extensive cleanup, refactoring, bug fix, and verification by Qiuxu Zhuo
>>
>> Patch verified on Intel Kabylake client platform + Intel KBL BIOS:(10/24/2016):
>> - modprobe capsule-pstore
>> - echo 1 > /sys/module/kernel/parameters/panic
>> - echo c > /proc/sysrq-trigger
>> - system reboot...
>> - ls -l /sys/fs/pstore/
>>   -r--r--r-- 1 root root 4386 Feb 12 00:31 console-efi-capsule-0
>>   -r--r--r-- 1 root root 9065 Feb 12 00:29 dmesg-efi-capsule-6250071391647825921
>>   -r--r--r-- 1 root root 9073 Feb 12 00:29 dmesg-efi-capsule-6250071391647825922
>>   -r--r--r-- 1 root root 9096 Feb 12 00:29 dmesg-efi-capsule-6250071391647825923
>>   -r--r--r-- 1 root root 9073 Feb 12 00:29 dmesg-efi-capsule-6250071391647825924
>>   -r--r--r-- 1 root root 9048 Feb 12 00:29 dmesg-efi-capsule-6250071391647825925
>>
>> The above files contain the last complete logs.
>>
>
> I managed to run this on a 64-bit ARM virtual machine under QEMU.
>
> My kernel spits out the following:
>
> [    1.820650] efi-capsule: Allocating: 16412
> [    1.820978] efi-capsule: Allocated 16384 bytes of capsule memory
> [    1.821256] efi-capsule: Allocating: 16412
> [    1.821447] efi-capsule: Allocated 16384 bytes of capsule memory
> [    1.821731] efi-capsule: Allocating: 16412
> [    1.821926] efi-capsule: Allocated 16384 bytes of capsule memory
> [    1.971762] efi-capsule: Registered dmesg with firmware
> [    2.111314] efi-capsule: Registered ftrace with firmware
> [    2.241585] efi-capsule: Registered console with firmware
> [    2.241907] efi-capsule: Looking up old capsule
> [    2.244081] efi-capsule: Found Linux crash capsule
> [    2.244502] efi-capsule: Registering with pstore
>
> and
>
> [    6.617506] efi-capsule: Read record type 0 size 3129 id
> 6392920603753447425 compressed 1 timestamp 1488467819
> [    6.620643] efi-capsule: Read record type 0 size 3477 id
> 6392920603753447426 compressed 1 timestamp 1488467819
> [    6.621471] efi-capsule: Read record type 0 size 2466 id
> 6392920603753447427 compressed 1 timestamp 1488467819
> [    6.622116] efi-capsule: Read record type 0 size 1200 id
> 6392920603753447428 compressed 1 timestamp 1488467819
> [    6.622635] efi-capsule: Read record type 0 size 3155 id
> 6392920603753447429 compressed 1 timestamp 1488467819
>
>
> Do we really need all that noise? Could we tone it down a bit?

Agreed, that all seems like debugging to me.

> Other than that, this looks ok to me, although I am by no means an expert
>
> Kees: might we have your opinion on this patch, please? Thanks.

Sure! Please add me to CC for future revisions, too.

>> Signed-off-by: Qiuxu Zhuo <qiuxu.zhuo-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
>> ---
>>  drivers/firmware/efi/Kconfig          |  21 ++
>>  drivers/firmware/efi/Makefile         |   1 +
>>  drivers/firmware/efi/capsule-pstore.c | 527 ++++++++++++++++++++++++++++++++++
>>  3 files changed, 549 insertions(+)
>>  create mode 100644 drivers/firmware/efi/capsule-pstore.c
>>
>> diff --git a/drivers/firmware/efi/Kconfig b/drivers/firmware/efi/Kconfig
>> index 2e78b0b..aab78a7 100644
>> --- a/drivers/firmware/efi/Kconfig
>> +++ b/drivers/firmware/efi/Kconfig
>> @@ -142,6 +142,27 @@ config APPLE_PROPERTIES
>>
>>           If unsure, say Y if you have a Mac.  Otherwise N.
>>
>> +config EFI_CAPSULE_PSTORE
>> +       tristate "EFI capsule pstore backend"
>> +       depends on EFI && PSTORE
>> +       help
>> +         Saying Y here enable the EFI capsule mechanism to store crash dumps,
>> +         console log, and function tracing data.
>> +
>> +         To compile this driver as a module, choose M here.
>> +
>> +         Not many firmware implementations fully support EFI capsules.
>> +         If you plan to rely on this you should test whether yours works by
>> +         forcing a crash. Most people should not enable this.
>> +
>> +config EFI_CAPSULE_PSTORE_DEFAULT_DISABLE
>> +       bool "Disable using efi capsule as a pstore backend by default"
>> +       depends on EFI_CAPSULE_PSTORE
>> +       help
>> +         Saying Y here will disable the use of efi capsule as a storage
>> +         backend for pstore by default. This setting can be overridden
>> +         using the capsule-pstore module's pstore_disable parameter.
>> +
>>  endmenu
>>
>>  config UEFI_CPER
>> diff --git a/drivers/firmware/efi/Makefile b/drivers/firmware/efi/Makefile
>> index ad67342..1417e653 100644
>> --- a/drivers/firmware/efi/Makefile
>> +++ b/drivers/firmware/efi/Makefile
>> @@ -14,6 +14,7 @@ obj-$(CONFIG_EFI)                     += capsule.o memmap.o
>>  obj-$(CONFIG_EFI_VARS)                 += efivars.o
>>  obj-$(CONFIG_EFI_ESRT)                 += esrt.o
>>  obj-$(CONFIG_EFI_VARS_PSTORE)          += efi-pstore.o
>> +obj-$(CONFIG_EFI_CAPSULE_PSTORE)               += capsule-pstore.o
>>  obj-$(CONFIG_UEFI_CPER)                        += cper.o
>>  obj-$(CONFIG_EFI_RUNTIME_MAP)          += runtime-map.o
>>  obj-$(CONFIG_EFI_RUNTIME_WRAPPERS)     += runtime-wrappers.o
>> diff --git a/drivers/firmware/efi/capsule-pstore.c b/drivers/firmware/efi/capsule-pstore.c
>> new file mode 100644
>> index 0000000..b94cfb9
>> --- /dev/null
>> +++ b/drivers/firmware/efi/capsule-pstore.c
>> @@ -0,0 +1,527 @@

I'd include a file header comment here describing what the code
is/does, dependencies, etc.

>> +#define pr_fmt(fmt) "efi-capsule: " fmt
>> +#include <linux/slab.h>
>> +#include <linux/vmalloc.h>
>> +#include <linux/efi.h>
>> +#include <linux/module.h>
>> +#include <linux/pstore.h>
>> +
>> +#define CAPSULE_SIZE (16 * 1024)
>> +#define CRASH_SIZE 4096
>> +#define CAPSULE_MAGIC 0x63617073 /* 'caps' */
>> +
>> +static bool efi_capsule_pstore_disable =
>> +       IS_ENABLED(CONFIG_CAPSULE_PSTORE_DEFAULT_DISABLE);
>> +
>> +static int efi_reset_type = -1;
>> +
>> +struct efi_capsule_ctx {
>> +       struct page **pages;
>> +       unsigned int nr_pages;
>> +       efi_capsule_header_t *capsule;
>> +       size_t capsule_size;
>> +       void *data;
>> +       size_t data_size;
>> +};
>> +
>> +struct efi_capsule_pstore_buf {
>> +       void *buf;
>> +       size_t size;
>> +       atomic_long_t offset;
>> +};
>> +
>> +struct efi_capsule_pstore {
>> +       /* Previous records */
>> +       efi_capsule_header_t **hdrs;
>> +       uint32_t hdrs_num;
>> +       off_t hdr_offset;  /* Offset into current header */
>> +
>> +       /* New records */
>> +       struct efi_capsule_pstore_buf console;
>> +       struct efi_capsule_pstore_buf ftrace;
>> +       struct efi_capsule_pstore_buf dmesg;
>> +};
>> +
>> +struct efi_capsule_pstore_record {
>> +       u64 timestamp;
>> +       u64 id;
>> +       enum pstore_type_id type;
>> +       size_t size;
>> +       bool compressed;
>> +       u32 magic;
>> +       char data[];
>> +} __packed;
>> +
>> +static struct pstore_info efi_capsule_pstore_info;

I'd move this struct initialization up to here and try to fill in as
much of it with static initializers as possible (spinlock, buf size,
etc, are all already known).

>> +static __init void efi_capsule_destroy(struct efi_capsule_ctx *ctx)

This will need to drop __init if you use it in __exit (as I suggest later...)

>> +{
>> +       if (!ctx)
>> +               return;
>> +

I think this should un-vmap before removing the pages.

>> +       while (ctx->nr_pages--)
>> +               __free_page(ctx->pages[ctx->nr_pages]);
>> +
>> +       kfree(ctx->pages);
>> +       kfree(ctx);
>> +}
>> +
>> +/**
>> + * efi_capsule_build - alloc data buffer and fill out the header
>> + * @guid: vendor's guid
>> + * @data_size: size in bytes of the capsule data
>> + *
>> + * This is a helper function for allocating enough room for user data
>> + * + the size of an EFI capsule header.
>> + *
>> + * Returns a pointer to an allocated capsule on success, an ERR_PTR()
>> + * value on error.
>> + */
>> +static __init struct efi_capsule_ctx *
>> +efi_capsule_build(efi_guid_t guid, size_t data_size)
>> +{
>> +       struct efi_capsule_ctx *ctx;
>> +       size_t capsule_size, needed_pages;
>> +       u32 flags;
>> +       int rv;
>> +
>> +       flags = EFI_CAPSULE_PERSIST_ACROSS_RESET |
>> +               EFI_CAPSULE_POPULATE_SYSTEM_TABLE;
>> +       capsule_size = data_size + sizeof(efi_capsule_header_t);
>> +       rv = efi_capsule_supported(LINUX_EFI_CRASH_GUID, flags,
>> +               capsule_size, &efi_reset_type);
>> +       if (rv)
>> +               return ERR_PTR(rv);
>> +
>> +       ctx = kzalloc(sizeof(*ctx), GFP_KERNEL);
>> +       if (!ctx)
>> +               return ERR_PTR(-ENOMEM);
>> +
>> +       pr_info("Allocating: %zu\n", capsule_size);
>> +       needed_pages = ALIGN(capsule_size, PAGE_SIZE) >> PAGE_SHIFT;
>> +       ctx->pages = kcalloc(needed_pages, sizeof(*ctx->pages), GFP_KERNEL);
>> +       if (!ctx->pages)
>> +               goto fail;
>> +
>> +       while (needed_pages--) {
>> +               struct page *page;
>> +
>> +               page = alloc_page(GFP_KERNEL | __GFP_HIGHMEM);
>> +               if (!page)
>> +                       goto fail;
>> +
>> +               ctx->pages[ctx->nr_pages++] = page;
>> +       }
>> +
>> +       ctx->capsule = vmap(ctx->pages, ctx->nr_pages, 0, PAGE_KERNEL);
>> +       if (!ctx->capsule)
>> +               goto fail;

Seems like this alloc_page+vmap pattern should be a standard
efi_capsule helper function?

>> +
>> +       ctx->capsule_size = capsule_size;
>> +       ctx->data = (void *)ctx->capsule + sizeof(efi_capsule_header_t);
>> +       ctx->data_size = data_size;

Is it worth adjusting the data_size here to the actual size allocated?
Since capsule_size allocation was rounded up to a multiple of
PAGE_SIZE, there may be more than data_size available now...

>> +
>> +       pr_info("Allocated %zd bytes of capsule memory\n", data_size);
>> +
>> +       /*
>> +        * Setup the EFI capsule header.
>> +        */
>> +       memcpy(&ctx->capsule->guid, &guid, sizeof(guid));
>> +
>> +       ctx->capsule->flags = flags;
>> +       ctx->capsule->headersize = sizeof(*ctx->capsule);
>> +       ctx->capsule->imagesize = capsule_size;
>> +
>> +       return ctx;
>> +
>> +fail:
>> +       efi_capsule_destroy(ctx);
>> +       return ERR_PTR(-ENOMEM);
>> +}
>> +
>> +/**
>> + * We may not be in a position to allocate memory at the time of a
>> + * crash, so pre-allocate some space now and register it with the
>> + * firmware via efi_capsule_update().
>> + *
>> + * Also, iterate through the array of capsules pointed to from the EFI
>> + * system table and take note of any LINUX_EFI_CRASH_GUID
>> + * capsules. They will be parsed by efi_capsule_pstore_read().
>> + */
>> +static __init int efi_capsule_pstore_setup(void)
>> +{
>> +       struct efi_capsule_pstore_record *rec;
>> +       struct efi_capsule_pstore *pctx = NULL;
>> +       struct efi_capsule_ctx *console_ctx = NULL;
>> +       struct efi_capsule_ctx *ftrace_ctx = NULL;
>> +       struct efi_capsule_ctx *dmesg_ctx = NULL;
>> +       efi_capsule_header_t **hdrs;
>> +       uint32_t hdrs_num;
>> +       void *crash_buf = NULL;
>> +       int rv;
>> +
>> +       pctx = kzalloc(sizeof(*pctx), GFP_KERNEL);
>> +       if (!pctx)
>> +               return -ENOMEM;
>> +
>> +       crash_buf = kmalloc(CRASH_SIZE, GFP_KERNEL);
>> +       if (!crash_buf) {
>> +               rv = -ENOMEM;
>> +               goto fail;
>> +       }
>> +
>> +       /* Allocate all the capsules upfront */
>> +       dmesg_ctx = efi_capsule_build(LINUX_EFI_CRASH_GUID, CAPSULE_SIZE);
>> +       if (IS_ERR(dmesg_ctx)) {
>> +               rv = PTR_ERR(dmesg_ctx);
>> +               dmesg_ctx = NULL;
>> +               goto fail2;
>> +       }
>> +
>> +       ftrace_ctx = efi_capsule_build(LINUX_EFI_CRASH_GUID, CAPSULE_SIZE);
>> +       if (IS_ERR(ftrace_ctx)) {
>> +               rv = PTR_ERR(ftrace_ctx);
>> +               ftrace_ctx = NULL;
>> +               goto fail3;
>> +       }
>> +
>> +       console_ctx = efi_capsule_build(LINUX_EFI_CRASH_GUID, CAPSULE_SIZE);
>> +       if (IS_ERR(console_ctx)) {
>> +               rv = PTR_ERR(console_ctx);
>> +               console_ctx = NULL;
>> +               goto fail4;
>> +       }
>> +
>> +       /* Register with the firmware */
>> +       rv = efi_capsule_update(dmesg_ctx->capsule, dmesg_ctx->pages);
>> +       if (rv)
>> +               goto fail5;
>> +       pr_info("Registered dmesg with firmware\n");
>> +
>> +       rv = efi_capsule_update(ftrace_ctx->capsule, ftrace_ctx->pages);
>> +       if (rv)
>> +               goto fail5;
>> +       pr_info("Registered ftrace with firmware\n");
>> +
>> +       rv = efi_capsule_update(console_ctx->capsule, console_ctx->pages);
>> +       if (rv)
>> +               goto fail5;

On failures, shouldn't there be some kind of "unregistering" with
efi_capsule? The comments for efi_capsule_update() say:

 * If the capsule is successfully submitted to the firmware, any
 * subsequent calls to efi_capsule_pending() will return true. @pages
 * must not be released or modified if this function returns
 * successfully.

Regardless, there's a lot common between the efi_capsule_build() call
blocks and efi_capsule_update() call blocks. Maybe those should be
macros?

>> +       pr_info("Registered console with firmware\n");
>> +
>> +       memset(dmesg_ctx->data, 0, dmesg_ctx->data_size);
>> +       pctx->dmesg.size = dmesg_ctx->data_size;
>> +       pctx->dmesg.buf = dmesg_ctx->data;
>> +       atomic_long_set(&pctx->dmesg.offset, 0);
>> +
>> +       /* Setup the pstore records for the ring-buffers. */
>> +       memset(ftrace_ctx->data, 0, ftrace_ctx->data_size);
>> +       pctx->ftrace.size = ftrace_ctx->data_size - offsetof(typeof(*rec), data);
>> +       pctx->ftrace.buf = ftrace_ctx->data + offsetof(typeof(*rec), data);
>> +       atomic_long_set(&pctx->ftrace.offset, 0);
>> +       rec = ftrace_ctx->data;
>> +       rec->type = PSTORE_TYPE_FTRACE;
>> +
>> +       memset(console_ctx->data, 0, console_ctx->data_size);
>> +       pctx->console.size = console_ctx->data_size - offsetof(typeof(*rec), data);
>> +       pctx->console.buf = console_ctx->data + offsetof(typeof(*rec), data);
>> +       atomic_long_set(&pctx->console.offset, 0);
>> +       rec = console_ctx->data;
>> +       rec->type = PSTORE_TYPE_CONSOLE;

These setups feel like they need a macro too...

>> +       /* Read any pstore entries that were passed across a reboot. */
>> +       pr_info("Looking up old capsule\n");
>> +       hdrs = efi_capsule_lookup(LINUX_EFI_CRASH_GUID, &hdrs_num);
>> +       pctx->hdrs_num = hdrs_num;
>> +       pctx->hdrs = IS_ERR(hdrs) ? NULL : hdrs;
>> +
>> +       if (pctx->hdrs_num)
>> +               pr_info("Found Linux crash capsule\n");

Is there a limit to the number of capsules EFI supports? Could a
system fill up with crashes, for example? i.e. the setup above would
fail since new capsules couldn't be added, but the old capsules would
still be in there but couldn't be seen in the pstore filesystem to be
deleted...

>> +
>> +       /* Register the capsule backend with pstore. */
>> +       spin_lock_init(&efi_capsule_pstore_info.buf_lock);
>> +       efi_capsule_pstore_info.buf = crash_buf;
>> +       efi_capsule_pstore_info.bufsize = CRASH_SIZE;
>> +       efi_capsule_pstore_info.data = pctx;
>> +       pr_info("Registering with pstore\n");
>> +       rv = pstore_register(&efi_capsule_pstore_info);
>> +       if (rv)
>> +               pr_err("Capsule support registration failed for pstore: %d\n", rv);
>> +       else
>> +               return 0;

A style suggestion: I'd find this easier to review written as:

if (!rv)
    return 0;

pr_err("Capsule support registration failed for pstore: %d\n", rv);
...

>> +
>> +fail5:
>> +       efi_capsule_destroy(console_ctx);
>> +fail4:
>> +       efi_capsule_destroy(ftrace_ctx);
>> +fail3:
>> +       efi_capsule_destroy(dmesg_ctx);
>> +fail2:
>> +       kfree(crash_buf);
>> +fail:
>> +       kfree(pctx);
>> +       return rv;
>> +}
>> +
>> +/**
>> + * Return the next pstore record that was passed to us across a reboot
>> + * in an EFI capsule.
>> + *
>> + * This is expected to be called under the pstore
>> + * read_mutex. Therefore, no serialisation is done here.
>> + */
>> +static struct efi_capsule_pstore_record *
>> +get_pstore_read_record(struct efi_capsule_pstore *pctx)
>> +{
>> +       struct efi_capsule_pstore_record *rec;
>> +       efi_capsule_header_t *hdr;
>> +       off_t remaining;
>> +
>> +next:
>> +       if (!pctx->hdrs_num)
>> +               return NULL;
>> +
>> +       hdr = pctx->hdrs[pctx->hdrs_num - 1];
>> +       rec = (void *)hdr + hdr->headersize + pctx->hdr_offset;

The rec->size needs to be validated here: you're effectively reading
untrusted data (it could have come from the prior (malicious?) system
boot). Make sure it's not larger than the capsule data size, etc.

>> +
>> +       remaining = hdr->imagesize - hdr->headersize - pctx->hdr_offset - offsetof(typeof(*rec), data);
>> +
>> +       /*
>> +        * A single EFI capsule may contain multiple pstore
>> +        * records. It may also only be partially filled with pstore
>> +        * records, which we can detect by checking for a record with
>> +        * zero size.
>> +        *
>> +        * If there are no more entries in this capsule try the next.
>> +        */
>> +       if (!rec->size) {
>> +               pctx->hdrs_num--;
>> +               pctx->hdr_offset = 0;
>> +               goto next;

Can this be reworked into a proper loop? back-gotos aren't great to
reason about...

>> +       }
>> +
>> +       /*
>> +        * If we've finished parsing all records in this capsule, move
>> +        * onto the next. Otherwise, increment the offset into the
>> +        * current capsule (pctx->hdr_offset).
>> +        */
>> +       if (rec->size == remaining) {
>> +               pctx->hdrs_num--;
>> +               pctx->hdr_offset = 0;
>> +       } else
>> +               pctx->hdr_offset += rec->size + offsetof(typeof(*rec), data);

Make sure this doesn't point beyond the capsule (i.e. rec->size is 1
less than capsule size, which means the next rec would read past the
end.

>> +
>> +       return rec;
>> +}
>> +
>> +static ssize_t efi_capsule_pstore_read(u64 *id, enum pstore_type_id *type,
>> +                                      int *count, struct timespec *time,
>> +                                      char **buf, bool *compressed, ssize_t *ecc_notice_size,
>> +                                          struct pstore_info *psi)
>> +{
>> +       struct efi_capsule_pstore_record *rec;
>> +       struct efi_capsule_pstore *pctx = psi->data;
>> +       ssize_t size;
>> +
>> +       rec = get_pstore_read_record(pctx);
>> +       if (!rec)
>> +               return 0;
>> +
>> +       if (rec->magic != CAPSULE_MAGIC) {
>> +               pr_info("%s Invalid capsule record!\n", __func__);
>> +               return 0;
>> +       }
>> +
>> +       *type = rec->type;
>> +       time->tv_sec = rec->timestamp;
>> +       time->tv_nsec = 0;
>> +       size = rec->size;
>> +       *id = rec->id;
>> +       *compressed = rec->compressed;
>> +       *ecc_notice_size = 0;
>> +
>> +       pr_info("Read record type %d size %zu id %llu compressed %d timestamp %llu\n",
>> +                       rec->type, rec->size, rec->id, rec->compressed, rec->timestamp);
>> +
>> +       *buf = kmalloc(size, GFP_KERNEL);
>> +       if (!*buf)
>> +               return -ENOMEM;
>> +
>> +       memcpy(*buf, rec->data, size);
>> +
>> +       return size;
>> +}
>> +
>> +/*
>> + * We expect to be called with ->buf_lock held, and so don't perform
>> + * any serialisation.
>> + */
>> +static struct notrace efi_capsule_pstore_record *

Why are these marked notrace?

>> +get_pstore_write_record(struct efi_capsule_pstore_buf *pbuf, size_t *size)
>> +{
>> +       struct efi_capsule_pstore_record *rec;
>> +       long offset = atomic_long_read(&pbuf->offset);
>> +
>> +       if (offset + offsetof(typeof(*rec), data) >= pbuf->size)
>> +               return NULL;
>> +
>> +       /* Trim 'size' if there isn't enough remaining space */
>> +       if (offset + *size + offsetof(typeof(*rec), data) > pbuf->size)
>> +               *size = pbuf->size - offset - offsetof(typeof(*rec), data);
>> +
>> +       rec = pbuf->buf + offset;
>> +       atomic_long_add(offsetof(typeof(*rec), data) + *size, &pbuf->offset);
>> +
>> +       return rec;
>> +}
>> +
>> +static int notrace
>> +efi_capsule_pstore_write(enum pstore_type_id type,
>> +                        enum kmsg_dump_reason reason, u64 *id,
>> +                        unsigned int part, int count, bool compressed,
>> +                        size_t size, struct pstore_info *psi)
>> +{
>> +       struct efi_capsule_pstore_record *rec;
>> +       struct efi_capsule_pstore *pctx = psi->data;
>> +       size_t sz = size;
>> +       static atomic64_t seq;
>> +
>> +       /*
>> +        * A zero size record would break our detection of
>> +        * partially-filled capsules.
>> +        */
>> +       if (!size)
>> +               return -EINVAL;
>> +
>> +       rec = get_pstore_write_record(&pctx->dmesg, &size);
>> +       if (!rec || (compressed && size < sz))
>> +               return -ENOSPC;
>> +
>> +       rec->type = type;
>> +       rec->timestamp = get_seconds();
>> +       rec->size = size;
>> +       if (!atomic64_read(&seq))
>> +               atomic64_set(&seq, rec->timestamp << 32);
>> +       rec->id = (*id) = atomic64_inc_return(&seq);
>> +       rec->compressed = compressed;
>> +       rec->magic = CAPSULE_MAGIC;
>> +       memcpy(rec->data, psi->buf, size);
>> +
>> +       return 0;
>> +}
>> +
>> +static notrace void *
>> +get_pstore_buf(struct efi_capsule_pstore_buf *pbuf, size_t size)
>> +{
>> +       long next, curr;
>> +       struct efi_capsule_pstore_record *rec;
>> +
>> +       if (size > pbuf->size)
>> +               return NULL;
>> +
>> +       rec = pbuf->buf - offsetof(typeof(*rec), data);
>> +       rec->magic = CAPSULE_MAGIC;
>> +       if (rec->size + size > pbuf->size)
>> +               rec->size = pbuf->size;
>> +       else
>> +               rec->size += size;
>> +
>> +       do {
>> +               curr = atomic_long_read(&pbuf->offset);
>> +               next = curr + size;
>> +
>> +               /* Wrap? */
>> +               if (next > pbuf->size) {
>> +                       next = size;
>> +                       if (atomic_long_cmpxchg(&pbuf->offset, curr, next)) {
>> +                               curr = 0;
>> +                               break;
>> +                       }
>> +
>> +                       continue;
>> +               }
>> +
>> +       } while (atomic_long_cmpxchg(&pbuf->offset, curr, next) != curr);
>> +
>> +       return pbuf->buf + curr;
>> +}
>> +
>> +static int notrace
>> +efi_capsule_pstore_write_buf(enum pstore_type_id type,
>> +                            enum kmsg_dump_reason reason,
>> +                            u64 *id, unsigned int part,
>> +                            const char *buf, bool compressed,
>> +                            size_t size, struct pstore_info *psi)
>> +{
>> +       struct efi_capsule_pstore *pctx = psi->data;
>> +       void *dst;
>> +
>> +       if (type == PSTORE_TYPE_FTRACE)
>> +               dst = get_pstore_buf(&pctx->ftrace, size);
>> +       else if (type == PSTORE_TYPE_CONSOLE)
>> +               dst = get_pstore_buf(&pctx->console, size);
>> +       else
>> +               return -EINVAL;
>> +
>> +       if (!dst)
>> +               return -ENOSPC;
>> +
>> +       memcpy(dst, buf, size);
>> +
>> +       return 0;
>> +}
>> +
>> +static struct pstore_info efi_capsule_pstore_info = {
>> +       .owner     = THIS_MODULE,
>> +       .name      = "efi-capsule",
>> +       .flags     = PSTORE_FLAGS_DMESG | PSTORE_FLAGS_CONSOLE | PSTORE_FLAGS_FTRACE,

Yay flags! :) (Any reason not to support pmsg?)

>> +       .read      = efi_capsule_pstore_read,
>> +       .write     = efi_capsule_pstore_write,
>> +       .write_buf = efi_capsule_pstore_write_buf,
>> +};
>> +
>> +static __init int check_capsule_support(void)
>> +{
>> +       int rv;
>> +
>> +       efi_guid_t guid = LINUX_EFI_CRASH_GUID;
>> +       u32 flags = EFI_CAPSULE_PERSIST_ACROSS_RESET |
>> +               EFI_CAPSULE_POPULATE_SYSTEM_TABLE;
>> +
>> +       rv = efi_capsule_supported(guid, flags, CAPSULE_SIZE, &efi_reset_type);
>> +       if (rv)
>> +               return rv;
>> +
>> +       return 0;
>> +}
>> +
>> +static __init int efi_capsule_pstore_init(void)
>> +{
>> +       int rv = 0;
>> +
>> +       if (!efi_enabled(EFI_RUNTIME_SERVICES))
>> +               return -ENODEV;
>> +
>> +       if (efi_capsule_pstore_disable)
>> +               return 0;
>> +
>> +       rv = check_capsule_support();
>> +       if (rv)
>> +               return rv;
>> +
>> +       rv = efi_capsule_pstore_setup();
>> +       if (rv)
>> +               return rv;

I think it would be friendly to report a text error at each of the
returns here describing which step failed.

>> +
>> +       return 0;
>> +}
>> +
>> +static __exit void efi_capsule_pstore_exit(void)
>> +{
>> +}

This seems bad! :) Unloading this would leave pstore attempting to
call functions in freed memory. This should unregister from pstore,
release the vmap, free the pages, etc...

If you want to force it to never be unloaded, you'll need to manually
take an extra module reference in your init so that the kernel will
refuse to unload it.

>> +
>> +module_init(efi_capsule_pstore_init);
>> +module_exit(efi_capsule_pstore_exit);
>> +
>> +module_param_named(pstore_disable, efi_capsule_pstore_disable, bool, 0644);
>> +
>> +MODULE_DESCRIPTION("EFI capsule backend for pstore");
>> +MODULE_LICENSE("GPL v2");

Glad to see more pstore backends! Thanks for working on this!

-Kees

-- 
Kees Cook
Pixel Security

Re: [PATCH v2 0/2] capsule pstore backend

[Ard Biesheuvel]

On 1 March 2017 at 17:59, Qiuxu Zhuo <qiuxu.zhuo-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org> wrote:
> *** BLURB HERE ***
>

FYI the UEFI spec is vague about the exact format of the per-GUID
capsule configuration table entries, and so we have asked the USWG for
clarification. In the mean time, I'd like to refrain from merging
anything based on it, so unfortunately, we are going to have to park
this for now.

Regards,
Ard.

> Qiuxu Zhuo (2):
>   efi/capsule: Add 'capsule' lookup support
>   efi: capsule pstore backend
>
>  drivers/firmware/efi/Kconfig          |  21 ++
>  drivers/firmware/efi/Makefile         |   1 +
>  drivers/firmware/efi/capsule-pstore.c | 527 ++++++++++++++++++++++++++++++++++
>  drivers/firmware/efi/capsule.c        | 107 +++++++
>  drivers/firmware/efi/efi.c            |   4 +
>  include/linux/efi.h                   |   4 +
>  6 files changed, 664 insertions(+)
>  create mode 100644 drivers/firmware/efi/capsule-pstore.c
> --
>
> Change Log (v1->v2):
>   - Use memremap() instead of ioremap() for firmware tables
>   - Remove "default n" in Kconfig file
>   - Move check_capsule_support() up a bit
>   - Update SoB chain
> --
> 2.9.0.GIT
>

Re: [PATCH v2 0/2] capsule pstore backend

[Luck, Tony]

On Fri, Mar 03, 2017 at 03:19:51PM +0000, Ard Biesheuvel wrote:
> On 1 March 2017 at 17:59, Qiuxu Zhuo <qiuxu.zhuo-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org> wrote:
> > *** BLURB HERE ***
> >
> 
> FYI the UEFI spec is vague about the exact format of the per-GUID
> capsule configuration table entries, and so we have asked the USWG for
> clarification. In the mean time, I'd like to refrain from merging
> anything based on it, so unfortunately, we are going to have to park
> this for now.

Ard,

Any estimates on how long that process takes? How often does the USWG
meet? How would clarification be communicated (would we have to wait
for the next version of the UEFI spec!?)

Thanks

-Tony

Re: [PATCH v2 0/2] capsule pstore backend

[Ard Biesheuvel]

On 3 March 2017 at 17:23, Luck, Tony <tony.luck-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org> wrote:
> On Fri, Mar 03, 2017 at 03:19:51PM +0000, Ard Biesheuvel wrote:
>> On 1 March 2017 at 17:59, Qiuxu Zhuo <qiuxu.zhuo-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org> wrote:
>> > *** BLURB HERE ***
>> >
>>
>> FYI the UEFI spec is vague about the exact format of the per-GUID
>> capsule configuration table entries, and so we have asked the USWG for
>> clarification. In the mean time, I'd like to refrain from merging
>> anything based on it, so unfortunately, we are going to have to park
>> this for now.
>
> Ard,
>
> Any estimates on how long that process takes? How often does the USWG
> meet? How would clarification be communicated (would we have to wait
> for the next version of the UEFI spec!?)
>

The USWG meets weekly, and it doesn't necessarily takes lots of time
to clarify something that is already in the spec, i.e., we wouldn't
have to wait for the errata to be released to actually share the
clarified information.

I'd say order weeks

Regards,
Ard.

RE: [PATCH v2 0/2] capsule pstore backend

[Zhuo, Qiuxu]

Hi Ard,

   There is some information in UEFI 2.4 spec  section 7.5.3  p258:  "The EFI System Table entry must use the GUID from the CapsuleGuid field of the EFI_CAPSULE_HEADER. The EFI System Table entry must point to an array of capsules that contain the same CapsuleGuid value. The array must be prefixed by a UINT32 that represents the size of the array of capsules". 

So my understanding is that the per-GUID capsule configuration table entries look like below layout:

For capsule guid xxx:          |  (u32) capsule number a | (u64)  1st capsule physical addr  | (u64)  2nd capsule physical addr | ...  |  (u64)  a-st capsule physical addr |
For capsule guid yyy:          |  (u32) capsule number b | (u64)  1st capsule physical addr  | (u64)  2nd capsule physical addr | ...  |  (u64)  b-st capsule physical addr |
...  
For capsule guid zzz:           |  (u32) capsule number c | (u64)  1st capsule physical addr  | (u64)  2nd capsule physical addr | ...  |  (u64)  c-st capsule physical addr |

The way you mentioned to go over the config table array (then we can get rid of global staff 'capsule' in struct efi) to get a group of capsules per-GUID is suitable.

This is my understanding, if wrong please correct me, thanks!

BR
qiuxu

-----Original Message-----
From: Ard Biesheuvel [mailto:ard.biesheuvel@linaro.org] 
Sent: Friday, March 3, 2017 11:20 PM
To: Zhuo, Qiuxu <qiuxu.zhuo@intel.com>
Cc: Matt Fleming <matt@codeblueprint.co.uk>; linux-efi@vger.kernel.org; Luck, Tony <tony.luck@intel.com>
Subject: Re: [PATCH v2 0/2] capsule pstore backend

On 1 March 2017 at 17:59, Qiuxu Zhuo <qiuxu.zhuo@intel.com> wrote:
> *** BLURB HERE ***
>

FYI the UEFI spec is vague about the exact format of the per-GUID capsule configuration table entries, and so we have asked the USWG for clarification. In the mean time, I'd like to refrain from merging anything based on it, so unfortunately, we are going to have to park this for now.

Regards,
Ard.

> Qiuxu Zhuo (2):
>   efi/capsule: Add 'capsule' lookup support
>   efi: capsule pstore backend
>
>  drivers/firmware/efi/Kconfig          |  21 ++
>  drivers/firmware/efi/Makefile         |   1 +
>  drivers/firmware/efi/capsule-pstore.c | 527 ++++++++++++++++++++++++++++++++++
>  drivers/firmware/efi/capsule.c        | 107 +++++++
>  drivers/firmware/efi/efi.c            |   4 +
>  include/linux/efi.h                   |   4 +
>  6 files changed, 664 insertions(+)
>  create mode 100644 drivers/firmware/efi/capsule-pstore.c
> --
>
> Change Log (v1->v2):
>   - Use memremap() instead of ioremap() for firmware tables
>   - Remove "default n" in Kconfig file
>   - Move check_capsule_support() up a bit
>   - Update SoB chain
> --
> 2.9.0.GIT
>