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 <c...@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?
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
>
