[PATCH v6 0/2] Add capsule-pstore backend support

[PATCH v6 0/2] Add capsule-pstore backend support

[Qiuxu Zhuo]

Change Log v5->v6:
  Part1: No changes.
  Part2: Update capsule-pstore.c file header.
         Move the variable 'efi_capsule_pstore_info' backward (for removing some function declarations).


History:
  The v5 was sent about two years ago [1]. At that time there was a concern about the EFI system table
  entry 'CapsulePtr' as below: whether it was an array of capsules or an array of pointers to capsules.

      typedef struct { UINT32 CapsuleArrayNumber; VOID* CapsulePtr[1]; } EFI_CAPSULE_TABLE;

  The latest UEFI specification v2.8 [2] has a clarification in P268 for 'CapsulePtr'  as below:

      "CapsulePtr A pointer to an array of capsules that contain the same CapsuleGuid value.
       Each CapsulePtr points to an instance of an EFI_CAPSULE_HEADER, with the capsule data
       ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
       concatenated on its end."
       ^^^^^^^^^^^^^^^^^^^^^^^

  According to the highlighted description above, the 'CapsulePtr' should be an array of pointers to capsules.
  The capsule-pstore driver and the tested BIOS also use the 'CapsulePtr' as an array of pointers to capsules.


Test:
  The v6 is well-tested on an Intel Kaby Lake client platform + BIOS(10/24/2016) and an Intel Ice Lake
  client platform + BIOS(09/12/2019).

Reference:
  [1] https://lore.kernel.org/linux-efi/?q=capsule-pstore
  [2] https://uefi.org/sites/default/files/resources/UEFI_Spec_2_8_A_Feb14.pdf 

Qiuxu Zhuo (2):
  efi: Add 'nr_config_table' variable in efi structure
  eif/capsule-pstore: Add capsule pstore backend

 arch/x86/platform/efi/efi.c           |   1 +
 drivers/firmware/efi/Kconfig          |  21 +
 drivers/firmware/efi/Makefile         |   1 +
 drivers/firmware/efi/arm-init.c       |   4 +-
 drivers/firmware/efi/capsule-pstore.c | 692 ++++++++++++++++++++++++++
 drivers/firmware/efi/efi.c            |   1 +
 include/linux/efi.h                   |   1 +
 7 files changed, 720 insertions(+), 1 deletion(-)
 create mode 100644 drivers/firmware/efi/capsule-pstore.c

-- 
2.17.1

Re: [PATCH v6 0/2] Add capsule-pstore backend support

[Ard Biesheuvel]

On Wed, 11 Mar 2020 at 21:12, Qiuxu Zhuo <qiuxu.zhuo@intel.com> wrote:
>
> Change Log v5->v6:
>   Part1: No changes.
>   Part2: Update capsule-pstore.c file header.
>          Move the variable 'efi_capsule_pstore_info' backward (for removing some function declarations).
>

This conflicts badly with the efi/core changes queued up in the tip
tree (and hence linux-next) Please rebase and repost


>
> History:
>   The v5 was sent about two years ago [1]. At that time there was a concern about the EFI system table
>   entry 'CapsulePtr' as below: whether it was an array of capsules or an array of pointers to capsules.
>
>       typedef struct { UINT32 CapsuleArrayNumber; VOID* CapsulePtr[1]; } EFI_CAPSULE_TABLE;
>
>   The latest UEFI specification v2.8 [2] has a clarification in P268 for 'CapsulePtr'  as below:
>
>       "CapsulePtr A pointer to an array of capsules that contain the same CapsuleGuid value.
>        Each CapsulePtr points to an instance of an EFI_CAPSULE_HEADER, with the capsule data
>        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>        concatenated on its end."
>        ^^^^^^^^^^^^^^^^^^^^^^^
>
>   According to the highlighted description above, the 'CapsulePtr' should be an array of pointers to capsules.
>   The capsule-pstore driver and the tested BIOS also use the 'CapsulePtr' as an array of pointers to capsules.
>
>
> Test:
>   The v6 is well-tested on an Intel Kaby Lake client platform + BIOS(10/24/2016) and an Intel Ice Lake
>   client platform + BIOS(09/12/2019).
>
> Reference:
>   [1] https://lore.kernel.org/linux-efi/?q=capsule-pstore
>   [2] https://uefi.org/sites/default/files/resources/UEFI_Spec_2_8_A_Feb14.pdf
>
> Qiuxu Zhuo (2):
>   efi: Add 'nr_config_table' variable in efi structure
>   eif/capsule-pstore: Add capsule pstore backend
>
>  arch/x86/platform/efi/efi.c           |   1 +
>  drivers/firmware/efi/Kconfig          |  21 +
>  drivers/firmware/efi/Makefile         |   1 +
>  drivers/firmware/efi/arm-init.c       |   4 +-
>  drivers/firmware/efi/capsule-pstore.c | 692 ++++++++++++++++++++++++++
>  drivers/firmware/efi/efi.c            |   1 +
>  include/linux/efi.h                   |   1 +
>  7 files changed, 720 insertions(+), 1 deletion(-)
>  create mode 100644 drivers/firmware/efi/capsule-pstore.c
>
> --
> 2.17.1
>

RE: [PATCH v6 0/2] Add capsule-pstore backend support

[Zhuo, Qiuxu]

> From: linux-efi-owner@vger.kernel.org <linux-efi-owner@vger.kernel.org> On Behalf Of Ard Biesheuvel
>> ...
> >          Move the variable 'efi_capsule_pstore_info' backward (for removing some function declarations).
> 
> This conflicts badly with the efi/core changes queued up in the tip tree (and hence linux-next) Please rebase and repost

OK. Will rebase the patches on top of tip tree and repost.

-Qiuxu

RE: [PATCH v6 0/2] Add capsule-pstore backend support

[Zhuo, Qiuxu]

> From: linux-efi-owner@vger.kernel.org <linux-efi-owner@vger.kernel.org> On
>...
> On Wed, 11 Mar 2020 at 21:12, Qiuxu Zhuo <qiuxu.zhuo@intel.com> wrote:
> >
> > Change Log v5->v6:
> >   Part1: No changes.
> >   Part2: Update capsule-pstore.c file header.
> >          Move the variable 'efi_capsule_pstore_info' backward (for removing
> some function declarations).
> >
> 
> This conflicts badly with the efi/core changes queued up in the tip tree (and hence linux-next) Please rebase and repost

Hi Ard,

The following commit on the tip tree removes the variables 'config_table' from the efi structure.

    9cd437ac0ef4 ("efi/x86: Make fw_vendor, config_table and runtime sysfs nodes x86 specific")

But the external driver "capsule-pstore.ko" needs to access 'config_table' and 'nr_tables' to go through
the configuration table to extract crash capsules.

Adding 'config_table' and 'nr_tables' back to the efi structure looks like not a good way. 
Do you have any  suggestion on how to export 'config_table' and 'nr_tables' variables for the external driver "capsule-pstore.ko"?

Thanks!
-Qiuxu

Re: [PATCH v6 0/2] Add capsule-pstore backend support

[Ard Biesheuvel]

On Mon, 16 Mar 2020 at 01:33, Zhuo, Qiuxu <qiuxu.zhuo@intel.com> wrote:
>
> > From: linux-efi-owner@vger.kernel.org <linux-efi-owner@vger.kernel.org> On
> >...
> > On Wed, 11 Mar 2020 at 21:12, Qiuxu Zhuo <qiuxu.zhuo@intel.com> wrote:
> > >
> > > Change Log v5->v6:
> > >   Part1: No changes.
> > >   Part2: Update capsule-pstore.c file header.
> > >          Move the variable 'efi_capsule_pstore_info' backward (for removing
> > some function declarations).
> > >
> >
> > This conflicts badly with the efi/core changes queued up in the tip tree (and hence linux-next) Please rebase and repost
>
> Hi Ard,
>
> The following commit on the tip tree removes the variables 'config_table' from the efi structure.
>
>     9cd437ac0ef4 ("efi/x86: Make fw_vendor, config_table and runtime sysfs nodes x86 specific")
>
> But the external driver "capsule-pstore.ko" needs to access 'config_table' and 'nr_tables' to go through
> the configuration table to extract crash capsules.
>
> Adding 'config_table' and 'nr_tables' back to the efi structure looks like not a good way.
> Do you have any  suggestion on how to export 'config_table' and 'nr_tables' variables for the external driver "capsule-pstore.ko"?
>

I will get back to you on monday about this. In any case, this will
have to wait until v5.8

>

RE: [PATCH v6 0/2] Add capsule-pstore backend support

[Zhuo, Qiuxu]

> From: linux-efi-owner@vger.kernel.org <linux-efi-owner@vger.kernel.org> On> Behalf Of Ard Biesheuvel
> ...
> > The following commit on the tip tree removes the variables 'config_table' from the efi structure.
> >
> >     9cd437ac0ef4 ("efi/x86: Make fw_vendor, config_table and runtime sysfs nodes x86 specific")
> >
> > But the external driver "capsule-pstore.ko" needs to access  'config_table' and 'nr_tables' to go through the configuration table to extract  crash capsules.
> >
> > Adding 'config_table' and 'nr_tables' back to the efi structure looks like not a good way.
> > Do you have any  suggestion on how to export 'config_table' and 'nr_tables' variables for the external driver "capsule-pstore.ko"?
> >
> 
> I will get back to you on monday about this. In any case, this will have to wait until v5.8

OK. Thanks!

-Qiuxu

Re: [PATCH v6 0/2] Add capsule-pstore backend support

[Luck, Tony]

On Thu, Mar 19, 2020 at 01:53:38AM -0700, Zhuo, Qiuxu wrote:
> > From: linux-efi-owner@vger.kernel.org <linux-efi-owner@vger.kernel.org> On> Behalf Of Ard Biesheuvel
> > ...
> > > The following commit on the tip tree removes the variables 'config_table' from the efi structure.
> > >
> > >     9cd437ac0ef4 ("efi/x86: Make fw_vendor, config_table and runtime sysfs nodes x86 specific")
> > >
> > > But the external driver "capsule-pstore.ko" needs to access  'config_table' and 'nr_tables' to go through the configuration table to extract  crash capsules.
> > >
> > > Adding 'config_table' and 'nr_tables' back to the efi structure looks like not a good way.
> > > Do you have any  suggestion on how to export 'config_table' and 'nr_tables' variables for the external driver "capsule-pstore.ko"?
> > >
> > 
> > I will get back to you on monday about this. In any case, this will have to wait until v5.8
> 
> OK. Thanks!

Would it be acceptable to take the first half of Qiuxu's
function efi_capsule_table_get() and move it into the
generic efi code naming it something like: "efi_get_table_by_guid()"?
Then EXPORT_GPL that function?

-Tony

Re: [PATCH v6 0/2] Add capsule-pstore backend support

[Ard Biesheuvel]

On Fri, 20 Mar 2020 at 20:25, Luck, Tony <tony.luck@intel.com> wrote:
>
> On Thu, Mar 19, 2020 at 01:53:38AM -0700, Zhuo, Qiuxu wrote:
> > > From: linux-efi-owner@vger.kernel.org <linux-efi-owner@vger.kernel.org> On> Behalf Of Ard Biesheuvel
> > > ...
> > > > The following commit on the tip tree removes the variables 'config_table' from the efi structure.
> > > >
> > > >     9cd437ac0ef4 ("efi/x86: Make fw_vendor, config_table and runtime sysfs nodes x86 specific")
> > > >
> > > > But the external driver "capsule-pstore.ko" needs to access  'config_table' and 'nr_tables' to go through the configuration table to extract  crash capsules.
> > > >
> > > > Adding 'config_table' and 'nr_tables' back to the efi structure looks like not a good way.
> > > > Do you have any  suggestion on how to export 'config_table' and 'nr_tables' variables for the external driver "capsule-pstore.ko"?
> > > >
> > >
> > > I will get back to you on monday about this. In any case, this will have to wait until v5.8
> >
> > OK. Thanks!
>
> Would it be acceptable to take the first half of Qiuxu's
> function efi_capsule_table_get() and move it into the
> generic efi code naming it something like: "efi_get_table_by_guid()"?
> Then EXPORT_GPL that function?
>

That sounds reasonable to me.

Re: [PATCH v6 0/2] Add capsule-pstore backend support

[Ard Biesheuvel]

On Wed, 25 Mar 2020 at 16:52, Ard Biesheuvel <ardb@kernel.org> wrote:
>
> On Fri, 20 Mar 2020 at 20:25, Luck, Tony <tony.luck@intel.com> wrote:
> >
> > On Thu, Mar 19, 2020 at 01:53:38AM -0700, Zhuo, Qiuxu wrote:
> > > > From: linux-efi-owner@vger.kernel.org <linux-efi-owner@vger.kernel.org> On> Behalf Of Ard Biesheuvel
> > > > ...
> > > > > The following commit on the tip tree removes the variables 'config_table' from the efi structure.
> > > > >
> > > > >     9cd437ac0ef4 ("efi/x86: Make fw_vendor, config_table and runtime sysfs nodes x86 specific")
> > > > >
> > > > > But the external driver "capsule-pstore.ko" needs to access  'config_table' and 'nr_tables' to go through the configuration table to extract  crash capsules.
> > > > >
> > > > > Adding 'config_table' and 'nr_tables' back to the efi structure looks like not a good way.
> > > > > Do you have any  suggestion on how to export 'config_table' and 'nr_tables' variables for the external driver "capsule-pstore.ko"?
> > > > >
> > > >
> > > > I will get back to you on monday about this. In any case, this will have to wait until v5.8
> > >
> > > OK. Thanks!
> >
> > Would it be acceptable to take the first half of Qiuxu's
> > function efi_capsule_table_get() and move it into the
> > generic efi code naming it something like: "efi_get_table_by_guid()"?
> > Then EXPORT_GPL that function?
> >
>
> That sounds reasonable to me.

Actually, I don't think this is necessary. The capsule pstore
implementation uses a fixed GUID, which we could simply add to the
common_tables[] array so that its address gets recorded when we
iterate over the list of config tables.