Feedback on Current OpenBMC and Proposing Some Improvements ---- "Improvements" Ideas

Feedback on Current OpenBMC and Proposing Some Improvements ---- "Improvements" Ideas

[Alex Qiu]

[-- Attachment #1: Type: text/plain, Size: 5313 bytes --]

(Splitted from main email thread "Feedback on Current OpenBMC and Proposing
Some Improvements")

"Improvements" Ideas

With the issue examples described above, I came up with some general ideas
on how "Improvements" should look like, which I concluded them into some
high-level design ideas.
Abstraction on entities

I think we need some abstraction to gather the control flow and data into
objects in the code to represent hardware entities. This will greatly
improve debugging, hardware configuration and workaround implementation.
The developers can easily find the code for the hardware or module that
they are dealing with, and it is also clear on the location to implement a
feature for a specific hardware. The control flow can be abstracted into
some interface functions like: onBmcInit(), onHostPowerEvent(),
updateSensorReadings(), readEepromContent(), etc. For most of the hardware,
they can use a common default implementation; for special hardware, they
can override the function to achieve their requirements. For example,
reconfiguring a device register when host 12V power is up; aggregating lots
of temperature sensors to expose only one temperature sensor with maximum
temperature; applying special handling to the emulated EEPROM described
above.
Having a top-down framework of hardware topology

The existing JSON files used in the dynamic software stack can only
represent data, but not any control flow. This led to difficulties where
sometimes some code is preferred to have for aiding the discovery of
hardware topology, condensing redundant configurations, etc. With a good
framework for hardware topology, combining the entity abstraction described
above, developers can easily find the best places to aid the topology
discovery, implement hardware initialization logics, and optimize BMC tasks
according to Linux behaviors.
Better open source and proprietary part management

Construct "Improvements" like a proprietary software supporting plugins.
The philosophy is that the architecture of "Improvements" should be solid
enough that the community won't have to modify the upstream code much. The
community can look at and reference the code upstream to develop their own
code and configs according to their hardware, while the plugin-able part
may be proprietary and can be kept downstream without conflicts.
"Improvements" should have a reasonable plugin API to support common BMC
functionality in the high level, and provide common low-level APIs to
support the plugins by abstracting things like hwmon sysfs interface. This
can be implemented using a plugin system or a flexible build system, as we
are working on an open source project indeed. Whenever we find a potential
conflict between upstream and downstream, let us work it out to see if it
is appropriate to make it pluginable or configurable via config files.
Flexibility for alternatives

Although hwmon sysfs interface is a good starting point for getting sensor
reads from devices, they have their own limitations. The interface does not
abstract every register perfectly, especially when device registers are not
designed to follow some common specs like PMBus, and it does not provide
controls to the devices.

I propose a Device Abstraction Layer to wrap around devices. The underlying
can completely map to hwmon sysfs, or allow user-space driver
implementation if necessary, or even hybrid. This will easily provide an
additional interface to bypass the driver and control the devices, while
still maintaining the benefit to use an off-the-shelf Linux device driver.
Decouple protocol layers more

Quite some existing code is heavily bound to or influenced by the IPMI
protocol layer that we are having right now: We use “uint8_t” type for I2C
bus number in entity-manager for example, while Linux kernel can extend the
logical I2C bus number to more than 512 without any issues. The current
dynamic software stack emphasizes individual sensors, but the BMC handles
many more tasks than just only sensors. The practicality of OpenBMC for
hardware engineers is also hindered by the IPMI as described above in Issue
Examples.

We should have a core designed to consider varieties of tasks that BMC may
be asked to handle: GPIO modifications, I2C manipulations,  The core should
not be hindered by any protocol, but the protocol layer should find its own
way to map the core APIs to its own protocol. This will help us to transit
from IPMI to Redfish.
Backward Compatibility

Although the current dynamic software stack configuration file naming
schema has already taken in some bad label naming like “Name1”, I
understand that the community has also put in a lot of effort to the
current dynamic software stack, and would like to maintain some backward
compatibility somehow to mitigate the transition. I do not have too much
understanding of the compatibility burden that we are dealing with right
now, but just to give a couple of examples: The current JSON configuration
files can be addressed by a common topology discovery module provided as a
basis. For Device Abstraction Layer, we can start with a common module to
still use hwmon sysfs interface for sensors as a basis.

(Back to main thread)

- Alex Qiu

[-- Attachment #2: Type: text/html, Size: 9989 bytes --]

Re: Feedback on Current OpenBMC and Proposing Some Improvements ---- "Improvements" Ideas

[Ed Tanous]

On Thu, Jun 18, 2020 at 2:29 PM Alex Qiu <xqiu@google.com> wrote:
>
> Abstraction on entities
>
> I think we need some abstraction to gather the control flow and data into objects in the code to represent hardware entities. This will greatly improve debugging, hardware configuration and workaround implementation. The developers can easily find the code for the hardware or module that they are dealing with, and it is also clear on the location to implement a feature for a specific hardware. The control flow can be abstracted into some interface functions like: onBmcInit(), onHostPowerEvent(), updateSensorReadings(), readEepromContent(), etc. For most of the hardware, they can use a common default implementation; for special hardware, they can override the function to achieve their requirements. For example, reconfiguring a device register when host 12V power is up; aggregating lots of temperature sensors to expose only one temperature sensor with maximum temperature; applying special handling to the emulated EEPROM described above.

One thing that seems to be implied in your model is that all control
flow for hardware entities are running as a part of a single process,
and possibly single thread.  Most of the complexity you're finding is
in the interfaces between applications, and the threading model.
Dropping to a single threaded model removes a lot of operating system
safeties that we gain by separating applications, complicates some
applications that have operations that might be long, blocking, or
need to keep internal state, and makes less efficient use of the
multicore core BMCs in future generations.  It also makes it harder
for distributions to "compile out" the applications and functions they
don't need (although still solvable), or solve licensing hurdles
between applications.

With that said, most of the functions you listed have multithreaded
counterparts for a given application.  I agree, we could much better
organize these, and I'm interested to see how, given the constraints
of threading, eventing, and timing we could build something that's
simpler to extend.
onBmcInit() - The OpenBMC equivalent would be on application launch.
This adds an assumption that all onBMCInit() methods are idempotent
(as an application crash can cause this to get run again without a
complete BMC reboot).  In general, for the things that aren't
idempotent here, consumers will make them idempotent with some sort of
implementation specific flag.
onHostPowerEvent() - Would be a match expression waiting for
PropertiesChanged events coming from the power subsystem.  Again, you
have to match these with your threading model.  If a power down event
occurs during a sensor read, it's application specific how it should
be handled.  From an performance standpoint, dbus also isn't great to
us here, and there is no sense in pulling down 15 copies of the same
event (although power events don't happen that often).  I agree,
setting this up is non-trivial, but there is a utils function for it.
https://github.com/openbmc/dbus-sensors/blob/63f386691107a67e5fbfeb11fbac4f434d7a3ee6/src/Utils.cpp#L140
 Maybe you could suggest an alternative naming in a patch?
Also, Host power is far from the only event that a FRU model needs to
handle.  Off the top of my head, NVMe insert and removal (Both via
presence detection pins and without).  Firmware attestation and
lockdown.  Boot state/driver init state.  Maybe these could all be
encapsulated within some sort of single event, but more research would
need to be done.
updateSensorReadings() - Can't really exist in the current model, as
it implies that all sensors are scanned at a fixed rate, have no
threading model, and block all other sensors.  While this is true in
some BMC proprietary stacks, this is not true on OpenBMC.  There are
many cases (like calculated CFM sensors) where values are only updated
on another event (like a fan speed changing in the case of the CFM
sensor), and there is no scan loop at all; It's entirely event driven.
There are further cases for things like ME Sensors or MCTP sensors,
where the actual "update" logic has a very high latency, but can be
pipelined with multiple requests in flight to make it more efficient.
Having a blocking "updateSensorReadings" call negates that ability,
and negates a lot of the very useful error handling cases where a
failed sensor might block.  In the dbus-sensors model, there is
nothing specifying an individual sensor's refresh rate, which lets us
make optimizations like scanning a single MCTP device at a time,
because we know both operations require the bus.
readEepromContent() - What if it's a FRU device without an EEPROM?
Fru devices can be detected many different ways, with content from the
operating system PCIe tables, or from an out of band interface.  There
are some commercial BMC stacks where all FRU devices can be assumed to
have eeproms, but in OpenBMC there are FRUs that have no eeproms, so
you can't bake that requirement into the FRU interface itself.  You
could bake it in in a more generic, key value type API, but that's
basically what Entity manager has today (for exactly this reason).
One high level idea that I'm struggling with in the ideas above that
there isn't an abstraction of similar devices.  There are many FRU
devices that contain an LM75.  The hope would be that you wouldn't
need to write the code again to access an LM75 for each possible
device, but I don't see any sort of backend on your proposal.  Maybe
mocking up a simple card as an example, with an eeprom, a couple
sensors, and a proprietary device driver, and walking through the code
one might need to create it might help solidify this?

>
> The existing JSON files used in the dynamic software stack can only represent data, but not any control flow. This led to difficulties where sometimes some code is preferred to have for aiding the discovery of hardware topology, condensing redundant configurations, etc. With a good framework for hardware topology, combining the entity abstraction described above, developers can easily find the best places to aid the topology discovery, implement hardware initialization logics, and optimize BMC tasks according to Linux behaviors.

If you need code or control flow to aid in the discovery of hardware
topology, write an application that exposes an interface similar to
FruDevice, or the soon to be submitted peci-pcie.  These can be used
in entity manager configs.  I'm not quite following what "redundant
configurations" means in this context.  In my experience, most
redundant configurations tend to be for things like power supplies, or
drives, where a single device can fit in many different slots.  WIth
that said, we already have an abstraction for that, so I'm not quite
following.
>
> Better open source and proprietary part management
>
> Construct "Improvements" like a proprietary software supporting plugins. The philosophy is that the architecture of "Improvements" should be solid enough that the community won't have to modify the upstream code much. The community can look at and reference the code upstream to develop their own code and configs according to their hardware, while the plugin-able part may be proprietary and can be kept downstream without conflicts. "Improvements" should have a reasonable plugin API to support common BMC functionality in the high level, and provide common low-level APIs to support the plugins by abstracting things like hwmon sysfs interface. This can be implemented using a plugin system or a flexible build system, as we are working on an open source project indeed. Whenever we find a potential conflict between upstream and downstream, let us work it out to see if it is appropriate to make it pluginable or configurable via config files.

I must be misreading this, as I feel like openbmc already has
"plugins" in the form of Dbus applications.  Many applications have
been written that required no modification to upstream code.  Tha API
you're looking for is reasonably well defined in phosphor dbus
interfaces, and is intended to be reasonably stable, even if it's not
guaranteed over time.  I'm also a little confused at what you're
calling low-level APIs.  hwmon sysfs is a low level API.  Are you
wanting to wrap it in yet another API that's OpenBMC specific?
"can be kept downstream without conflicts" - In my experience, you're
going to be hard pressed to find support for supporting closed source
development in an open source project.  That's not to say individuals
aren't out there, but they tend to keep their heads down :)

>
> Flexibility for alternatives
>
> Although hwmon sysfs interface is a good starting point for getting sensor reads from devices, they have their own limitations. The interface does not abstract every register perfectly, especially when device registers are not designed to follow some common specs like PMBus, and it does not provide controls to the devices.
>
>
> I propose a Device Abstraction Layer to wrap around devices. The underlying can completely map to hwmon sysfs, or allow user-space driver implementation if necessary, or even hybrid. This will easily provide an additional interface to bypass the driver and control the devices, while still maintaining the benefit to use an off-the-shelf Linux device driver.

In this context, what are you calling a "device"?  I think everything
you're looking for exists, although it sounds like it's not in the
form you're wanting to see.  Dbus sensors already does a hwmon to Dbus
sensor abstraction conversion, that in some cases maps 1:1, or in some
cases is a "hybrid" as you call it.  Are you looking for something in
the middle, so instead of going hwmon -> Dbus  and libmctp -> Dbus you
would want hwmon -> DAL -> Dbus  and libmctp -> DAL -> Dbus?  There
could be some advantages here, but I have a worry that it'll be
difficult to come up with a reasonable "device" api.  Devices take a
lot of forms, in band, out of band, all with varying requirements
around threading, permissions, and eventing.  While it's possible to
cover everything that's needed, I'd be worried we'd be able to cover a
majority of them.

>
> Quite some existing code is heavily bound to or influenced by the IPMI protocol layer that we are having right now: We use “uint8_t” type for I2C bus number in entity-manager for example, while Linux kernel can extend the logical I2C bus number to more than 512 without any issues.
Can you come up with a better example?  We've tried to be very careful
to not have IPMI-specific things in the interfaces, and to make them
as generic as possible.  In that case, uint8_t is used to represent
the 7 bit addressing (plus read write bit) on the I2C bus itself, not
the uint8_t in the IPMI spec.  The API you listed neglected to handle
the possibility of 11 bit I2C addressing, as it isn't very common in
practice, but the argument could certainly be made that the interface
should be changed to a uint16_t, and I would expect the IPMI layer to
simply filter addresses above 127 that it's not able to support.

> The current dynamic software stack emphasizes individual sensors, but the BMC handles many more tasks than just only sensors. The practicality of OpenBMC for hardware engineers is also hindered by the IPMI as described above in Issue Examples.
Sensors were the first thing tackled, as those are the things that
tend to be the most different platform to platform, and have the most
peculiar settings.  We do also handle topology to some extent, as well
as a lot of other commands that are not IPMI specific.  I agree, IPMI
has its flaws, but OpenBMC also has pretty good support for Redfish,
direct dbus, and upcoming MCTP if that's what you'd rather use as an
outbound interface.

>
>
> We should have a core designed to consider varieties of tasks that BMC may be asked to handle: GPIO modifications, I2C manipulations,  The core should not be hindered by any protocol, but the protocol layer should find its own way to map the core APIs to its own protocol. This will help us to transit from IPMI to Redfish.
Again, I'm having some trouble following here, as I think the "core"
that exists now is very close to what you're asking for, and anywhere
we've deviated has been very explicitly done.
GPIO modifications and inputs are handled by the libgpio abstraction
(or sysfs).   This is well documented, and has a lot of tooling
wrapped around it.  There are lots of examples, as well as
phosphor-gpio, which in some cases, can directly manage the GPIO->Dbus
conversions (disclaimer, I have not used it myself, as I tend to go
directly to libgpio when I need that ability).
I2C is handled by the linux i2c devices, or the linux i2c bus
abstraction.  Again, it's well documented with lots of tooling.  There
are lots of examples of it being used.
Neither of the above have any hindrance on any protocol, and both are
used indirectly through the abstraction in Redfish, IPMI, and the
custom REST implementation. The "core APIs" as you call them are
supported by DBus, although I suspect in your case, core implies
single producer, which dbus is not.

Hopefully the above helped,

-Ed

Re: Feedback on Current OpenBMC and Proposing Some Improvements ---- "Improvements" Ideas

[Alex Qiu]

Hi Ed,

-Internal email list
+A couple of folks who might be interested in this topic

I don't know if you saw the updated reply in the main thread, but I
foresaw some possible communication gap, so I created a simple demo to
illustrate my ideas: https://github.com/alex310110/bmc-proto-20q2
Please note that I'm not trying to code a BMC with Python, but it's
just for the ease to set up a demo fast. Other replies inlined.
Thanks!

- Alex Qiu

On Sun, Jun 21, 2020 at 3:16 PM Ed Tanous <ed@tanous.net> wrote:
>
> On Thu, Jun 18, 2020 at 2:29 PM Alex Qiu <xqiu@google.com> wrote:
> >
> > Abstraction on entities
> >
> > I think we need some abstraction to gather the control flow and data into objects in the code to represent hardware entities. This will greatly improve debugging, hardware configuration and workaround implementation. The developers can easily find the code for the hardware or module that they are dealing with, and it is also clear on the location to implement a feature for a specific hardware. The control flow can be abstracted into some interface functions like: onBmcInit(), onHostPowerEvent(), updateSensorReadings(), readEepromContent(), etc. For most of the hardware, they can use a common default implementation; for special hardware, they can override the function to achieve their requirements. For example, reconfiguring a device register when host 12V power is up; aggregating lots of temperature sensors to expose only one temperature sensor with maximum temperature; applying special handling to the emulated EEPROM described above.
>
> One thing that seems to be implied in your model is that all control
> flow for hardware entities are running as a part of a single process,
> and possibly single thread.  Most of the complexity you're finding is
> in the interfaces between applications, and the threading model.
> Dropping to a single threaded model removes a lot of operating system
> safeties that we gain by separating applications, complicates some
> applications that have operations that might be long, blocking, or
> need to keep internal state, and makes less efficient use of the
> multicore core BMCs in future generations.  It also makes it harder
> for distributions to "compile out" the applications and functions they
> don't need (although still solvable), or solve licensing hurdles
> between applications.
>
> With that said, most of the functions you listed have multithreaded
> counterparts for a given application.  I agree, we could much better
> organize these, and I'm interested to see how, given the constraints
> of threading, eventing, and timing we could build something that's
> simpler to extend.
> onBmcInit() - The OpenBMC equivalent would be on application launch.
> This adds an assumption that all onBMCInit() methods are idempotent
> (as an application crash can cause this to get run again without a
> complete BMC reboot).  In general, for the things that aren't
> idempotent here, consumers will make them idempotent with some sort of
> implementation specific flag.
> onHostPowerEvent() - Would be a match expression waiting for
> PropertiesChanged events coming from the power subsystem.  Again, you
> have to match these with your threading model.  If a power down event
> occurs during a sensor read, it's application specific how it should
> be handled.  From an performance standpoint, dbus also isn't great to
> us here, and there is no sense in pulling down 15 copies of the same
> event (although power events don't happen that often).  I agree,
> setting this up is non-trivial, but there is a utils function for it.
> https://github.com/openbmc/dbus-sensors/blob/63f386691107a67e5fbfeb11fbac4f434d7a3ee6/src/Utils.cpp#L140
>  Maybe you could suggest an alternative naming in a patch?
> Also, Host power is far from the only event that a FRU model needs to
> handle.  Off the top of my head, NVMe insert and removal (Both via
> presence detection pins and without).  Firmware attestation and
> lockdown.  Boot state/driver init state.  Maybe these could all be
> encapsulated within some sort of single event, but more research would
> need to be done.
> updateSensorReadings() - Can't really exist in the current model, as
> it implies that all sensors are scanned at a fixed rate, have no
> threading model, and block all other sensors.  While this is true in
> some BMC proprietary stacks, this is not true on OpenBMC.  There are
> many cases (like calculated CFM sensors) where values are only updated
> on another event (like a fan speed changing in the case of the CFM
> sensor), and there is no scan loop at all; It's entirely event driven.
> There are further cases for things like ME Sensors or MCTP sensors,
> where the actual "update" logic has a very high latency, but can be
> pipelined with multiple requests in flight to make it more efficient.
> Having a blocking "updateSensorReadings" call negates that ability,
> and negates a lot of the very useful error handling cases where a
> failed sensor might block.  In the dbus-sensors model, there is
> nothing specifying an individual sensor's refresh rate, which lets us
> make optimizations like scanning a single MCTP device at a time,
> because we know both operations require the bus.
> readEepromContent() - What if it's a FRU device without an EEPROM?
> Fru devices can be detected many different ways, with content from the
> operating system PCIe tables, or from an out of band interface.  There
> are some commercial BMC stacks where all FRU devices can be assumed to
> have eeproms, but in OpenBMC there are FRUs that have no eeproms, so
> you can't bake that requirement into the FRU interface itself.  You
> could bake it in in a more generic, key value type API, but that's
> basically what Entity manager has today (for exactly this reason).
> One high level idea that I'm struggling with in the ideas above that
> there isn't an abstraction of similar devices.  There are many FRU
> devices that contain an LM75.  The hope would be that you wouldn't
> need to write the code again to access an LM75 for each possible
> device, but I don't see any sort of backend on your proposal.  Maybe
> mocking up a simple card as an example, with an eeprom, a couple
> sensors, and a proprietary device driver, and walking through the code
> one might need to create it might help solidify this?

I didn't start with multi-threading too much in mind, but It's not
necessarily a single-threaded model. As you can see in the demo, each
entity instance has its own function of update_sensors(), and the
entities are collected in the main class, which may be implemented as
a higher-level inter-process communication API for entities to adapt
to instead of merely a single main function or thread. So this model
can potentially be threaded on an entity basis, or even fork each
entity into individual processes. The model can be further threaded
within each entity into service threads: separating sensor polling
loop and event handling loop for example. But I do wonder about the
performance overhead of making every function call into IPC.

The base class for entities may have a default implementation that
doesn't hurt, for example, throwing an exception or returning an error
code to say that it doesn't have an EEPROM, so that inherited class
doesn't need to necessarily implement functions around EEPROM. Devices
are abstracted into the hwmon interface as the kernel does today, and
we need to config the names of each input attribute to make them
meaningful anyway.

I do see your concerns, and I do believe this requires further
research into if this model can handle all the concerns or
requirements we have today.

>
> >
> > The existing JSON files used in the dynamic software stack can only represent data, but not any control flow. This led to difficulties where sometimes some code is preferred to have for aiding the discovery of hardware topology, condensing redundant configurations, etc. With a good framework for hardware topology, combining the entity abstraction described above, developers can easily find the best places to aid the topology discovery, implement hardware initialization logics, and optimize BMC tasks according to Linux behaviors.
>
> If you need code or control flow to aid in the discovery of hardware
> topology, write an application that exposes an interface similar to
> FruDevice, or the soon to be submitted peci-pcie.  These can be used
> in entity manager configs.  I'm not quite following what "redundant
> configurations" means in this context.  In my experience, most
> redundant configurations tend to be for things like power supplies, or
> drives, where a single device can fit in many different slots.  WIth
> that said, we already have an abstraction for that, so I'm not quite
> following.

Please see this for a complicated discovery logic:
https://github.com/alex310110/bmc-proto-20q2/blob/master/downstream/board_example_a.py

Based on your reply, I have a concern that, if we have a hardware
topology complicated enough, does that mean we should probably opt out
of FruDevice and use downstream daemon to replace it?

> >
> > Better open source and proprietary part management
> >
> > Construct "Improvements" like a proprietary software supporting plugins. The philosophy is that the architecture of "Improvements" should be solid enough that the community won't have to modify the upstream code much. The community can look at and reference the code upstream to develop their own code and configs according to their hardware, while the plugin-able part may be proprietary and can be kept downstream without conflicts. "Improvements" should have a reasonable plugin API to support common BMC functionality in the high level, and provide common low-level APIs to support the plugins by abstracting things like hwmon sysfs interface. This can be implemented using a plugin system or a flexible build system, as we are working on an open source project indeed. Whenever we find a potential conflict between upstream and downstream, let us work it out to see if it is appropriate to make it pluginable or configurable via config files.
>
> I must be misreading this, as I feel like openbmc already has
> "plugins" in the form of Dbus applications.  Many applications have
> been written that required no modification to upstream code.  Tha API
> you're looking for is reasonably well defined in phosphor dbus
> interfaces, and is intended to be reasonably stable, even if it's not
> guaranteed over time.  I'm also a little confused at what you're
> calling low-level APIs.  hwmon sysfs is a low level API.  Are you
> wanting to wrap it in yet another API that's OpenBMC specific?
> "can be kept downstream without conflicts" - In my experience, you're
> going to be hard pressed to find support for supporting closed source
> development in an open source project.  That's not to say individuals
> aren't out there, but they tend to keep their heads down :)

Apologies for my wording; the low-level API may be probably called
lower level libraries offered by OpenBMC. See I2CHwmonDevice in
https://github.com/alex310110/bmc-proto-20q2/blob/master/i2c.py

Although we make a lot of efforts to upstream software to the open
source community as much as possible, BMC is heavily involved with
hardware, and we're also restricted to hardware's restrictions. We are
having difficulties to upstream drivers or code containing
confidential hardware code names, or containing part numbers under NDA
with vendors. Personally I was also involved with a lengthy and
exhausting internal legal review to publicize a part number which is
under NDA with our vendor, involving email exchanges between attorneys
in Google and our vendor's support engineer. I hope this explains my
point. For today, these part numbers are required to pass onto dbus
from entity-manager in order for dbus-sensors to determine the correct
sensor daemon for them.

>
> >
> > Flexibility for alternatives
> >
> > Although hwmon sysfs interface is a good starting point for getting sensor reads from devices, they have their own limitations. The interface does not abstract every register perfectly, especially when device registers are not designed to follow some common specs like PMBus, and it does not provide controls to the devices.
> >
> >
> > I propose a Device Abstraction Layer to wrap around devices. The underlying can completely map to hwmon sysfs, or allow user-space driver implementation if necessary, or even hybrid. This will easily provide an additional interface to bypass the driver and control the devices, while still maintaining the benefit to use an off-the-shelf Linux device driver.
>
> In this context, what are you calling a "device"?  I think everything
> you're looking for exists, although it sounds like it's not in the
> form you're wanting to see.  Dbus sensors already does a hwmon to Dbus
> sensor abstraction conversion, that in some cases maps 1:1, or in some
> cases is a "hybrid" as you call it.  Are you looking for something in
> the middle, so instead of going hwmon -> Dbus  and libmctp -> Dbus you
> would want hwmon -> DAL -> Dbus  and libmctp -> DAL -> Dbus?  There
> could be some advantages here, but I have a worry that it'll be
> difficult to come up with a reasonable "device" api.  Devices take a
> lot of forms, in band, out of band, all with varying requirements
> around threading, permissions, and eventing.  While it's possible to
> cover everything that's needed, I'd be worried we'd be able to cover a
> majority of them.

Yep, that requires some research or others' experience; I'm mostly
familiar with I2C devices in my area of work.

>
> >
> > Quite some existing code is heavily bound to or influenced by the IPMI protocol layer that we are having right now: We use “uint8_t” type for I2C bus number in entity-manager for example, while Linux kernel can extend the logical I2C bus number to more than 512 without any issues.
> Can you come up with a better example?  We've tried to be very careful
> to not have IPMI-specific things in the interfaces, and to make them
> as generic as possible.  In that case, uint8_t is used to represent
> the 7 bit addressing (plus read write bit) on the I2C bus itself, not
> the uint8_t in the IPMI spec.  The API you listed neglected to handle
> the possibility of 11 bit I2C addressing, as it isn't very common in
> practice, but the argument could certainly be made that the interface
> should be changed to a uint16_t, and I would expect the IPMI layer to
> simply filter addresses above 127 that it's not able to support.

Please see getFruInfo() calls in FruDevice.cpp:
https://github.com/openbmc/entity-manager/blob/master/src/FruDevice.cpp#L1108

The uint8_t bus of getFruInfo() restricted the number of logical I2C
buses that we could implement in the sysfs interface, and it was
unfortunately static_cast'ed to uint_8 which created a bug hard to
debug. I don't have much experience to find you more examples,
however... I believe some of these can be fixed within the current
architecture, nevertheless I'm still trying to emphasize this concept.

>
> > The current dynamic software stack emphasizes individual sensors, but the BMC handles many more tasks than just only sensors. The practicality of OpenBMC for hardware engineers is also hindered by the IPMI as described above in Issue Examples.
> Sensors were the first thing tackled, as those are the things that
> tend to be the most different platform to platform, and have the most
> peculiar settings.  We do also handle topology to some extent, as well
> as a lot of other commands that are not IPMI specific.  I agree, IPMI
> has its flaws, but OpenBMC also has pretty good support for Redfish,
> direct dbus, and upcoming MCTP if that's what you'd rather use as an
> outbound interface.

On that, I'm also looking forward to the ability to read sensors
within the BMC console in a human-friendly way for hardware engineers,
so that we don't have to rely on the host or network to read them
during bring-up, or simply because we don't have RedFish ready yet,
and hardware engineers just want to see tons of sensor readings for
bring-up.

>
> >
> >
> > We should have a core designed to consider varieties of tasks that BMC may be asked to handle: GPIO modifications, I2C manipulations,  The core should not be hindered by any protocol, but the protocol layer should find its own way to map the core APIs to its own protocol. This will help us to transit from IPMI to Redfish.
> Again, I'm having some trouble following here, as I think the "core"
> that exists now is very close to what you're asking for, and anywhere
> we've deviated has been very explicitly done.
> GPIO modifications and inputs are handled by the libgpio abstraction
> (or sysfs).   This is well documented, and has a lot of tooling
> wrapped around it.  There are lots of examples, as well as
> phosphor-gpio, which in some cases, can directly manage the GPIO->Dbus
> conversions (disclaimer, I have not used it myself, as I tend to go
> directly to libgpio when I need that ability).
> I2C is handled by the linux i2c devices, or the linux i2c bus
> abstraction.  Again, it's well documented with lots of tooling.  There
> are lots of examples of it being used.
> Neither of the above have any hindrance on any protocol, and both are
> used indirectly through the abstraction in Redfish, IPMI, and the
> custom REST implementation. The "core APIs" as you call them are
> supported by DBus, although I suspect in your case, core implies
> single producer, which dbus is not.

Sorry for any confusion. I think I'm trying to repeat myself by
emphasizing on interleaving protocol layer in this paragraph. Today's
OpenBMC does build with this in mind, but there are still some flaws
left to improve, the uint8_t bus variable described above for example.

>
> Hopefully the above helped,
>
> -Ed

Re: Feedback on Current OpenBMC and Proposing Some Improvements ---- "Improvements" Ideas

[Ed Tanous]

[-- Attachment #1: Type: text/plain, Size: 19464 bytes --]

On Tue, Jun 23, 2020 at 6:31 PM Alex Qiu <xqiu@google.com> wrote:
>
> Hi Ed,
>
> -Internal email list
> +A couple of folks who might be interested in this topic
>
> I don't know if you saw the updated reply in the main thread, but I
> foresaw some possible communication gap, so I created a simple demo to
> illustrate my ideas: https://github.com/alex310110/bmc-proto-20q2
> Please note that I'm not trying to code a BMC with Python, but it's
> just for the ease to set up a demo fast. Other replies inlined.

I did see it, and it shows a lot of my problems with that approach.  Out of
curiosity, why did you start with python, instead of something we could try
on a BMC?  Even if it doesn't compile, it might be a starting point for
someone else?

I see a few anti-patterns there that I'd like to see you address, you've
hardcoded lots of data that's not specific to the card.  At first glance in
board_example_a.py

1. Line 22-23.  You've initialized 2 Muxes.  Both of these buses are
present on your (guessing a little here) baseboard, and not the card
itself.  This means that every single card will need to duplicate the
initialization of these muxes.  So first step, you need to break apart your
baseboard into a separate entity, so the "board" does not own the card.
Also, you haven't provided any mapping of a PCIe mux lane to a physical
user-facing name "Slot 1, Slot 2, ect".  Entity-manager configs do both of
these things.
2. You've only expressed the slot topology here.  CardExampleG, and
CardExampleV need to know what bus they're on, what muxes they need to go
through to get to that bus, and the organization of those things, as in
your example, none of the busses have been created in the kernel, and some
of the mux busses are shared.
3. You've hardcoded to only search for 2 different cards (card_g, and
card_v), at 1 address (0x52).  While it would be great if systems in
practice had that kind of consistency in addressing, PCIe add in cards have
many different eeprom addresses.  So you'd have to update your loops to
search for all possible.  Also, that loop scales great if you only support
2 cards.  What happens when OpenBMC supports 100 cards?  1000?  You've
hardcoded the list of supported cards in the entity above it, which means
every baseboard needs to explicitly add support for every possible card.
This stops scaling really fast.
4. You're looping over the PCIe slots as part of the board control flow.
What if slots are based on a riser plugged into said slot?
5. You've abstracted an eeprom to a simple device.  In practice, you need
to parse the FRU data, which might be in several formats.  Sure, you could
have a library function, but you still need a global structure to keep
that, in case some other control flow needs it downstream.
6. You've hardcoded a mux address, and a physical channel again later on.
7. Line 71-72.  Both of those are blocking calls.  For devices with a large
number of sensors, those blocking calls will cause performance bottlenecks.
also, see my previous comments about non-cyclic timing of some sensors.
8. You're missing a lot of features that entity manager does today.  Fan
control configs being the most important, which have a relation to how the
chassis looks.  Can you add an example of a chassis with some fans and
thermal configs in it?

If you made all the changes I'm suggesting your code starts to look a LOT
like entity manager, FRUDevice, and dbus-sensors combined into a single
app.  The biggest difference is you've replaced config files and exposes
records for library functions.  There's nothing inherently wrong with
combining them like that, but we wanted to isolate the topology scanning
logic from the config logic, so people would feel free to swap them out
with their own.  In the case of some systems, there's a complete database
of the hardware inventory in a proprietary format.  In the case of
infrastructure managed systems, we wanted developers to have the ability to
swap out the topology scanning logic for some fixed "Here are the list of
the hardware devices that should be present" type daemons that support the
various formats, without necessarily having to care about the
implementations.  Said another way, it separates "How do I determine if
this device is present" from "Here's how to interact with this device".  We
could combine those again, but we lose out on the static case.  If nobody
cares about the full config case, we could certainly consider it.
 One other big thing I wanted to be able to support in the future with this
was adding previously unknown devices at runtime, with zero need to compile
code.  Imagine being able to support a temp sensor on a new card by simply
uploading an entity manager config file to the webserver, having it rerun
the detect, and suddenly that card is "supported" by that image.  When you
mix the code in with the metadata or config, you lose that ability, as we
can't easily upload unsigned code.  It's a tradeoff for sure, but being
able to hand tweak a config at runtime can be invaluable for quick
turnaround during debugging and platform bringup.


>
> On Sun, Jun 21, 2020 at 3:16 PM Ed Tanous <ed@tanous.net> wrote:
> >
> > On Thu, Jun 18, 2020 at 2:29 PM Alex Qiu <xqiu@google.com> wrote:
>
> I didn't start with multi-threading too much in mind, but It's not
> necessarily a single-threaded model. As you can see in the demo, each
> entity instance has its own function of update_sensors(), and the
> entities are collected in the main class, which may be implemented as
> a higher-level inter-process communication API for entities to adapt
> to instead of merely a single main function or thread. So this model
> can potentially be threaded on an entity basis, or even fork each
> entity into individual processes. The model can be further threaded
> within each entity into service threads: separating sensor polling
> loop and event handling loop for example. But I do wonder about the
> performance overhead of making every function call into IPC.
I'm not really following.  Could you give an example of calling 4 commands,
in parallel, to a MCTP/IPMB device and posting them as they are received?
This is something that the existing sensor daemons do today.  Yeah, IPC is
expensive, but moving away from dbus, and onto something else is a much
bigger discussion.


>
> The base class for entities may have a default implementation that
> doesn't hurt, for example, throwing an exception or returning an error
> code to say that it doesn't have an EEPROM, so that inherited class
> doesn't need to necessarily implement functions around EEPROM. Devices
> are abstracted into the hwmon interface as the kernel does today, and
> we need to config the names of each input attribute to make them
> meaningful anyway.
>
> I do see your concerns, and I do believe this requires further
> research into if this model can handle all the concerns or
> requirements we have today.

Looking forward to seeing it.

>
> >
> > >
> > > The existing JSON files used in the dynamic software stack can only
represent data, but not any control flow. This led to difficulties where
sometimes some code is preferred to have for aiding the discovery of
hardware topology, condensing redundant configurations, etc. With a good
framework for hardware topology, combining the entity abstraction described
above, developers can easily find the best places to aid the topology
discovery, implement hardware initialization logics, and optimize BMC tasks
according to Linux behaviors.
> >
> > If you need code or control flow to aid in the discovery of hardware
> > topology, write an application that exposes an interface similar to
> > FruDevice, or the soon to be submitted peci-pcie.  These can be used
> > in entity manager configs.  I'm not quite following what "redundant
> > configurations" means in this context.  In my experience, most
> > redundant configurations tend to be for things like power supplies, or
> > drives, where a single device can fit in many different slots.  WIth
> > that said, we already have an abstraction for that, so I'm not quite
> > following.
>
> Please see this for a complicated discovery logic:
>
https://github.com/alex310110/bmc-proto-20q2/blob/master/downstream/board_example_a.py
>
> Based on your reply, I have a concern that, if we have a hardware
> topology complicated enough, does that mean we should probably opt out
> of FruDevice and use downstream daemon to replace it?

FruDevice is poorly named these days (sorry James).  It should really be
called I2cFruEepromLocator.  In theory, it can handle any I2C topology we
were able to throw at it, including one that I tested that was 4 levels
deep.  If you're trying to manage an automatically detected i2c eeprom/mux
topology, that is the tool I would expect to use.  With that said, you're
welcome to write others, if you need to handle other things on I2C, or the
static config case from above.
If you're managing a different source of data (like a host driven map,
MCTP, or out of band PCIe registers) I would expect you'd likely want to
write another daemon that's capable of posting that topology data to dbus,
but I would expect you can still use entity manager to consume it, and
apply the correct settings to sensors/busses/kernel/Fans.

>
> > >
> > > Better open source and proprietary part management
> > >
> > > Construct "Improvements" like a proprietary software supporting
plugins. The philosophy is that the architecture of "Improvements" should
be solid enough that the community won't have to modify the upstream code
much. The community can look at and reference the code upstream to develop
their own code and configs according to their hardware, while the
plugin-able part may be proprietary and can be kept downstream without
conflicts. "Improvements" should have a reasonable plugin API to support
common BMC functionality in the high level, and provide common low-level
APIs to support the plugins by abstracting things like hwmon sysfs
interface. This can be implemented using a plugin system or a flexible
build system, as we are working on an open source project indeed. Whenever
we find a potential conflict between upstream and downstream, let us work
it out to see if it is appropriate to make it pluginable or configurable
via config files.
> >
> > I must be misreading this, as I feel like openbmc already has
> > "plugins" in the form of Dbus applications.  Many applications have
> > been written that required no modification to upstream code.  Tha API
> > you're looking for is reasonably well defined in phosphor dbus
> > interfaces, and is intended to be reasonably stable, even if it's not
> > guaranteed over time.  I'm also a little confused at what you're
> > calling low-level APIs.  hwmon sysfs is a low level API.  Are you
> > wanting to wrap it in yet another API that's OpenBMC specific?
> > "can be kept downstream without conflicts" - In my experience, you're
> > going to be hard pressed to find support for supporting closed source
> > development in an open source project.  That's not to say individuals
> > aren't out there, but they tend to keep their heads down :)
>
> Apologies for my wording; the low-level API may be probably called
> lower level libraries offered by OpenBMC. See I2CHwmonDevice in
> https://github.com/alex310110/bmc-proto-20q2/blob/master/i2c.py
I2CDevices, i2CMuxes, HWMonDevices, and i2ceeproms exist in the kernel
already, behind a well defined interface.  Your file feels a little bit
like it's reinventing some things.  I'm not sure whether or not I'd be
against inventing libopenbmc, but that's likely where those types of
interfaces would need to go.
It should also be noted, all of those devices are addable with only EM
configuration file changes today.


>
> Although we make a lot of efforts to upstream software to the open
> source community as much as possible, BMC is heavily involved with
> hardware, and we're also restricted to hardware's restrictions. We are
> having difficulties to upstream drivers or code containing
> confidential hardware code names, or containing part numbers under NDA
> with vendors. Personally I was also involved with a lengthy and
> exhausting internal legal review to publicize a part number which is
> under NDA with our vendor, involving email exchanges between attorneys
> in Google and our vendor's support engineer. I hope this explains my
> point. For today, these part numbers are required to pass onto dbus
> from entity-manager in order for dbus-sensors to determine the correct
> sensor daemon for them.

Understood, and I've felt your pain before.  I'm not going to claim this is
easily solved, but the best way IMO, is to create a downstream application
for each hardware device you need to manage, and patch your entity manager
configs to add the configuration data for those components to your boards
(or keep the board configs totally private).  Any changes to the detection
logic, or entity manager itself can be easily upstreamed.  The application
boundary also means that there's a well defined dbus interface, and any
licencing conflicts between GPL and proprietary code are resolved.


>
> >
> > >
> > > Flexibility for alternatives
> > >
> > > Although hwmon sysfs interface is a good starting point for getting
sensor reads from devices, they have their own limitations. The interface
does not abstract every register perfectly, especially when device
registers are not designed to follow some common specs like PMBus, and it
does not provide controls to the devices.
> > >
> > >
> > > I propose a Device Abstraction Layer to wrap around devices. The
underlying can completely map to hwmon sysfs, or allow user-space driver
implementation if necessary, or even hybrid. This will easily provide an
additional interface to bypass the driver and control the devices, while
still maintaining the benefit to use an off-the-shelf Linux device driver.
> >
> > In this context, what are you calling a "device"?  I think everything
> > you're looking for exists, although it sounds like it's not in the
> > form you're wanting to see.  Dbus sensors already does a hwmon to Dbus
> > sensor abstraction conversion, that in some cases maps 1:1, or in some
> > cases is a "hybrid" as you call it.  Are you looking for something in
> > the middle, so instead of going hwmon -> Dbus  and libmctp -> Dbus you
> > would want hwmon -> DAL -> Dbus  and libmctp -> DAL -> Dbus?  There
> > could be some advantages here, but I have a worry that it'll be
> > difficult to come up with a reasonable "device" api.  Devices take a
> > lot of forms, in band, out of band, all with varying requirements
> > around threading, permissions, and eventing.  While it's possible to
> > cover everything that's needed, I'd be worried we'd be able to cover a
> > majority of them.
>
> Yep, that requires some research or others' experience; I'm mostly
> familiar with I2C devices in my area of work.
>
> >
> > >
> > > Quite some existing code is heavily bound to or influenced by the
IPMI protocol layer that we are having right now: We use “uint8_t” type for
I2C bus number in entity-manager for example, while Linux kernel can extend
the logical I2C bus number to more than 512 without any issues.
> > Can you come up with a better example?  We've tried to be very careful
> > to not have IPMI-specific things in the interfaces, and to make them
> > as generic as possible.  In that case, uint8_t is used to represent
> > the 7 bit addressing (plus read write bit) on the I2C bus itself, not
> > the uint8_t in the IPMI spec.  The API you listed neglected to handle
> > the possibility of 11 bit I2C addressing, as it isn't very common in
> > practice, but the argument could certainly be made that the interface
> > should be changed to a uint16_t, and I would expect the IPMI layer to
> > simply filter addresses above 127 that it's not able to support.
>
> Please see getFruInfo() calls in FruDevice.cpp:
>
https://github.com/openbmc/entity-manager/blob/master/src/FruDevice.cpp#L1108
>
> The uint8_t bus of getFruInfo() restricted the number of logical I2C
> buses that we could implement in the sysfs interface, and it was
> unfortunately static_cast'ed to uint_8 which created a bug hard to
> debug. I don't have much experience to find you more examples,
> however... I believe some of these can be fixed within the current
> architecture, nevertheless I'm still trying to emphasize this concept.
OH, you mean you hit a uint8_t limit on busses!  I don't know of anyone
that has crossed the 256 bus limit, so you've clearly found a bug/missing
feature.  Now it's your time to shine.  You've found an issue, you know
what the fix is, exactly where the code needs to go and you have the
ability to test it.  Write a patch to fix it, test that it does what you
want, write up a commit message explaining exactly what you detailed above,
how you tested it, and submit it to gerrit with the maintainer as a
reviewer.  The maintainer is very responsive, and you'll have fixed
something hard to debug for the next person that runs into this.


>
> >
> > > The current dynamic software stack emphasizes individual sensors, but
the BMC handles many more tasks than just only sensors. The practicality of
OpenBMC for hardware engineers is also hindered by the IPMI as described
above in Issue Examples.
> > Sensors were the first thing tackled, as those are the things that
> > tend to be the most different platform to platform, and have the most
> > peculiar settings.  We do also handle topology to some extent, as well
> > as a lot of other commands that are not IPMI specific.  I agree, IPMI
> > has its flaws, but OpenBMC also has pretty good support for Redfish,
> > direct dbus, and upcoming MCTP if that's what you'd rather use as an
> > outbound interface.
>
> On that, I'm also looking forward to the ability to read sensors
> within the BMC console in a human-friendly way for hardware engineers,
> so that we don't have to rely on the host or network to read them
> during bring-up, or simply because we don't have RedFish ready yet,
> and hardware engineers just want to see tons of sensor readings for
> bring-up.

I'm not following this as anything actionable.  OpenBMC has IPMItool, dbus
tools, i2c-tools, the Redfish GUI, the rest-dbus GUI and the Webui to pick
from for "human friendly way for hardware engineers".  Heck, if you're
feeling really enterprising, you can install the HWmon devices in the bash
console, and CAT out the values in another.  In this comment, are you
wanting something else?  Surely one of those meets your prototyping needs?


>
> Sorry for any confusion. I think I'm trying to repeat myself by
> emphasizing on interleaving protocol layer in this paragraph. Today's
> OpenBMC does build with this in mind, but there are still some flaws
> left to improve, the uint8_t bus variable described above for example.
>
See above.  Let's get that uint8_t thing fixed on master so we're not all
talking about it here again in 6 months when the next poor person hits the
same issue and spends a week debugging it.
-- 
-Ed

[-- Attachment #2: Type: text/html, Size: 22465 bytes --]

Re: Feedback on Current OpenBMC and Proposing Some Improvements ---- "Improvements" Ideas

[Alex Qiu]

Hi Ed,

I used Python because I wanted to create a demo fast, and get back to
the issues that I need to work on. The thing I'm doing here is kinda
out of my direct work area based on the organization.

Yes, there are some restrictions in my current demo, and I'm afraid
that I may not have the bandwidth to cover it further alone. My point
is that, sometimes hardwares is designed with some unexpected
complexity on topology (EEPROM behind MUX for example). Having the
ability to aid the topology discovery with code, and having the
topology info available to other functionalities can help a lot. JSON
config files are having a hard time bearing these logics, and any
extra logic implemented in JSON config files requires some kind of
script parser in daemons processing them. Based on your replies, the
concept for functionally extensions that I was asking for should be
implemented as daemons either standalone or plugged onto dbus?

On "reading sensors within the BMC console", I'm actually using a
script to directly read from hwmon right now, because we are having
sensor number limit on IPMI and performance issues with IPMI and dbus.
We are still actively investigating these performance issues now to
unblock the project, but based on the current findings, I think it's
better to have this tool before the protocol layers.

On issues like uint8_t, yes, we've noted them down, but they are still
tech debts on our backlog, and dealing with the performance issue
described above remains as our priority right now.

Thank you!

- Alex Qiu


On Thu, Jun 25, 2020 at 7:44 AM Ed Tanous <ed@tanous.net> wrote:
>
>
> On Tue, Jun 23, 2020 at 6:31 PM Alex Qiu <xqiu@google.com> wrote:
> >
> > Hi Ed,
> >
> > -Internal email list
> > +A couple of folks who might be interested in this topic
> >
> > I don't know if you saw the updated reply in the main thread, but I
> > foresaw some possible communication gap, so I created a simple demo to
> > illustrate my ideas: https://github.com/alex310110/bmc-proto-20q2
> > Please note that I'm not trying to code a BMC with Python, but it's
> > just for the ease to set up a demo fast. Other replies inlined.
>
> I did see it, and it shows a lot of my problems with that approach.  Out of curiosity, why did you start with python, instead of something we could try on a BMC?  Even if it doesn't compile, it might be a starting point for someone else?
>
> I see a few anti-patterns there that I'd like to see you address, you've hardcoded lots of data that's not specific to the card.  At first glance in board_example_a.py
>
> 1. Line 22-23.  You've initialized 2 Muxes.  Both of these buses are present on your (guessing a little here) baseboard, and not the card itself.  This means that every single card will need to duplicate the initialization of these muxes.  So first step, you need to break apart your baseboard into a separate entity, so the "board" does not own the card.  Also, you haven't provided any mapping of a PCIe mux lane to a physical user-facing name "Slot 1, Slot 2, ect".  Entity-manager configs do both of these things.
> 2. You've only expressed the slot topology here.  CardExampleG, and CardExampleV need to know what bus they're on, what muxes they need to go through to get to that bus, and the organization of those things, as in your example, none of the busses have been created in the kernel, and some of the mux busses are shared.
> 3. You've hardcoded to only search for 2 different cards (card_g, and card_v), at 1 address (0x52).  While it would be great if systems in practice had that kind of consistency in addressing, PCIe add in cards have many different eeprom addresses.  So you'd have to update your loops to search for all possible.  Also, that loop scales great if you only support 2 cards.  What happens when OpenBMC supports 100 cards?  1000?  You've hardcoded the list of supported cards in the entity above it, which means every baseboard needs to explicitly add support for every possible card.  This stops scaling really fast.
> 4. You're looping over the PCIe slots as part of the board control flow.  What if slots are based on a riser plugged into said slot?
> 5. You've abstracted an eeprom to a simple device.  In practice, you need to parse the FRU data, which might be in several formats.  Sure, you could have a library function, but you still need a global structure to keep that, in case some other control flow needs it downstream.
> 6. You've hardcoded a mux address, and a physical channel again later on.
> 7. Line 71-72.  Both of those are blocking calls.  For devices with a large number of sensors, those blocking calls will cause performance bottlenecks. also, see my previous comments about non-cyclic timing of some sensors.
> 8. You're missing a lot of features that entity manager does today.  Fan control configs being the most important, which have a relation to how the chassis looks.  Can you add an example of a chassis with some fans and thermal configs in it?
>
> If you made all the changes I'm suggesting your code starts to look a LOT like entity manager, FRUDevice, and dbus-sensors combined into a single app.  The biggest difference is you've replaced config files and exposes records for library functions.  There's nothing inherently wrong with combining them like that, but we wanted to isolate the topology scanning logic from the config logic, so people would feel free to swap them out with their own.  In the case of some systems, there's a complete database of the hardware inventory in a proprietary format.  In the case of infrastructure managed systems, we wanted developers to have the ability to swap out the topology scanning logic for some fixed "Here are the list of the hardware devices that should be present" type daemons that support the various formats, without necessarily having to care about the implementations.  Said another way, it separates "How do I determine if this device is present" from "Here's how to interact with this device".  We could combine those again, but we lose out on the static case.  If nobody cares about the full config case, we could certainly consider it.
>  One other big thing I wanted to be able to support in the future with this was adding previously unknown devices at runtime, with zero need to compile code.  Imagine being able to support a temp sensor on a new card by simply uploading an entity manager config file to the webserver, having it rerun the detect, and suddenly that card is "supported" by that image.  When you mix the code in with the metadata or config, you lose that ability, as we can't easily upload unsigned code.  It's a tradeoff for sure, but being able to hand tweak a config at runtime can be invaluable for quick turnaround during debugging and platform bringup.
>
>
> >
> > On Sun, Jun 21, 2020 at 3:16 PM Ed Tanous <ed@tanous.net> wrote:
> > >
> > > On Thu, Jun 18, 2020 at 2:29 PM Alex Qiu <xqiu@google.com> wrote:
> >
> > I didn't start with multi-threading too much in mind, but It's not
> > necessarily a single-threaded model. As you can see in the demo, each
> > entity instance has its own function of update_sensors(), and the
> > entities are collected in the main class, which may be implemented as
> > a higher-level inter-process communication API for entities to adapt
> > to instead of merely a single main function or thread. So this model
> > can potentially be threaded on an entity basis, or even fork each
> > entity into individual processes. The model can be further threaded
> > within each entity into service threads: separating sensor polling
> > loop and event handling loop for example. But I do wonder about the
> > performance overhead of making every function call into IPC.
> I'm not really following.  Could you give an example of calling 4 commands, in parallel, to a MCTP/IPMB device and posting them as they are received?  This is something that the existing sensor daemons do today.  Yeah, IPC is expensive, but moving away from dbus, and onto something else is a much bigger discussion.
>
>
> >
> > The base class for entities may have a default implementation that
> > doesn't hurt, for example, throwing an exception or returning an error
> > code to say that it doesn't have an EEPROM, so that inherited class
> > doesn't need to necessarily implement functions around EEPROM. Devices
> > are abstracted into the hwmon interface as the kernel does today, and
> > we need to config the names of each input attribute to make them
> > meaningful anyway.
> >
> > I do see your concerns, and I do believe this requires further
> > research into if this model can handle all the concerns or
> > requirements we have today.
>
> Looking forward to seeing it.
>
> >
> > >
> > > >
> > > > The existing JSON files used in the dynamic software stack can only represent data, but not any control flow. This led to difficulties where sometimes some code is preferred to have for aiding the discovery of hardware topology, condensing redundant configurations, etc. With a good framework for hardware topology, combining the entity abstraction described above, developers can easily find the best places to aid the topology discovery, implement hardware initialization logics, and optimize BMC tasks according to Linux behaviors.
> > >
> > > If you need code or control flow to aid in the discovery of hardware
> > > topology, write an application that exposes an interface similar to
> > > FruDevice, or the soon to be submitted peci-pcie.  These can be used
> > > in entity manager configs.  I'm not quite following what "redundant
> > > configurations" means in this context.  In my experience, most
> > > redundant configurations tend to be for things like power supplies, or
> > > drives, where a single device can fit in many different slots.  WIth
> > > that said, we already have an abstraction for that, so I'm not quite
> > > following.
> >
> > Please see this for a complicated discovery logic:
> > https://github.com/alex310110/bmc-proto-20q2/blob/master/downstream/board_example_a.py
> >
> > Based on your reply, I have a concern that, if we have a hardware
> > topology complicated enough, does that mean we should probably opt out
> > of FruDevice and use downstream daemon to replace it?
>
> FruDevice is poorly named these days (sorry James).  It should really be called I2cFruEepromLocator.  In theory, it can handle any I2C topology we were able to throw at it, including one that I tested that was 4 levels deep.  If you're trying to manage an automatically detected i2c eeprom/mux topology, that is the tool I would expect to use.  With that said, you're welcome to write others, if you need to handle other things on I2C, or the static config case from above.
> If you're managing a different source of data (like a host driven map, MCTP, or out of band PCIe registers) I would expect you'd likely want to write another daemon that's capable of posting that topology data to dbus, but I would expect you can still use entity manager to consume it, and apply the correct settings to sensors/busses/kernel/Fans.
>
> >
> > > >
> > > > Better open source and proprietary part management
> > > >
> > > > Construct "Improvements" like a proprietary software supporting plugins. The philosophy is that the architecture of "Improvements" should be solid enough that the community won't have to modify the upstream code much. The community can look at and reference the code upstream to develop their own code and configs according to their hardware, while the plugin-able part may be proprietary and can be kept downstream without conflicts. "Improvements" should have a reasonable plugin API to support common BMC functionality in the high level, and provide common low-level APIs to support the plugins by abstracting things like hwmon sysfs interface. This can be implemented using a plugin system or a flexible build system, as we are working on an open source project indeed. Whenever we find a potential conflict between upstream and downstream, let us work it out to see if it is appropriate to make it pluginable or configurable via config files.
> > >
> > > I must be misreading this, as I feel like openbmc already has
> > > "plugins" in the form of Dbus applications.  Many applications have
> > > been written that required no modification to upstream code.  Tha API
> > > you're looking for is reasonably well defined in phosphor dbus
> > > interfaces, and is intended to be reasonably stable, even if it's not
> > > guaranteed over time.  I'm also a little confused at what you're
> > > calling low-level APIs.  hwmon sysfs is a low level API.  Are you
> > > wanting to wrap it in yet another API that's OpenBMC specific?
> > > "can be kept downstream without conflicts" - In my experience, you're
> > > going to be hard pressed to find support for supporting closed source
> > > development in an open source project.  That's not to say individuals
> > > aren't out there, but they tend to keep their heads down :)
> >
> > Apologies for my wording; the low-level API may be probably called
> > lower level libraries offered by OpenBMC. See I2CHwmonDevice in
> > https://github.com/alex310110/bmc-proto-20q2/blob/master/i2c.py
> I2CDevices, i2CMuxes, HWMonDevices, and i2ceeproms exist in the kernel already, behind a well defined interface.  Your file feels a little bit like it's reinventing some things.  I'm not sure whether or not I'd be against inventing libopenbmc, but that's likely where those types of interfaces would need to go.
> It should also be noted, all of those devices are addable with only EM configuration file changes today.
>
>
> >
> > Although we make a lot of efforts to upstream software to the open
> > source community as much as possible, BMC is heavily involved with
> > hardware, and we're also restricted to hardware's restrictions. We are
> > having difficulties to upstream drivers or code containing
> > confidential hardware code names, or containing part numbers under NDA
> > with vendors. Personally I was also involved with a lengthy and
> > exhausting internal legal review to publicize a part number which is
> > under NDA with our vendor, involving email exchanges between attorneys
> > in Google and our vendor's support engineer. I hope this explains my
> > point. For today, these part numbers are required to pass onto dbus
> > from entity-manager in order for dbus-sensors to determine the correct
> > sensor daemon for them.
>
> Understood, and I've felt your pain before.  I'm not going to claim this is easily solved, but the best way IMO, is to create a downstream application for each hardware device you need to manage, and patch your entity manager configs to add the configuration data for those components to your boards (or keep the board configs totally private).  Any changes to the detection logic, or entity manager itself can be easily upstreamed.  The application boundary also means that there's a well defined dbus interface, and any licencing conflicts between GPL and proprietary code are resolved.
>
>
> >
> > >
> > > >
> > > > Flexibility for alternatives
> > > >
> > > > Although hwmon sysfs interface is a good starting point for getting sensor reads from devices, they have their own limitations. The interface does not abstract every register perfectly, especially when device registers are not designed to follow some common specs like PMBus, and it does not provide controls to the devices.
> > > >
> > > >
> > > > I propose a Device Abstraction Layer to wrap around devices. The underlying can completely map to hwmon sysfs, or allow user-space driver implementation if necessary, or even hybrid. This will easily provide an additional interface to bypass the driver and control the devices, while still maintaining the benefit to use an off-the-shelf Linux device driver.
> > >
> > > In this context, what are you calling a "device"?  I think everything
> > > you're looking for exists, although it sounds like it's not in the
> > > form you're wanting to see.  Dbus sensors already does a hwmon to Dbus
> > > sensor abstraction conversion, that in some cases maps 1:1, or in some
> > > cases is a "hybrid" as you call it.  Are you looking for something in
> > > the middle, so instead of going hwmon -> Dbus  and libmctp -> Dbus you
> > > would want hwmon -> DAL -> Dbus  and libmctp -> DAL -> Dbus?  There
> > > could be some advantages here, but I have a worry that it'll be
> > > difficult to come up with a reasonable "device" api.  Devices take a
> > > lot of forms, in band, out of band, all with varying requirements
> > > around threading, permissions, and eventing.  While it's possible to
> > > cover everything that's needed, I'd be worried we'd be able to cover a
> > > majority of them.
> >
> > Yep, that requires some research or others' experience; I'm mostly
> > familiar with I2C devices in my area of work.
> >
> > >
> > > >
> > > > Quite some existing code is heavily bound to or influenced by the IPMI protocol layer that we are having right now: We use “uint8_t” type for I2C bus number in entity-manager for example, while Linux kernel can extend the logical I2C bus number to more than 512 without any issues.
> > > Can you come up with a better example?  We've tried to be very careful
> > > to not have IPMI-specific things in the interfaces, and to make them
> > > as generic as possible.  In that case, uint8_t is used to represent
> > > the 7 bit addressing (plus read write bit) on the I2C bus itself, not
> > > the uint8_t in the IPMI spec.  The API you listed neglected to handle
> > > the possibility of 11 bit I2C addressing, as it isn't very common in
> > > practice, but the argument could certainly be made that the interface
> > > should be changed to a uint16_t, and I would expect the IPMI layer to
> > > simply filter addresses above 127 that it's not able to support.
> >
> > Please see getFruInfo() calls in FruDevice.cpp:
> > https://github.com/openbmc/entity-manager/blob/master/src/FruDevice.cpp#L1108
> >
> > The uint8_t bus of getFruInfo() restricted the number of logical I2C
> > buses that we could implement in the sysfs interface, and it was
> > unfortunately static_cast'ed to uint_8 which created a bug hard to
> > debug. I don't have much experience to find you more examples,
> > however... I believe some of these can be fixed within the current
> > architecture, nevertheless I'm still trying to emphasize this concept.
> OH, you mean you hit a uint8_t limit on busses!  I don't know of anyone that has crossed the 256 bus limit, so you've clearly found a bug/missing feature.  Now it's your time to shine.  You've found an issue, you know what the fix is, exactly where the code needs to go and you have the ability to test it.  Write a patch to fix it, test that it does what you want, write up a commit message explaining exactly what you detailed above, how you tested it, and submit it to gerrit with the maintainer as a reviewer.  The maintainer is very responsive, and you'll have fixed something hard to debug for the next person that runs into this.
>
>
> >
> > >
> > > > The current dynamic software stack emphasizes individual sensors, but the BMC handles many more tasks than just only sensors. The practicality of OpenBMC for hardware engineers is also hindered by the IPMI as described above in Issue Examples.
> > > Sensors were the first thing tackled, as those are the things that
> > > tend to be the most different platform to platform, and have the most
> > > peculiar settings.  We do also handle topology to some extent, as well
> > > as a lot of other commands that are not IPMI specific.  I agree, IPMI
> > > has its flaws, but OpenBMC also has pretty good support for Redfish,
> > > direct dbus, and upcoming MCTP if that's what you'd rather use as an
> > > outbound interface.
> >
> > On that, I'm also looking forward to the ability to read sensors
> > within the BMC console in a human-friendly way for hardware engineers,
> > so that we don't have to rely on the host or network to read them
> > during bring-up, or simply because we don't have RedFish ready yet,
> > and hardware engineers just want to see tons of sensor readings for
> > bring-up.
>
> I'm not following this as anything actionable.  OpenBMC has IPMItool, dbus tools, i2c-tools, the Redfish GUI, the rest-dbus GUI and the Webui to pick from for "human friendly way for hardware engineers".  Heck, if you're feeling really enterprising, you can install the HWmon devices in the bash console, and CAT out the values in another.  In this comment, are you wanting something else?  Surely one of those meets your prototyping needs?
>
>
> >
> > Sorry for any confusion. I think I'm trying to repeat myself by
> > emphasizing on interleaving protocol layer in this paragraph. Today's
> > OpenBMC does build with this in mind, but there are still some flaws
> > left to improve, the uint8_t bus variable described above for example.
> >
> See above.  Let's get that uint8_t thing fixed on master so we're not all talking about it here again in 6 months when the next poor person hits the same issue and spends a week debugging it.
> --
> -Ed

Re: Feedback on Current OpenBMC and Proposing Some Improvements ---- "Improvements" Ideas

[Ed Tanous]

[-- Attachment #1: Type: text/plain, Size: 3714 bytes --]

On Thu, Jun 25, 2020 at 6:08 PM Alex Qiu <xqiu@google.com> wrote:
> Yes, there are some restrictions in my current demo, and I'm afraid
> that I may not have the bandwidth to cover it further alone. My point
> is that, sometimes hardwares is designed with some unexpected
> complexity on topology (EEPROM behind MUX for example).
To my understanding this case is already handled.  Assign the mux to the
parent FRU config file, and the eeprom behind it will be detected
correctly.  With that said, this type of hardware (optional mux with an
eeprom behind it) is difficult to identify automatically with no other
impact, hence needing to explicitly add it to the parent board.  Can you
think of any other examples of unexpected topology that aren't covered?


> Having the
> ability to aid the topology discovery with code, and having the
> topology info available to other functionalities can help a lot. JSON
> config files are having a hard time bearing these logics, and any
> extra logic implemented in JSON config files requires some kind of
> script parser in daemons processing them.
The majority of the config parsing is also able to be done at compile time,
it just isn't implemented today.  With that said, the config file parsing
in practice takes up very little CPU time in the last profile I did, so it
hasn't been a priority.


> Based on your replies, the
> concept for functionally extensions that I was asking for should be
> implemented as daemons either standalone or plugged onto dbus?

I'm not understanding the distinction of standalone vs plugged into dbus,
but I'll hazard a guess, and say yes, the dbus interfaces to the rest of
the system is (one of) the project's intended extension points.  You can
either manipulate them from an existing daemon, or create an all new daemon
that has exactly the behavior you want.


>
> On "reading sensors within the BMC console", I'm actually using a
> script to directly read from hwmon right now, because we are having
> sensor number limit on IPMI and performance issues with IPMI and dbus.
> We are still actively investigating these performance issues now to
> unblock the project, but based on the current findings, I think it's
> better to have this tool before the protocol layers.
Have you considered opening a review with this tool to make it available to
others?  I'd recommend opening a review to put it in here:
https://github.com/openbmc/openbmc-tools
This repo is much less formal, but gives people a place for these "might be
useful to others" type scripts.  Write up a commit message with something
to the effect of "I wrote this tool, this is how you use it, I find it
makes platform development easier because X." and get it checked in.


>
> On issues like uint8_t, yes, we've noted them down, but they are still
> tech debts on our backlog, and dealing with the performance issue
> described above remains as our priority right now.

It sounds like you're swamped for time, which I can respect.  With that
said, If you start by making technical improvements on small things like
the above, you're much more likely to have feedback (and help) when you
propose more wide sweeping changes, like your python example.
If you ever get free time, and want to continue moving your proposal more
toward an actionable change we can make, I'm happy to help discuss
options.  To be clear, I think if you can resolve some of the technical
limitations of your proposal, and put together a patchset that implements
it in a language that the project can use on a majority of platforms, I
think it could be a better developer experience.  We just can't remove some
of the user facing features that are implemented and/or planned already.
-- 
-Ed

[-- Attachment #2: Type: text/html, Size: 4443 bytes --]

Re: Feedback on Current OpenBMC and Proposing Some Improvements ---- "Improvements" Ideas

[Alex Qiu]

On Mon, Jun 29, 2020 at 7:53 AM Ed Tanous <ed@tanous.net> wrote:
>
>
> On Thu, Jun 25, 2020 at 6:08 PM Alex Qiu <xqiu@google.com> wrote:
> > Yes, there are some restrictions in my current demo, and I'm afraid
> > that I may not have the bandwidth to cover it further alone. My point
> > is that, sometimes hardwares is designed with some unexpected
> > complexity on topology (EEPROM behind MUX for example).
> To my understanding this case is already handled.  Assign the mux to the parent FRU config file, and the eeprom behind it will be detected correctly.  With that said, this type of hardware (optional mux with an eeprom behind it) is difficult to identify automatically with no other impact, hence needing to explicitly add it to the parent board.  Can you think of any other examples of unexpected topology that aren't covered?

There's no parent FRU in this case; the MUX belongs to the specific
FRU, and its EEPROM is behind the MUX. Unfortunately, maybe usually we
only realize some hardware design is problematic to the software until
we see it? :) I haven't started in Google when these boards were
designed, and I'm not so sure if I could point it out even if I had
been started in Google.

>
>
> > Having the
> > ability to aid the topology discovery with code, and having the
> > topology info available to other functionalities can help a lot. JSON
> > config files are having a hard time bearing these logics, and any
> > extra logic implemented in JSON config files requires some kind of
> > script parser in daemons processing them.
> The majority of the config parsing is also able to be done at compile time, it just isn't implemented today.  With that said, the config file parsing in practice takes up very little CPU time in the last profile I did, so it hasn't been a priority.

I'm not quite concerned about CPU time on the parsing, but more on the
burden of developing. Because right now I feel like we need to
implement a parser per daemon for what it's consuming. Unless we agree
on a format and implement an OpenBMC library for it. Take the Virtual
Sensor design doc under review for example:
https://gerrit.openbmc-project.xyz/c/openbmc/docs/+/32345/ I think it
will also have its own parser to deal with the "Algo" attribute. To
make more fragments, right now entity-manager does the calculation
without support for parenthesis and does not follow arithmetic order
of operations, and we are trying to come up with one supporting
parenthesis without breaking the compatibility.

>
>
> > Based on your replies, the
> > concept for functionally extensions that I was asking for should be
> > implemented as daemons either standalone or plugged onto dbus?
>
> I'm not understanding the distinction of standalone vs plugged into dbus, but I'll hazard a guess, and say yes, the dbus interfaces to the rest of the system is (one of) the project's intended extension points.  You can either manipulate them from an existing daemon, or create an all new daemon that has exactly the behavior you want.
>
>
> >
> > On "reading sensors within the BMC console", I'm actually using a
> > script to directly read from hwmon right now, because we are having
> > sensor number limit on IPMI and performance issues with IPMI and dbus.
> > We are still actively investigating these performance issues now to
> > unblock the project, but based on the current findings, I think it's
> > better to have this tool before the protocol layers.
> Have you considered opening a review with this tool to make it available to others?  I'd recommend opening a review to put it in here:
> https://github.com/openbmc/openbmc-tools
> This repo is much less formal, but gives people a place for these "might be useful to others" type scripts.  Write up a commit message with something to the effect of "I wrote this tool, this is how you use it, I find it makes platform development easier because X." and get it checked in.

It had topology information and sensor information that we would like
baked in as its major part, so unfortunately it's not an upstream-able
script...

>
>
> >
> > On issues like uint8_t, yes, we've noted them down, but they are still
> > tech debts on our backlog, and dealing with the performance issue
> > described above remains as our priority right now.
>
> It sounds like you're swamped for time, which I can respect.  With that said, If you start by making technical improvements on small things like the above, you're much more likely to have feedback (and help) when you propose more wide sweeping changes, like your python example.
> If you ever get free time, and want to continue moving your proposal more toward an actionable change we can make, I'm happy to help discuss options.  To be clear, I think if you can resolve some of the technical limitations of your proposal, and put together a patchset that implements it in a language that the project can use on a majority of platforms, I think it could be a better developer experience.  We just can't remove some of the user facing features that are implemented and/or planned already.

Makes sense. We'll see if we could gather enough resources at some
time to actually make it a concrete product, or we can come up with a
plan to improve the existing ones bit by bit. It's been a pleasure to
hear from you on what I haven't realized or taken into account yet,
because my team was more hardware project focused and had less
exposure to the general OpenBMC discussions or design philosophies.
Thank you!

- Alex Qiu

Re: Feedback on Current OpenBMC and Proposing Some Improvements ---- "Improvements" Ideas

[Ed Tanous]

On Mon, Jun 29, 2020 at 3:09 PM Alex Qiu <xqiu@google.com> wrote:
>
> On Mon, Jun 29, 2020 at 7:53 AM Ed Tanous <ed@tanous.net> wrote:
> >
> >
> > On Thu, Jun 25, 2020 at 6:08 PM Alex Qiu <xqiu@google.com> wrote:
> > > Yes, there are some restrictions in my current demo, and I'm afraid
> > > that I may not have the bandwidth to cover it further alone. My point
> > > is that, sometimes hardwares is designed with some unexpected
> > > complexity on topology (EEPROM behind MUX for example).
> > To my understanding this case is already handled.  Assign the mux to the parent FRU config file, and the eeprom behind it will be detected correctly.  With that said, this type of hardware (optional mux with an eeprom behind it) is difficult to identify automatically with no other impact, hence needing to explicitly add it to the parent board.  Can you think of any other examples of unexpected topology that aren't covered?
>
> There's no parent FRU in this case; the MUX belongs to the specific
> FRU, and its EEPROM is behind the MUX.

I called the baseboard a FRU, that was my bad and I suspect you got
confused.  I should've said baseboard "entity".  The FRU you're trying
to detect is plugged into _something_ else.  If it's not detectable by
other means, you need to add the circuity to the parent component.  If
you've implemented entity manager as intended, you would have a
configuration file for your baseboard/motherboard/primary comms board.
That is the one I was suggesting you should put it in.  This is the
exact reason the baseboard is a first class component in EM.
Look at one of the *_baseboard.json as an example.  I believe Wolf
Pass handles this exact case for a PCIe riser (although I'm not sure
about the state of it in EM).

> Unfortunately, maybe usually we
> only realize some hardware design is problematic to the software until
> we see it? :) I haven't started in Google when these boards were
> designed, and I'm not so sure if I could point it out even if I had
> been started in Google.

To reiterate, I think this case is handled, in a similar way to what
you'd have to implement in control flow code, given the fact that the
card itself is undetectable through conventional scanning.  You have
to make some assumption about "If I'm on Platform X, I MIGHT have an
undetectable card behind a mux at address Y.  If I see something
there, assume it's that type, and try to scan behind it.

With that said, I'm still interested to see if there's a way to make
your hardcoded approach viable for the things we need entity manager
to do.  Again, if you have time, start hacking on it and see what you
can integrate.

>
> >
> >
> > > Having the
> > > ability to aid the topology discovery with code, and having the
> > > topology info available to other functionalities can help a lot. JSON
> > > config files are having a hard time bearing these logics, and any
> > > extra logic implemented in JSON config files requires some kind of
> > > script parser in daemons processing them.
> > The majority of the config parsing is also able to be done at compile time, it just isn't implemented today.  With that said, the config file parsing in practice takes up very little CPU time in the last profile I did, so it hasn't been a priority.
>
> I'm not quite concerned about CPU time on the parsing, but more on the
> burden of developing. Because right now I feel like we need to
> implement a parser per daemon for what it's consuming.

I have no clue where you got that idea.  There is, by design, one json
parser, and it lives in entity manager.  Entity manager runs the
detection and posts the relevant interfaces to Dbus.  You do not have
to reimplement it for every single daemon.  Can you point out any
example of a json parser in one of the existing sensor daemons?  I
suspect you're not quite understanding the code you're looking at.

If you're talking about the sensor daemons "parsing" dbus, I agree,
dbus interfaces are relatively complicated and error prone, but at
this point, a non-dbus OpenBMC is probably a massive undertaking
(although I'm sure you'd get a lot of support if you did it).

> Unless we agree
> on a format and implement an OpenBMC library for it. Take the Virtual
> Sensor design doc under review for example:
> https://gerrit.openbmc-project.xyz/c/openbmc/docs/+/32345/ I think it
> will also have its own parser to deal with the "Algo" attribute.
Yes, I agree.  If I'm honest, I think the virtual sensor design goes
against some of the principles that EM was built on, as it moves large
amounts of complexity into config files (in exactly the way you've
noted), essentially ignores the dynamic nature of system topology, is
parsing "code" at runtime and makes debugging issues difficult.  I
think it will be hard to build, and even harder to maintain.  I (nor
any of the EM/dbus-sensors maintainers last time I looked) have
weighed in on it, so it's far from done (update, I just did).  Clearly
I should've left feedback on it earlier, but I, like you, don't have
much time for openbmc these days, so I pick my battles carefully.
> To
> make more fragments, right now entity-manager does the calculation
> without support for parenthesis and does not follow arithmetic order
> of operations, and we are trying to come up with one supporting
> parenthesis without breaking the compatibility.

Again, remember that you're looking at something not on master.  I had
a bunch of comments staged on that review that I just pushed.  I'm
glad to see you left some similar comments to what I posted.
If you're talking about the parser in entity manager, I'm confused.
There aren't any arithmetic operations (besides one hack), nor is it
doing any DSL level parsing at that level.  That would go against a
lot of the intent.

One thing to remember is that so long as you update the relevant
config files, you should feel free to change semantics of how some of
these things work.  There aren't that many config files on master.

>
> >
> >
> > > Based on your replies, the
> > > concept for functionally extensions that I was asking for should be
> > > implemented as daemons either standalone or plugged onto dbus?
> >
> > I'm not understanding the distinction of standalone vs plugged into dbus, but I''ll hazard a guess, and say yes, the dbus interfaces to the rest of the system is (one of) the project's intended extension points.  You can either manipulate them from an existing daemon, or create an all new daemon that has exactly the behavior you want.
> >
> >
> > >
> > > On "reading sensors within the BMC console", I'm actually using a
> > > script to directly read from hwmon right now, because we are having
> > > sensor number limit on IPMI and performance issues with IPMI and dbus.
> > > We are still actively investigating these performance issues now to
> > > unblock the project, but based on the current findings, I think it's
> > > better to have this tool before the protocol layers.
> > Have you considered opening a review with this tool to make it available to others?  I'd recommend opening a review to put it in here:
> > https://github.com/openbmc/openbmc-tools
> > This repo is much less formal, but gives people a place for these "might be useful to others" type scripts.  Write up a commit message with something to the effect of "I wrote this tool, this is how you use it, I find it makes platform development easier because X." and get it checked in.
>
> It had topology information and sensor information that we would like
> baked in as its major part, so unfortunately it's not an upstream-able
> script...
Here is yet another opportunity to make things better, and I feel like
you're squandering it.  I like to complain about the current state as
much as anyone, but if we're not putting up patchsets, it will never
improve, and the next person will just come in with the same
complaints.  If you have tools that you think are better, or provide
the start to a better tool, consider putting them up under your
username so other people can benefit, and see the ideas encapsulated
within it.

>
> >
> >
> > >
> > > On issues like uint8_t, yes, we've noted them down, but they are still
> > > tech debts on our backlog, and dealing with the performance issue
> > > described above remains as our priority right now.
> >
> > It sounds like you're swamped for time, which I can respect.  With that said, If you start by making technical improvements on small things like the above, you're much more likely to have feedback (and help) when you propose more wide sweeping changes, like your python example.
> > If you ever get free time, and want to continue moving your proposal more toward an actionable change we can make, I'm happy to help discuss options.  To be clear, I think if you can resolve some of the technical limitations of your proposal, and put together a patchset that implements it in a language that the project can use on a majority of platforms, I think it could be a better developer experience.  We just can't remove some of the user facing features that are implemented and/or planned already.
>
> Makes sense. We'll see if we could gather enough resources at some
> time to actually make it a concrete product, or we can come up with a
> plan to improve the existing ones bit by bit. It's been a pleasure to
> hear from you on what I haven't realized or taken into account yet,
> because my team was more hardware project focused and had less
> exposure to the general OpenBMC discussions or design philosophies.
> Thank you!
>
You're very welcome.

Re: Feedback on Current OpenBMC and Proposing Some Improvements ---- "Improvements" Ideas

[Alex Qiu]

On Mon, Jun 29, 2020 at 6:28 PM Ed Tanous <ed@tanous.net> wrote:
>
> On Mon, Jun 29, 2020 at 3:09 PM Alex Qiu <xqiu@google.com> wrote:
> >
> > On Mon, Jun 29, 2020 at 7:53 AM Ed Tanous <ed@tanous.net> wrote:
> > >
> > >
> > > On Thu, Jun 25, 2020 at 6:08 PM Alex Qiu <xqiu@google.com> wrote:
> > > > Yes, there are some restrictions in my current demo, and I'm afraid
> > > > that I may not have the bandwidth to cover it further alone. My point
> > > > is that, sometimes hardwares is designed with some unexpected
> > > > complexity on topology (EEPROM behind MUX for example).
> > > To my understanding this case is already handled.  Assign the mux to the parent FRU config file, and the eeprom behind it will be detected correctly.  With that said, this type of hardware (optional mux with an eeprom behind it) is difficult to identify automatically with no other impact, hence needing to explicitly add it to the parent board.  Can you think of any other examples of unexpected topology that aren't covered?
> >
> > There's no parent FRU in this case; the MUX belongs to the specific
> > FRU, and its EEPROM is behind the MUX.
>
> I called the baseboard a FRU, that was my bad and I suspect you got
> confused.  I should've said baseboard "entity".  The FRU you're trying
> to detect is plugged into _something_ else.  If it's not detectable by
> other means, you need to add the circuity to the parent component.  If
> you've implemented entity manager as intended, you would have a
> configuration file for your baseboard/motherboard/primary comms board.
> That is the one I was suggesting you should put it in.  This is the
> exact reason the baseboard is a first class component in EM.
> Look at one of the *_baseboard.json as an example.  I believe Wolf
> Pass handles this exact case for a PCIe riser (although I'm not sure
> about the state of it in EM).

Ah, I see. So basically it's a workaround to register the MUX that may
be plugged onto the baseboard? On the other hand, I just realized
today that our current workaround to statically assign these possible
MUX in the device tree could make these logical I2C bus numbers fixed,
which is very friendly for engineers to issue raw I2C commands with
i2ctools. Non-BMC engineers would probably have a headache when they
are told how to find the bus number in sysfs for a device instead of
being given a formula to calculate (which is already a headache to
explain).

>
> > Unfortunately, maybe usually we
> > only realize some hardware design is problematic to the software until
> > we see it? :) I haven't started in Google when these boards were
> > designed, and I'm not so sure if I could point it out even if I had
> > been started in Google.
>
> To reiterate, I think this case is handled, in a similar way to what
> you'd have to implement in control flow code, given the fact that the
> card itself is undetectable through conventional scanning.  You have
> to make some assumption about "If I'm on Platform X, I MIGHT have an
> undetectable card behind a mux at address Y.  If I see something
> there, assume it's that type, and try to scan behind it.
>
> With that said, I'm still interested to see if there's a way to make
> your hardcoded approach viable for the things we need entity manager
> to do.  Again, if you have time, start hacking on it and see what you
> can integrate.
>
> >
> > >
> > >
> > > > Having the
> > > > ability to aid the topology discovery with code, and having the
> > > > topology info available to other functionalities can help a lot. JSON
> > > > config files are having a hard time bearing these logics, and any
> > > > extra logic implemented in JSON config files requires some kind of
> > > > script parser in daemons processing them.
> > > The majority of the config parsing is also able to be done at compile time, it just isn't implemented today.  With that said, the config file parsing in practice takes up very little CPU time in the last profile I did, so it hasn't been a priority.
> >
> > I'm not quite concerned about CPU time on the parsing, but more on the
> > burden of developing. Because right now I feel like we need to
> > implement a parser per daemon for what it's consuming.
>
> I have no clue where you got that idea.  There is, by design, one json
> parser, and it lives in entity manager.  Entity manager runs the
> detection and posts the relevant interfaces to Dbus.  You do not have
> to reimplement it for every single daemon.  Can you point out any
> example of a json parser in one of the existing sensor daemons?  I
> suspect you're not quite understanding the code you're looking at.

I think I just got this idea from the virtual sensor design doc, which
is against the design principle of EM in your opinion...

>
> If you're talking about the sensor daemons "parsing" dbus, I agree,
> dbus interfaces are relatively complicated and error prone, but at
> this point, a non-dbus OpenBMC is probably a massive undertaking
> (although I'm sure you'd get a lot of support if you did it).
>
> > Unless we agree
> > on a format and implement an OpenBMC library for it. Take the Virtual
> > Sensor design doc under review for example:
> > https://gerrit.openbmc-project.xyz/c/openbmc/docs/+/32345/ I think it
> > will also have its own parser to deal with the "Algo" attribute.
> Yes, I agree.  If I'm honest, I think the virtual sensor design goes
> against some of the principles that EM was built on, as it moves large
> amounts of complexity into config files (in exactly the way you've
> noted), essentially ignores the dynamic nature of system topology, is
> parsing "code" at runtime and makes debugging issues difficult.  I
> think it will be hard to build, and even harder to maintain.  I (nor
> any of the EM/dbus-sensors maintainers last time I looked) have
> weighed in on it, so it's far from done (update, I just did).  Clearly
> I should've left feedback on it earlier, but I, like you, don't have
> much time for openbmc these days, so I pick my battles carefully.
> > To
> > make more fragments, right now entity-manager does the calculation
> > without support for parenthesis and does not follow arithmetic order
> > of operations, and we are trying to come up with one supporting
> > parenthesis without breaking the compatibility.
>
> Again, remember that you're looking at something not on master.  I had
> a bunch of comments staged on that review that I just pushed.  I'm
> glad to see you left some similar comments to what I posted.
> If you're talking about the parser in entity manager, I'm confused.
> There aren't any arithmetic operations (besides one hack), nor is it
> doing any DSL level parsing at that level.  That would go against a
> lot of the intent.

For the parser, I'm referring to the function templateCharReplace() in
https://github.com/openbmc/entity-manager/blob/master/src/Utils.cpp#L154.
We found it unintuitive that it does not support parenthesis and does
not follow arithmetic order of operations. If we try to improve it to
support parenthesis and arithmetic order of operations, it will break
compatibility if we don't watch it carefully.

>
> One thing to remember is that so long as you update the relevant
> config files, you should feel free to change semantics of how some of
> these things work.  There aren't that many config files on master.
>
> >
> > >
> > >
> > > > Based on your replies, the
> > > > concept for functionally extensions that I was asking for should be
> > > > implemented as daemons either standalone or plugged onto dbus?
> > >
> > > I'm not understanding the distinction of standalone vs plugged into dbus, but I''ll hazard a guess, and say yes, the dbus interfaces to the rest of the system is (one of) the project's intended extension points.  You can either manipulate them from an existing daemon, or create an all new daemon that has exactly the behavior you want.
> > >
> > >
> > > >
> > > > On "reading sensors within the BMC console", I'm actually using a
> > > > script to directly read from hwmon right now, because we are having
> > > > sensor number limit on IPMI and performance issues with IPMI and dbus.
> > > > We are still actively investigating these performance issues now to
> > > > unblock the project, but based on the current findings, I think it's
> > > > better to have this tool before the protocol layers.
> > > Have you considered opening a review with this tool to make it available to others?  I'd recommend opening a review to put it in here:
> > > https://github.com/openbmc/openbmc-tools
> > > This repo is much less formal, but gives people a place for these "might be useful to others" type scripts.  Write up a commit message with something to the effect of "I wrote this tool, this is how you use it, I find it makes platform development easier because X." and get it checked in.
> >
> > It had topology information and sensor information that we would like
> > baked in as its major part, so unfortunately it's not an upstream-able
> > script...
> Here is yet another opportunity to make things better, and I feel like
> you're squandering it.  I like to complain about the current state as
> much as anyone, but if we're not putting up patchsets, it will never
> improve, and the next person will just come in with the same
> complaints.  If you have tools that you think are better, or provide
> the start to a better tool, consider putting them up under your
> username so other people can benefit, and see the ideas encapsulated
> within it.

Sorry about that, but we've been really doing a lot of
platform-specific scripts or hacks, and it's non-trivial or losing a
lot of its core to upstream them.

>
> >
> > >
> > >
> > > >
> > > > On issues like uint8_t, yes, we've noted them down, but they are still
> > > > tech debts on our backlog, and dealing with the performance issue
> > > > described above remains as our priority right now.
> > >
> > > It sounds like you're swamped for time, which I can respect.  With that said, If you start by making technical improvements on small things like the above, you're much more likely to have feedback (and help) when you propose more wide sweeping changes, like your python example.
> > > If you ever get free time, and want to continue moving your proposal more toward an actionable change we can make, I'm happy to help discuss options.  To be clear, I think if you can resolve some of the technical limitations of your proposal, and put together a patchset that implements it in a language that the project can use on a majority of platforms, I think it could be a better developer experience.  We just can't remove some of the user facing features that are implemented and/or planned already.
> >
> > Makes sense. We'll see if we could gather enough resources at some
> > time to actually make it a concrete product, or we can come up with a
> > plan to improve the existing ones bit by bit. It's been a pleasure to
> > hear from you on what I haven't realized or taken into account yet,
> > because my team was more hardware project focused and had less
> > exposure to the general OpenBMC discussions or design philosophies.
> > Thank you!
> >
> You're very welcome.

- Alex Qiu

Re: Feedback on Current OpenBMC and Proposing Some Improvements ---- "Improvements" Ideas

[Ed Tanous]

On Tue, Jun 30, 2020 at 2:28 PM Alex Qiu <xqiu@google.com> wrote:
>
> On Mon, Jun 29, 2020 at 6:28 PM Ed Tanous <ed@tanous.net> wrote:
> >
> > On Mon, Jun 29, 2020 at 3:09 PM Alex Qiu <xqiu@google.com> wrote:
> > >
> > > On Mon, Jun 29, 2020 at 7:53 AM Ed Tanous <ed@tanous.net> wrote:
> > > >
> > > >
> > > > On Thu, Jun 25, 2020 at 6:08 PM Alex Qiu <xqiu@google.com> wrote:
> > > > > Yes, there are some restrictions in my current demo, and I'm afraid
> > > > > that I may not have the bandwidth to cover it further alone. My point
> > > > > is that, sometimes hardwares is designed with some unexpected
> > > > > complexity on topology (EEPROM behind MUX for example).
> > > > To my understanding this case is already handled.  Assign the mux to the parent FRU config file, and the eeprom behind it will be detected correctly.  With that said, this type of hardware (optional mux with an eeprom behind it) is difficult to identify automatically with no other impact, hence needing to explicitly add it to the parent board.  Can you think of any other examples of unexpected topology that aren't covered?
> > >
> > > There's no parent FRU in this case; the MUX belongs to the specific
> > > FRU, and its EEPROM is behind the MUX.
> >
> > I called the baseboard a FRU, that was my bad and I suspect you got
> > confused.  I should've said baseboard "entity".  The FRU you're trying
> > to detect is plugged into _something_ else.  If it's not detectable by
> > other means, you need to add the circuity to the parent component.  If
> > you've implemented entity manager as intended, you would have a
> > configuration file for your baseboard/motherboard/primary comms board.
> > That is the one I was suggesting you should put it in.  This is the
> > exact reason the baseboard is a first class component in EM.
> > Look at one of the *_baseboard.json as an example.  I believe Wolf
> > Pass handles this exact case for a PCIe riser (although I'm not sure
> > about the state of it in EM).
>
> Ah, I see. So basically it's a workaround to register the MUX that may
> be plugged onto the baseboard?

Correct.

> On the other hand, I just realized
> today that our current workaround to statically assign these possible
> MUX in the device tree could make these logical I2C bus numbers fixed,
> which is very friendly for engineers to issue raw I2C commands with
> i2ctools.
For a given configuration, entity manager will give consistent bus
numbers as well, and also provides helpful symlinks in the filesystem,
for example, /dev/PCIE_SLOT1 points to the bus of the first PCIe slot,
be it a root bus plugged directly into the bmc, or 3 levels of mux
connected through several boards.  I believe the i2c tools can also
use the symlink to interact directly with that in a named way that's
friendly.
> Non-BMC engineers would probably have a headache when they
> are told how to find the bus number in sysfs for a device instead of
> being given a formula to calculate (which is already a headache to
> explain).
I'm not following that statement.  "find the bus number" would occur
whether or not you have the busses hardcoded.  Are you advocating for
not using hwmon sensors here?  Needing to do a calculation for the new
part you're adding would need to be done regardless.  If you turn it
into a hwmon sensor, you could have the kernel do the math for you,
and keep your debugability.

> > >
> > > >
> > > >
> > > > > Having the
> > > > > ability to aid the topology discovery with code, and having the
> > > > > topology info available to other functionalities can help a lot. JSON
> > > > > config files are having a hard time bearing these logics, and any
> > > > > extra logic implemented in JSON config files requires some kind of
> > > > > script parser in daemons processing them.
> > > > The majority of the config parsing is also able to be done at compile time, it just isn't implemented today.  With that said, the config file parsing in practice takes up very little CPU time in the last profile I did, so it hasn't been a priority.
> > >
> > > I'm not quite concerned about CPU time on the parsing, but more on the
> > > burden of developing. Because right now I feel like we need to
> > > implement a parser per daemon for what it's consuming.
> >
> > I have no clue where you got that idea.  There is, by design, one json
> > parser, and it lives in entity manager.  Entity manager runs the
> > detection and posts the relevant interfaces to Dbus.  You do not have
> > to reimplement it for every single daemon.  Can you point out any
> > example of a json parser in one of the existing sensor daemons?  I
> > suspect you're not quite understanding the code you're looking at.
>
> I think I just got this idea from the virtual sensor design doc, which
> is against the design principle of EM in your opinion...

Sounds like we're on the same page here then.  See my comments on that review.

>
> >
> > If you're talking about the sensor daemons "parsing" dbus, I agree,
> > dbus interfaces are relatively complicated and error prone, but at
> > this point, a non-dbus OpenBMC is probably a massive undertaking
> > (although I'm sure you'd get a lot of support if you did it).
> >
> > > Unless we agree
> > > on a format and implement an OpenBMC library for it. Take the Virtual
> > > Sensor design doc under review for example:
> > > https://gerrit.openbmc-project.xyz/c/openbmc/docs/+/32345/ I think it
> > > will also have its own parser to deal with the "Algo" attribute.
> > Yes, I agree.  If I'm honest, I think the virtual sensor design goes
> > against some of the principles that EM was built on, as it moves large
> > amounts of complexity into config files (in exactly the way you've
> > noted), essentially ignores the dynamic nature of system topology, is
> > parsing "code" at runtime and makes debugging issues difficult.  I
> > think it will be hard to build, and even harder to maintain.  I (nor
> > any of the EM/dbus-sensors maintainers last time I looked) have
> > weighed in on it, so it's far from done (update, I just did).  Clearly
> > I should've left feedback on it earlier, but I, like you, don't have
> > much time for openbmc these days, so I pick my battles carefully.
> > > To
> > > make more fragments, right now entity-manager does the calculation
> > > without support for parenthesis and does not follow arithmetic order
> > > of operations, and we are trying to come up with one supporting
> > > parenthesis without breaking the compatibility.
> >
> > Again, remember that you're looking at something not on master.  I had
> > a bunch of comments staged on that review that I just pushed.  I'm
> > glad to see you left some similar comments to what I posted.
> > If you're talking about the parser in entity manager, I'm confused.
> > There aren't any arithmetic operations (besides one hack), nor is it
> > doing any DSL level parsing at that level.  That would go against a
> > lot of the intent.
>
> For the parser, I'm referring to the function templateCharReplace() in
> https://github.com/openbmc/entity-manager/blob/master/src/Utils.cpp#L154.
> We found it unintuitive that it does not support parenthesis and does
> not follow arithmetic order of operations. If we try to improve it to
> support parenthesis and arithmetic order of operations, it will break
> compatibility if we don't watch it carefully.

Yes, it's not a real parser, but if you look at the commit for the
problem it was fixing (massively duplicated config files for power
supplies because of minor changes) then it starts to make more sense
that what's there is better than what came before.  If it's important
to you, then put together a patch to add a real parser?  Remember that
the relevant config files are checked into that repo, so you can
actually dump every single config statement and flush it through your
parser to test that it gives the same result, and in the cases it
doesn't, add parenthesis where required to get the same result.  I
would really only expect the quad mode power supply files to even be
effected, and I believe (based on how their expression is parsed) they
won't be.



>
> >
> > One thing to remember is that so long as you update the relevant
> > config files, you should feel free to change semantics of how some of
> > these things work.  There aren't that many config files on master.
> >
> > >
> > > >
> > > >
> > > > > Based on your replies, the
> > > > > concept for functionally extensions that I was asking for should be
> > > > > implemented as daemons either standalone or plugged onto dbus?
> > > >
> > > > I'm not understanding the distinction of standalone vs plugged into dbus, but I''ll hazard a guess, and say yes, the dbus interfaces to the rest of the system is (one of) the project's intended extension points.  You can either manipulate them from an existing daemon, or create an all new daemon that has exactly the behavior you want.
> > > >
> > > >
> > > > >
> > > > > On "reading sensors within the BMC console", I'm actually using a
> > > > > script to directly read from hwmon right now, because we are having
> > > > > sensor number limit on IPMI and performance issues with IPMI and dbus.
> > > > > We are still actively investigating these performance issues now to
> > > > > unblock the project, but based on the current findings, I think it's
> > > > > better to have this tool before the protocol layers.
> > > > Have you considered opening a review with this tool to make it available to others?  I'd recommend opening a review to put it in here:
> > > > https://github.com/openbmc/openbmc-tools
> > > > This repo is much less formal, but gives people a place for these "might be useful to others" type scripts.  Write up a commit message with something to the effect of "I wrote this tool, this is how you use it, I find it makes platform development easier because X." and get it checked in.
> > >
> > > It had topology information and sensor information that we would like
> > > baked in as its major part, so unfortunately it's not an upstream-able
> > > script...
> > Here is yet another opportunity to make things better, and I feel like
> > you're squandering it.  I like to complain about the current state as
> > much as anyone, but if we're not putting up patchsets, it will never
> > improve, and the next person will just come in with the same
> > complaints.  If you have tools that you think are better, or provide
> > the start to a better tool, consider putting them up under your
> > username so other people can benefit, and see the ideas encapsulated
> > within it.
>
> Sorry about that, but we've been really doing a lot of
> platform-specific scripts or hacks, and it's non-trivial or losing a
> lot of its core to upstream them.
>
> >
> > >
> > > >
> > > >
> > > > >
> > > > > On issues like uint8_t, yes, we've noted them down, but they are still
> > > > > tech debts on our backlog, and dealing with the performance issue
> > > > > described above remains as our priority right now.
> > > >
> > > > It sounds like you're swamped for time, which I can respect.  With that said, If you start by making technical improvements on small things like the above, you're much more likely to have feedback (and help) when you propose more wide sweeping changes, like your python example.
> > > > If you ever get free time, and want to continue moving your proposal more toward an actionable change we can make, I'm happy to help discuss options.  To be clear, I think if you can resolve some of the technical limitations of your proposal, and put together a patchset that implements it in a language that the project can use on a majority of platforms, I think it could be a better developer experience.  We just can't remove some of the user facing features that are implemented and/or planned already.
> > >
> > > Makes sense. We'll see if we could gather enough resources at some
> > > time to actually make it a concrete product, or we can come up with a
> > > plan to improve the existing ones bit by bit. It's been a pleasure to
> > > hear from you on what I haven't realized or taken into account yet,
> > > because my team was more hardware project focused and had less
> > > exposure to the general OpenBMC discussions or design philosophies.
> > > Thank you!
> > >
> > You're very welcome.
>
> - Alex Qiu

Re: Feedback on Current OpenBMC and Proposing Some Improvements ---- "Improvements" Ideas

[Alex Qiu]

On Tue, Jun 30, 2020 at 7:00 PM Ed Tanous <ed@tanous.net> wrote:
>
> On Tue, Jun 30, 2020 at 2:28 PM Alex Qiu <xqiu@google.com> wrote:
> >
> > On Mon, Jun 29, 2020 at 6:28 PM Ed Tanous <ed@tanous.net> wrote:
> > >
> > > On Mon, Jun 29, 2020 at 3:09 PM Alex Qiu <xqiu@google.com> wrote:
> > > >
> > > > On Mon, Jun 29, 2020 at 7:53 AM Ed Tanous <ed@tanous.net> wrote:
> > > > >
> > > > >
> > > > > On Thu, Jun 25, 2020 at 6:08 PM Alex Qiu <xqiu@google.com> wrote:
> > > > > > Yes, there are some restrictions in my current demo, and I'm afraid
> > > > > > that I may not have the bandwidth to cover it further alone. My point
> > > > > > is that, sometimes hardwares is designed with some unexpected
> > > > > > complexity on topology (EEPROM behind MUX for example).
> > > > > To my understanding this case is already handled.  Assign the mux to the parent FRU config file, and the eeprom behind it will be detected correctly.  With that said, this type of hardware (optional mux with an eeprom behind it) is difficult to identify automatically with no other impact, hence needing to explicitly add it to the parent board.  Can you think of any other examples of unexpected topology that aren't covered?
> > > >
> > > > There's no parent FRU in this case; the MUX belongs to the specific
> > > > FRU, and its EEPROM is behind the MUX.
> > >
> > > I called the baseboard a FRU, that was my bad and I suspect you got
> > > confused.  I should've said baseboard "entity".  The FRU you're trying
> > > to detect is plugged into _something_ else.  If it's not detectable by
> > > other means, you need to add the circuity to the parent component.  If
> > > you've implemented entity manager as intended, you would have a
> > > configuration file for your baseboard/motherboard/primary comms board.
> > > That is the one I was suggesting you should put it in.  This is the
> > > exact reason the baseboard is a first class component in EM.
> > > Look at one of the *_baseboard.json as an example.  I believe Wolf
> > > Pass handles this exact case for a PCIe riser (although I'm not sure
> > > about the state of it in EM).
> >
> > Ah, I see. So basically it's a workaround to register the MUX that may
> > be plugged onto the baseboard?
>
> Correct.
>
> > On the other hand, I just realized
> > today that our current workaround to statically assign these possible
> > MUX in the device tree could make these logical I2C bus numbers fixed,
> > which is very friendly for engineers to issue raw I2C commands with
> > i2ctools.
> For a given configuration, entity manager will give consistent bus
> numbers as well, and also provides helpful symlinks in the filesystem,
> for example, /dev/PCIE_SLOT1 points to the bus of the first PCIe slot,
> be it a root bus plugged directly into the bmc, or 3 levels of mux
> connected through several boards.  I believe the i2c tools can also
> use the symlink to interact directly with that in a named way that's
> friendly.
> > Non-BMC engineers would probably have a headache when they
> > are told how to find the bus number in sysfs for a device instead of
> > being given a formula to calculate (which is already a headache to
> > explain).
> I'm not following that statement.  "find the bus number" would occur
> whether or not you have the busses hardcoded.  Are you advocating for
> not using hwmon sensors here?  Needing to do a calculation for the new
> part you're adding would need to be done regardless.  If you turn it
> into a hwmon sensor, you could have the kernel do the math for you,
> and keep your debugability.

Hardware engineers want to set the output voltage for voltage
regulators for debugging, which is not covered by hwmon interface or
drivers, so we need to send raw I2C commands. When a system is not
fully populated, I believe the kernel always assigns the largest
sequential numbers to newly created MUX channels, so that number will
vary based on the debug system configurations. On the other hand, our
current workaround to populate the MUXes in the device tree while they
may not exist fixes the bus numbers which can be calculated from a
formula, instead of tracing symlinks.

>
> >
> > >
> > > If you're talking about the sensor daemons "parsing" dbus, I agree,
> > > dbus interfaces are relatively complicated and error prone, but at
> > > this point, a non-dbus OpenBMC is probably a massive undertaking
> > > (although I'm sure you'd get a lot of support if you did it).
> > >
> > > > Unless we agree
> > > > on a format and implement an OpenBMC library for it. Take the Virtual
> > > > Sensor design doc under review for example:
> > > > https://gerrit.openbmc-project.xyz/c/openbmc/docs/+/32345/ I think it
> > > > will also have its own parser to deal with the "Algo" attribute.
> > > Yes, I agree.  If I'm honest, I think the virtual sensor design goes
> > > against some of the principles that EM was built on, as it moves large
> > > amounts of complexity into config files (in exactly the way you've
> > > noted), essentially ignores the dynamic nature of system topology, is
> > > parsing "code" at runtime and makes debugging issues difficult.  I
> > > think it will be hard to build, and even harder to maintain.  I (nor
> > > any of the EM/dbus-sensors maintainers last time I looked) have
> > > weighed in on it, so it's far from done (update, I just did).  Clearly
> > > I should've left feedback on it earlier, but I, like you, don't have
> > > much time for openbmc these days, so I pick my battles carefully.
> > > > To
> > > > make more fragments, right now entity-manager does the calculation
> > > > without support for parenthesis and does not follow arithmetic order
> > > > of operations, and we are trying to come up with one supporting
> > > > parenthesis without breaking the compatibility.
> > >
> > > Again, remember that you're looking at something not on master.  I had
> > > a bunch of comments staged on that review that I just pushed.  I'm
> > > glad to see you left some similar comments to what I posted.
> > > If you're talking about the parser in entity manager, I'm confused.
> > > There aren't any arithmetic operations (besides one hack), nor is it
> > > doing any DSL level parsing at that level.  That would go against a
> > > lot of the intent.
> >
> > For the parser, I'm referring to the function templateCharReplace() in
> > https://github.com/openbmc/entity-manager/blob/master/src/Utils.cpp#L154.
> > We found it unintuitive that it does not support parenthesis and does
> > not follow arithmetic order of operations. If we try to improve it to
> > support parenthesis and arithmetic order of operations, it will break
> > compatibility if we don't watch it carefully.
>
> Yes, it's not a real parser, but if you look at the commit for the
> problem it was fixing (massively duplicated config files for power
> supplies because of minor changes) then it starts to make more sense
> that what's there is better than what came before.  If it's important
> to you, then put together a patch to add a real parser?  Remember that
> the relevant config files are checked into that repo, so you can
> actually dump every single config statement and flush it through your
> parser to test that it gives the same result, and in the cases it
> doesn't, add parenthesis where required to get the same result.  I
> would really only expect the quad mode power supply files to even be
> effected, and I believe (based on how their expression is parsed) they
> won't be.

For the concern of compatibility, we worry that other companies are
also using these features downstream. FYI, we are heavily relying on
it right now, even though we find out it's not following arithmetic
order of operations.

- Alex Qiu

Re: Feedback on Current OpenBMC and Proposing Some Improvements ---- "Improvements" Ideas

[James Feist]

On 7/1/2020 10:06 AM, Alex Qiu wrote:
>>> For the parser, I'm referring to the function templateCharReplace() in
>>> https://github.com/openbmc/entity-manager/blob/master/src/Utils.cpp#L154.
>>> We found it unintuitive that it does not support parenthesis and does
>>> not follow arithmetic order of operations. If we try to improve it to
>>> support parenthesis and arithmetic order of operations, it will break
>>> compatibility if we don't watch it carefully.

A real parser would be a great addition, at the time we had no idea all 
of the different replacement functions we would be adding.

>> Yes, it's not a real parser, but if you look at the commit for the
>> problem it was fixing (massively duplicated config files for power
>> supplies because of minor changes) then it starts to make more sense
>> that what's there is better than what came before.  If it's important
>> to you, then put together a patch to add a real parser?  Remember that
>> the relevant config files are checked into that repo, so you can
>> actually dump every single config statement and flush it through your
>> parser to test that it gives the same result, and in the cases it
>> doesn't, add parenthesis where required to get the same result.  I
>> would really only expect the quad mode power supply files to even be
>> effected, and I believe (based on how their expression is parsed) they
>> won't be.
> For the concern of compatibility, we worry that other companies are
> also using these features downstream. FYI, we are heavily relying on
> it right now, even though we find out it's not following arithmetic
> order of operations.

I think that is outside of the projects hands, and should influence 
others to upstream their configurations so that they get all necessary 
changes/support when new features are added.

Re: Feedback on Current OpenBMC and Proposing Some Improvements ---- "Improvements" Ideas

[Ed Tanous]

On Wed, Jul 1, 2020 at 10:06 AM Alex Qiu <xqiu@google.com> wrote:
>
> On Tue, Jun 30, 2020 at 7:00 PM Ed Tanous <ed@tanous.net> wrote:
> >
> > I'm not following that statement.  "find the bus number" would occur
> > whether or not you have the busses hardcoded.  Are you advocating for
> > not using hwmon sensors here?  Needing to do a calculation for the new
> > part you're adding would need to be done regardless.  If you turn it
> > into a hwmon sensor, you could have the kernel do the math for you,
> > and keep your debugability.
>
> Hardware engineers want to set the output voltage for voltage
> regulators for debugging, which is not covered by hwmon interface or
> drivers, so we need to send raw I2C commands.
Or add support to the drivers.....
I'm not against raw i2c commands for debugging, but long term, it's
really hard to maintain (as you seem to be finding).

> When a system is not
> fully populated, I believe the kernel always assigns the largest
> sequential numbers to newly created MUX channels, so that number will
> vary based on the debug system configurations. On the other hand, our
> current workaround to populate the MUXes in the device tree while they
> may not exist fixes the bus numbers which can be calculated from a
> formula, instead of tracing symlinks.
It sounds like we have different priorities.  You want to make it easy
to debug a single given hardware configuration, entity managers goal
was to make it straightforward to debug any hardware configuration on
any platform.  Different goals, neither are wrong, just different.

>
> For the concern of compatibility, we worry that other companies are
> also using these features downstream. FYI, we are heavily relying on
> it right now, even though we find out it's not following arithmetic
> order of operations.
>
With respect to those companies, their downstream is their problem.
That's why upstreaming is important.  I'm not saying we need to break
things unnecessarily, but it's a really poor excuse to say we can't
change things because of an unknown entity that didn't share their
code.  If they exist, they're using a feature that's relatively new to
entity manager, and so far as I know, is only used in a single case,
and in that case, mod operator is at the same or greater precedence
than + operator, so you could make the change with zero impact to a
anyone that I'm aware of.


We've gone several rounds on this email, with a lot of places where
you could make improvements, including many that wouldn't break
anything, but you always seem to come back to being too busy for it,
or it not being "upstreamable".  Is there anything that the project
could do to help convince you to at least share some changes that
you've suggested?  The feedback is really great, but I was hoping with
the level of interest you have in this, you'd be interested in putting
together some patchsets to do some of these things, even if they're
minor, like adding support for your new chip.

Re: Feedback on Current OpenBMC and Proposing Some Improvements ---- "Improvements" Ideas

[Alex Qiu]

On Wed, Jul 8, 2020 at 11:06 AM Ed Tanous <ed@tanous.net> wrote:
>
> On Wed, Jul 1, 2020 at 10:06 AM Alex Qiu <xqiu@google.com> wrote:
> >
> > On Tue, Jun 30, 2020 at 7:00 PM Ed Tanous <ed@tanous.net> wrote:
> > >
> > > I'm not following that statement.  "find the bus number" would occur
> > > whether or not you have the busses hardcoded.  Are you advocating for
> > > not using hwmon sensors here?  Needing to do a calculation for the new
> > > part you're adding would need to be done regardless.  If you turn it
> > > into a hwmon sensor, you could have the kernel do the math for you,
> > > and keep your debugability.
> >
> > Hardware engineers want to set the output voltage for voltage
> > regulators for debugging, which is not covered by hwmon interface or
> > drivers, so we need to send raw I2C commands.
> Or add support to the drivers.....
> I'm not against raw i2c commands for debugging, but long term, it's
> really hard to maintain (as you seem to be finding).

We've talked with the maintainer of hwmon, and he doesn't think adding
these to hwmon interface is a good idea, as hwmon is supposed to be a
monitoring interface. Although we haven't yet met the need to set the
voltage other than debugging.

>
> With respect to those companies, their downstream is their problem.
> That's why upstreaming is important.  I'm not saying we need to break
> things unnecessarily, but it's a really poor excuse to say we can't
> change things because of an unknown entity that didn't share their
> code.  If they exist, they're using a feature that's relatively new to
> entity manager, and so far as I know, is only used in a single case,
> and in that case, mod operator is at the same or greater precedence
> than + operator, so you could make the change with zero impact to a
> anyone that I'm aware of.
>
>
> We've gone several rounds on this email, with a lot of places where
> you could make improvements, including many that wouldn't break
> anything, but you always seem to come back to being too busy for it,
> or it not being "upstreamable".  Is there anything that the project
> could do to help convince you to at least share some changes that
> you've suggested?  The feedback is really great, but I was hoping with
> the level of interest you have in this, you'd be interested in putting
> together some patchsets to do some of these things, even if they're
> minor, like adding support for your new chip.

First of all, I don't have the magic to turn downstream patches into
an upstream fix in one day, one week or so, and we currently have the
priority to fix sensor performance issues so that we can get our new
platform out of the door this month. On the other hand, my intention
is to get this conversation started and rolling before we eventually
have the free time to deal with all the technical debt we accumulated
downstream, so that we know the aim is as soon as we are at that
stage.

Clearly, I see the conversation we had so far is greatly valuable at
least to us, and I believe we have an internal communication gap to
fill between different teams first. For example, before the
conversation, I never knew that we were supposed to upstream our
configuration files. By contrast, I was told that these code names
can't be publicized, and we've been keeping patches downstream, so
there's definitely something to resolve internally first. Our team has
been working underwater for a long time without knowing these
intentions upstream, and I think it's the first time that we reach
upstream in this level of detail.

I hope this explains that I can't put up patches for things that I've
been aware of right away, since we might have been doing something in
the wrong way for quite some time. Thanks.

- Alex Qiu

Re: Feedback on Current OpenBMC and Proposing Some Improvements ---- "Improvements" Ideas

[Ed Tanous]

On Wed, Jul 8, 2020 at 2:46 PM Alex Qiu <xqiu@google.com> wrote:
>
> On Wed, Jul 8, 2020 at 11:06 AM Ed Tanous <ed@tanous.net> wrote:
> >
> > On Wed, Jul 1, 2020 at 10:06 AM Alex Qiu <xqiu@google.com> wrote:
> > >
> > > On Tue, Jun 30, 2020 at 7:00 PM Ed Tanous <ed@tanous.net> wrote:
> > > >
> > > > I'm not following that statement.  "find the bus number" would occur
> > > > whether or not you have the busses hardcoded.  Are you advocating for
> > > > not using hwmon sensors here?  Needing to do a calculation for the new
> > > > part you're adding would need to be done regardless.  If you turn it
> > > > into a hwmon sensor, you could have the kernel do the math for you,
> > > > and keep your debugability.
> > >
> > > Hardware engineers want to set the output voltage for voltage
> > > regulators for debugging, which is not covered by hwmon interface or
> > > drivers, so we need to send raw I2C commands.
> > Or add support to the drivers.....
> > I'm not against raw i2c commands for debugging, but long term, it's
> > really hard to maintain (as you seem to be finding).
>
> We've talked with the maintainer of hwmon, and he doesn't think adding
> these to hwmon interface is a good idea, as hwmon is supposed to be a
> monitoring interface. Although we haven't yet met the need to set the
> voltage other than debugging.

Excellent.  I hadn't realized you'd done that.  You're right,
userspace is probably the right spot for this then.

>
> >
> > With respect to those companies, their downstream is their problem.
> > That's why upstreaming is important.  I'm not saying we need to break
> > things unnecessarily, but it's a really poor excuse to say we can't
> > change things because of an unknown entity that didn't share their
> > code.  If they exist, they're using a feature that's relatively new to
> > entity manager, and so far as I know, is only used in a single case,
> > and in that case, mod operator is at the same or greater precedence
> > than + operator, so you could make the change with zero impact to a
> > anyone that I'm aware of.
> >
> >
> > We've gone several rounds on this email, with a lot of places where
> > you could make improvements, including many that wouldn't break
> > anything, but you always seem to come back to being too busy for it,
> > or it not being "upstreamable".  Is there anything that the project
> > could do to help convince you to at least share some changes that
> > you've suggested?  The feedback is really great, but I was hoping with
> > the level of interest you have in this, you'd be interested in putting
> > together some patchsets to do some of these things, even if they're
> > minor, like adding support for your new chip.
>
> First of all, I don't have the magic to turn downstream patches into
> an upstream fix in one day, one week or so, and we currently have the
> priority to fix sensor performance issues so that we can get our new
> platform out of the door this month. On the other hand, my intention
> is to get this conversation started and rolling before we eventually
> have the free time to deal with all the technical debt we accumulated
> downstream, so that we know the aim is as soon as we are at that
> stage.

To be clear, I wasn't expecting you to turn patches around in a day or
week.  Even patches that are public, but not upstreamable are a start,
as they show what you're trying to accomplish, and someone else might
pick it up from there.  Tech debt is a tough mistress, I don't envy
the position you're in.

>
> Clearly, I see the conversation we had so far is greatly valuable at
> least to us, and I believe we have an internal communication gap to
> fill between different teams first. For example, before the
> conversation, I never knew that we were supposed to upstream our
> configuration files. By contrast, I was told that these code names
> can't be publicized, and we've been keeping patches downstream, so
> there's definitely something to resolve internally first. Our team has
> been working underwater for a long time without knowing these
> intentions upstream, and I think it's the first time that we reach
> upstream in this level of detail.

Great!  I'm glad we could help out.  To be clear, for non-public
codename platforms, I wouldn't expect you to upstream the config
files, but you'll have continuing tech debt as the schemas for those
files evolve, so there is an argument to be made with your powers that
be that they should be upstreamed anyway, even if the codename isn't
public, or you need a different codename.
With that said, the config files really aren't the interesting part,
and only serve to make upstream build and run out of the box.  The
real wins are the new sensor and config types, and fixes to the
existing daemons.  In general, those help everyone.

>
> I hope this explains that I can't put up patches for things that I've
> been aware of right away, since we might have been doing something in
> the wrong way for quite some time. Thanks.
>

Whether you upsteam them next week, or a year from now, so long as
that's the direction you're going, and you're willing to make
improvements, I'm happy.

Good luck getting your platform launched.

Cheers,

-Ed

Re: Feedback on Current OpenBMC and Proposing Some Improvements ---- "Improvements" Ideas

[Brad Bishop]

On Wed, Jul 08, 2020 at 11:15:03PM -0700, Ed Tanous wrote:
>On Wed, Jul 8, 2020 at 2:46 PM Alex Qiu <xqiu@google.com> wrote:

>> We've talked with the maintainer of hwmon, and he doesn't think adding
>> these to hwmon interface is a good idea, as hwmon is supposed to be a
>> monitoring interface. 

Except most "monitoring devices" are configurable and/or have a dual 
nature - e.g. voltage regulators.

>> Although we haven't yet met the need to set the
>> voltage other than debugging.

We have a need to tweak things in the production firmware.

>Excellent.  I hadn't realized you'd done that.  You're right,
>userspace is probably the right spot for this then.

I dunno.  It would be nice to have an in-kernel solution that allows 
VRMs to be both monitored and controlled.  Mixing i2c-dev with the 
actual vrm driver makes us do strange things like unbinding drivers 
while the vrms are configured, or rewriting the vrm driver monitor 
function in usersapce and missing out on the benefit of a well 
maintained kernel driver.

This problem comes up over and over again.  What would be great is if 
someone with good relationships and standing in the kernel community 
could work with the kernel maintainers to build their understanding of 
how we are using the kernel and figure out a good solution we could 
implement other than just: "use i2c-dev".

Re: Feedback on Current OpenBMC and Proposing Some Improvements ---- "Improvements" Ideas

[Alex Qiu]

Thank you, Ed!

Hi Brad,

Last time we checked with the maintainer Guenter Roeck
<linux@roeck-us.net>, I remembered that he suggested we could add
these controls in debugfs. Feel free to reach out to him and see if
anything new is coming up.

On the other hand, my solution to this of the proposals in these email
series is to create a wrapper library around I2C devices and hwmon. We
can continue to have the benefit of hwmon to take care of the reading
tasks, and we can also lock all the I2C access at this level and issue
raw I2C commands as needed without worrying races between I2C
transactions. However, we need to ensure that no other applications
are sneaking below this library and interacting with hwmon sysfs.

Thanks!

- Alex Qiu


On Thu, Jul 9, 2020 at 5:43 AM Brad Bishop <bradleyb@fuzziesquirrel.com> wrote:
>
> On Wed, Jul 08, 2020 at 11:15:03PM -0700, Ed Tanous wrote:
> >On Wed, Jul 8, 2020 at 2:46 PM Alex Qiu <xqiu@google.com> wrote:
>
> >> We've talked with the maintainer of hwmon, and he doesn't think adding
> >> these to hwmon interface is a good idea, as hwmon is supposed to be a
> >> monitoring interface.
>
> Except most "monitoring devices" are configurable and/or have a dual
> nature - e.g. voltage regulators.
>
> >> Although we haven't yet met the need to set the
> >> voltage other than debugging.
>
> We have a need to tweak things in the production firmware.
>
> >Excellent.  I hadn't realized you'd done that.  You're right,
> >userspace is probably the right spot for this then.
>
> I dunno.  It would be nice to have an in-kernel solution that allows
> VRMs to be both monitored and controlled.  Mixing i2c-dev with the
> actual vrm driver makes us do strange things like unbinding drivers
> while the vrms are configured, or rewriting the vrm driver monitor
> function in usersapce and missing out on the benefit of a well
> maintained kernel driver.
>
> This problem comes up over and over again.  What would be great is if
> someone with good relationships and standing in the kernel community
> could work with the kernel maintainers to build their understanding of
> how we are using the kernel and figure out a good solution we could
> implement other than just: "use i2c-dev".