[Skiboot] [PATCH] HDAT: Add IPMI sensor data under /bmc node
Oliver
oohall at gmail.com
Mon Jun 26 17:25:14 AEST 2017
On Mon, Jun 26, 2017 at 3:57 PM, Joel Stanley <joel at jms.id.au> wrote:
> On Fri, Jun 23, 2017 at 9:09 PM, Vasant Hegde
> <hegdevasant at linux.vnet.ibm.com> wrote:
>> Add IPMI sensor data under /bmc node.
>
> This doesn't add any tests.
>
> How did you test it?
>
> Which machines support this feature?
You grab a hdat dump from a system and feed it into hdata_to_dt and
eyeball the result. The way we do hdat testing is pretty poor since it
doesn't actually "test" anything, it just verifies that the output
didn't change for a given input. We generally don't implement parsing
of something until hostboot has implemented it so I've been resistant
to adding a P9 HDAT blob to the repo since we would need to update it
frequently and I don't want to be littering the repo with stale blobs.
>
>>
>> CC: Joel Stanley <joel at jms.id.au>
>> Signed-off-by: Vasant Hegde <hegdevasant at linux.vnet.ibm.com>
>> ---
>> hdata/fsp.c | 51 ++++++++++++++++++++++++++++++++++++++++++++++++++-
>> hdata/spira.c | 1 +
>> hdata/spira.h | 24 +++++++++++++++++++++++-
>> 3 files changed, 74 insertions(+), 2 deletions(-)
>>
>> diff --git a/hdata/fsp.c b/hdata/fsp.c
>> index 6953d97..b941ee2 100644
>> --- a/hdata/fsp.c
>> +++ b/hdata/fsp.c
>> @@ -280,6 +280,53 @@ static void add_uart(const struct spss_iopath *iopath, struct dt_node *lpc)
>> be32_to_cpu(iopath->lpc.uart_baud));
>> }
>>
>> +static void add_ipmi_sensors(struct dt_node *bmc_node)
>> +{
>> + int i;
>> + const void *hdif_sensor;
Shouldn't this be a struct HDIF_common_hdr *?
>> + struct dt_node *sensors_node, *sensor_node;
>> + const struct ipmi_sensors *ipmi_sensors;
>> +
>> + hdif_sensor = get_hdif(&spira.ntuples.ipmi_sensor, IPMI_SENSORS_HDIF_SIG);
>
> What is hdif?
>
> (Oh, I just noticed that this is FSP stuff. I guess that's why it
> makes no sense to me?)
Hypervisor Data Interface or something. It's just the name of the data
structures that make up the HDAT.
>
>> + if (!hdif_sensor) {
>> + prlog(PR_DEBUG, "SENSORS: Invalid data\n");
Can this say "missing IPMI sensor Mappings tuple" or something else
that's a bit more descriptive?
>> + return;
>> + }
>> +
>> + ipmi_sensors = HDIF_get_idata(hdif_sensor, IPMI_SENSORS_IDATA_SENSORS, NULL);
>> + if (!ipmi_sensors) {
>> + prlog(PR_DEBUG, "SENSORS: bad data\n");
>> + return;
>> + }
>> +
>> + sensors_node = dt_new(bmc_node, "sensors");
>> + assert(sensors_node);
>> +
>> + dt_add_property_cells(sensors_node, "#address-cells", 1);
>> + dt_add_property_cells(sensors_node, "#size-cells", 0);
>> +
>> + for (i = 0; i < be32_to_cpu(ipmi_sensors->count); i++) {
>> + if(dt_find_by_name_addr(sensors_node, "sensor",
>
> if (condition)
>
>> + ipmi_sensors->data[i].id)) {
>> + prlog(PR_WARNING, "SENSORS: Duplicate sensor ID : %x\n",
>> + ipmi_sensors->data[i].id);
>
> What causes a duplicate sensor ID?
>
> What can the user do with this information?
Duplicate IDs indicate bad input data. I'm not exactly sure where the
IPMI sensor list is sourced from, but I imagine it's the MRW. We want
to be loud about it so vendors will see it and hopefully fix it since
getting the MRW right isn't something that we (the OPAL team)
shouldn't have to be involved in.
>
>> + continue;
>> + }
>> +
>> + /* We support only < MAX_IPMI_SENSORS sensors */
>> + if (ipmi_sensors->data[i].type > 0xfe)
>
> use MAX_IPMI_SENSORS.
>
> ie.
>
> if (!(ipmi_sensors->data[i].type < MAX_IPMI_SENSORS))
>
Given sensor type is a single byte field we should just check that
it's not 0xff. Maybe #define that as IPMI_SENSOR_INVALID or some
such.
>> + continue;
>> +
>> + sensor_node = dt_new_addr(sensors_node, "sensor",
>> + ipmi_sensors->data[i].id);
>> + assert(sensor_node);
>> + dt_add_property_string(sensor_node, "compatible", "ibm,ipmi-sensor");
>> + dt_add_property_cells(sensor_node, "reg", ipmi_sensors->data[i].id);
>> + dt_add_property_cells(sensor_node, "ipmi-sensor-type",
>> + ipmi_sensors->data[i].type);
>> + }
>> +}
>> +
>> static void bmc_create_node(const struct HDIF_common_hdr *sp)
>> {
>> struct dt_node *bmc_node;
>> @@ -297,7 +344,9 @@ static void bmc_create_node(const struct HDIF_common_hdr *sp)
>> dt_add_property_cells(bmc_node, "#address-cells", 1);
>> dt_add_property_cells(bmc_node, "#size-cells", 0);
>>
>> - /* TODO: add sensor info under /bmc */
>> + /* Add sensor info under /bmc */
>> + add_ipmi_sensors(bmc_node);
>> +
>> sp_impl = HDIF_get_idata(sp, SPSS_IDATA_SP_IMPL, &size);
>> if (CHECK_SPPTR(sp_impl) && (size > 8)) {
>> dt_add_property_strings(bmc_node, "compatible", sp_impl->sp_family);
>> diff --git a/hdata/spira.c b/hdata/spira.c
>> index fd7f351..601c1b3 100644
>> --- a/hdata/spira.c
>> +++ b/hdata/spira.c
>> @@ -1234,6 +1234,7 @@ static void fixup_spira(void)
>> spira.ntuples.pcia = spiras->ntuples.pcia;
>> spira.ntuples.proc_chip = spiras->ntuples.proc_chip;
>> spira.ntuples.hs_data = spiras->ntuples.hs_data;
>> + spira.ntuples.ipmi_sensor = spiras->ntuples.ipmi_sensor;
>> }
>>
>> int parse_hdat(bool is_opal)
>> diff --git a/hdata/spira.h b/hdata/spira.h
>> index bb7ad3e..f141b72 100644
>> --- a/hdata/spira.h
>> +++ b/hdata/spira.h
>> @@ -68,6 +68,7 @@ struct spira_ntuples {
>> struct spira_ntuple pcia; /* 0x2e0 */
>> struct spira_ntuple proc_chip; /* 0x300 */
>> struct spira_ntuple hs_data; /* 0x320 */
>> + struct spira_ntuple ipmi_sensor; /* 0x360 */
>> };
>>
>> struct spira {
>> @@ -81,7 +82,7 @@ struct spira {
>> *
>> * According to FSP engineers, this is an okay thing to do.
>> */
>> - u8 reserved[0xc0];
>> + u8 reserved[0xa0];
>> } __packed __align(0x100);
>>
>> extern struct spira spira;
>> @@ -1063,6 +1064,27 @@ struct sppcrd_chip_tod {
>> /* Idata index 0 : System attribute data */
>> #define HSERV_IDATA_SYS_ATTR 0
>>
>> +/* IPMI sensors mapping data */
>> +#define IPMI_SENSORS_HDIF_SIG "FRUSE "
>
> What does FRUSE mean?
It's probably "FRU Sensors" squashed into six characters. No idea why
there's a space at the end though...
>
> What is it used for?
It's the ID string for this type of HDIF structure. The ID strings are
defined in the HDAT spec so there's not much we can do about them.
'FRUSE ' is pretty bad, but too late now.
>
>> +
>> +/* Idata index 0 : Sensor mapping data */
>> +#define IPMI_SENSORS_IDATA_SENSORS 0
>> +
>> +struct ipmi_sensors_data {
>> + __be32 slca_index;
>> + uint8_t type;
>> + uint8_t id;
>> + __be16 reserved;
>> +};
>
> Perhaps do this so you don't get padding:
>
> struct ipmi_sensors_data {
> __be32 slca_index;
> uint8_t type;
> uint8_t id;
> uint8_t reserved[2];
> };
>
> And add a packed annotation so that we don't get holes.
Just using the packed annotation should solve both problems.
Looks fine otherwise. Thanks for implementing this Vasant.
Oliver
More information about the Skiboot
mailing list