[PATCH v2] lib: Export the parsing functions and related data structures of the PLDM library

[PATCH v2] lib: Export the parsing functions and related data structures of the PLDM library

[Shawn.Shao]

From: Shawn Shao <shawn.shao@jaguarmicro.com>

v1 -> v2: Updated the commit message, added a description
	of the changes related to `DeviceUpdateOptionFlags`, etc.

The PLDM library is used to implement firmware upgrades,
but the current library functions only support the
`pldmfw_flash_image` function to complete a fixed
process of parsing, sending data to the backend,
and flashing (allowing users to implement custom
logic using `pldmfw_ops`). However, this poses
significant challenges for device vendors using
PLDM for firmware upgrades.
The following scenarios are not supported:
1. Only using the PLDM parsing functions, as the
   current library does not support this operation.
2. The firmware upgrade process differs from this
   fixed flow (the firmware upgrade process may
   vary across different vendors).
	|-> pldmfw_flash_image
		|-> pldm_parse_image
			|-> pldm_parse_header
			|-> pldm_parse_records
			|-> pldm_parse_components
			-> pldm_verify_header_crc
		|-> pldm_find_matching_record (xxx_match_record)
		|-> pldm_send_package_data (xxx_send_package_data)
		|-> pldm_send_component_tables (xxx_send_package_data)
		|-> pldm_flash_components (xxx_flash_component)
		|-> pldm_finalize_update (xxx_finalize_update)
3. The current PLDM library does not support parsing the
   DeviceUpdateOptionFlags parameter, which is defined in the PLDM
   specification to facilitate the transfer of control information
   between the UA (Update Agent) and the firmware.Please refer to:
   https://www.dmtf.org/sites/default/files/standards/documents
   /DSP0267_1.3.0.pdf P37.

Signed-off-by: Shawn Shao <shawn.shao@jaguarmicro.com>
---
 include/linux/pldmfw.h | 38 +++++++++++++++++++++++++++++++++++++
 lib/pldmfw/pldmfw.c    | 43 +++++-------------------------------------
 2 files changed, 43 insertions(+), 38 deletions(-)

diff --git a/include/linux/pldmfw.h b/include/linux/pldmfw.h
index 0fc831338226..5058a07a5ea4 100644
--- a/include/linux/pldmfw.h
+++ b/include/linux/pldmfw.h
@@ -130,6 +130,42 @@ struct pldmfw {
 	struct device *dev;
 };
 
+/* pldmfw_priv structure used to store details about the PLDM image file as it is
+ * being validated and processed.
+ */
+struct pldmfw_priv {
+	struct pldmfw *context;
+	const struct firmware *fw;
+
+	/* current offset of firmware image */
+	size_t offset;
+
+	struct list_head records;
+	struct list_head components;
+
+	/* PLDM Firmware Package Header */
+	const struct __pldm_header *header;
+	u16 total_header_size;
+
+	/* length of the component bitmap */
+	u16 component_bitmap_len;
+	u16 bitmap_size;
+
+	/* Start of the component image information */
+	u16 component_count;
+	const u8 *component_start;
+
+	/* Start pf the firmware device id records */
+	const u8 *record_start;
+	u8 record_count;
+
+	/* The CRC at the end of the package header */
+	u32 header_crc;
+
+	struct pldmfw_record *matching_record;
+};
+
+
 bool pldmfw_op_pci_match_record(struct pldmfw *context, struct pldmfw_record *record);
 
 /* Operations invoked by the generic PLDM firmware update engine. Used to
@@ -160,6 +196,8 @@ struct pldmfw_ops {
 	int (*finalize_update)(struct pldmfw *context);
 };
 
+int pldm_parse_image(struct pldmfw_priv *data);
+void pldmfw_free_priv(struct pldmfw_priv *data);
 int pldmfw_flash_image(struct pldmfw *context, const struct firmware *fw);
 
 #endif
diff --git a/lib/pldmfw/pldmfw.c b/lib/pldmfw/pldmfw.c
index 54e1809a38fd..cd1698e9c340 100644
--- a/lib/pldmfw/pldmfw.c
+++ b/lib/pldmfw/pldmfw.c
@@ -14,41 +14,6 @@
 
 #include "pldmfw_private.h"
 
-/* Internal structure used to store details about the PLDM image file as it is
- * being validated and processed.
- */
-struct pldmfw_priv {
-	struct pldmfw *context;
-	const struct firmware *fw;
-
-	/* current offset of firmware image */
-	size_t offset;
-
-	struct list_head records;
-	struct list_head components;
-
-	/* PLDM Firmware Package Header */
-	const struct __pldm_header *header;
-	u16 total_header_size;
-
-	/* length of the component bitmap */
-	u16 component_bitmap_len;
-	u16 bitmap_size;
-
-	/* Start of the component image information */
-	u16 component_count;
-	const u8 *component_start;
-
-	/* Start pf the firmware device id records */
-	const u8 *record_start;
-	u8 record_count;
-
-	/* The CRC at the end of the package header */
-	u32 header_crc;
-
-	struct pldmfw_record *matching_record;
-};
-
 /**
  * pldm_check_fw_space - Verify that the firmware image has space left
  * @data: pointer to private data
@@ -341,6 +306,7 @@ pldm_parse_one_record(struct pldmfw_priv *data,
 		return err;
 
 	record_len = get_unaligned_le16(&__record->record_len);
+	record->device_update_flags = get_unaligned_le32(&__record->device_update_flags);
 	record->package_data_len = get_unaligned_le16(&__record->package_data_len);
 	record->version_len = __record->version_len;
 	record->version_type = __record->version_type;
@@ -540,7 +506,7 @@ static int pldm_verify_header_crc(struct pldmfw_priv *data)
  * Loops through and clears all allocated memory associated with each
  * allocated descriptor, record, and component.
  */
-static void pldmfw_free_priv(struct pldmfw_priv *data)
+void pldmfw_free_priv(struct pldmfw_priv *data)
 {
 	struct pldmfw_component *component, *c_safe;
 	struct pldmfw_record *record, *r_safe;
@@ -566,7 +532,7 @@ static void pldmfw_free_priv(struct pldmfw_priv *data)
 		kfree(record);
 	}
 }
-
+EXPORT_SYMBOL(pldmfw_free_priv);
 /**
  * pldm_parse_image - parse and extract details from PLDM image
  * @data: pointer to private data
@@ -581,7 +547,7 @@ static void pldmfw_free_priv(struct pldmfw_priv *data)
  *
  * Returns: zero on success, or a negative error code on failure.
  */
-static int pldm_parse_image(struct pldmfw_priv *data)
+int pldm_parse_image(struct pldmfw_priv *data)
 {
 	int err;
 
@@ -602,6 +568,7 @@ static int pldm_parse_image(struct pldmfw_priv *data)
 
 	return pldm_verify_header_crc(data);
 }
+EXPORT_SYMBOL(pldm_parse_image);
 
 /* these are u32 so that we can store PCI_ANY_ID */
 struct pldm_pci_record_id {
-- 
2.34.1

Re: [PATCH v2] lib: Export the parsing functions and related data structures of the PLDM library

[Jacob Keller]

On 9/9/2024 12:17 AM, Shawn.Shao wrote:
> From: Shawn Shao <shawn.shao@jaguarmicro.com>
> 
> v1 -> v2: Updated the commit message, added a description
> 	of the changes related to `DeviceUpdateOptionFlags`, etc.
> 
> The PLDM library is used to implement firmware upgrades,
> but the current library functions only support the
> `pldmfw_flash_image` function to complete a fixed
> process of parsing, sending data to the backend,
> and flashing (allowing users to implement custom
> logic using `pldmfw_ops`). However, this poses
> significant challenges for device vendors using
> PLDM for firmware upgrades.
> The following scenarios are not supported:
> 1. Only using the PLDM parsing functions, as the
>    current library does not support this operation.
> 2. The firmware upgrade process differs from this
>    fixed flow (the firmware upgrade process may
>    vary across different vendors).
> 	|-> pldmfw_flash_image
> 		|-> pldm_parse_image
> 			|-> pldm_parse_header
> 			|-> pldm_parse_records
> 			|-> pldm_parse_components
> 			-> pldm_verify_header_crc
> 		|-> pldm_find_matching_record (xxx_match_record)
> 		|-> pldm_send_package_data (xxx_send_package_data)
> 		|-> pldm_send_component_tables (xxx_send_package_data)
> 		|-> pldm_flash_components (xxx_flash_component)
> 		|-> pldm_finalize_update (xxx_finalize_update)
> 3. The current PLDM library does not support parsing the
>    DeviceUpdateOptionFlags parameter, which is defined in the PLDM
>    specification to facilitate the transfer of control information
>    between the UA (Update Agent) and the firmware.Please refer to:
>    https://www.dmtf.org/sites/default/files/standards/documents
>    /DSP0267_1.3.0.pdf P37.
> 

Thanks! I'd prefer the DeviceUpdateOptionFlags to be separate, but I
think the changes are good.

Reviewed-by: Jacob Keller <jacob.e.keller@intel.com>

答复: [PATCH v2] lib: Export the parsing functions and related data structures of the PLDM library

[shawn.shao]

> > On 9/9/2024 12:17 AM, Shawn.Shao wrote:
> > From: Shawn Shao <shawn.shao@jaguarmicro.com>
> >
> > v1 -> v2: Updated the commit message, added a description
> >       of the changes related to `DeviceUpdateOptionFlags`, etc.
> >
> > The PLDM library is used to implement firmware upgrades,
> > but the current library functions only support the
> > `pldmfw_flash_image` function to complete a fixed
> > process of parsing, sending data to the backend,
> > and flashing (allowing users to implement custom
> > logic using `pldmfw_ops`). However, this poses
> > significant challenges for device vendors using
> > PLDM for firmware upgrades.
> > The following scenarios are not supported:
> > 1. Only using the PLDM parsing functions, as the
> >    current library does not support this operation.
> > 2. The firmware upgrade process differs from this
> >    fixed flow (the firmware upgrade process may
> >    vary across different vendors).
> >       |-> pldmfw_flash_image
> >               |-> pldm_parse_image
> >                       |-> pldm_parse_header
> >                       |-> pldm_parse_records
> >                       |-> pldm_parse_components
> >                       -> pldm_verify_header_crc
> >               |-> pldm_find_matching_record (xxx_match_record)
> >               |-> pldm_send_package_data (xxx_send_package_data)
> >               |-> pldm_send_component_tables
> (xxx_send_package_data)
> >               |-> pldm_flash_components (xxx_flash_component)
> >               |-> pldm_finalize_update (xxx_finalize_update)
> > 3. The current PLDM library does not support parsing the
> >    DeviceUpdateOptionFlags parameter, which is defined in the PLDM
> >    specification to facilitate the transfer of control information
> >    between the UA (Update Agent) and the firmware.Please refer to:
> >    https://www.dmtf.org/sites/default/files/standards/documents
> >    /DSP0267_1.3.0.pdf P37.
> >
> 
> Thanks! I'd prefer the DeviceUpdateOptionFlags to be separate, but I
> think the changes are good.
> 
> Reviewed-by: Jacob Keller <jacob.e.keller@intel.com>

Firstly, thanks for your reply and guidance so quickly.

1. I will separate the device_update_flags into another patch for submission, as you suggested.
2. I have another question I’d like to ask you. For support of higher versions of the PLDM library, version 2.0/Version 3.0 supports ComponentOpaqueData/ComponentOpaqueDataLength`, and requires adjustments to the `__pldmfw_component_info` structure. I would like to continue supplementing this adjustment(submit other patches). I’m not sure if you agree with this, thank you.

Please refer to：
https://www.dmtf.org/sites/default/files/standards/documents/DSP0267_1.2.0.pdf P42

Thank you very much！

Re: 答复: [PATCH v2] lib: Export the parsing functions and related data structures of the PLDM library

[Jacob Keller]

On 9/9/2024 7:08 PM, shawn.shao wrote:
>>> On 9/9/2024 12:17 AM, Shawn.Shao wrote:
>>> From: Shawn Shao <shawn.shao@jaguarmicro.com>
>>>
>>> v1 -> v2: Updated the commit message, added a description
>>>       of the changes related to `DeviceUpdateOptionFlags`, etc.
>>>
>>> The PLDM library is used to implement firmware upgrades,
>>> but the current library functions only support the
>>> `pldmfw_flash_image` function to complete a fixed
>>> process of parsing, sending data to the backend,
>>> and flashing (allowing users to implement custom
>>> logic using `pldmfw_ops`). However, this poses
>>> significant challenges for device vendors using
>>> PLDM for firmware upgrades.
>>> The following scenarios are not supported:
>>> 1. Only using the PLDM parsing functions, as the
>>>    current library does not support this operation.
>>> 2. The firmware upgrade process differs from this
>>>    fixed flow (the firmware upgrade process may
>>>    vary across different vendors).
>>>       |-> pldmfw_flash_image
>>>               |-> pldm_parse_image
>>>                       |-> pldm_parse_header
>>>                       |-> pldm_parse_records
>>>                       |-> pldm_parse_components
>>>                       -> pldm_verify_header_crc
>>>               |-> pldm_find_matching_record (xxx_match_record)
>>>               |-> pldm_send_package_data (xxx_send_package_data)
>>>               |-> pldm_send_component_tables
>> (xxx_send_package_data)
>>>               |-> pldm_flash_components (xxx_flash_component)
>>>               |-> pldm_finalize_update (xxx_finalize_update)
>>> 3. The current PLDM library does not support parsing the
>>>    DeviceUpdateOptionFlags parameter, which is defined in the PLDM
>>>    specification to facilitate the transfer of control information
>>>    between the UA (Update Agent) and the firmware.Please refer to:
>>>    https://www.dmtf.org/sites/default/files/standards/documents
>>>    /DSP0267_1.3.0.pdf P37.
>>>
>>
>> Thanks! I'd prefer the DeviceUpdateOptionFlags to be separate, but I
>> think the changes are good.
>>
>> Reviewed-by: Jacob Keller <jacob.e.keller@intel.com>
> 
> Firstly, thanks for your reply and guidance so quickly.
> 
> 1. I will separate the device_update_flags into another patch for submission, as you suggested.

Appreciated, thanks.

> 2. I have another question I’d like to ask you. For support of higher versions of the PLDM library, version 2.0/Version 3.0 supports ComponentOpaqueData/ComponentOpaqueDataLength`, and requires adjustments to the `__pldmfw_component_info` structure. I would like to continue supplementing this adjustment(submit other patches). I’m not sure if you agree with this, thank you.
> 

I'm not opposed to extending the library. However, we need to be careful
that any changes do not break existing files. Do the new fields come as
part of reserved sections of the previous data structures? Or do we need
to identify the file format version and use an alternative structure for
newer version? Or is this data something that was already there which my
library code simply ignored?

It looks like this is also further complicated because the extra opaque
data is itself variable length and follows the variable length version
string.

I think this will be tricky to add, but we should be able to treat it as
a separate structure, maybe __pldmfw_component_info_opaque_data, and we
can check for it based on some sort of version format in the header? We
can't simply append to __pldmfw_component_info because it already has a
variable length structure.

As long as care is taken to ensure that existing files do not break, I
see no issues with supporting the additions of future versions of the
standard. Hopefully the PLDM standards body properly implemented
version/format in the header?

It looks like there is 0x1 for the initial 1.0 release, and then 0x2 for
the Downstream Devices support, and 0x3 for the component opaque data.

We currently only support revision 0x1, but extending this shouldn't be
too tricky. Care will have to be taken to ensure the code is structured
to minimize exposing revision changes to as few parts of the parsing as
necessary.

Thanks for your interested in the library!

> Please refer to：
> https://www.dmtf.org/sites/default/files/standards/documents/DSP0267_1.2.0.pdf P42
> 
> Thank you very much！