| Message ID | 1452015002-28493-8-git-send-email-clg@fr.ibm.com |
|---|---|
| State | New |
| Headers | show |
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
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 <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? 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 >
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; + + /* 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
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', }, ... }; 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(-)