Re: [Qemu-devel] [PATCH 7/8] ipmi: introduce an ipmi_bmc_init_sensor() API

[Corey Minyard <cminyard@mvista.com>]

The way the SDR and sensors are handled currently in the code I wrote is 
far from ideal, it's not scalable.  In my mind, the BMC in qemu would 
never be a very elaborate one, you would use an external BMC for that.

There are a couple of issues to deal with here:

We need support for SDRs besides just sensor type 2, there are sensor 
type 1 and 3, management device locator, FRU device locator, entity 
association, and a few others.  Those are not important for the BMC, but 
they are important for management software using the BMC.  If we need to 
add all those, we probably need something more sophisticated, like using 
the openipmi library SDR compiler and loading the SDRs externally.  But 
if your needs are basic, then this is ok.

It would be nice if the SDR repository time did not change every time 
you restarted the system.  It's not a big deal for a few SDRs, but it 
could be for a larger system.

I'm ok with the patch as-is assuming your needs are simple, but if you 
need something more extensive we probably should think about something else.

A few comments inline, too.

On 01/05/2016 11:30 AM, Cédric Le Goater wrote:
> This routine will let qemu platforms populate the sdr/sensor tables of
> the IPMI BMC simulator with their customs needs.
>
> The patch adds a compact sensor record typedef to ease definition of
> sdrs. To be used in the code the following way:
>
>      static ipmi_sdr_compact_buffer my_init_sdrs[] = {
>          {   /* Firmware Progress Sensor */
>              0xff, 0xff, 0x51, 0x02,   43, 0x20, 0x00, 0xff,
>              0x22, 0x00, 0xff, 0x40, 0x0f, 0x6f, 0x07, 0x00,
>              0x00, 0x00, 0xff, 0xff, 0xc0, 0x00, 0x00, 0x01,
>              0x81, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0xd0,
>              'F',  'W',  ' ',  'B',  'o',  'o',  't',  ' ',
>              'P',  'r',  'o',  'g',  'r',  'e',  's',  's',
>          },
>          ...

I assume the idea is that you use struct ipmi_sdr_compact to define 
these so you can get names associated with the values.  Is that the case?

>      };
>
>      struct ipmi_sdr_compact *sdr =
>             (struct ipmi_sdr_compact *) &my_init_sdrs[0];
>
>      ipmi_bmc_init_sensor(IPMI_BMC(obj), my_init_sdrs[0],
>                       sdr->rec_length + 5, &sdr->sensor_owner_number);
>
> Signed-off-by: Cédric Le Goater <clg@fr.ibm.com>
> ---
>   hw/ipmi/ipmi_bmc_sim.c | 61 +++++++++++++++++++++++++++++++++++++++++---------
>   include/hw/ipmi/ipmi.h | 37 ++++++++++++++++++++++++++++++
>   2 files changed, 87 insertions(+), 11 deletions(-)
>
> diff --git a/hw/ipmi/ipmi_bmc_sim.c b/hw/ipmi/ipmi_bmc_sim.c
> index 4f7c74da4b6b..9618db44ce69 100644
> --- a/hw/ipmi/ipmi_bmc_sim.c
> +++ b/hw/ipmi/ipmi_bmc_sim.c
> @@ -527,6 +527,22 @@ static void sensor_set_discrete_bit(IPMIBmcSim *ibs, unsigned int sensor,
>       }
>   }
>   
> +static void ipmi_init_sensor(IPMISensor *sens, const uint8_t *sdr)
> +{
> +    IPMI_SENSOR_SET_PRESENT(sens, 1);
> +    IPMI_SENSOR_SET_SCAN_ON(sens, (sdr[10] >> 6) & 1);
> +    IPMI_SENSOR_SET_EVENTS_ON(sens, (sdr[10] >> 5) & 1);
> +    sens->assert_suppt = sdr[14] | (sdr[15] << 8);
> +    sens->deassert_suppt = sdr[16] | (sdr[17] << 8);
> +    sens->states_suppt = sdr[18] | (sdr[19] << 8);
> +    sens->sensor_type = sdr[12];
> +    sens->evt_reading_type_code = sdr[13] & 0x7f;

Can you use struct ipmi_sdr_compact to extract these?

> +
> +    /* Enable all the events that are supported. */
> +    sens->assert_enable = sens->assert_suppt;
> +    sens->deassert_enable = sens->deassert_suppt;
> +}
> +
>   static void ipmi_init_sensors_from_sdrs(IPMIBmcSim *s)
>   {
>       unsigned int i, pos;
> @@ -553,19 +569,42 @@ static void ipmi_init_sensors_from_sdrs(IPMIBmcSim *s)
>           }
>           sens = s->sensors + sdr[7];
>   
> -        IPMI_SENSOR_SET_PRESENT(sens, 1);
> -        IPMI_SENSOR_SET_SCAN_ON(sens, (sdr[10] >> 6) & 1);
> -        IPMI_SENSOR_SET_EVENTS_ON(sens, (sdr[10] >> 5) & 1);
> -        sens->assert_suppt = sdr[14] | (sdr[15] << 8);
> -        sens->deassert_suppt = sdr[16] | (sdr[17] << 8);
> -        sens->states_suppt = sdr[18] | (sdr[19] << 8);
> -        sens->sensor_type = sdr[12];
> -        sens->evt_reading_type_code = sdr[13] & 0x7f;
> +        ipmi_init_sensor(sens, sdr);
> +    }
> +}
> +
> +int ipmi_bmc_init_sensor(IPMIBmc *b, const uint8_t *sdr,
> +                         unsigned int len, uint8_t *sensor_num)
> +{
> +    IPMIBmcSim *ibs = IPMI_BMC_SIMULATOR(b);
> +    int ret;
> +    unsigned int i;
> +    IPMISensor *sens;
>   
> -        /* Enable all the events that are supported. */
> -        sens->assert_enable = sens->assert_suppt;
> -        sens->deassert_enable = sens->deassert_suppt;
> +    for (i = 0; i < MAX_SENSORS; i++) {
> +        sens = ibs->sensors + i;
> +        if (!IPMI_SENSOR_GET_PRESENT(sens)) {
> +            break;
> +        }
> +    }
> +
> +    if (i == MAX_SENSORS) {
> +        return 1;
>       }
> +
> +    ret = sdr_add_entry(ibs, sdr, len, NULL);
> +    if (ret) {
> +        return ret;
> +    }
> +
> +    ipmi_init_sensor(sens, sdr);
> +    if (sensor_num) {
> +        *sensor_num = i;
> +    }
> +
> +    /* patch sensor in sdr table. This is a little hacky. */
> +    ibs->sdr.sdr[ibs->sdr.next_free - len + 7] = i;
> +    return 0;
>   }
>   
>   static int ipmi_register_netfn(IPMIBmcSim *s, unsigned int netfn,
> diff --git a/include/hw/ipmi/ipmi.h b/include/hw/ipmi/ipmi.h
> index 32bac0fa8877..ce1f539754be 100644
> --- a/include/hw/ipmi/ipmi.h
> +++ b/include/hw/ipmi/ipmi.h
> @@ -210,4 +210,41 @@ IPMIFwInfo *ipmi_next_fwinfo(IPMIFwInfo *current);
>   #define ipmi_debug(fs, ...)
>   #endif
>   
> +/*
> + * 43.2 SDR Type 02h. Compact Sensor Record
> + */
> +struct ipmi_sdr_compact {
> +    uint16_t rec_id;
> +    uint8_t  sdr_version;               /* 0x51 */
> +    uint8_t  rec_type;                  /* 0x02 Compact Sensor Record */
> +    uint8_t  rec_length;
> +    uint8_t  sensor_owner_id;
> +    uint8_t  sensor_owner_lun;
> +    uint8_t  sensor_owner_number;       /* byte 8 */
> +    uint8_t  entity_id;
> +    uint8_t  entity_instance;
> +    uint8_t  sensor_init;
> +    uint8_t  sensor_caps;
> +    uint8_t  sensor_type;
> +    uint8_t  reading_type;
> +    uint8_t  assert_mask[2];            /* byte 16 */
> +    uint8_t  deassert_mask[2];
> +    uint8_t  discrete_mask[2];
> +    uint8_t  sensor_unit1;
> +    uint8_t  sensor_unit2;
> +    uint8_t  sensor_unit3;
> +    uint8_t  sensor_direction[2];       /* byte 24 */
> +    uint8_t  positive_threshold;
> +    uint8_t  negative_threshold;
> +    uint8_t  reserved[3];
> +    uint8_t  oem;
> +    uint8_t  id_str_len;                /* byte 32 */
> +    uint8_t  id_string[16];
> +};
> +
> +typedef uint8_t ipmi_sdr_compact_buffer[sizeof(struct ipmi_sdr_compact)];
> +
> +int ipmi_bmc_init_sensor(IPMIBmc *b, const uint8_t *entry,
> +                         unsigned int len, uint8_t *sensor_num);
> +
>   #endif