Re: [PATCH v2 1/4] tools: mkeficapsule: Add support for parsing capsule params from config file

2024-04-23 Thread Sughosh Ganu
hi Heinrich,

On Fri, 19 Apr 2024 at 12:44, Heinrich Schuchardt  wrote:
>
> On 19.04.24 08:55, Sughosh Ganu wrote:
> > Add support for specifying the parameters needed for capsule
> > generation through a config file, instead of passing them through
> > command-line. Parameters for more than a single capsule file can be
> > specified, resulting in generation of multiple capsules through a
> > single invocation of the command.
> >
> > The config file can be passed to the mkeficapsule tool in such manner
> >
> >   $ ./tools/mkeficapsule -f 
>
> Please, mention the long option.

Okay

>
> >
> > Signed-off-by: Sughosh Ganu 
> > ---
> >   tools/Kconfig  |  15 ++
> >   tools/Makefile |   1 +
> >   tools/eficapsule.h | 114 
> >   tools/mkeficapsule.c   |  87 +
> >   tools/mkeficapsule_parse.c | 352 +
> >   5 files changed, 538 insertions(+), 31 deletions(-)
> >   create mode 100644 tools/mkeficapsule_parse.c
> >
> > diff --git a/tools/Kconfig b/tools/Kconfig
> > index 667807b331..0362ca8e45 100644
> > --- a/tools/Kconfig
> > +++ b/tools/Kconfig
> > @@ -104,6 +104,21 @@ config TOOLS_MKEFICAPSULE
> > optionally sign that file. If you want to enable UEFI capsule
> > update feature on your target, you certainly need this.
> >
> > +config EFI_CAPSULE_CFG_FILE
> > + string "Path to the EFI Capsule Config File"
> > + help
> > +   Path to the EFI capsule config file which provides the
> > +   parameters needed to build capsule(s). Parameters can be
> > +   provided for multiple payloads resulting in corresponding
> > +   capsule images being generated.
>
> This help test does not explain if this is a parameter for binman or
> something built into mkeficapsule.
>
> We should not hard code any path inside mkeficapsule.
>
> I can't see the new CONFIG parameters used within the code changes of
> this patch. Please, add them into the patches where they are needed.

As discussed over IRC, I need these config options only for the CI
testing. I will replace these with hard-coded paths for the tests.

>
> > +
> > +config EFI_USE_CAPSULE_CFG_FILE
> > + bool "Use the config file for generating capsules"
> > + help
> > +   Boolean option used to specify if the EFI capsules are to
> > +   be generated through parameters specified via the config
> > +   file or through command line.
>
> Given this help text I would not know if this option changes how
> mkeficapsule is built or how binman invokes it.
>
> I would expect that mkeficapsule is always built in a way that a
> configuration file can be passed.
>
> Furthermore I would expect binman to invoke mkeficapsule with the
> appropriate command line parameters if you have enabled building capsules.
>
> Why do we need this configuration parameter? Just always build
> mkeficapsule with support for the -f parameter.

Will remove these config flags.

-sughosh

>
> Best regards
>
> Heinrich
>
> > +
> >   menuconfig FSPI_CONF_HEADER
> >   bool "FlexSPI Header Configuration"
> >   help
> > diff --git a/tools/Makefile b/tools/Makefile
> > index 6a4280e366..4311f5914f 100644
> > --- a/tools/Makefile
> > +++ b/tools/Makefile
> > @@ -253,6 +253,7 @@ HOSTLDLIBS_mkeficapsule += \
> >   HOSTLDLIBS_mkeficapsule += \
> >   $(shell pkg-config --libs uuid 2> /dev/null || echo "-luuid")
> >   hostprogs-$(CONFIG_TOOLS_MKEFICAPSULE) += mkeficapsule
> > +mkeficapsule-objs := mkeficapsule.o mkeficapsule_parse.o
> >
> >   mkfwumdata-objs := mkfwumdata.o generated/lib/crc32.o
> >   HOSTLDLIBS_mkfwumdata += -luuid
> > diff --git a/tools/eficapsule.h b/tools/eficapsule.h
> > index 6efd07d2eb..71a08b62e6 100644
> > --- a/tools/eficapsule.h
> > +++ b/tools/eficapsule.h
> > @@ -54,6 +54,12 @@ typedef struct {
> >   /* flags */
> >   #define CAPSULE_FLAGS_PERSIST_ACROSS_RESET  0x0001
> >
> > +enum capsule_type {
> > + CAPSULE_NORMAL_BLOB = 0,
> > + CAPSULE_ACCEPT,
> > + CAPSULE_REVERT,
> > +};
> > +
> >   struct efi_capsule_header {
> >   efi_guid_t capsule_guid;
> >   uint32_t header_size;
> > @@ -145,4 +151,112 @@ struct fmp_payload_header_params {
> >   uint32_t fw_version;
> >   };
> >
> > +/**
> > + * struct efi_capsule_params - Capsule parameters
> > + * @image_guid: Guid value of the payload input image
> > + * @image_index: Image index value
> > + * @hardware_instance: Hardware instance to be used for the image
> > + * @fmp: FMP payload header used for storing firmware version
> > + * @monotonic_count: Monotonic count value to be used for signed capsule
> > + * @privkey_file: Path to private key used in capsule signing
> > + * @cert_file: Path to public key certificate used in capsule signing
> > + * @input_file: Path to payload input image
> > + * @capsule_file: Path to the output capsule file
> > + * @oemflags: Oemflags to be populated in the capsule header
> > + * @capsule: Capsule Type, normal or accept or 

Re: [PATCH v2 1/4] tools: mkeficapsule: Add support for parsing capsule params from config file

2024-04-19 Thread Heinrich Schuchardt

On 19.04.24 08:55, Sughosh Ganu wrote:

Add support for specifying the parameters needed for capsule
generation through a config file, instead of passing them through
command-line. Parameters for more than a single capsule file can be
specified, resulting in generation of multiple capsules through a
single invocation of the command.

The config file can be passed to the mkeficapsule tool in such manner

  $ ./tools/mkeficapsule -f 


Please, mention the long option.



Signed-off-by: Sughosh Ganu 
---
  tools/Kconfig  |  15 ++
  tools/Makefile |   1 +
  tools/eficapsule.h | 114 
  tools/mkeficapsule.c   |  87 +
  tools/mkeficapsule_parse.c | 352 +
  5 files changed, 538 insertions(+), 31 deletions(-)
  create mode 100644 tools/mkeficapsule_parse.c

diff --git a/tools/Kconfig b/tools/Kconfig
index 667807b331..0362ca8e45 100644
--- a/tools/Kconfig
+++ b/tools/Kconfig
@@ -104,6 +104,21 @@ config TOOLS_MKEFICAPSULE
  optionally sign that file. If you want to enable UEFI capsule
  update feature on your target, you certainly need this.

+config EFI_CAPSULE_CFG_FILE
+   string "Path to the EFI Capsule Config File"
+   help
+ Path to the EFI capsule config file which provides the
+ parameters needed to build capsule(s). Parameters can be
+ provided for multiple payloads resulting in corresponding
+ capsule images being generated.


This help test does not explain if this is a parameter for binman or
something built into mkeficapsule.

We should not hard code any path inside mkeficapsule.

I can't see the new CONFIG parameters used within the code changes of
this patch. Please, add them into the patches where they are needed.


+
+config EFI_USE_CAPSULE_CFG_FILE
+   bool "Use the config file for generating capsules"
+   help
+ Boolean option used to specify if the EFI capsules are to
+ be generated through parameters specified via the config
+ file or through command line.


Given this help text I would not know if this option changes how
mkeficapsule is built or how binman invokes it.

I would expect that mkeficapsule is always built in a way that a
configuration file can be passed.

Furthermore I would expect binman to invoke mkeficapsule with the
appropriate command line parameters if you have enabled building capsules.

Why do we need this configuration parameter? Just always build
mkeficapsule with support for the -f parameter.

Best regards

Heinrich


+
  menuconfig FSPI_CONF_HEADER
bool "FlexSPI Header Configuration"
help
diff --git a/tools/Makefile b/tools/Makefile
index 6a4280e366..4311f5914f 100644
--- a/tools/Makefile
+++ b/tools/Makefile
@@ -253,6 +253,7 @@ HOSTLDLIBS_mkeficapsule += \
  HOSTLDLIBS_mkeficapsule += \
$(shell pkg-config --libs uuid 2> /dev/null || echo "-luuid")
  hostprogs-$(CONFIG_TOOLS_MKEFICAPSULE) += mkeficapsule
+mkeficapsule-objs := mkeficapsule.o mkeficapsule_parse.o

  mkfwumdata-objs := mkfwumdata.o generated/lib/crc32.o
  HOSTLDLIBS_mkfwumdata += -luuid
diff --git a/tools/eficapsule.h b/tools/eficapsule.h
index 6efd07d2eb..71a08b62e6 100644
--- a/tools/eficapsule.h
+++ b/tools/eficapsule.h
@@ -54,6 +54,12 @@ typedef struct {
  /* flags */
  #define CAPSULE_FLAGS_PERSIST_ACROSS_RESET  0x0001

+enum capsule_type {
+   CAPSULE_NORMAL_BLOB = 0,
+   CAPSULE_ACCEPT,
+   CAPSULE_REVERT,
+};
+
  struct efi_capsule_header {
efi_guid_t capsule_guid;
uint32_t header_size;
@@ -145,4 +151,112 @@ struct fmp_payload_header_params {
uint32_t fw_version;
  };

+/**
+ * struct efi_capsule_params - Capsule parameters
+ * @image_guid: Guid value of the payload input image
+ * @image_index: Image index value
+ * @hardware_instance: Hardware instance to be used for the image
+ * @fmp: FMP payload header used for storing firmware version
+ * @monotonic_count: Monotonic count value to be used for signed capsule
+ * @privkey_file: Path to private key used in capsule signing
+ * @cert_file: Path to public key certificate used in capsule signing
+ * @input_file: Path to payload input image
+ * @capsule_file: Path to the output capsule file
+ * @oemflags: Oemflags to be populated in the capsule header
+ * @capsule: Capsule Type, normal or accept or revert
+ */
+struct efi_capsule_params {
+   efi_guid_t *image_guid;
+   unsigned long image_index;
+   unsigned long hardware_instance;
+   struct fmp_payload_header_params fmp;
+   uint64_t monotonic_count;
+   char *privkey_file;
+   char *cert_file;
+   char *input_file;
+   char *capsule_file;
+   unsigned long oemflags;
+   enum capsule_type capsule;
+};
+
+/**
+ * capsule_with_cfg_file() - Generate capsule from config file
+ * @cfg_file: Path to the config file
+ *
+ * Parse the capsule parameters from the config file and use the
+ * parameters for generating one 

[PATCH v2 1/4] tools: mkeficapsule: Add support for parsing capsule params from config file

2024-04-19 Thread Sughosh Ganu
Add support for specifying the parameters needed for capsule
generation through a config file, instead of passing them through
command-line. Parameters for more than a single capsule file can be
specified, resulting in generation of multiple capsules through a
single invocation of the command.

The config file can be passed to the mkeficapsule tool in such manner

 $ ./tools/mkeficapsule -f 

Signed-off-by: Sughosh Ganu 
---
 tools/Kconfig  |  15 ++
 tools/Makefile |   1 +
 tools/eficapsule.h | 114 
 tools/mkeficapsule.c   |  87 +
 tools/mkeficapsule_parse.c | 352 +
 5 files changed, 538 insertions(+), 31 deletions(-)
 create mode 100644 tools/mkeficapsule_parse.c

diff --git a/tools/Kconfig b/tools/Kconfig
index 667807b331..0362ca8e45 100644
--- a/tools/Kconfig
+++ b/tools/Kconfig
@@ -104,6 +104,21 @@ config TOOLS_MKEFICAPSULE
  optionally sign that file. If you want to enable UEFI capsule
  update feature on your target, you certainly need this.
 
+config EFI_CAPSULE_CFG_FILE
+   string "Path to the EFI Capsule Config File"
+   help
+ Path to the EFI capsule config file which provides the
+ parameters needed to build capsule(s). Parameters can be
+ provided for multiple payloads resulting in corresponding
+ capsule images being generated.
+
+config EFI_USE_CAPSULE_CFG_FILE
+   bool "Use the config file for generating capsules"
+   help
+ Boolean option used to specify if the EFI capsules are to
+ be generated through parameters specified via the config
+ file or through command line.
+
 menuconfig FSPI_CONF_HEADER
bool "FlexSPI Header Configuration"
help
diff --git a/tools/Makefile b/tools/Makefile
index 6a4280e366..4311f5914f 100644
--- a/tools/Makefile
+++ b/tools/Makefile
@@ -253,6 +253,7 @@ HOSTLDLIBS_mkeficapsule += \
 HOSTLDLIBS_mkeficapsule += \
$(shell pkg-config --libs uuid 2> /dev/null || echo "-luuid")
 hostprogs-$(CONFIG_TOOLS_MKEFICAPSULE) += mkeficapsule
+mkeficapsule-objs := mkeficapsule.o mkeficapsule_parse.o
 
 mkfwumdata-objs := mkfwumdata.o generated/lib/crc32.o
 HOSTLDLIBS_mkfwumdata += -luuid
diff --git a/tools/eficapsule.h b/tools/eficapsule.h
index 6efd07d2eb..71a08b62e6 100644
--- a/tools/eficapsule.h
+++ b/tools/eficapsule.h
@@ -54,6 +54,12 @@ typedef struct {
 /* flags */
 #define CAPSULE_FLAGS_PERSIST_ACROSS_RESET  0x0001
 
+enum capsule_type {
+   CAPSULE_NORMAL_BLOB = 0,
+   CAPSULE_ACCEPT,
+   CAPSULE_REVERT,
+};
+
 struct efi_capsule_header {
efi_guid_t capsule_guid;
uint32_t header_size;
@@ -145,4 +151,112 @@ struct fmp_payload_header_params {
uint32_t fw_version;
 };
 
+/**
+ * struct efi_capsule_params - Capsule parameters
+ * @image_guid: Guid value of the payload input image
+ * @image_index: Image index value
+ * @hardware_instance: Hardware instance to be used for the image
+ * @fmp: FMP payload header used for storing firmware version
+ * @monotonic_count: Monotonic count value to be used for signed capsule
+ * @privkey_file: Path to private key used in capsule signing
+ * @cert_file: Path to public key certificate used in capsule signing
+ * @input_file: Path to payload input image
+ * @capsule_file: Path to the output capsule file
+ * @oemflags: Oemflags to be populated in the capsule header
+ * @capsule: Capsule Type, normal or accept or revert
+ */
+struct efi_capsule_params {
+   efi_guid_t *image_guid;
+   unsigned long image_index;
+   unsigned long hardware_instance;
+   struct fmp_payload_header_params fmp;
+   uint64_t monotonic_count;
+   char *privkey_file;
+   char *cert_file;
+   char *input_file;
+   char *capsule_file;
+   unsigned long oemflags;
+   enum capsule_type capsule;
+};
+
+/**
+ * capsule_with_cfg_file() - Generate capsule from config file
+ * @cfg_file: Path to the config file
+ *
+ * Parse the capsule parameters from the config file and use the
+ * parameters for generating one or more capsules.
+ *
+ * Return: None
+ *
+ */
+void capsule_with_cfg_file(const char *cfg_file);
+
+/**
+ * convert_uuid_to_guid() - convert UUID to GUID
+ * @buf:   UUID binary
+ *
+ * UUID and GUID have the same data structure, but their binary
+ * formats are different due to the endianness. See lib/uuid.c.
+ * Since uuid_parse() can handle only UUID, this function must
+ * be called to get correct data for GUID when parsing a string.
+ *
+ * The correct data will be returned in @buf.
+ */
+void convert_uuid_to_guid(unsigned char *buf);
+
+/**
+ * create_empty_capsule() - Generate an empty capsule
+ * @path: Path to the empty capsule file to be generated
+ * @guid: Guid value of the image for which empty capsule is generated
+ * @fw_accept: Flag to specify whether to generate accept or revert capsule
+ *
+ * Generate an empty capsule, either an accept or a revert