-
Notifications
You must be signed in to change notification settings - Fork 97
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.
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
Support for extended FRU data for memory modules #147
Closed
Closed
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
According to IPMI FRU specification, value 0 for the length means that the fields is empty, while value 1 indicates 'end-of-fields'. Thus, value 1 is always reserved and can't be used. Which means that the minimum size of a string is 2. This is not checked here.
Also, the maximum length of plain ASCII string in IPMI is 31 bytes. This is 1 byte shorter than, for instance, "DDR4-2400 16GiB 64-bit ECC RDIMM". Things like "LPDDR4-2400 16GiB 64-bit ECC SORDIMM 72b" will get even more fiercely truncated. Something must be invented to overcome that. I'd propose converting the string to all-uppercase and encoding it using 6-bit ASCII (see FRU spec). That would give us 41 bytes max. We could also save on using non-SI units for size (
GB
instead ofGiB
), on saying64B
instead of64-BIT
, and on shortening long module types to, say these:MINIRDIMM
MINIUDIMM
SORDIMM72
SODIMM16
SODIMM32
That way the longest string will be
LPDDR4-2400 16GB 32B NON-ECC MINIRDIMM
which is 38 characters.It must be checked though whether or not the BMC firmwares can properly decode 6-bit ASCII.
If we ditch Mini and SO modules, that are, AFAIK, rarely used in OpenPOWER systems, and also use empty string instead of 'NON-ECC', then we can fit into 31 bytes with the longest
LPDDR4-2400 16GB 32b ECC LRDIMM
, and that's the good old plain ASCII+Latin1 encoding.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we use something other than straight ascii, what are the chances that the BMC will actually display it in any useful way? Might need to ask in the openbmc forum what they support. Have you by chance tried this change on a system running OpenBMC (or SuperMicro or AMI)? Where/how is the data displayed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OpenBMC always shows manufacture and pretty names as text.
We have checked this solution on our VESNIN server.
WebUI example:
Inventory list:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@artemsen, I believe @dcrowell77 was referring to my proposal to use IPMI FRU's 6-bit ASCII encoding. I have checked and am quite sure now that openbmc/ipmi-fru-parser does not support any encodings beyond binary (code 0) and plain ASCII (code 3 / 0xC0). Can't say for AMI. We of course could add support for 6-bit ASCII to openbmc, but hardly to MegaRAC or any other commercial products.
This means that our best way is to use plain ASCII and ditch support for the uncommon module types.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mea culpa. I misread the spec. The type/length byte has 6 bits for data length, which gives maximum string length of 63, not 31. I don't know where I took that '31' from. Nothing needs to be done in regard to string length.