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

[Cédric Le Goater]

On 01/08/2016 09:18 PM, Corey Minyard wrote:
> 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.

Yes. I agree. It is a simulator.

> 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.

yes for the moment, it is enough to satisfy the guest but may be we can
anticipate a bit and improve the initial loading of the SDR without 
rewriting a full BMC.

> 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.

So are you thinking on using a file backend for the SDR repository ? 

That would solve the time issue I suppose and let a system use a custom 
base for its BMC. It also sounds like a better API for the BMC simulator 
than the one proposed by this patch. 

We could start with a simple file loader handling type 2 SDR and later 
use the openipmi library if the needs arise. Ideally, if the initial 
loader could use a raw SDR dump data file, that would be perfect. May
be too complex for the job. 

> 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?

Yes. The changelog was not explicit on the goal. I got tired on counting the
bytes :)
 
>>      };
>>
>>      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 <address@hidden>
>> ---
>>   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?

Yes. Next version of the patchset will start with such a patch.

Thanks,

C.

> 
>> +
>> +    /* 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
>