Re: [Freeipmi-devel] [PATCH 0/5] Parse SPMI tables using sysfs

[dann frazier]

hey Al - replying inline....

On Tue, Jul 31, 2018 at 6:17 PM Albert Chu <address@hidden> wrote:
>
> Hey Dann,
>
> Code looks good.  One thing I noticed.  On one of my systems I have:
>
> # ls /sys/firmware/acpi/tables/ | grep SPMI
> SPMI1
> SPMI2
>
> So I'm thinking a similar "table instance" (like w/ the /dev/mem) needs
> to be supported?  I hacked up something and it seems on one of my
> systems there is IPMI info in the SPMI1 path (also verifies the core
> code of your patches works!).
>
> But I guess on your systems there is only a
> /sys/firmware/acpi/tables/SPMI path?

Correct. To be honest, I didn't grok the "table instance" concept.

> In my hack I passed table_instance into ipmi_acpi_get_table_sysfs and
> adjusted the path accordingly.  Perhaps to support both path types, if
> table_instance == 0, try both "SPMI" and "SPMI0"?  Sort of hackish, but
> probably ok.

Seems very reasonable to me. I've been testing with a simulated
/sys/firmware/acpi/tables directory already, so I'll just build a
simulation of yours as well.

> I'm more than happy to apply my fix after yours, but since you have way
> more systems with this support, I think it'd be better to come from
> your side and me to just make sure it works on the one random node I
> have that works with this (I tried a few other nodes and there was no
> IPMI info in sysfs SPMI).
>
> While hacking on this, I noticed a corner case in the acpi code.  I
> pushed a fix upstream that's independent of your code.  But you should
> probably pull it down to make sure things still work.

Will do! Thanks. I'll look to submit a v2 patch later today.

  -dann

> Al
>
> On Tue, 2018-07-31 at 11:42 -0600, dann frazier wrote:
> > I ran into an ARM server that describes its system interface only via
> > the SPMI ACPI table and not via SMBIOS. freeipmi has SPMI support,
> > but
> > its implementation accesses ACPI tables using /dev/mem, which isn't
> > safe to do on ARM. In fact, this code is #ifdef'd out for ARM
> > platforms.
> >
> > This series teaches freeipmi how to parse ACPI tables out of sysfs,
> > keeping the fallback /dev/mem implementation as a fallback. It also
> > fixes a couple of apparent bugs in the SPMI parsing itself.
> >
> > Tested on a HiSilicon D06 ARM Server, and 4 x86 servers:
> > HP ProLiant DL165 G7
> > HP ProLiant DL385 G7
> > QuantaGrid D52B
> > Supermicro Super Server
> >
> > I've actually not been able to get the existing /dev/mem code to work
> > on any platform I've tried myself, so I was unable to regression test
> > it.
> >
> > dann frazier (5):
> >   Don't try to separate the header from the ACPI table data
> >   Split RSDT/XSDT parsing into new function
> >   Add support for parsing SPMI table exposed via sysfs
> >   Allow sysfs SPMI parsing on ARM platforms
> >   Correct order of bytes in specification_revision field of ACPI SPMI
> >     table
> >
> >  libfreeipmi/locate/ipmi-locate-acpi-spmi.c | 240 ++++++++++++++++---
> > --
> >  1 file changed, 184 insertions(+), 56 deletions(-)
> >
> --
> Albert Chu
> address@hidden
> Computer Scientist
> High Performance Systems Division
> Lawrence Livermore National Laboratory
>