Skip to content

Conversation

@spargaon
Copy link
Collaborator

@spargaon spargaon commented Nov 14, 2025

Adding support for Seamless Firmware Servicing (SFS) protocol. Implement code to support Get Firmware Version and Update Firmware using mctpRaw option of pldmtool.

@spargaon
Copy link
Collaborator Author

pldmtool_test.txt

Test pldmtool commands with the changes.

@spargaon
Copy link
Collaborator Author

Few of the considerations:

Since MCTP layer extracts its header in a response and only sends the payload to the upper layer (e.g. libpldm), there is no IC bit and Message Type information in the payload. I think the SFS protocol probably needs to retain that information as part of its protocol (e.g. a byte). Since the IC bit information is missing, parsing code for SFS response has to either a) disregard checksum and verification of the checksum or b) explicitly expect a checksum in a response and validate the checksum. Currently b) is coded.

Since the Message Type information is not available, the parsing code needs to decide whether the response is for either
a PLDM request or a SFS request, perhaps by considering presence of four bytes of IANA number.

I think existing pldmtool mctpRaw testcases need to be exercised to make sure there is no regression with this change.

@spargaon spargaon force-pushed the fwdeve125145_review branch 2 times, most recently from 80f97ac to e09e62b Compare November 18, 2025 22:05
@spargaon spargaon requested a review from mahkurap November 18, 2025 22:55
@spargaon spargaon force-pushed the fwdeve125145_review branch 2 times, most recently from a017b64 to 022450c Compare November 24, 2025 12:09
Copy link
Collaborator

@mahkurap mahkurap left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

couple of comments. please check.

@spargaon spargaon force-pushed the fwdeve125145_review branch from 022450c to 85d4717 Compare November 24, 2025 21:04
@spargaon spargaon requested a review from mahkurap November 24, 2025 22:02
std::vector<std::unique_ptr<CommandInterface>> commands;
}

/*
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

File Scope:

  • Please follow commit message 50(subject(/72(body) Rule,
  • Please make sure rin clang-format tool.
  • Upstream-status: Inappropriate - Not required here . Is used for patches only

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Except for clang-format tool, where I ran into errors, completed this.

*/

inline constexpr unsigned int DEVHDRSIZE = 7;
inline constexpr unsigned int FSFIELD = 4;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since we are using c++23, inline for constants is generally not required because:

constexpr variables at namespace scope have internal linkage by default (since C++17), so they don’t cause multiple definition issues across translation units.

@spargaon spargaon force-pushed the fwdeve125145_review branch from 85d4717 to d24badd Compare December 5, 2025 00:08
@spargaon
Copy link
Collaborator Author

spargaon commented Dec 5, 2025

@ojayanth, I have addressed most of the comments except:
Please make sure run clang-format tool.

Also, function pldm_edac_crc32() does not accept Use std::span as input, its definition is different. Had a compilation error...

| ../../../../../../workspace/sources/pldm/pldmtool/pldmtool.cpp:214:93: error: cannot convert 'std::span' to 'const void*' | 214 | uint32_t csField = to_big_endian(static_cast<uint32_t>(pldm_edac_crc32(std::span(fData)))); | | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ | | | | | std::span

@spargaon
Copy link
Collaborator Author

spargaon commented Dec 5, 2025

root@congo-0057:/tmp# pldmtool mctpraw --help
send an MCTP raw request and print response
Usage: pldmtool mctpRaw [OPTIONS]

Options:
-h,--help Print this help message and exit
-m,--mctp_eid UINT MCTP endpoint ID
-v,--verbose
-n,--retry-count UINT Number of retry when PLDM request message is failed
-e,--network-id UINT REQUIRED
MCTP NetworkId
-d,--data UINT ... REQUIRED raw MCTP data
-i,--file-in TEXT Read in the file to be sent
-o,--file-out TEXT Write out the received file
-c,--checksum Append/Validate checksum
-p,--prealloc-tag use pre-allocated MCTP tag for this request

Example: pldmtool mctpRaw -m 21 -e 1
--data 0x7F 0x00 0x00 0x0E 0x78 0x80 0x02
--file-in req.bin --file-out resp.bin --checksum

@spargaon
Copy link
Collaborator Author

spargaon commented Dec 5, 2025

Verified with the new code, when the same command is issued:
root@congo-0057:/tmp# pldmtool mctpraw -m 17 -e 1 --data 0x7F 0x00 0x00 0x0E 0x78 0x80 0x02 -i sfs.pkg -c

metadata (e.g. AMD IANA number and file size in Big Endian format) and checksum (also in Big Endian format)
matches with the same in prior coded version of pldmtool

e.g. Some initial bytes with the current vesion
pldmtool: Tx: ff 00 00 0e 78 80 02 00 00 53 80 0a 29 8a 5f fe 02 6d 21 c6 d2 79 1a b2 2e 14 b1 24
checksum 3a 8d cd 87

and this with the prior version
pldmtool: Tx: ff 00 00 0e 78 80 02 00 00 53 80 0a 29 8a 5f fe 02 6d 21 c6 d2 79 1a
checksum: 3a 8d cd 87

@spargaon spargaon requested a review from ojayanth December 8, 2025 22:39
@spargaon spargaon force-pushed the fwdeve125145_review branch 3 times, most recently from 57fdeb2 to 2b742e6 Compare December 19, 2025 04:23
Enhance the `mctpRaw` subcommand with options to read request
data from a file (`--file-in`), write response data to a file
(`--file-out`), and append or validate checksum (`--checksum`).
These additions improve usability for large payloads and
ensure data integrity during raw MCTP transactions.

Contents of the file specified in option --file-in are appended
to the contents of the option --data.

Signed-off-by: Shirish Pargaonkar <Shirish.Pargaonkar@amd.com>
@spargaon spargaon force-pushed the fwdeve125145_review branch from 2b742e6 to 03b51fb Compare December 19, 2025 13:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants