Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix chunk_seq_no wrap issue. #2952

Merged
merged 4 commits into from
Feb 7, 2025
Merged

Fix chunk_seq_no wrap issue. #2952

merged 4 commits into from
Feb 7, 2025
+489 −14

Conversation

Li-Aaron
Copy link
Contributor

@Li-Aaron Li-Aaron commented Jan 13, 2025 •
edited
Loading

Fix #2875

@Li-Aaron Li-Aaron closed this Jan 13, 2025
@Li-Aaron Li-Aaron deleted the chunk-wrap branch January 13, 2025 02:16
@Li-Aaron Li-Aaron restored the chunk-wrap branch January 13, 2025 02:16
@Li-Aaron Li-Aaron reopened this Jan 13, 2025
@Li-Aaron Li-Aaron marked this pull request as draft January 13, 2025 02:59
@Li-Aaron Li-Aaron force-pushed the chunk-wrap branch 3 times, most recently from 3c503f8 to 2f8dd0c Compare January 13, 2025 06:07
@Li-Aaron Li-Aaron marked this pull request as ready for review January 14, 2025 00:44
jyao1
jyao1 reviewed Jan 14, 2025
View reviewed changes
library/spdm_requester_lib/libspdm_req_send_receive.c
spdm_context->connection_info.capability.data_transfer_size,
spdm_context->local_context.capability.sender_data_transfer_size);

max_chunk_data_transfer_size =
Copy link
Member

Choose a reason for hiding this comment

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

we should also consult max_spdm_msg_size.

Copy link
Contributor

Choose a reason for hiding this comment

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

The Responder's max_spdm_msg_size is already checked in libspdm_send_spdm_request. Checking the Requester's max_spdm_msg_size doesn't make sense, as the message already exists at this point and so must be small enough to fit.

Copy link
Member

Choose a reason for hiding this comment

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

Ok. If it is already checked, just add a comment to say : it is already checked.

jyao1
jyao1 reviewed Jan 14, 2025
View reviewed changes
library/spdm_responder_lib/libspdm_rsp_chunk_get.c
min_data_transfer_size = LIBSPDM_MIN(
spdm_context->connection_info.capability.data_transfer_size,
spdm_context->local_context.capability.sender_data_transfer_size);

/* Fail if exceed max chunks */
max_chunk_data_transfer_size =
Copy link
Member

Choose a reason for hiding this comment

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

we should also consult max_spdm_msg_size.

Copy link
Contributor

Choose a reason for hiding this comment

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

Same feedback as in https://github.com/DMTF/libspdm/pull/2952/files#r1943830275.

Li-Aaron

This comment was marked as spam.

Sign in to view

@Li-Aaron Li-Aaron marked this pull request as draft January 14, 2025 01:58
@Li-Aaron Li-Aaron force-pushed the chunk-wrap branch 2 times, most recently from 18184ef to ca07ba9 Compare January 16, 2025 06:09
@Li-Aaron
Copy link
Contributor Author

The newly added test case for chunk get in requester will cost ~90s if print to console or ~10s if print to file.

@Li-Aaron Li-Aaron marked this pull request as ready for review January 16, 2025 09:47
@steven-bellock
Copy link
Contributor

When the Requester receives the first CHUNK_RESPONSE it can use LargeMessageSize to determine whether ChunkSeqNo will wrap, assuming the size of each chunk is equal to the Requester's DataTransferSize. A similar calculation can take place on the Responder side when it receives the first CHUNK_SEND.

@Li-Aaron
Copy link
Contributor Author

Li-Aaron commented Jan 20, 2025 •
edited
Loading

When the Requester receives the first CHUNK_RESPONSE it can use LargeMessageSize to determine whether ChunkSeqNo will wrap, assuming the size of each chunk is equal to the Requester's DataTransferSize. A similar calculation can take place on the Responder side when it receives the first CHUNK_SEND.

In Spec line 812:

ChunkSize shall be a value where the total size of CHUNK_SEND or CHUNK_RESPONSE shall be from MinDataTransferSize to the DataTransferSize of the receiving SPDM endpoint.

Which means the Responder could send chunk messages with ChunkSize less than Requester's DataTransferSize.

@steven-bellock
Copy link
Contributor

Are you saying that even if the check passes then the ChunkSeqNo may still wrap? That is correct. However to determine that the Requester has to retrieve 65536 messages to determine that, whereas if the check fails then the Requester knows from the beginning the ChunkSeqNo would wrap and not spend the time retrieving all those messages only for ChunkSeqNo to wrap. It's a performance improvement. The original check is still needed.

@Li-Aaron
Copy link
Contributor Author

Got it, it make sense to add additional check to improve the performance.

@Li-Aaron Li-Aaron marked this pull request as draft January 21, 2025 02:40
@Li-Aaron Li-Aaron marked this pull request as ready for review January 21, 2025 03:09
@jyao1 jyao1 requested a review from steven-bellock February 5, 2025 01:52
@steven-bellock
Copy link
Contributor

Re-opened two comments.

@jyao1 jyao1 merged commit 18a2654 into DMTF:main Feb 7, 2025
97 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jyao1 jyao1 jyao1 left review comments

@steven-bellock steven-bellock steven-bellock approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

ChunkSeqNo should not wrap
3 participants
@Li-Aaron @steven-bellock @jyao1