[Skiboot] [PATCH V6 04/21] core/pldm: Add PLDM responder support

[Christophe Lombard]

Le 12/04/2023 à 16:45, Frederic Barrat a écrit :
>
>
> On 13/09/2022 12:26, Christophe Lombard wrote:
>> PLDM defines data representations and commands that abstract the 
>> platform
>> management hardware.
>>
>> A PLDM Terminus (or responder) is defined as the point of communication
>> termination for PLDM messages and the PLDM functions associated with 
>> those
>> messages.
>> A PLDM terminus is not required to process more than one request at a 
>> time
>> (that is, it can be "single threaded" and does not have to accept and 
>> act
>> on new requests until it has finished responding to any previous 
>> request).
>>
>> Some PLDM control and discovery requests (PLDM_TYPE = PLDM_BASE) are
>> mandatory a PLDM terminus has to answer.
>>
>> These following mandatory PLDM command codes for PLDM messaging control
>> and discovery will be defined in next patches.
>>      GetTID          0x02
>>      GetPLDMVersion  0x03
>>      GetPLDMTypes    0x04
>>      GetPLDMCommands 0x05
>>
>> Signed-off-by: Christophe Lombard <clombard at linux.vnet.ibm.com>
>> ---
>>   core/pldm/Makefile.inc     |   2 +-
>>   core/pldm/pldm-mctp.c      |  18 ++++-
>>   core/pldm/pldm-responder.c | 145 +++++++++++++++++++++++++++++++++++++
>>   core/pldm/pldm.h           |   6 ++
>>   4 files changed, 169 insertions(+), 2 deletions(-)
>>   create mode 100644 core/pldm/pldm-responder.c
>>
>> diff --git a/core/pldm/Makefile.inc b/core/pldm/Makefile.inc
>> index c878ef1f..dbca102a 100644
>> --- a/core/pldm/Makefile.inc
>> +++ b/core/pldm/Makefile.inc
>> @@ -7,7 +7,7 @@ SUBDIRS += $(PLDM_DIR)
>>   CPPFLAGS += -I$(SRC)/pldm/libpldm/
>>   CPPFLAGS += -I$(SRC)/pldm/ibm/libpldm/
>>   -PLDM_OBJS = pldm-mctp.o
>> +PLDM_OBJS = pldm-mctp.o pldm-responder.o
>>     PLDM = $(PLDM_DIR)/built-in.a
>>   $(PLDM): $(PLDM_OBJS:%=$(PLDM_DIR)/%)
>> diff --git a/core/pldm/pldm-mctp.c b/core/pldm/pldm-mctp.c
>> index 832b767a..51dbad0a 100644
>> --- a/core/pldm/pldm-mctp.c
>> +++ b/core/pldm/pldm-mctp.c
>> @@ -75,6 +75,14 @@ static int handle_message_rx(uint8_t eid, const 
>> uint8_t *buf, int len)
>>           return OPAL_EMPTY;
>>       }
>>   +    switch (rx.hdrinf.msg_type) {
>> +    case PLDM_REQUEST:
>> +        return pldm_responder_handle_request(&rx);
>> +    break;
>> +    default:
>> +    break;
>
>
> The indentation of "break" should be further.
> Print an error in the default case, just in case? With prlog_once() to 
> avoid flooding the log
>

sounds good.

>
>
>> diff --git a/core/pldm/pldm.h b/core/pldm/pldm.h
>> index a006d1f4..3add1bac 100644
>> --- a/core/pldm/pldm.h
>> +++ b/core/pldm/pldm.h
>> @@ -20,6 +20,8 @@ void printbuf(const char *name, const char *msg, 
>> int len);
>>   #define BMC_EID  8
>>   #define HOST_EID 9
>>   +#define PKT_SIZE(x) (sizeof(struct pldm_msg_hdr) + sizeof(x))
>
>
> This is where I'm thinking we could add the +1 byte mentioned in the 
> previous patch. And maybe provide macros to easily access the header, 
> payload and MCTP type byte.
>
> Regarding the name, it is my understanding that MCTP can break a 
> single PLDM message into several packets. Here, we're allocating the 
> full PLDM message. Maybe rename to PLDM_MSG_SIZE(x) or someting 
> similar, avoiding 'PKT' to avoid confusion?
>
>   Fred


That could be an idea, but don't forget that some callers are not using 
PKT_SIZE.
They need to allocate a local buffer.