(no subject)

[Bills, Jason M]

I just added a comment to a github discussion about the IPMI SEL and thought I should share it here as well:

I have been working on a proof-of-concept to move the IPMI SEL entries out of D-Bus into journald instead.

Since journald allows custom metadata for log entries, I've thought of having the SEL message logged to the journal and using metadata to store the necessary IPMI info associated with the entry. Here is an example of logging a type 0x02 system event entry to journald:

sd_journal_send("MESSAGE=%s", message.c_str(),
                            "PRIORITY=%i", selPriority,
                            "MESSAGE_ID=%s", selMessageId,
                            "IPMI_SEL_RECORD_ID=%d", recordId,
                            "IPMI_SEL_RECORD_TYPE=%x", selSystemType,
                            "IPMI_SEL_GENERATOR_ID=%x", genId,
                            "IPMI_SEL_SENSOR_PATH=%s", path.c_str(),
                            "IPMI_SEL_EVENT_DIR=%x", assert,
                            "IPMI_SEL_DATA=%s", selDataStr,
                            NULL);
Using journald should allow for scaling to more SEL entries which should also enable us to support more generic IPMI behavior such as the Add SEL command.

-Jason

Re:

[Brad Bishop]

> On Aug 28, 2018, at 1:34 PM, Bills, Jason M <jason.m.bills@intel.com> wrote:
> 
> I just added a comment to a github discussion about the IPMI SEL and thought I should share it here as well:

Can you send a link to the issue?

> 
> I have been working on a proof-of-concept to move the IPMI SEL entries out of D-Bus into journald instead.
> 
> Since journald allows custom metadata for log entries, I've thought of having the SEL message logged to the journal and using metadata to store the necessary IPMI info associated with the entry. Here is an example of logging a type 0x02 system event entry to journald:
> 
> sd_journal_send("MESSAGE=%s", message.c_str(),
>                            "PRIORITY=%i", selPriority,
>                            "MESSAGE_ID=%s", selMessageId,
>                            "IPMI_SEL_RECORD_ID=%d", recordId,
>                            "IPMI_SEL_RECORD_TYPE=%x", selSystemType,
>                            "IPMI_SEL_GENERATOR_ID=%x", genId,
>                            "IPMI_SEL_SENSOR_PATH=%s", path.c_str(),
>                            "IPMI_SEL_EVENT_DIR=%x", assert,
>                            "IPMI_SEL_DATA=%s", selDataStr,
>                            NULL);
> Using journald should allow for scaling to more SEL entries which should also enable us to support more generic IPMI behavior such as the Add SEL command.

A design point of OpenBMC from day one was to not design it around IPMI.
At a glance this feels counter to that goal.

I’m not immediately opposed to moving our error logs out of DBus, but can you provide
an extendible abstraction?  Not everyone uses SEL, or IPMI even.  At a minimum please
drop the letters ‘ipmi’ and ‘sel’ :-) from the base design, and save those for something
that translates to IPMI-speak.

As some background, our systems tend towards fewer ‘error logs’ with much more data per
log (4-16k), and yes I admit the current design is biased towards that and does not scale
when we approach 1000s of small SEL entries.

thx - brad

> 
> -Jason

RE:

[Bills, Jason M]

Here is a link to the issue: https://github.com/openbmc/openbmc/issues/3283#issuecomment-414361325.

The main things that started this proof-of-concept are that we have requirements to be fully IPMI compliant and to support 4000+ SEL entries.  Our attempts to scale the D-Bus logs to that level were not successful, so we started considering directly accessing journald as an alternative.

So far, I've been focused only on IPMI SEL, so I hadn't considered extending the change to non-IPMI error logs; however, these IPMI SEL entries should still fit in well as a subset of all other error logs which could also be moved to the journal.

My goal is to align with the OpenBMC design and keep anything IPMI-related isolated only to things that care about IPMI.  My thinking was that the metadata is a bit like background info, so it is a good place to hide data that only matters to the minority, such as the IPMI-specific data.  With this, the IPMI SEL logs can be included among all the existing error logs but still have the metadata for additional IPMI stuff that doesn't matter for anyone else.

So, for writing logs:
A. non-IPMI error logs can be written as normal
B. IPMI SEL entries are written with the IPMI-specific metadata populated

For reading logs:
A. non-IPMI readers see IPMI SEL entries as normal text logs
B. IPMI readers dump just the IPMI SEL entries and get the associated IPMI-specific info from the metadata

Thanks,
-Jason

-----Original Message-----
From: Brad Bishop <bradleyb@fuzziesquirrel.com> 
Sent: Tuesday, August 28, 2018 10:59 AM
To: Bills, Jason M <jason.m.bills@intel.com>
Cc: openbmc@lists.ozlabs.org
Subject: Re: 



> On Aug 28, 2018, at 1:34 PM, Bills, Jason M <jason.m.bills@intel.com> wrote:
> 
> I just added a comment to a github discussion about the IPMI SEL and thought I should share it here as well:

Can you send a link to the issue?

> 
> I have been working on a proof-of-concept to move the IPMI SEL entries out of D-Bus into journald instead.
> 
> Since journald allows custom metadata for log entries, I've thought of having the SEL message logged to the journal and using metadata to store the necessary IPMI info associated with the entry. Here is an example of logging a type 0x02 system event entry to journald:
> 
> sd_journal_send("MESSAGE=%s", message.c_str(),
>                            "PRIORITY=%i", selPriority,
>                            "MESSAGE_ID=%s", selMessageId,
>                            "IPMI_SEL_RECORD_ID=%d", recordId,
>                            "IPMI_SEL_RECORD_TYPE=%x", selSystemType,
>                            "IPMI_SEL_GENERATOR_ID=%x", genId,
>                            "IPMI_SEL_SENSOR_PATH=%s", path.c_str(),
>                            "IPMI_SEL_EVENT_DIR=%x", assert,
>                            "IPMI_SEL_DATA=%s", selDataStr,
>                            NULL);
> Using journald should allow for scaling to more SEL entries which should also enable us to support more generic IPMI behavior such as the Add SEL command.

A design point of OpenBMC from day one was to not design it around IPMI.
At a glance this feels counter to that goal.

I’m not immediately opposed to moving our error logs out of DBus, but can you provide an extendible abstraction?  Not everyone uses SEL, or IPMI even.  At a minimum please drop the letters ‘ipmi’ and ‘sel’ :-) from the base design, and save those for something that translates to IPMI-speak.

As some background, our systems tend towards fewer ‘error logs’ with much more data per log (4-16k), and yes I admit the current design is biased towards that and does not scale when we approach 1000s of small SEL entries.

thx - brad

> 
> -Jason

Re:

[Brad Bishop]

> On Aug 28, 2018, at 7:26 PM, Bills, Jason M <jason.m.bills@intel.com> wrote:
> 
> Here is a link to the issue: https://github.com/openbmc/openbmc/issues/3283#issuecomment-414361325.
> 
> The main things that started this proof-of-concept are that we have requirements to be fully IPMI compliant and to support 4000+ SEL entries.  Our attempts to scale the D-Bus logs to that level were not successful, so we started considering directly accessing journald as an alternative.
> 
> So far, I've been focused only on IPMI SEL, so I hadn't considered extending the change to non-IPMI error logs; however, these IPMI SEL entries should still fit in well as a subset of all other error logs which could also be moved to the journal.
> 
> My goal is to align with the OpenBMC design and keep anything IPMI-related isolated only to things that care about IPMI.  

But it seems like you are proposing that every application that wants to make
a log needs to have the logic to translate its internal data model to IPMI speak,
so it can make a journal call with all the IPMI metadata populated.  Am I
understanding correctly?  That doesn’t seem aligned with keeping IPMI isolated.

A concrete example - phosphor-hwmon.  How do you intend to figure out something
like IPMI_SEL_SENSOR_PATH in the phosphor-hwmon application?  Actually it would
help quite a bit to understand how each of the fields in your sample below would
be determined by an arbitrary dbus application (like phosphor-hwmon).

Further, if you expand this approach to further log formats other than SEL,
won’t the applications become a mess of translation logic from the applications
data mode <-> log format in use?

> My thinking was that the metadata is a bit like background info, so it is a good place to hide data that only matters to the minority, such as the IPMI-specific data.  With this, the IPMI SEL logs can be included among all the existing error logs but still have the metadata for additional IPMI stuff that doesn't matter for anyone else.
> 
> So, for writing logs:
> A. non-IPMI error logs can be written as normal
> B. IPMI SEL entries are written with the IPMI-specific metadata populated
> 
> For reading logs:
> A. non-IPMI readers see IPMI SEL entries as normal text logs
> B. IPMI readers dump just the IPMI SEL entries and get the associated IPMI-specific info from the metadata

I’d rather have a single approach that works for everyone; although, I’m
not sure how that would look.

> 
> Thanks,
> -Jason

This is called top posting, please try to avoid when using the mail-list.
It makes threaded conversation hard to follow and respond to.  thx.

> 
> -----Original Message-----
> From: Brad Bishop <bradleyb@fuzziesquirrel.com> 
> Sent: Tuesday, August 28, 2018 10:59 AM
> To: Bills, Jason M <jason.m.bills@intel.com>
> Cc: openbmc@lists.ozlabs.org
> Subject: Re: 
> 
> 
> 
>> On Aug 28, 2018, at 1:34 PM, Bills, Jason M <jason.m.bills@intel.com> wrote:
>> 
>> I just added a comment to a github discussion about the IPMI SEL and thought I should share it here as well:
> 
> Can you send a link to the issue?
> 
>> 
>> I have been working on a proof-of-concept to move the IPMI SEL entries out of D-Bus into journald instead.
>> 
>> Since journald allows custom metadata for log entries, I've thought of having the SEL message logged to the journal and using metadata to store the necessary IPMI info associated with the entry. Here is an example of logging a type 0x02 system event entry to journald:
>> 
>> sd_journal_send("MESSAGE=%s", message.c_str(),
>>                           "PRIORITY=%i", selPriority,
>>                           "MESSAGE_ID=%s", selMessageId,
>>                           "IPMI_SEL_RECORD_ID=%d", recordId,
>>                           "IPMI_SEL_RECORD_TYPE=%x", selSystemType,
>>                           "IPMI_SEL_GENERATOR_ID=%x", genId,
>>                           "IPMI_SEL_SENSOR_PATH=%s", path.c_str(),
>>                           "IPMI_SEL_EVENT_DIR=%x", assert,
>>                           "IPMI_SEL_DATA=%s", selDataStr,
>>                           NULL);
>> Using journald should allow for scaling to more SEL entries which should also enable us to support more generic IPMI behavior such as the Add SEL command.
> 
> A design point of OpenBMC from day one was to not design it around IPMI.
> At a glance this feels counter to that goal.
> 
> I’m not immediately opposed to moving our error logs out of DBus, but can you provide an extendible abstraction?  Not everyone uses SEL, or IPMI even.  At a minimum please drop the letters ‘ipmi’ and ‘sel’ :-) from the base design, and save those for something that translates to IPMI-speak.
> 
> As some background, our systems tend towards fewer ‘error logs’ with much more data per log (4-16k), and yes I admit the current design is biased towards that and does not scale when we approach 1000s of small SEL entries.
> 
> thx - brad
> 
>> 
>> -Jason

Re: Re:

[Ed Tanous]

On 09/04/2018 01:46 PM, Brad Bishop wrote:
> 
> But it seems like you are proposing that every application that wants to make
> a log needs to have the logic to translate its internal data model to IPMI speak,
> so it can make a journal call with all the IPMI metadata populated.  Am I
> understanding correctly?  That doesn’t seem aligned with keeping IPMI isolated.
> 

I think a key here is that not all logs will be implicitly converted to 
IPMI logs.  Having them be identical was the design that we started 
with, and abandoned because IPMI has some requirements that don't 
cleanly map from a standard syslog/text style to IPMI.


> A concrete example - phosphor-hwmon.  How do you intend to figure out something
> like IPMI_SEL_SENSOR_PATH in the phosphor-hwmon application?  Actually it would
> help quite a bit to understand how each of the fields in your sample below would
> be determined by an arbitrary dbus application (like phosphor-hwmon).

I'm not really understanding the root of the question.  If 
phosphor-hwmon is generating a threshold crossing log that stemmed from 
the /xyz/openbmc_project/sensors/my_super_awesome_temp_sensor, then it 
would simply fill that path into the IPMI_SEL_SENSOR_PATH field.  This 
is the same kind of mapping that the associations produce today, but 
captured in journald instead of the mapper.

Our thinking was that we could build either a static library, or a dbus 
daemon to simplify producing IPMI logs.  Because of the IPMI 
requirements around unique record ids, right now we're leaning toward 
the dbus interface with a single daemon responsible for IPMI SEL creation.
While technically it could be a part of phosphor-logging, we really want 
it to be easily removable for future platforms that have no need for 
IPMI, so the thought at this time it to keep it separate.

> 
> Further, if you expand this approach to further log formats other than SEL,
> won’t the applications become a mess of translation logic from the applications
> data mode <-> log format in use?
> 

I'm not really following this question.  Are there other binary log 
formats that we expect to come in the future that aren't text based, and 
could just be a journald translation?  So far as I know, IPMI SEL is the 
only one on my road map that has weird requirements, and needs some 
translation.  I don't expect it to be a mess, and I'm running under the 
assumption that _most_ daemons won't care about or support IPMI given 
its limitations.
You're right, this isn't intended to be a general solution for all 
binary logging formats, it's intended to be a short term hack while the 
industry transitions away from IPMI and toward something easier to 
generate arbitrarily.

> 
> I’d rather have a single approach that works for everyone; although, I’m
> not sure how that would look.
> 
The single approach is where we started, and weren't able to come up 
with anything that even came close to working in a production sense.  If 
you have ideas here on how this could be built that are cleaner than 
what we're proposing, we're very much interested.

> 
> This is called top posting, please try to avoid when using the mail-list.
> It makes threaded conversation hard to follow and respond to.  thx.
> 

(Ed beats Jason with very big stick)

Re:

[Brad Bishop]

> On Sep 4, 2018, at 5:28 PM, Ed Tanous <ed.tanous@intel.com> wrote:
> 
> On 09/04/2018 01:46 PM, Brad Bishop wrote:
>> But it seems like you are proposing that every application that wants to make
>> a log needs to have the logic to translate its internal data model to IPMI speak,
>> so it can make a journal call with all the IPMI metadata populated.  Am I
>> understanding correctly?  That doesn’t seem aligned with keeping IPMI isolated.
> 
> I think a key here is that not all logs will be implicitly converted to IPMI logs.  Having them be identical was the design that we started with, and abandoned because IPMI has some requirements that don't cleanly map from a standard syslog/text style to IPMI.
> 
> 
>> A concrete example - phosphor-hwmon.  How do you intend to figure out something
>> like IPMI_SEL_SENSOR_PATH in the phosphor-hwmon application?  Actually it would
>> help quite a bit to understand how each of the fields in your sample below would
>> be determined by an arbitrary dbus application (like phosphor-hwmon).
> 
> I'm not really understanding the root of the question.  If phosphor-hwmon is generating a threshold crossing log that stemmed from the /xyz/openbmc_project/sensors/my_super_awesome_temp_sensor, then it would simply fill that path into the IPMI_SEL_SENSOR_PATH field.  

ok, then this is my ignorance of IPMI showing.  I thought IPMI_SEL_SENSOR_PATH was
an IPMI construct...

If this is the case then why not just call it SENSOR_PATH?  Then other logging formats
could use that metadata key without it being weird that it has ‘ipmi_sel’ in the name
of it.  And can we apply the same logic to the other keys or do some of the other keys
have more complicated translation logic (than none at all as in the case of the sensor
path) ?

> This is the same kind of mapping that the associations produce today, but captured in journald instead of the mapper.
> 
> Our thinking was that we could build either a static library, or a dbus daemon to simplify producing IPMI logs.  Because of the IPMI requirements around unique record ids, right now we're leaning toward the dbus interface with a single daemon responsible for IPMI SEL creation.

Thats great!  This is similar to how the phosphor-logging daemon creates dbus error
objects today.

Would you mind elaborating on this daemon and its dbus API?  I’m guessing it would probably
clear up any concerns I have.

> While technically it could be a part of phosphor-logging,

That isn’t what I was going for.  If you plan to implement a (separate) daemon that acts on
the journald metadata I think that is right approach too.

> we really want it to be easily removable for future platforms that have no need for IPMI, so the thought at this time it to keep it separate.

Agreed.

> 
>> Further, if you expand this approach to further log formats other than SEL,
>> won’t the applications become a mess of translation logic from the applications
>> data mode <-> log format in use?
> 
> I'm not really following this question.  Are there other binary log formats that we expect to come in the future that aren't text based, and could just be a journald translation?

Yes.  We have a binary format called PEL.  I doubt anyone would be interested in using
it but we need a foundation in OpenBMC that enables us to use it...

>  So far as I know, IPMI SEL is the only one on my road map that has weird requirements, and needs some translation.

Where is the translation happening?  In the new ipmi-sel daemon?  Or somewhere else?

>  I don't expect it to be a mess, and I'm running under the assumption that _most_ daemons won't care about or support IPMI given its limitations.

Well _all_ daemons already support IPMI SEL today.  The problem is just that the
implementation doesn’t scale.  I’m confused by _most_ daemons wouldn’t support
IPMI?

> You're right, this isn't intended to be a general solution for all binary logging formats, it's intended to be a short term hack while the industry transitions away from IPMI and toward something easier to generate arbitrarily.
> 
>> I’d rather have a single approach that works for everyone; although, I’m
>> not sure how that would look.
> The single approach is where we started, and weren't able to come up with anything that even came close to working in a production sense.  If you have ideas here on how this could be built that are cleaner than what we're proposing, we're very much interested.

I’m still trying to understand what is being proposed.

> 
>> This is called top posting, please try to avoid when using the mail-list.
>> It makes threaded conversation hard to follow and respond to.  thx.
> 
> (Ed beats Jason with very big stick)

Re: Re:

[Ed Tanous]

On 09/04/2018 03:34 PM, Brad Bishop wrote:
> 
> ok, then this is my ignorance of IPMI showing.  I thought IPMI_SEL_SENSOR_PATH was
> an IPMI construct...
> 
> If this is the case then why not just call it SENSOR_PATH?  Then other logging formats
> could use that metadata key without it being weird that it has ‘ipmi_sel’ in the name
> of it.  And can we apply the same logic to the other keys or do some of the other keys
> have more complicated translation logic (than none at all as in the case of the sensor
> path) ?

The thinking was that we would namespace all the parameters using 
IPMI_SEL to make it clear that was the only place they were used, and to 
avoid someone else using it inadvertently.  With that said, I could 
understand how it could be confusing.  Jason, any objections to 
un-namespacing the parameters?

> 
> Thats great!  This is similar to how the phosphor-logging daemon creates dbus error
> objects today.
> 
> Would you mind elaborating on this daemon and its dbus API?  I’m guessing it would probably
> clear up any concerns I have.
> 

Patches to phosphor-dbus-interfaces for a suggested interface are being 
put together as we speak.  Hopefully that will clarify it a little bit.

>> While technically it could be a part of phosphor-logging,
> 
> That isn’t what I was going for.  If you plan to implement a (separate) daemon that acts on
> the journald metadata I think that is right approach too.
> 
Agreed.

>> we really want it to be easily removable for future platforms that have no need for IPMI, so the thought at this time it to keep it separate.
> 
> Agreed.
> 
>>
>>> Further, if you expand this approach to further log formats other than SEL,
>>> won’t the applications become a mess of translation logic from the applications
>>> data mode <-> log format in use?
>>
>> I'm not really following this question.  Are there other binary log formats that we expect to come in the future that aren't text based, and could just be a journald translation?
> 
> Yes.  We have a binary format called PEL.  I doubt anyone would be interested in using
> it but we need a foundation in OpenBMC that enables us to use it...
> 

That makes more sense now.  A quick google on PEL makes it look like it 
could follow a similar model to what we're doing with IPMI by adding the 
extra metadata to journald when needed, while still producing the string 
versions for the basic cases.  By foundation do you mean shared code?  A 
quick skim of the implementation makes me suspect that there isn't going 
to be a lot of shared code, although they could share a similar design 
with a different implementation.

>>   So far as I know, IPMI SEL is the only one on my road map that has weird requirements, and needs some translation.
> 
> Where is the translation happening?  In the new ipmi-sel daemon?  Or somewhere else?

The translation would happen on the "addSel" IPMI command that gets used 
in-band by most BIOS implementations.  The ipmi-sel daemon will 
translate the raw bytes to a string, to be used in most modern loggers, 
along with the IPMI metadata, to be used in IPMI to source the various 
"get" SEL entry commands.

> 
>>   I don't expect it to be a mess, and I'm running under the assumption that _most_ daemons won't care about or support IPMI given its limitations.
> 
> Well _all_ daemons already support IPMI SEL today.  The problem is just that the
> implementation doesn’t scale.  I’m confused by _most_ daemons wouldn’t support
> IPMI?
> 

That should've clarified that most daemons won't care about IPMI _SEL_, 
given the extra calls and metadata that needs to be provided to 
implement it correctly.  My teams intention was to support the minimum 
subset of SEL that we can for backward compatibility, while providing 
the advanced logging (journald/syslog/redfish LogService) for a greater 
level of detail and capability.
If this assumption turns out to not be true, and we end up adding IPMI 
SEL logging to all the daemons, so be it, I think it will still scale, 
but I really hope that's not the path we go down.


>> You're right, this isn't intended to be a general solution for all binary logging formats, it's intended to be a short term hack while the industry transitions away from IPMI and toward something easier to generate arbitrarily.
>>
>>> I’d rather have a single approach that works for everyone; although, I’m
>>> not sure how that would look.
>> The single approach is where we started, and weren't able to come up with anything that even came close to working in a production sense.  If you have ideas here on how this could be built that are cleaner than what we're proposing, we're very much interested.
> 
> I’m still trying to understand what is being proposed.
> 
>>
>>> This is called top posting, please try to avoid when using the mail-list.
>>> It makes threaded conversation hard to follow and respond to.  thx.
>>
>> (Ed beats Jason with very big stick)

Re:

[Brad Bishop]

> On Sep 4, 2018, at 7:18 PM, Ed Tanous <ed.tanous@intel.com> wrote:
> 
> On 09/04/2018 03:34 PM, Brad Bishop wrote:
>> ok, then this is my ignorance of IPMI showing.  I thought IPMI_SEL_SENSOR_PATH was
>> an IPMI construct...
>> If this is the case then why not just call it SENSOR_PATH?  Then other logging formats
>> could use that metadata key without it being weird that it has ‘ipmi_sel’ in the name
>> of it.  And can we apply the same logic to the other keys or do some of the other keys
>> have more complicated translation logic (than none at all as in the case of the sensor
>> path) ?
> 
> The thinking was that we would namespace all the parameters using IPMI_SEL to make it clear that was the only place they were used, and to avoid someone else using it inadvertently.  
> With that said, I could understand how it could be confusing.  Jason, any objections to un-namespacing the parameters?

Thanks for being flexible on this but lets wait until we are on the same page before
changing anything.  Why would you want to discourage it from being used in another
context?

> 
>> Thats great!  This is similar to how the phosphor-logging daemon creates dbus error
>> objects today.
>> Would you mind elaborating on this daemon and its dbus API?  I’m guessing it would probably
>> clear up any concerns I have.
> 
> Patches to phosphor-dbus-interfaces for a suggested interface are being put together as we speak.  Hopefully that will clarify it a little bit.

Great, thank you.

> 
>>> While technically it could be a part of phosphor-logging,
>> That isn’t what I was going for.  If you plan to implement a (separate) daemon that acts on
>> the journald metadata I think that is right approach too.
> Agreed.
> 
>>> we really want it to be easily removable for future platforms that have no need for IPMI, so the thought at this time it to keep it separate.
>> Agreed.
>>> 
>>>> Further, if you expand this approach to further log formats other than SEL,
>>>> won’t the applications become a mess of translation logic from the applications
>>>> data mode <-> log format in use?
>>> 
>>> I'm not really following this question.  Are there other binary log formats that we expect to come in the future that aren't text based, and could just be a journald translation?
>> Yes.  We have a binary format called PEL.  I doubt anyone would be interested in using
>> it but we need a foundation in OpenBMC that enables us to use it...
> 
> That makes more sense now.  A quick google on PEL makes it look like it could follow a similar model to what we're doing with IPMI by adding the extra metadata to journald when needed, while still producing the string versions for the basic cases.  By foundation do you mean shared code?  A quick skim of the implementation makes me suspect that there isn't going to be a lot of shared code, although they could share a similar design with a different implementation.

By foundation I simply mean we need a way to support multiple logging formats that doesn’t
require every OpenBMC application to know how to translate from its internal data model
(usually dbus) to N logging formats.

> 
>>>  So far as I know, IPMI SEL is the only one on my road map that has weird requirements, and needs some translation.
>> Where is the translation happening?  In the new ipmi-sel daemon?  Or somewhere else?
> 
> The translation would happen on the "addSel" IPMI command that gets used in-band by most BIOS implementations.  The ipmi-sel daemon will translate the raw bytes to a string, to be used in most modern loggers, along with the IPMI metadata, to be used in IPMI to source the various "get" SEL entry commands.

That all sounds fine.  But what about applications on the BMC creating SELs for their
own errors?  Do you want to do that?  How will that work?

> 
>>>  I don't expect it to be a mess, and I'm running under the assumption that _most_ daemons won't care about or support IPMI given its limitations.
>> Well _all_ daemons already support IPMI SEL today.  The problem is just that the
>> implementation doesn’t scale.  I’m confused by _most_ daemons wouldn’t support
>> IPMI?
> 
> That should've clarified that most daemons won't care about IPMI _SEL_, given the extra calls and metadata that needs to be provided to implement it correctly.  My teams intention was to support the minimum subset of SEL that we can for backward compatibility, while providing the advanced logging (journald/syslog/redfish LogService) for a greater level of detail and capability.
> If this assumption turns out to not be true, and we end up adding IPMI SEL logging to all the daemons, so be it, I think it will still scale, but I really hope that's not the path we go down.

Oh.  Does this mean you intend for code like Jason originally proposed to _only_ appear in
the ipmi-sel daemon?  And not in applications like phosphor-hwmon?

Re:

[Bills, Jason M]

>>> ok, then this is my ignorance of IPMI showing.  I thought IPMI_SEL_SENSOR_PATH was
>>> an IPMI construct...
>>> If this is the case then why not just call it SENSOR_PATH?  Then other logging formats
>>> could use that metadata key without it being weird that it has ‘ipmi_sel’ in the name
>>> of it.  And can we apply the same logic to the other keys or do some of the other keys
>>> have more complicated translation logic (than none at all as in the case of the sensor
>>> path) ?
>>
>> The thinking was that we would namespace all the parameters using IPMI_SEL to make it clear that was the only place they were used, and to avoid someone else using it inadvertently.
>> With that said, I could understand how it could be confusing.  Jason, any objections to un-namespacing the parameters?
> 
> Thanks for being flexible on this but lets wait until we are on the same page before
> changing anything.  Why would you want to discourage it from being used in another
> context?
> 
Except for the sensor path, all of the proposed IPMI metadata is 
specific for IPMI:
"IPMI_SEL_RECORD_ID" = Two byte unique ID number for each SEL entry
"IPMI_SEL_RECORD_TYPE" = The type of SEL entry (system or OEM) which 
determines the definition of the remaining bytes
"IPMI_SEL_GENERATOR_ID" = The IPMI Generator ID (usually the IPMB Slave 
Address) of the SEL entry requester
"IPMI_SEL_SENSOR_PATH" = Path of the sensor used to find IPMI data (such 
as sensor number) for the sensor
"IPMI_SEL_EVENT_DIR" = Whether the sensor is asserting or de-asserting
"IPMI_SEL_DATA" = Raw binary data included in the SEL entry

I named them all as IPMI_SEL as a group so they would be clearly 
separate and easy to remove in the future.  However, if any of the 
metadata would be useful elsewhere, the names can be more generic.

>>
>>> Thats great!  This is similar to how the phosphor-logging daemon creates dbus error
>>> objects today.
>>> Would you mind elaborating on this daemon and its dbus API?  I’m guessing it would probably
>>> clear up any concerns I have.
>>
>> Patches to phosphor-dbus-interfaces for a suggested interface are being put together as we speak.  Hopefully that will clarify it a little bit.
> 
> Great, thank you.
> 
I have pushed a suggestion for the interface here: 
https://gerrit.openbmc-project.xyz/#/c/openbmc/phosphor-dbus-interfaces/+/12494/

>>
>>>> While technically it could be a part of phosphor-logging,
>>> That isn’t what I was going for.  If you plan to implement a (separate) daemon that acts on
>>> the journald metadata I think that is right approach too.
>> Agreed.
>>
>>>> we really want it to be easily removable for future platforms that have no need for IPMI, so the thought at this time it to keep it separate.
>>> Agreed.
>>>>
>>>>> Further, if you expand this approach to further log formats other than SEL,
>>>>> won’t the applications become a mess of translation logic from the applications
>>>>> data mode <-> log format in use?
>>>>
>>>> I'm not really following this question.  Are there other binary log formats that we expect to come in the future that aren't text based, and could just be a journald translation?
>>> Yes.  We have a binary format called PEL.  I doubt anyone would be interested in using
>>> it but we need a foundation in OpenBMC that enables us to use it...
>>
>> That makes more sense now.  A quick google on PEL makes it look like it could follow a similar model to what we're doing with IPMI by adding the extra metadata to journald when needed, while still producing the string versions for the basic cases.  By foundation do you mean shared code?  A quick skim of the implementation makes me suspect that there isn't going to be a lot of shared code, although they could share a similar design with a different implementation.
> 
> By foundation I simply mean we need a way to support multiple logging formats that doesn’t
> require every OpenBMC application to know how to translate from its internal data model
> (usually dbus) to N logging formats.
> 
>>
>>>>   So far as I know, IPMI SEL is the only one on my road map that has weird requirements, and needs some translation.
>>> Where is the translation happening?  In the new ipmi-sel daemon?  Or somewhere else?
>>
>> The translation would happen on the "addSel" IPMI command that gets used in-band by most BIOS implementations.  The ipmi-sel daemon will translate the raw bytes to a string, to be used in most modern loggers, along with the IPMI metadata, to be used in IPMI to source the various "get" SEL entry commands.
> 
> That all sounds fine.  But what about applications on the BMC creating SELs for their
> own errors?  Do you want to do that?  How will that work?
> 

An application on the BMC that needs to create a SEL can call the 
IpmiSelAdd method to request a new SEL entry in the journal.
>>
>>>>   I don't expect it to be a mess, and I'm running under the assumption that _most_ daemons won't care about or support IPMI given its limitations.
>>> Well _all_ daemons already support IPMI SEL today.  The problem is just that the
>>> implementation doesn’t scale.  I’m confused by _most_ daemons wouldn’t support
>>> IPMI?
>>
>> That should've clarified that most daemons won't care about IPMI _SEL_, given the extra calls and metadata that needs to be provided to implement it correctly.  My teams intention was to support the minimum subset of SEL that we can for backward compatibility, while providing the advanced logging (journald/syslog/redfish LogService) for a greater level of detail and capability.
>> If this assumption turns out to not be true, and we end up adding IPMI SEL logging to all the daemons, so be it, I think it will still scale, but I really hope that's not the path we go down.
> 
> Oh.  Does this mean you intend for code like Jason originally proposed to _only_ appear in
> the ipmi-sel daemon?  And not in applications like phosphor-hwmon?
> 

Yes, the original proposed code:
             sd_journal_send("MESSAGE=%s", message.c_str(),
                             "PRIORITY=%i", selPriority,
                             "MESSAGE_ID=%s", selMessageId,
                             "IPMI_SEL_RECORD_ID=%d", recordId,
                             "IPMI_SEL_RECORD_TYPE=%x", selSystemType,
                             "IPMI_SEL_GENERATOR_ID=%x", genId,
                             "IPMI_SEL_SENSOR_PATH=%s", path.c_str(),
                             "IPMI_SEL_EVENT_DIR=%x", assert,
                             "IPMI_SEL_DATA=%s", selDataStr,
                             NULL);
will only be in the ipmi-sel daemon.  Applications like phosphor-hwmon 
would use the IpmiSelAdd method to request SEL entries.	

Re:

[Kun Yi]

Much apologies for the super late reply.. I was out for an extended
period of time due to personal circumstances.
I have now addressed most of the comments in the v4 series.

Also cc'ed Supreeth who works on the AMD System Manageability stack.

On Wed, Dec 2, 2020 at 8:57 AM Kun Yi <kunyi@google.com> wrote:
>
> On Sat, Apr 04, 2020 at 08:01:16PM -0700, Kun Yi wrote:
> > SB Temperature Sensor Interface (SB-TSI) is an SMBus compatible
> > interface that reports AMD SoC's Ttcl (normalized temperature),
> > and resembles a typical 8-pin remote temperature sensor's I2C interface
> > to BMC.
> >
> > This commit adds basic support using this interface to read CPU
> > temperature, and read/write high/low CPU temp thresholds.
> >
> > To instantiate this driver on an AMD CPU with SB-TSI
> > support, the i2c bus number would be the bus connected from the board
> > management controller (BMC) to the CPU. The i2c address is specified in
> > Section 6.3.1 of the spec [1]: The SB-TSI address is normally 98h for socket 0
> > and 90h for socket 1, but it could vary based on hardware address select pins.
> >
> > [1]: https://www.amd.com/system/files/TechDocs/56255_OSRR.pdf
> >
> > Test status: tested reading temp1_input, and reading/writing
> > temp1_max/min.
> >
> > Signed-off-by: Kun Yi <kunyi at google.com>
> > ---
> >  drivers/hwmon/Kconfig      |  10 ++
> >  drivers/hwmon/Makefile     |   1 +
> >  drivers/hwmon/sbtsi_temp.c | 259 +++++++++++++++++++++++++++++++++++++
> >  3 files changed, 270 insertions(+)
> >  create mode 100644 drivers/hwmon/sbtsi_temp.c
> >
> > diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
> > index 05a30832c6ba..9585dcd01d1b 100644
> > --- a/drivers/hwmon/Kconfig
> > +++ b/drivers/hwmon/Kconfig
> > @@ -1412,6 +1412,16 @@ config SENSORS_RASPBERRYPI_HWMON
> >    This driver can also be built as a module. If so, the module
> >    will be called raspberrypi-hwmon.
> >
> > +config SENSORS_SBTSI
> > + tristate "Emulated SB-TSI temperature sensor"
> > + depends on I2C
> > + help
> > +  If you say yes here you get support for emulated temperature
> > +  sensors on AMD SoCs with SB-TSI interface connected to a BMC device.
> > +
> > +  This driver can also be built as a module. If so, the module will
> > +  be called sbtsi_temp.
> > +
> >  config SENSORS_SHT15
> >   tristate "Sensiron humidity and temperature sensors. SHT15 and compat."
> >   depends on GPIOLIB || COMPILE_TEST
> > diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
> > index b0b9c8e57176..cd109f003ce4 100644
> > --- a/drivers/hwmon/Makefile
> > +++ b/drivers/hwmon/Makefile
> > @@ -152,6 +152,7 @@ obj-$(CONFIG_SENSORS_POWR1220)  += powr1220.o
> >  obj-$(CONFIG_SENSORS_PWM_FAN) += pwm-fan.o
> >  obj-$(CONFIG_SENSORS_RASPBERRYPI_HWMON) += raspberrypi-hwmon.o
> >  obj-$(CONFIG_SENSORS_S3C) += s3c-hwmon.o
> > +obj-$(CONFIG_SENSORS_SBTSI) += sbtsi_temp.o
> >  obj-$(CONFIG_SENSORS_SCH56XX_COMMON)+= sch56xx-common.o
> >  obj-$(CONFIG_SENSORS_SCH5627) += sch5627.o
> >  obj-$(CONFIG_SENSORS_SCH5636) += sch5636.o
> > diff --git a/drivers/hwmon/sbtsi_temp.c b/drivers/hwmon/sbtsi_temp.c
> > new file mode 100644
> > index 000000000000..e3ad6a9f7ec1
> > --- /dev/null
> > +++ b/drivers/hwmon/sbtsi_temp.c
> > @@ -0,0 +1,259 @@
> > +// SPDX-License-Identifier: GPL-2.0-or-later
> > +/*
> > + * sbtsi_temp.c - hwmon driver for a SBI Temperature Sensor Interface (SB-TSI)
> > + *                compliant AMD SoC temperature device.
> > + *
> > + * Copyright (c) 2020, Google Inc.
> > + * Copyright (c) 2020, Kun Yi <kunyi at google.com>
> > + */
> > +
> > +#include <linux/err.h>
> > +#include <linux/i2c.h>
> > +#include <linux/init.h>
> > +#include <linux/hwmon.h>
> > +#include <linux/module.h>
> > +#include <linux/mutex.h>
> > +#include <linux/of_device.h>
> > +#include <linux/of.h>
> > +
> > +/*
> > + * SB-TSI registers only support SMBus byte data access. "_INT" registers are
> > + * the integer part of a temperature value or limit, and "_DEC" registers are
> > + * corresponding decimal parts.
> > + */
> > +#define SBTSI_REG_TEMP_INT 0x01 /* RO */
> > +#define SBTSI_REG_STATUS 0x02 /* RO */
> > +#define SBTSI_REG_CONFIG 0x03 /* RO */
> > +#define SBTSI_REG_TEMP_HIGH_INT 0x07 /* RW */
> > +#define SBTSI_REG_TEMP_LOW_INT 0x08 /* RW */
> > +#define SBTSI_REG_TEMP_DEC 0x10 /* RW */
> > +#define SBTSI_REG_TEMP_HIGH_DEC 0x13 /* RW */
> > +#define SBTSI_REG_TEMP_LOW_DEC 0x14 /* RW */
> > +#define SBTSI_REG_REV 0xFF /* RO */
>
> The revision register is not actually used.
Thanks. Removed. I agree that the register is not well documented, at
least publicly.
It shouldn't affect functionality of this driver, so I removed the
definition altogether.
>
> > +
> > +#define SBTSI_CONFIG_READ_ORDER_SHIFT 5
> > +
> > +#define SBTSI_TEMP_MIN 0
> > +#define SBTSI_TEMP_MAX 255875
> > +#define SBTSI_REV_MAX_VALID_ID 4
>
> Not actually used, and I am not sure if it would make sense to check it.
> If at all, it would only make sense if you also check SBTSIxFE (Manufacture
> ID). Unfortunately, the actual SB-TSI specification seems to be non-public,
> so I can't check if the driver as-is supports versions 0..3 (assuming those
> exist).

Thanks. Removed.

>
> > +
> > +/* Each client has this additional data */
> > +struct sbtsi_data {
> > + struct i2c_client *client;
> > + struct mutex lock;
> > +};
> > +
> > +/*
> > + * From SB-TSI spec: CPU temperature readings and limit registers encode the
> > + * temperature in increments of 0.125 from 0 to 255.875. The "high byte"
> > + * register encodes the base-2 of the integer portion, and the upper 3 bits of
> > + * the "low byte" encode in base-2 the decimal portion.
> > + *
> > + * e.g. INT=0x19, DEC=0x20 represents 25.125 degrees Celsius
> > + *
> > + * Therefore temperature in millidegree Celsius =
> > + *   (INT + DEC / 256) * 1000 = (INT * 8 + DEC / 32) * 125
> > + */
> > +static inline int sbtsi_reg_to_mc(s32 integer, s32 decimal)
> > +{
> > + return ((integer << 3) + (decimal >> 5)) * 125;
> > +}
> > +
> > +/*
> > + * Inversely, given temperature in millidegree Celsius
> > + *   INT = (TEMP / 125) / 8
> > + *   DEC = ((TEMP / 125) % 8) * 32
> > + * Caller have to make sure temp doesn't exceed 255875, the max valid value.
> > + */
> > +static inline void sbtsi_mc_to_reg(s32 temp, u8 *integer, u8 *decimal)
> > +{
> > + temp /= 125;
> > + *integer = temp >> 3;
> > + *decimal = (temp & 0x7) << 5;
> > +}
> > +
> > +static int sbtsi_read(struct device *dev, enum hwmon_sensor_types type,
> > +      u32 attr, int channel, long *val)
> > +{
> > + struct sbtsi_data *data = dev_get_drvdata(dev);
> > + s32 temp_int, temp_dec;
> > + int err, reg_int, reg_dec;
> > + u8 read_order;
> > +
> > + if (type != hwmon_temp)
> > + return -EINVAL;
> > +
> > + read_order = 0;
> > + switch (attr) {
> > + case hwmon_temp_input:
> > + /*
> > + * ReadOrder bit specifies the reading order of integer and
> > + * decimal part of CPU temp for atomic reads. If bit == 0,
> > + * reading integer part triggers latching of the decimal part,
> > + * so integer part should be read first. If bit == 1, read
> > + * order should be reversed.
> > + */
> > + err = i2c_smbus_read_byte_data(data->client, SBTSI_REG_CONFIG);
> > + if (err < 0)
> > + return err;
> > +
> As I understand it, the idea is to set this configuration bit once and then
> just use it. Any chance to do that ? This would save an i2c read operation
> each time the temperature is read, and the if/else complexity below.

Unfortunately, the read-order register bit is read-only.

>
> > + read_order = (u8)err & BIT(SBTSI_CONFIG_READ_ORDER_SHIFT);
>
> Nit: typecast is unnecessary.

Done.

>
> > + reg_int = SBTSI_REG_TEMP_INT;
> > + reg_dec = SBTSI_REG_TEMP_DEC;
> > + break;
> > + case hwmon_temp_max:
> > + reg_int = SBTSI_REG_TEMP_HIGH_INT;
> > + reg_dec = SBTSI_REG_TEMP_HIGH_DEC;
> > + break;
> > + case hwmon_temp_min:
> > + reg_int = SBTSI_REG_TEMP_LOW_INT;
> > + reg_dec = SBTSI_REG_TEMP_LOW_DEC;
> > + break;
> > + default:
> > + return -EINVAL;
> > + }
> > +
> > + if (read_order == 0) {
> > + temp_int = i2c_smbus_read_byte_data(data->client, reg_int);
> > + temp_dec = i2c_smbus_read_byte_data(data->client, reg_dec);
> > + } else {
> > + temp_dec = i2c_smbus_read_byte_data(data->client, reg_dec);
> > + temp_int = i2c_smbus_read_byte_data(data->client, reg_int);
> > + }
>
> Just a thought: if you use regmap and tell it that the limit registers
> are non-volatile, this wouldn't actually read from the chip more than once.

That's a great suggestion, although in our normal use cases the limit
values are read and cached by the
userspace application. Seems changing to regmap would require some
messaging of the code. Would it
be acceptable to keep the initial driver as-is and do that in a following patch?

>
> Also, since the read involves reading two registers, and the first read
> locks the value for the second, you'll need mutex protection when reading
> the current temperature (not for limits, though).

Added mutex locking before/after the temp input reading.

>
> > +
> > + if (temp_int < 0)
> > + return temp_int;
> > + if (temp_dec < 0)
> > + return temp_dec;
> > +
> > + *val = sbtsi_reg_to_mc(temp_int, temp_dec);
> > +
> > + return 0;
> > +}
> > +
> > +static int sbtsi_write(struct device *dev, enum hwmon_sensor_types type,
> > +       u32 attr, int channel, long val)
> > +{
> > + struct sbtsi_data *data = dev_get_drvdata(dev);
> > + int reg_int, reg_dec, err;
> > + u8 temp_int, temp_dec;
> > +
> > + if (type != hwmon_temp)
> > + return -EINVAL;
> > +
> > + switch (attr) {
> > + case hwmon_temp_max:
> > + reg_int = SBTSI_REG_TEMP_HIGH_INT;
> > + reg_dec = SBTSI_REG_TEMP_HIGH_DEC;
> > + break;
> > + case hwmon_temp_min:
> > + reg_int = SBTSI_REG_TEMP_LOW_INT;
> > + reg_dec = SBTSI_REG_TEMP_LOW_DEC;
> > + break;
> > + default:
> > + return -EINVAL;
> > + }
> > +
> > + val = clamp_val(val, SBTSI_TEMP_MIN, SBTSI_TEMP_MAX);
> > + mutex_lock(&data->lock);
> > + sbtsi_mc_to_reg(val, &temp_int, &temp_dec);
> > + err = i2c_smbus_write_byte_data(data->client, reg_int, temp_int);
> > + if (err)
> > + goto exit;
> > +
> > + err = i2c_smbus_write_byte_data(data->client, reg_dec, temp_dec);
> > +exit:
> > + mutex_unlock(&data->lock);
> > + return err;
> > +}
> > +
> > +static umode_t sbtsi_is_visible(const void *data,
> > + enum hwmon_sensor_types type,
> > + u32 attr, int channel)
> > +{
> > + switch (type) {
> > + case hwmon_temp:
> > + switch (attr) {
> > + case hwmon_temp_input:
> > + return 0444;
> > + case hwmon_temp_min:
> > + return 0644;
> > + case hwmon_temp_max:
> > + return 0644;
> > + }
> > + break;
> > + default:
> > + break;
> > + }
> > + return 0;
> > +}
> > +
> > +static const struct hwmon_channel_info *sbtsi_info[] = {
> > + HWMON_CHANNEL_INFO(chip,
> > +   HWMON_C_REGISTER_TZ),
> > + HWMON_CHANNEL_INFO(temp,
> > +   HWMON_T_INPUT | HWMON_T_MIN | HWMON_T_MAX),
>
> For your consideration: SB-TSI supports reporting high/low alerts.
> With this, it would be possible to implement respective alarm attributes.
> In conjunction with https://patchwork.kernel.org/patch/11277347/mbox/,
> it should also be possible to add interrupt and thus userspace notification
> for those attributes.
>
> SBTSI also supports setting the update rate (SBTSIx04) and setting
> the temperature offset (SBTSIx11, SBTSIx12), which could also be
> implemented as standard attributes.
>
> I won't require that for the initial version, just something to keep
> in mind.

Ack and thanks for the suggestions. I will keep in mind for future improvements.


>
> > + NULL
> > +};
> > +
> > +static const struct hwmon_ops sbtsi_hwmon_ops = {
> > + .is_visible = sbtsi_is_visible,
> > + .read = sbtsi_read,
> > + .write = sbtsi_write,
> > +};
> > +
> > +static const struct hwmon_chip_info sbtsi_chip_info = {
> > + .ops = &sbtsi_hwmon_ops,
> > + .info = sbtsi_info,
> > +};
> > +
> > +static int sbtsi_probe(struct i2c_client *client,
> > +       const struct i2c_device_id *id)
> > +{
> > + struct device *dev = &client->dev;
> > + struct device *hwmon_dev;
> > + struct sbtsi_data *data;
> > +
> > + data = devm_kzalloc(dev, sizeof(struct sbtsi_data), GFP_KERNEL);
> > + if (!data)
> > + return -ENOMEM;
> > +
> > + data->client = client;
> > + mutex_init(&data->lock);
> > +
> > + hwmon_dev =
> > + devm_hwmon_device_register_with_info(dev, client->name, data,
> > +     &sbtsi_chip_info, NULL);
> > +
> > + return PTR_ERR_OR_ZERO(hwmon_dev);
> > +}
> > +
> > +static const struct i2c_device_id sbtsi_id[] = {
> > + {"sbtsi", 0},
> > + {}
> > +};
> > +MODULE_DEVICE_TABLE(i2c, sbtsi_id);
> > +
> > +static const struct of_device_id __maybe_unused sbtsi_of_match[] = {
> > + {
> > + .compatible = "amd,sbtsi",
> > + },
> > + { },
> > +};
> > +MODULE_DEVICE_TABLE(of, sbtsi_of_match);
> > +
> > +static struct i2c_driver sbtsi_driver = {
> > + .class = I2C_CLASS_HWMON,
> > + .driver = {
> > + .name = "sbtsi",
> > + .of_match_table = of_match_ptr(sbtsi_of_match),
> > + },
> > + .probe = sbtsi_probe,
> > + .id_table = sbtsi_id,
> > +};
> > +
> > +module_i2c_driver(sbtsi_driver);
> > +
> > +MODULE_AUTHOR("Kun Yi <kunyi at google.com>");
> > +MODULE_DESCRIPTION("Hwmon driver for AMD SB-TSI emulated sensor");
> > +MODULE_LICENSE("GPL");



--
Regards,
Kun