ManageabilityPkg/IpmiSmbiosTransferDxe: add support of sending SMBIOS table to BMC #187

nicklela · 2024-09-04T08:09:58Z

Add IpmiSmbiosTransferDxe driver to ManageabilityPkg. This enables the ability of sending SMBIOS table to BMC by using IPMI Blob transfer protocol.

I also fixed several build issues observed on GCC5 build.

Add missing "EFIAPI" to fix GCC5 build failure. Signed-off-by: Nickle Wang <[email protected]>

Add missing PCD: PcdIpmiSsifSmbusSlaveAddr. Signed-off-by: Nickle Wang <[email protected]>

nhivp · 2024-09-05T03:21:41Z

I'm getting the following error when testing on Ampere Mt. Jade platform.

IpmiBlobTransferSendIpmi: SendDataSize: 00
Data:
IpmiBlobTransferSendIpmi: IpmiSendDataSize: 04
Data: CFC20000
IpmiBlobTransferSendIpmi: IPMI Response:
IpmiBlobTransferSendIpmi: ResponseDataSize: 0A
Data: 00CFC200A47801000000
CalculateCrc16Ccitt: CRC-16-CCITT 78A4
CalculateCrc16Ccitt: CRC-16-CCITT E10
IpmiBlobTransferSendIpmi: Inputs:
IpmiBlobTransferSendIpmi: SendDataSize: 04
Data: 00000000
IpmiBlobTransferSendIpmi: IpmiSendDataSize: 0A
Data: CFC20001100E00000000
IpmiBlobTransferSendIpmi: IPMI Response:
IpmiBlobTransferSendIpmi: ResponseDataSize: 0E
Data: 00CFC20081132F736D62696F7300
CalculateCrc16Ccitt: CRC-16-CCITT 1381
CalculateCrc16Ccitt: CRC-16-CCITT A8CB
IpmiBlobTransferSendIpmi: Inputs:
IpmiBlobTransferSendIpmi: SendDataSize: 0A
Data: 02002F736D62696F7300
IpmiBlobTransferSendIpmi: IpmiSendDataSize: 10
Data: CFC20002CBA802002F736D62696F7300
IpmiBlobTransferSendIpmi: IPMI Response:
IpmiBlobTransferSendIpmi: ResponseDataSize: 08
Data: 00CFC200D0C52125
CalculateCrc16Ccitt: CRC-16-CCITT C5D0
IpmiSmbiosTransferSendTables: Failure writing to blob: Invalid Parameter
ERROR: C80000002:V03080000 I0 1E2F6B56-4675-446B-9C0A-1EB66E50C840 BFB13728
PROGRESS CODE: V03051001 I0

Features/ManageabilityPkg/ManageabilityPkg.dec

Features/ManageabilityPkg/Universal/IpmiSmbiosTransferDxe/IpmiSmbiosTransferDxe.c

Features/ManageabilityPkg/Universal/IpmiSmbiosTransferDxe/IpmiSmbiosTransferDxe.inf

Features/ManageabilityPkg/Universal/IpmiSmbiosTransferDxe/IpmiSmbiosTransferDxe.c

Features/ManageabilityPkg/ManageabilityPkg.dec

Features/ManageabilityPkg/Universal/IpmiSmbiosTransferDxe/IpmiSmbiosTransferDxe.inf

Features/ManageabilityPkg/Universal/IpmiSmbiosTransferDxe/IpmiSmbiosTransferDxe.c

nhivp · 2024-09-05T09:38:07Z

I'm getting the following error when testing on Ampere Mt. Jade platform.

IpmiBlobTransferSendIpmi: SendDataSize: 00
Data:
IpmiBlobTransferSendIpmi: IpmiSendDataSize: 04
Data: CFC20000
IpmiBlobTransferSendIpmi: IPMI Response:
IpmiBlobTransferSendIpmi: ResponseDataSize: 0A
Data: 00CFC200A47801000000
CalculateCrc16Ccitt: CRC-16-CCITT 78A4
CalculateCrc16Ccitt: CRC-16-CCITT E10
IpmiBlobTransferSendIpmi: Inputs:
IpmiBlobTransferSendIpmi: SendDataSize: 04
Data: 00000000
IpmiBlobTransferSendIpmi: IpmiSendDataSize: 0A
Data: CFC20001100E00000000
IpmiBlobTransferSendIpmi: IPMI Response:
IpmiBlobTransferSendIpmi: ResponseDataSize: 0E
Data: 00CFC20081132F736D62696F7300
CalculateCrc16Ccitt: CRC-16-CCITT 1381
CalculateCrc16Ccitt: CRC-16-CCITT A8CB
IpmiBlobTransferSendIpmi: Inputs:
IpmiBlobTransferSendIpmi: SendDataSize: 0A
Data: 02002F736D62696F7300
IpmiBlobTransferSendIpmi: IpmiSendDataSize: 10
Data: CFC20002CBA802002F736D62696F7300
IpmiBlobTransferSendIpmi: IPMI Response:
IpmiBlobTransferSendIpmi: ResponseDataSize: 08
Data: 00CFC200D0C52125
CalculateCrc16Ccitt: CRC-16-CCITT C5D0
IpmiSmbiosTransferSendTables: Failure writing to blob: Invalid Parameter
ERROR: C80000002:V03080000 I0 1E2F6B56-4675-446B-9C0A-1EB66E50C840 BFB13728
PROGRESS CODE: V03051001 I0

It could be a bug in the IpmiBlobTransferSendIpmi (). I think the ResponseData is an optional parameter. See the following patch. It works on my system.

--- a/Features/ManageabilityPkg/Universal/IpmiBlobTransferDxe/IpmiBlobTransferDxe.c
+++ b/Features/ManageabilityPkg/Universal/IpmiBlobTransferDxe/IpmiBlobTransferDxe.c
@@ -110,7 +110,9 @@ IpmiBlobTransferSendIpmi (
   UINT32                     IpmiResponseDataSize;
   IPMI_BLOB_TRANSFER_HEADER  Header;

-  if (((SendDataSize > 0) && (SendData == NULL)) || (ResponseData == NULL) || (ResponseDataSize == NULL)) {
+  if (  ((SendDataSize > 0) && (SendData == NULL))
+     || ((ResponseData == NULL) && (ResponseDataSize == NULL) && (*ResponseDataSize > 0)))
+  {
     return EFI_INVALID_PARAMETER;
   }

Features/ManageabilityPkg/Universal/IpmiSmbiosTransferDxe/IpmiSmbiosTransferDxe.c

nhivp · 2024-10-21T07:58:23Z

Hi @nicklela, for BMC-HOST synchronization, what will happen if users update the BMC firmware without preserving the configuration data while the HOST is still powered on? I think BMC will not be able to tell HOST to resend the SMBIOS table in that case.

Adding support to transfer SMBIOS binary blob to the BMC by using the IpmiBlobTransfer protocol. Signed-off-by: Nick Ramirez <[email protected]> Co-authored-by: Nickle Wang <[email protected]>

Add IpmiSmbiosTransferDxe to ManageabilityPkg. Signed-off-by: Nickle Wang <[email protected]>

nicklela · 2024-10-21T08:43:19Z

Hi @nicklela, for BMC-HOST synchronization, what will happen if users update the BMC firmware without preserving the configuration data while the HOST is still powered on? I think BMC will not be able to tell HOST to resend the SMBIOS table in that case.

When BMC works in that way, BIOS has to send smbios table on every boot because there is no defined interface between BMC and BIOS to know this. So probably we can define PcdSendSmbiosOnChanged default to FALSE?

nicklela · 2024-10-22T01:00:20Z

@nhivp @changab I think I addressed the most of review comments. Could you please help me to review it again?

nhivp · 2024-10-22T04:11:56Z

Hi @nicklela, for BMC-HOST synchronization, what will happen if users update the BMC firmware without preserving the configuration data while the HOST is still powered on? I think BMC will not be able to tell HOST to resend the SMBIOS table in that case.

When BMC works in that way, BIOS has to send smbios table on every boot because there is no defined interface between BMC and BIOS to know this. So probably we can define PcdSendSmbiosOnChanged default to FALSE?

I think we need a clean solution for this corner case, rather than setting the PCD to always transfer the SMBIOS which impacts boot time performance. I'm unsure whether this is a gap in the IPMI blob transfer specification. @changab any thoughts?

nhivp · 2024-10-22T06:24:21Z

I'm getting the following error when testing on Ampere Mt. Jade platform.

IpmiBlobTransferSendIpmi: SendDataSize: 00
Data:
IpmiBlobTransferSendIpmi: IpmiSendDataSize: 04
Data: CFC20000
IpmiBlobTransferSendIpmi: IPMI Response:
IpmiBlobTransferSendIpmi: ResponseDataSize: 0A
Data: 00CFC200A47801000000
CalculateCrc16Ccitt: CRC-16-CCITT 78A4
CalculateCrc16Ccitt: CRC-16-CCITT E10
IpmiBlobTransferSendIpmi: Inputs:
IpmiBlobTransferSendIpmi: SendDataSize: 04
Data: 00000000
IpmiBlobTransferSendIpmi: IpmiSendDataSize: 0A
Data: CFC20001100E00000000
IpmiBlobTransferSendIpmi: IPMI Response:
IpmiBlobTransferSendIpmi: ResponseDataSize: 0E
Data: 00CFC20081132F736D62696F7300
CalculateCrc16Ccitt: CRC-16-CCITT 1381
CalculateCrc16Ccitt: CRC-16-CCITT A8CB
IpmiBlobTransferSendIpmi: Inputs:
IpmiBlobTransferSendIpmi: SendDataSize: 0A
Data: 02002F736D62696F7300
IpmiBlobTransferSendIpmi: IpmiSendDataSize: 10
Data: CFC20002CBA802002F736D62696F7300
IpmiBlobTransferSendIpmi: IPMI Response:
IpmiBlobTransferSendIpmi: ResponseDataSize: 08
Data: 00CFC200D0C52125
CalculateCrc16Ccitt: CRC-16-CCITT C5D0
IpmiSmbiosTransferSendTables: Failure writing to blob: Invalid Parameter
ERROR: C80000002:V03080000 I0 1E2F6B56-4675-446B-9C0A-1EB66E50C840 BFB13728
PROGRESS CODE: V03051001 I0

It could be a bug in the IpmiBlobTransferSendIpmi (). I think the ResponseData is an optional parameter. See the following patch. It works on my system.

--- a/Features/ManageabilityPkg/Universal/IpmiBlobTransferDxe/IpmiBlobTransferDxe.c
+++ b/Features/ManageabilityPkg/Universal/IpmiBlobTransferDxe/IpmiBlobTransferDxe.c
@@ -110,7 +110,9 @@ IpmiBlobTransferSendIpmi (
   UINT32                     IpmiResponseDataSize;
   IPMI_BLOB_TRANSFER_HEADER  Header;

-  if (((SendDataSize > 0) && (SendData == NULL)) || (ResponseData == NULL) || (ResponseDataSize == NULL)) {
+  if (  ((SendDataSize > 0) && (SendData == NULL))
+     || ((ResponseData == NULL) && (ResponseDataSize == NULL) && (*ResponseDataSize > 0)))
+  {
     return EFI_INVALID_PARAMETER;
   }

Can you apply this fix? The Mt. Jade platform does not work without this fix.

IpmiBlobTransferWrite() is calling the IpmiBlobTransferSendIpmi() with NULL ResponseData

nicklela · 2024-10-24T01:42:55Z

if ( ((SendDataSize > 0) && (SendData == NULL))
|| ((ResponseData == NULL) && (ResponseDataSize == NULL) && (*ResponseDataSize > 0)))

@nhivp

I think the condition statement would be:

if (((SendDataSize > 0) && (SendData == NULL)) 
|| ((ResponseData == NULL) && (((ResponseDataSize != NULL) && (*ResponseDataSize > 0))))) {

When ResponseDataSize is NULL, we are not able to get its value by *ResponseDataSize

Please correct me if I misunderstand your comment.

nhivp · 2024-10-24T04:14:02Z

@nhivp

I think the condition statement would be:
if (((SendDataSize > 0) && (SendData == NULL)) 
|| ((ResponseData == NULL) && (((ResponseDataSize != NULL) && (*ResponseDataSize > 0))))) {
When ResponseDataSize is NULL, we are not able to get its value by *ResponseDataSize

Please correct me if I misunderstand your comment.

Ah, you're right. Please incorporate this change to the pull request.

While calling IpmiBlobTransferSendIpmi(), it is expected that there is no response data for certain command. Enhance this function to accept NULL response data parameter. Signed-off-by: Nickle Wang <[email protected]>

nicklela · 2024-10-24T06:45:08Z

Done. I add additional commit to handle the case when response data is optional.

@nhivp
I think the condition statement would be:
if (((SendDataSize > 0) && (SendData == NULL)) 
|| ((ResponseData == NULL) && (((ResponseDataSize != NULL) && (*ResponseDataSize > 0))))) {
When ResponseDataSize is NULL, we are not able to get its value by *ResponseDataSize
Please correct me if I misunderstand your comment.
Ah, you're right. Please incorporate this change to the pull request.

nhivp

Thanks @nicklela. It works perfectly on my Mt. Jade system.

nicklela · 2024-10-24T08:49:14Z

Thanks @nicklela. It works perfectly on my Mt. Jade system.

@changab if latest commit looks good to you, please help to merge it. Thanks :)

changab

I went through the changes and I have no problem with this commit.

nicklela added 2 commits September 4, 2024 15:49

ManageabilityPkg/PldmSmbiosTransferDxe: fix GCC5 build error

7223f4c

Add missing "EFIAPI" to fix GCC5 build failure. Signed-off-by: Nickle Wang <[email protected]>

ManageabilityPkg/IpmiProtocolSmm: fix build error

6caea43

Add missing PCD: PcdIpmiSsifSmbusSlaveAddr. Signed-off-by: Nickle Wang <[email protected]>

nicklela requested a review from changab as a code owner September 4, 2024 08:09

tianocore-assign-reviewers bot assigned nicklela Sep 4, 2024

tianocore-assign-reviewers bot requested a review from abdattar September 4, 2024 15:58

nhivp reviewed Sep 5, 2024

View reviewed changes

changab reviewed Sep 5, 2024

View reviewed changes

Features/ManageabilityPkg/Universal/IpmiSmbiosTransferDxe/IpmiSmbiosTransferDxe.c Show resolved Hide resolved

Features/ManageabilityPkg/ManageabilityPkg.dec Outdated Show resolved Hide resolved

nhivp reviewed Sep 5, 2024

View reviewed changes

Features/ManageabilityPkg/Universal/IpmiSmbiosTransferDxe/IpmiSmbiosTransferDxe.c Outdated Show resolved Hide resolved

nicklela added 2 commits October 21, 2024 16:33

ManageabilityPkg/IpmiSmbiosTransferDxe: Add SMBIOS transfer support

a2b112e

Adding support to transfer SMBIOS binary blob to the BMC by using the IpmiBlobTransfer protocol. Signed-off-by: Nick Ramirez <[email protected]> Co-authored-by: Nickle Wang <[email protected]>

ManageabilityPkg: add IpmiSmbiosTransferDxe

f423d1c

Add IpmiSmbiosTransferDxe to ManageabilityPkg. Signed-off-by: Nickle Wang <[email protected]>

nicklela force-pushed the smbios-table-transfer branch from 5544ef0 to f423d1c Compare October 21, 2024 08:33

ManageabilityPkg/IpmiBlobTransferDxe: response data is optional

b639285

While calling IpmiBlobTransferSendIpmi(), it is expected that there is no response data for certain command. Enhance this function to accept NULL response data parameter. Signed-off-by: Nickle Wang <[email protected]>

nhivp approved these changes Oct 24, 2024

View reviewed changes

nicklela requested a review from changab October 24, 2024 08:48

changab approved these changes Oct 24, 2024

View reviewed changes

changab merged commit 920dbcb into tianocore:master Oct 24, 2024
1 check passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ManageabilityPkg/IpmiSmbiosTransferDxe: add support of sending SMBIOS table to BMC #187

ManageabilityPkg/IpmiSmbiosTransferDxe: add support of sending SMBIOS table to BMC #187

nicklela commented Sep 4, 2024

nhivp commented Sep 5, 2024

nhivp commented Sep 5, 2024

nhivp commented Oct 21, 2024

nicklela commented Oct 21, 2024

nicklela commented Oct 22, 2024

nhivp commented Oct 22, 2024

nhivp commented Oct 22, 2024

nicklela commented Oct 24, 2024

nhivp commented Oct 24, 2024

nicklela commented Oct 24, 2024

nhivp left a comment

nicklela commented Oct 24, 2024

changab left a comment

ManageabilityPkg/IpmiSmbiosTransferDxe: add support of sending SMBIOS table to BMC #187

ManageabilityPkg/IpmiSmbiosTransferDxe: add support of sending SMBIOS table to BMC #187

Conversation

nicklela commented Sep 4, 2024

nhivp commented Sep 5, 2024

nhivp commented Sep 5, 2024

nhivp commented Oct 21, 2024

nicklela commented Oct 21, 2024

nicklela commented Oct 22, 2024

nhivp commented Oct 22, 2024

nhivp commented Oct 22, 2024

nicklela commented Oct 24, 2024

nhivp commented Oct 24, 2024

nicklela commented Oct 24, 2024

nhivp left a comment

Choose a reason for hiding this comment

nicklela commented Oct 24, 2024

changab left a comment

Choose a reason for hiding this comment