libpldmresponder comments

[Deepak Kodihalli]

On 05/10/19 3:01 AM, Supreeth Venkatesh wrote:
> On Fri, 2019-10-04 at 09:28 -0500, Deepak Kodihalli wrote:
>> On 04/10/19 7:26 PM, Supreeth Venkatesh wrote:
>>> Hello Deepak/all,
>>>
>>> Sorry for the late comments/feedback.
>>
>> No problem, thanks for the feedback Supreeth!
>>
>>> I was looking at porting libpldmresponder and libpldm to an Arm
>>> based
>>> platform.
>>
>> libpldm - sure, it is meant to be portable across OpenBMC, but
>> libpldmresponder is not :)
>>
>>> These are  few observations with the design, some of them were
>>> discussed
>>> during OSF OpenBMC Hackathon, summarizing them here:
>>>
>>> Assumption was that libpldmresponder can be easily ported to
>>> Host/other
>>> Satellite/Service Management controller,
>>
>> libpldmresponder was not meant to be ported outside OpenBMC - it's
>> the
>> OpenBMC implementation of the BMC as a PLDM responder.
> 
>  From our previous discussion, I was under the impression you wanted
> libPLDMresponder to get publicity and ported over as well.
> If I was mistaken, the rest of discussion is moot.

Well, there has to be an aspect of the stack that transforms PLDM 
concepts to OpenBMC concepts, and vice versa. That's libpldmresponder 
today. However, I think we're talking about a layer above this (which 
can probably sit in libpldm itself). I believe that's what you're 
suggesting below as well.

>>
>>> However, in the current design,
>>>
>>>   1. libpldmresponder implements standard Commands/APIs defined by
>>> PLDM
>>>      specifications in C++.
>>>   2. libpldmresponder PDR, BIOS config structures are defined by
>>> PLDM
>>>      specifications, However, the library uses Json format, thus
>>> making
>>>      JSON parser mandatory for
>>> Host/Service management controller firmware.
>>>
>>>   3. libpldmresponder has DBUS/other OpenBMC implementation
>>> dependencies,
>>>      thus making portability harder. >  4. I guess the expectation
>>> when we started with the design was that
>>>      there will be one **single** library which will handle all pldm
>>>      requests/responses and
>>>
>>> upper layer application/Daemon will call the APIs provided by PLDM
>>> library to implement use cases as they fit.
>>
>> https://github.com/openbmc/docs/blob/master/designs/pldm-stack.md
>> makes
>> the distinction between a portable libpldm (which handles the
>> protocol
>> encode/decode), and an OpenBMC specific responder implementation.
>>
>>>   5. Libpldm also has dependencies on OpenBMC structures/DBUS
>>> objects,
>>>      making it a little harder to port.
>>
>> No, libpldm has no OpenBMC dependencies. It for example is used by
> If I remember right, in libpldm, for getting date and time, there was
> dependency on OpenBMC object to get it from the timer as opposed to
> upper level app calling into libpldm and providing date and time?
> Now that I look at the code in repo, it does not seem to be the case.
> Please ignore my rant.
> 
>> IBM's
>> host firmware stacks today without any code changes. Can you point me
>> to
>> where you see D-Bus dependencies in libpldm? Do you just mean you
>> find
>> this hard to build outside an OpenBMC environment?
> 
> 
>>
>>> Please let me know, how I can help fix some of these, so that it
>>> is
>>> easily portable.
>>
>> No change should be needed to libpldm code, it should already be
>> portable like noted earlier. As far as the libpldmresponder is
>> considered, like we discussed at the OSFC, one thing we could do is
>> to
>> write a responder API layer, the implementation of which is platform
>> (for eg OpenBMC/ARM) specific. Would you like to propose a design
>> update
>> for this to the existing document? I think we need to understand the
>> usefulness of such a layer though. I mean for example if you consider
>> a
>> PLDM command along the lines of ReadSensor, such a sensor maybe a D-
>> Bus
>> object on OpenBMC and something else on the ARM platform, those
>> specifics *must* be implemented on the platform, so what would the
>> portable layer consist of? Those details would be good to capture in
>> the
>> design doc.
> Ok. Thanks for the example command.
> Let's take the example of "GetSensorReading" as specified by one of the
> PLDM specification,
> 
> Use case: BMC is trying to get the current reading from a platform
> smart (MCTP enabled) sensor.
> Request is:
> GetSensorReading (uint16 sensorID, bool8 rearmEventStatus)

Just to clarify, the examples I was talking about, and what 
libpldmresponder today is meant to serve, is the case where the BMC is 
the final PLDM responder. Another example - say a requester wants to 
reboot the BMC. The native API for the same is a D-Bus property, so at 
some layer, a PLDM SetEffecter to reboot the BMC needs to be converted 
to a D-Bus operation - that's what libpldmresponder does.

> Request Message to the smart MCTP enabled sensor (not including headers
> and other field for illustration only) will be somthing like this
> GetSensorReading | Sensor ID | rearmEventStatus
> 
> Response will contain the following fields:
> enum8 completionCode,
> enum8 sensorDataSize,
> enum8 sensorOperationalState,
> bool8 sensorEventMessageEnable,
> enum8 presentState,
> enum8 previousState,
> enum8 eventState,
> var   presentReading
> These fields will be filled in by smart MCTP enabled sensor or
> microcontroller(MCTP endpoint) which controls the sensor.
> 
> Hence for this use case, my current thinking is that PLDM library will
> provide the standard template for GetSensorReading API as a function
> pointer.

I'm not sure I followed this (the standard template part) entirely, but 
I can wait for details in the design doc update.

> The implementation of this GetSensorReading in openBMC and in smart
> sensor software/firmware will be different.
> To elaborate a little further, the implementation of this API in
> OpenBMC for this particular use case is to fill in the PLDM request
> fields and send it over the  to the smart sensor over MCTP over sensor
> specific physical media and get the response. (or can use DBUS
> specifics to get the response, if possible)
> 
> The smart sensor implementation of GetSensorReading will be different
> and it will be to parse PLDM request and then to fill in the response
> fields and send back the PLDM message over MCTP over sensor specific
> physical media.
> 
> So, my thinking is to have a wrapper with function pointers, which will
> have all standard  commands defined by the PLDM specification.
> This wrapper can then be ported across management controller software
> and devices alike.

So the wrapper will not have any handlers of incoming PLDM requests, 
right? It expects platforms to provide handlers?

> I am thinking from the specification view point, but from your email it
> looks like to you want merge DBUS and PLDM concept. please confirm.

libpldmresponder is the sink of an incoming message meant intended to be 
addressed by the BMC as the PLDM responder, so at that level OpenBMC 
specifics are expected to be seen. Although I think you're suggesting 
(and I like the concept in general, like I mentioned at the OSFC) a 
layer above the platform specific responder implementation, which 
provides hooks for platforms to provide their specific implementations 
of a PLDM command. Please let me know if I've misunderstood what you're 
suggesting.

> I can add more details when I propose a design.

Sounds good!

> 
>>
>>> Thanks,
>>>
>>> Supreeth
>>>
>>> IMPORTANT NOTICE: The contents of this email and any attachments
>>> are
>>> confidential and may also be privileged. If you are not the
>>> intended
>>> recipient, please notify the sender immediately and do not disclose
>>> the
>>> contents to any other person, use it for any purpose, or store or
>>> copy
>>> the information in any medium. Thank you.
>>
>>
>