commit | 92f6c3ca74a56e27689e8dd74d144321c59cd9b7 | [log] [tgz] |
---|---|---|
author | Andrew Jeffery <andrew@aj.id.au> | Thu Apr 13 15:50:10 2023 +0930 |
committer | Andrew Jeffery <andrew@aj.id.au> | Thu Apr 13 22:48:30 2023 +0930 |
tree | ce073ef8731aa52b66a4ab63976cd33e4aacb452 | |
parent | 6ad4dc0ff0a4ea5f4be986bb80cb16f764c3a460 [diff] |
platform: Fix LE encoding of present_reading The following portion of the test implementation demonstrates significant confusion in the handling of endiannes: ``` uint32_t presentReading = 305441741; ... sensorData->present_reading[3] = static_cast<uint8_t>(htole32(presentReading) & (0x000000ff)); sensorData->present_reading[2] = static_cast<uint8_t>((htole32(presentReading) & (0x0000ff00)) >> 8); sensorData->present_reading[1] = static_cast<uint8_t>((htole32(presentReading) & (0x00ff0000)) >> 16); sensorData->present_reading[0] = static_cast<uint8_t>((htole32(presentReading) & (0xff000000)) >> 24); ``` First up, plugging 305441741 into a calculator an converting it to hexadecimal representation reveals the the value 0x1234ABCD. So let's just state that directly, as it's an easily recognisable pattern. From there the test implementation tried to encode the value 0x1234ABCD in little-endian format into the present_reading byte buffer. `struct pldm_sensor_event_numeric_sensor_state` is allocated as if the present_reading member was a flexible array member, extending its range to 4 bytes, taking care of the space allocation concerns (modulo undefined behaviour). Analysing the implementation we have to start with the following concerns: 1. The host representation of the value 0x1234ABCD 2. The architectural representation of the value 0x1234ABCD 3. The behaviour of htole32() On point 1, value constants defined in the source are represented in the usual positional notation convention: Most-Significant (MS) digit on the left, Least-Significant (LS) digit on the right. This source representation is independent of any architectural representation in the storage for the value. As a forward reference, any operations on values in the source are in accordance with the value's type, and do not concern endianness, only the positional notation. The architectural representation is required because we have a byte-addressable memory system and a multi-byte object. As above, when dealt with as coherent type the object's value behaves as if it has the properties of positional notation. The ordering of the byte quantities composing the value only become visible when viewing the storage of the object as byte sequence: ``` Big Endian MS Little Endian +-------------- 12 --------------+ | +----------- 34 -----------+ | | | +-------- AB --------+ | | | | | +----- CD -----+ | | | | | | | LS | | | | v v v v || v v v v | MS | 12 34 AB CD | LS || LS | CD AB 34 12 | MS | Byte Value | | 0 1 2 3 | || | 0 1 2 3 | | Byte Address ``` On point 3, the post-condition of htole32() is that if the storage of the 32-bit unsigned integer result is viewed as a byte array then the value represented in the array is encoded as little-endian. Whether any re-arrangement of the argument's representation is required is a function of the architecture of the host. If the host is big-endian, a byte-swap will occur, and inspecting the value as its canonical type will yield a value significantly different from that which was provided as an argument. On the other hand, if the host is little-endian, then no byte-swap will occur, as the value is already encoded in the required fashion. The value of the result when used as its canonical type is the same as the input value. Concretely: +-----------------------+-----------------------------------+ | | Result | | Source |------------------+----------------| | Operation | BE Architecture | LE Achitecture | |-----------------------|------------------+----------------| | `htole32(0x1234ABCD)` | 0xCDAB3412 | 0x1234ABCD | | `htobe32(0x1234ABCD)` | 0x1234ABCD | 0xCDAB3412 | +-----------------------+------------------+----------------+ As a result, any operations on values, aside from endian conversion, must always be done in terms of the host endianness. Failing that the behaviour is beyond the semantics of the C specification as there are multiple possible results. The test implementation in question violates this requirement. The following sub-expression (and others) is an operation whose operands include a value of specific endianness and a value of host endianness: ``` htole32(presentReading) & (0x000000ff) ``` Having covered the problematic sub-expression we need to expand our view to the assignment into the `present_reading` member of `struct pldm_sensor_event_numeric_sensor_state`: ``` sensorData->present_reading[3] = static_cast<uint8_t>(htole32(presentReading) & (0x000000ff)); ... ``` We find that the values selected by the sub-expression are then encoded into an array that is later interpreted to contain a 32-bit little-endian value. Under the assumption that we're running on a little-endian host, htole32() becomes an identity function, yielding presentReading. Rewriting: ``` sensorData->present_reading[3] = static_cast<uint8_t>(presentReading & (0x000000ff)); ... ``` The mask thus selects the Least-Sigificant-Byte (LSB) of `presentReading`, however this LSB value is inserted into the present_reading array at the index of the Most-Significant-Byte (MSB) of the little-endian encoded value (index 3). Throw away all the trickery and just encode the value into the `present_reading` member directly, in the correct fashion. Also fix the implementation of decode_numeric_sensor_data() which demonstrated similar confusion. Signed-off-by: Andrew Jeffery <andrew@aj.id.au> Change-Id: I71990aa594da054bdef0ce42b137bfe44502831b
This is a library which deals with the encoding and decoding of PLDM messages. It should be possible to use this library by projects other than OpenBMC, and hence certain constraints apply to it:
Source files are named according to the PLDM Type, for eg base.[h/c], fru.[h/c], etc.
Given a PLDM command "foo", the library will provide the following API: For the Requester function:
encode_foo_req() - encode a foo request decode_foo_resp() - decode a response to foo
For the Responder function:
decode_foo_req() - decode a foo request encode_foo_resp() - encode a response to foo
The library also provides API to pack and unpack PLDM headers.
Need meson
and ninja
. Alternatively, source an OpenBMC ARM/x86 SDK.
meson setup builddir && ninja -C builddir
The simplest way of running the tests is as described by the meson man page:
meson setup builddir && meson test -C builddir
This will support OEM or vendor-specific functions and semantic information. Following directory structure has to be used:
libpldm |---- include/libpldm | |---- oem/<oem_name>/libpldm | |----<oem based .h files> |---- src | |---- oem/<oem_name> | |----<oem based .c files> |---- tests | |---- oem/<oem_name> | |----<oem based test files>
<oem_name> - This folder must be created with the name of the OEM/vendor in lower case.
Header files & source files having the oem functionality for the libpldm library should be placed under the respective folder hierarchy as mentioned in the above figure. They must be adhering to the rules mentioned under the libpldm section above.
Once the above is done a meson option has to be created in libpldm/meson_options.txt
with its mapped compiler flag to enable conditional compilation.
For consistency would recommend using "oem-<oem_name>".
The libpldm/meson.build
and the corresponding source file(s) will need to incorporate the logic of adding its mapped compiler flag to allow conditional compilation of the code.
The pldm requester API's are present in src/requester
folder and they are intended to provide API's to interact with the desired underlying transport layer to send/receive pldm messages.
NOTE : In the current state, the requester API's in the repository only works with specific transport mechanism & these are going to change in future & probably aren't appropriate to be writing code against.