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

[shawn . shao]

> 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@xxxxxxxxxxxxxxx>
> >>>
> >>> 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@xxxxxxxxx>
> >
> > 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!
> 

Thank you very much for your guidance and suggestions.

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