Re: [Qemu-devel] [PATCH v4 03/17] ipmi: Add a local BMC simulation

[Corey Minyard]

On 11/24/2015 07:31 AM, Cédric Le Goater wrote:
> A few comments below,

Thanks a bunch for the review.  As you probably have guessed, this was
not really intended as a fully functional BMC, though it has most of the
trappings
of what you would need to implement one.  I assume you are working on the
power systems, and it makes sense to extend this for that application.

> On 11/12/2015 08:02 PM, address@hidden wrote:
>> From: Corey Minyard <address@hidden>
>>
>> This provides a minimal local BMC, basically enough to comply with the
>> spec and provide a complete watchdog timer (including a sensor, SDR,
>> and event).
>>
>> Signed-off-by: Corey Minyard <address@hidden>
>> ---
>>  default-configs/i386-softmmu.mak   |    1 +
>>  default-configs/x86_64-softmmu.mak |    1 +
>>  hw/ipmi/Makefile.objs              |    1 +
>>  hw/ipmi/ipmi_bmc_sim.c             | 1731 
>> ++++++++++++++++++++++++++++++++++++
>>  4 files changed, 1734 insertions(+)
>>  create mode 100644 hw/ipmi/ipmi_bmc_sim.c
>>
>> diff --git a/default-configs/i386-softmmu.mak 
>> b/default-configs/i386-softmmu.mak
>> index 8fa751a..00a0346 100644
>> --- a/default-configs/i386-softmmu.mak
>> +++ b/default-configs/i386-softmmu.mak
>> @@ -10,6 +10,7 @@ CONFIG_VMWARE_VGA=y
>>  CONFIG_VIRTIO_VGA=y
>>  CONFIG_VMMOUSE=y
>>  CONFIG_IPMI=y
>> +CONFIG_IPMI_LOCAL=y
>>  CONFIG_SERIAL=y
>>  CONFIG_PARALLEL=y
>>  CONFIG_I8254=y
>> diff --git a/default-configs/x86_64-softmmu.mak 
>> b/default-configs/x86_64-softmmu.mak
>> index 6767f4f..39a619f 100644
>> --- a/default-configs/x86_64-softmmu.mak
>> +++ b/default-configs/x86_64-softmmu.mak
>> @@ -10,6 +10,7 @@ CONFIG_VMWARE_VGA=y
>>  CONFIG_VIRTIO_VGA=y
>>  CONFIG_VMMOUSE=y
>>  CONFIG_IPMI=y
>> +CONFIG_IPMI_LOCAL=y
>>  CONFIG_SERIAL=y
>>  CONFIG_PARALLEL=y
>>  CONFIG_I8254=y
>> diff --git a/hw/ipmi/Makefile.objs b/hw/ipmi/Makefile.objs
>> index 65bde11..875271c 100644
>> --- a/hw/ipmi/Makefile.objs
>> +++ b/hw/ipmi/Makefile.objs
>> @@ -1 +1,2 @@
>>  common-obj-$(CONFIG_IPMI) += ipmi.o
>> +common-obj-$(CONFIG_IPMI_LOCAL) += ipmi_bmc_sim.o
>> diff --git a/hw/ipmi/ipmi_bmc_sim.c b/hw/ipmi/ipmi_bmc_sim.c
>> new file mode 100644
>> index 0000000..d246029
>> --- /dev/null
>> +++ b/hw/ipmi/ipmi_bmc_sim.c
>> @@ -0,0 +1,1731 @@
>> +/*
>> + * IPMI BMC emulation
>> + *
>> + * Copyright (c) 2015 Corey Minyard, MontaVista Software, LLC
>> + *
>> + * Permission is hereby granted, free of charge, to any person obtaining a 
>> copy
>> + * of this software and associated documentation files (the "Software"), to 
>> deal
>> + * in the Software without restriction, including without limitation the 
>> rights
>> + * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
>> + * copies of the Software, and to permit persons to whom the Software is
>> + * furnished to do so, subject to the following conditions:
>> + *
>> + * The above copyright notice and this permission notice shall be included 
>> in
>> + * all copies or substantial portions of the Software.
>> + *
>> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS 
>> OR
>> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
>> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
>> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR 
>> OTHER
>> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING 
>> FROM,
>> + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
>> + * THE SOFTWARE.
>> + */
>> +
>> +#include <stdio.h>
>> +#include <string.h>
>> +#include <stdint.h>
>> +#include "qemu/timer.h"
>> +#include "hw/ipmi/ipmi.h"
>> +#include "qemu/error-report.h"
>> +
>> +#define IPMI_NETFN_CHASSIS            0x00
>> +#define IPMI_NETFN_CHASSIS_MAXCMD         0x03
>> +
>> +#define IPMI_CMD_GET_CHASSIS_CAPABILITIES 0x00
>> +#define IPMI_CMD_GET_CHASSIS_STATUS       0x01
>> +#define IPMI_CMD_CHASSIS_CONTROL          0x02
>> +
>> +#define IPMI_NETFN_SENSOR_EVENT       0x04
>> +#define IPMI_NETFN_SENSOR_EVENT_MAXCMD    0x2e
>> +
>> +#define IPMI_CMD_SET_SENSOR_EVT_ENABLE    0x28
>> +#define IPMI_CMD_GET_SENSOR_EVT_ENABLE    0x29
>> +#define IPMI_CMD_REARM_SENSOR_EVTS        0x2a
>> +#define IPMI_CMD_GET_SENSOR_EVT_STATUS    0x2b
>> +#define IPMI_CMD_GET_SENSOR_READING       0x2d
> I have started adding a few commands and got scared by 
> IPMI_CMD_SET_SENSOR_READING spec. By any chance, would 
> you  have one in stock ? 

I'm not sure what you mean.  That was added recently and is optional.
If your target system doesn't implement it, you don't really need to
implement it, either.

The IPMI spec is short on rationale, but I'm guessing that particular
command has two purposes:

  * Sensors whose value is provided by software running on the system.
  * Testing for situations that you cannot reasonably reproduce on a
    real system.

Is there a reason you would need this particular command?  If so, it will
take a little work but won't be too hard.

>> +
>> +/* #define IPMI_NETFN_APP             0x06 In ipmi.h */
>> +#define IPMI_NETFN_APP_MAXCMD             0x36
>> +
>> +#define IPMI_CMD_GET_DEVICE_ID            0x01
>> +#define IPMI_CMD_COLD_RESET               0x02
>> +#define IPMI_CMD_WARM_RESET               0x03
>> +#define IPMI_CMD_RESET_WATCHDOG_TIMER     0x22
>> +#define IPMI_CMD_SET_WATCHDOG_TIMER       0x24
>> +#define IPMI_CMD_GET_WATCHDOG_TIMER       0x25
>> +#define IPMI_CMD_SET_BMC_GLOBAL_ENABLES   0x2e
>> +#define IPMI_CMD_GET_BMC_GLOBAL_ENABLES   0x2f
>> +#define IPMI_CMD_CLR_MSG_FLAGS            0x30
>> +#define IPMI_CMD_GET_MSG_FLAGS            0x31
>> +#define IPMI_CMD_GET_MSG                  0x33
>> +#define IPMI_CMD_SEND_MSG                 0x34
>> +#define IPMI_CMD_READ_EVT_MSG_BUF         0x35
>> +
>> +#define IPMI_NETFN_STORAGE            0x0a
>> +#define IPMI_NETFN_STORAGE_MAXCMD         0x4a
>> +
>> +#define IPMI_CMD_GET_SDR_REP_INFO         0x20
>> +#define IPMI_CMD_GET_SDR_REP_ALLOC_INFO   0x21
>> +#define IPMI_CMD_RESERVE_SDR_REP          0x22
>> +#define IPMI_CMD_GET_SDR                  0x23
>> +#define IPMI_CMD_ADD_SDR                  0x24
>> +#define IPMI_CMD_PARTIAL_ADD_SDR          0x25
>> +#define IPMI_CMD_DELETE_SDR               0x26
>> +#define IPMI_CMD_CLEAR_SDR_REP            0x27
>> +#define IPMI_CMD_GET_SDR_REP_TIME         0x28
>> +#define IPMI_CMD_SET_SDR_REP_TIME         0x29
>> +#define IPMI_CMD_ENTER_SDR_REP_UPD_MODE   0x2A
>> +#define IPMI_CMD_EXIT_SDR_REP_UPD_MODE    0x2B
>> +#define IPMI_CMD_RUN_INIT_AGENT           0x2C
>> +#define IPMI_CMD_GET_SEL_INFO             0x40
>> +#define IPMI_CMD_GET_SEL_ALLOC_INFO       0x41
>> +#define IPMI_CMD_RESERVE_SEL              0x42
>> +#define IPMI_CMD_GET_SEL_ENTRY            0x43
>> +#define IPMI_CMD_ADD_SEL_ENTRY            0x44
>> +#define IPMI_CMD_PARTIAL_ADD_SEL_ENTRY    0x45
>> +#define IPMI_CMD_DELETE_SEL_ENTRY         0x46
>> +#define IPMI_CMD_CLEAR_SEL                0x47
>> +#define IPMI_CMD_GET_SEL_TIME             0x48
>> +#define IPMI_CMD_SET_SEL_TIME             0x49
>> +
>> +
>> +/* Same as a timespec struct. */
>> +struct ipmi_time {
>> +    long tv_sec;
>> +    long tv_nsec;
>> +};
>> +
>> +#define MAX_SEL_SIZE 128
>> +
>> +typedef struct IPMISel {
>> +    uint8_t sel[MAX_SEL_SIZE][16];
>> +    unsigned int next_free;
>> +    long time_offset;
>> +    uint16_t reservation;
>> +    uint8_t last_addition[4];
>> +    uint8_t last_clear[4];
>> +    uint8_t overflow;
>> +} IPMISel;
>> +
>> +#define MAX_SDR_SIZE 16384
>> +
>> +typedef struct IPMISdr {
>> +    uint8_t sdr[MAX_SDR_SIZE];
>> +    unsigned int next_free;
>> +    uint16_t next_rec_id;
>> +    uint16_t reservation;
>> +    uint8_t last_addition[4];
>> +    uint8_t last_clear[4];
>> +    uint8_t overflow;
>> +} IPMISdr;
>> +
>> +typedef struct IPMISensor {
>> +    uint8_t status;
>> +    uint8_t reading;
>> +    uint16_t states_suppt;
>> +    uint16_t assert_suppt;
>> +    uint16_t deassert_suppt;
>> +    uint16_t states;
>> +    uint16_t assert_states;
>> +    uint16_t deassert_states;
>> +    uint16_t assert_enable;
>> +    uint16_t deassert_enable;
>> +    uint8_t  sensor_type;
>> +    uint8_t  evt_reading_type_code;
>> +} IPMISensor;
>> +#define IPMI_SENSOR_GET_PRESENT(s)       ((s)->status & 0x01)
>> +#define IPMI_SENSOR_SET_PRESENT(s, v)    ((s)->status = (s->status & ~0x01) 
>> | \
>> +                                             !!(v))
>> +#define IPMI_SENSOR_GET_SCAN_ON(s)       ((s)->status & 0x40)
>> +#define IPMI_SENSOR_SET_SCAN_ON(s, v)    ((s)->status = (s->status & ~0x40) 
>> | \
>> +                                             ((!!(v)) << 6))
>> +#define IPMI_SENSOR_GET_EVENTS_ON(s)     ((s)->status & 0x80)
>> +#define IPMI_SENSOR_SET_EVENTS_ON(s, v)  ((s)->status = (s->status & ~0x80) 
>> | \
>> +                                             ((!!(v)) << 7))
>> +#define IPMI_SENSOR_GET_RET_STATUS(s)    ((s)->status & 0xc0)
>> +#define IPMI_SENSOR_SET_RET_STATUS(s, v) ((s)->status = (s->status & ~0xc0) 
>> | \
>> +                                             (v & 0xc0))
>> +#define IPMI_SENSOR_IS_DISCRETE(s) ((s)->evt_reading_type_code != 1)
>> +
>> +#define MAX_SENSORS 20
> Could we extend that a bit ? like 80 ? There are real systems with more 
> than 180 sensors. 

Actually, there are systems with more than that :).  This was a quick hack,
and I'm generally against arbitrary limitations.  But this can be fixed.

>> +#define IPMI_WATCHDOG_SENSOR 0
>> +
>> +typedef struct IPMIBmcSim IPMIBmcSim;
>> +
>> +#define MAX_NETFNS 64
>> +typedef void (*IPMICmdHandler)(IPMIBmcSim *s,
>> +                               uint8_t *cmd, unsigned int cmd_len,
>> +                               uint8_t *rsp, unsigned int *rsp_len,
>> +                               unsigned int max_rsp_len);
>> +typedef struct IPMINetfn {
>> +    unsigned int cmd_nums;
>> +    const IPMICmdHandler *cmd_handlers;
>> +} IPMINetfn;
>> +
>> +typedef struct IPMIRcvBufEntry {
>> +    QTAILQ_ENTRY(IPMIRcvBufEntry) entry;
>> +    uint8_t len;
>> +    uint8_t buf[MAX_IPMI_MSG_SIZE];
>> +} IPMIRcvBufEntry;
>> +
>> +#define TYPE_IPMI_BMC_SIMULATOR "ipmi-bmc-sim"
>> +#define IPMI_BMC_SIMULATOR(obj) OBJECT_CHECK(IPMIBmcSim, (obj), \
>> +                                        TYPE_IPMI_BMC_SIMULATOR)
>> +struct IPMIBmcSim {
>> +    IPMIBmc parent;
>> +
>> +    QEMUTimer *timer;
>> +
>> +    uint8_t bmc_global_enables;
>> +    uint8_t msg_flags;
>> +
>> +    bool     watchdog_initialized;
>> +    uint8_t  watchdog_use;
>> +    uint8_t  watchdog_action;
>> +    uint8_t  watchdog_pretimeout; /* In seconds */
>> +    bool     watchdog_expired;
>> +    uint16_t watchdog_timeout; /* in 100's of milliseconds */
>> +
>> +    bool     watchdog_running;
>> +    bool     watchdog_preaction_ran;
>> +    int64_t  watchdog_expiry;
>> +
>> +    uint8_t device_id;
>> +    uint8_t ipmi_version;
>> +    uint8_t device_rev;
>> +    uint8_t fwrev1;
>> +    uint8_t fwrev2;
>> +    uint8_t mfg_id[3];
>> +    uint8_t product_id[2];
>> +
>> +    IPMISel sel;
>> +    IPMISdr sdr;
>> +    IPMISensor sensors[MAX_SENSORS];
>> +
>> +    /* Odd netfns are for responses, so we only need the even ones. */
>> +    const IPMINetfn *netfns[MAX_NETFNS / 2];
>> +
>> +    QemuMutex lock;
>> +    /* We allow one event in the buffer */
>> +    uint8_t evtbuf[16];
>> +
>> +    QTAILQ_HEAD(, IPMIRcvBufEntry) rcvbufs;
>> +};
>> +
>> +#define IPMI_BMC_MSG_FLAG_WATCHDOG_TIMEOUT_MASK        (1 << 3)
>> +#define IPMI_BMC_MSG_FLAG_EVT_BUF_FULL                 (1 << 1)
>> +#define IPMI_BMC_MSG_FLAG_RCV_MSG_QUEUE                (1 << 0)
>> +#define IPMI_BMC_MSG_FLAG_WATCHDOG_TIMEOUT_MASK_SET(s) \
>> +    (IPMI_BMC_MSG_FLAG_WATCHDOG_TIMEOUT_MASK & (s)->msg_flags)
>> +#define IPMI_BMC_MSG_FLAG_EVT_BUF_FULL_SET(s) \
>> +    (IPMI_BMC_MSG_FLAG_EVT_BUF_FULL & (s)->msg_flags)
>> +#define IPMI_BMC_MSG_FLAG_RCV_MSG_QUEUE_SET(s) \
>> +    (IPMI_BMC_MSG_FLAG_RCV_MSG_QUEUE & (s)->msg_flags)
>> +
>> +#define IPMI_BMC_RCV_MSG_QUEUE_INT_BIT    0
>> +#define IPMI_BMC_EVBUF_FULL_INT_BIT       1
>> +#define IPMI_BMC_EVENT_MSG_BUF_BIT        2
>> +#define IPMI_BMC_EVENT_LOG_BIT            3
>> +#define IPMI_BMC_MSG_INTS_ON(s) ((s)->bmc_global_enables & \
>> +                                 (1 << IPMI_BMC_RCV_MSG_QUEUE_INT_BIT))
>> +#define IPMI_BMC_EVBUF_FULL_INT_ENABLED(s) ((s)->bmc_global_enables & \
>> +                                        (1 << IPMI_BMC_EVBUF_FULL_INT_BIT))
>> +#define IPMI_BMC_EVENT_LOG_ENABLED(s) ((s)->bmc_global_enables & \
>> +                                       (1 << IPMI_BMC_EVENT_LOG_BIT))
>> +#define IPMI_BMC_EVENT_MSG_BUF_ENABLED(s) ((s)->bmc_global_enables & \
>> +                                           (1 << 
>> IPMI_BMC_EVENT_MSG_BUF_BIT))
>> +
>> +#define IPMI_BMC_WATCHDOG_USE_MASK 0xc7
>> +#define IPMI_BMC_WATCHDOG_ACTION_MASK 0x77
>> +#define IPMI_BMC_WATCHDOG_GET_USE(s) ((s)->watchdog_use & 0x7)
>> +#define IPMI_BMC_WATCHDOG_GET_DONT_LOG(s) (((s)->watchdog_use >> 7) & 0x1)
>> +#define IPMI_BMC_WATCHDOG_GET_DONT_STOP(s) (((s)->watchdog_use >> 6) & 0x1)
>> +#define IPMI_BMC_WATCHDOG_GET_PRE_ACTION(s) (((s)->watchdog_action >> 4) & 
>> 0x7)
>> +#define IPMI_BMC_WATCHDOG_PRE_NONE               0
>> +#define IPMI_BMC_WATCHDOG_PRE_SMI                1
>> +#define IPMI_BMC_WATCHDOG_PRE_NMI                2
>> +#define IPMI_BMC_WATCHDOG_PRE_MSG_INT            3
>> +#define IPMI_BMC_WATCHDOG_GET_ACTION(s) ((s)->watchdog_action & 0x7)
>> +#define IPMI_BMC_WATCHDOG_ACTION_NONE            0
>> +#define IPMI_BMC_WATCHDOG_ACTION_RESET           1
>> +#define IPMI_BMC_WATCHDOG_ACTION_POWER_DOWN      2
>> +#define IPMI_BMC_WATCHDOG_ACTION_POWER_CYCLE     3
>> +
>> +
>> +/* Add a byte to the response. */
>> +#define IPMI_ADD_RSP_DATA(b) \
>> +    do {                                                   \
>> +        if (*rsp_len >= max_rsp_len) {                     \
>> +            rsp[2] = IPMI_CC_REQUEST_DATA_TRUNCATED;       \
>> +            goto out;                                      \
>> +        }                                                  \
>> +        rsp[(*rsp_len)++] = (b);                           \
>> +    } while (0)
>> +
>> +/* Verify that the received command is a certain length. */
>> +#define IPMI_CHECK_CMD_LEN(l) \
>> +    if (cmd_len < l) {                                     \
>> +        rsp[2] = IPMI_CC_REQUEST_DATA_LENGTH_INVALID;      \
>> +        goto out; \
>> +    }
>> +
>> +/* Check that the reservation in the command is valid. */
>> +#define IPMI_CHECK_RESERVATION(off, r) \
>> +    do {                                                   \
>> +        if ((cmd[off] | (cmd[off + 1] << 8)) != r) {       \
>> +            rsp[2] = IPMI_CC_INVALID_RESERVATION;          \
>> +            goto out;                                      \
>> +        }                                                  \
>> +    } while (0)
>> +
>> +
>> +static void ipmi_sim_handle_timeout(IPMIBmcSim *ibs);
>> +
>> +static void ipmi_gettime(struct ipmi_time *time)
>> +{
>> +    int64_t stime;
>> +
>> +    stime = qemu_clock_get_ns(QEMU_CLOCK_HOST);
>> +    time->tv_sec = stime / 1000000000LL;
>> +    time->tv_nsec = stime % 1000000000LL;
>> +}
>> +
>> +static int64_t ipmi_getmonotime(void)
>> +{
>> +    return qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
>> +}
>> +
>> +static void ipmi_timeout(void *opaque)
>> +{
>> +    IPMIBmcSim *ibs = opaque;
>> +
>> +    ipmi_sim_handle_timeout(ibs);
>> +}
>> +
>> +static void set_timestamp(IPMIBmcSim *ibs, uint8_t *ts)
>> +{
>> +    unsigned int val;
>> +    struct ipmi_time now;
>> +
>> +    ipmi_gettime(&now);
>> +    val = now.tv_sec + ibs->sel.time_offset;
>> +    ts[0] = val & 0xff;
>> +    ts[1] = (val >> 8) & 0xff;
>> +    ts[2] = (val >> 16) & 0xff;
>> +    ts[3] = (val >> 24) & 0xff;
>> +}
>> +
>> +static void sdr_inc_reservation(IPMISdr *sdr)
>> +{
>> +    sdr->reservation++;
>> +    if (sdr->reservation == 0) {
>> +        sdr->reservation = 1;
>> +    }
>> +}
> Could we externalize the following routine ? for platforms needing 
> to add a few initial sdrs.
>  

Yes.  You'll probably need a few others, too.

>> +static int sdr_add_entry(IPMIBmcSim *ibs, const uint8_t *entry,
>> +                         unsigned int len, uint16_t *recid)
>> +{
>> +    if ((len < 5) || (len > 255)) {
>> +        return 1;
>> +    }
>> +
>> +    if (entry[ibs->sdr.next_free + 4] != len) {
> I think you meant :
>
>       if (entry[4] != len) {
> no ? 

You are correct, this will be fixed in my next release.

>
>> +        return 1;
>> +    }
>> +
>> +    if (ibs->sdr.next_free + len > MAX_SDR_SIZE) {
>> +        ibs->sdr.overflow = 1;
>> +        return 1;
>> +    }
>> +
>> +    memcpy(ibs->sdr.sdr + ibs->sdr.next_free, entry, len);
>> +    ibs->sdr.sdr[ibs->sdr.next_free] = ibs->sdr.next_rec_id & 0xff;
>> +    ibs->sdr.sdr[ibs->sdr.next_free+1] = (ibs->sdr.next_rec_id >> 8) & 0xff;
>> +    ibs->sdr.sdr[ibs->sdr.next_free+2] = 0x51; /* Conform to IPMI 1.5 spec 
>> */
>> +
>> +    if (recid) {
>> +        *recid = ibs->sdr.next_rec_id;
>> +    }
>> +    ibs->sdr.next_rec_id++;
>> +    set_timestamp(ibs, ibs->sdr.last_addition);
>> +    ibs->sdr.next_free += len;
>> +    sdr_inc_reservation(&ibs->sdr);
>> +    return 0;
>> +}
>> +
> [ ... ]
>
>> +static void register_cmds(IPMIBmcSim *s)
>> +{
>> +    ipmi_register_netfn(s, IPMI_NETFN_CHASSIS, &chassis_netfn);
>> +    ipmi_register_netfn(s, IPMI_NETFN_SENSOR_EVENT, &sensor_event_netfn);
>> +    ipmi_register_netfn(s, IPMI_NETFN_APP, &app_netfn);
>> +    ipmi_register_netfn(s, IPMI_NETFN_STORAGE, &storage_netfn);
>> +}
>> +
>> +static const uint8_t init_sdrs[] = {
>> +    /* Watchdog device */
>> +    0x00, 0x00, 0x51, 0x02,   40, 0x20, 0x00, 0x00,
> Regarding the 'length' byte, the specs says : 
>
>       Record Length: Number of bytes of data following the Record Length 
> field.
>
> So it should be 35 and not 40 ?  This change will problably 
> have some consequences in the routine above.

Yes, I've fixed these both.

>
>> +    0x23, 0x01, 0x63, 0x00, 0x23, 0x6f, 0x0f, 0x01,
>> +    0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
>> +    0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0xc8,
>> +    'W',  'a',  't',  'c',  'h',  'd',  'o',  'g',
>> +    /* End */
>> +    0xff, 0xff, 0x00, 0x00, 0x00
>> +};
>> +
>> +static void ipmi_sim_init(Object *obj)
>> +{
>> +    IPMIBmc *b = IPMI_BMC(obj);
>> +    unsigned int i;
>> +    unsigned int next_recid, recid;
>> +    IPMIBmcSim *ibs = IPMI_BMC_SIMULATOR(b);
>> +
>> +    qemu_mutex_init(&ibs->lock);
>> +    QTAILQ_INIT(&ibs->rcvbufs);
>> +
>> +    ibs->bmc_global_enables = (1 << IPMI_BMC_EVENT_LOG_BIT);
>> +    ibs->device_id = 0x20;
>> +    ibs->ipmi_version = 0x02; /* IPMI 2.0 */
>> +    for (i = 0; i < 4; i++) {
>> +        ibs->sel.last_addition[i] = 0xff;
>> +        ibs->sel.last_clear[i] = 0xff;
>> +        ibs->sdr.last_addition[i] = 0xff;
>> +        ibs->sdr.last_clear[i] = 0xff;
>> +    }
>> +
>> +    next_recid = 0;
>> +    for (i = 0;;) {
>> +        int len;
>> +        if ((i + 5) > sizeof(init_sdrs)) {
>> +            error_report("Problem with recid 0x%4.4x\n", i);
>> +            return;
>> +        }
>> +        len = init_sdrs[i + 4];
>> +        recid = init_sdrs[i] | (init_sdrs[i + 1] << 8);
>> +        if (recid == 0xffff) {
>> +            break;
>> +        }
>> +        if ((i + len) > sizeof(init_sdrs)) {
>> +            error_report("Problem with recid 0x%4.4x\n", i);
>> +            return;
>> +        }
>> +        if (recid != next_recid) {
>> +            error_report("Problem with recid 0x%4.4x\n", i);
>> +            return;
>> +        }
> May be remove the check on recid  which is useless here ? 

Yes, that was left over from something else that was removed.

Thanks,

-corey

>> +        sdr_add_entry(ibs, init_sdrs + i, len, NULL);
>> +        i += len;
>> +    }
>> +
>> +    ipmi_init_sensors_from_sdrs(ibs);
>> +    register_cmds(ibs);
>> +
>> +    ibs->timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, ipmi_timeout, ibs);
>> +}
>> +
>> +static void ipmi_sim_class_init(ObjectClass *oc, void *data)
>> +{
>> +    IPMIBmcClass *bk = IPMI_BMC_CLASS(oc);
>> +
>> +    bk->handle_command = ipmi_sim_handle_command;
>> +}
>> +
>> +static const TypeInfo ipmi_sim_type = {
>> +    .name          = TYPE_IPMI_BMC_SIMULATOR,
>> +    .parent        = TYPE_IPMI_BMC,
>> +    .instance_size = sizeof(IPMIBmcSim),
>> +    .instance_init = ipmi_sim_init,
>> +    .class_init    = ipmi_sim_class_init,
>> +};
>> +
>> +static void ipmi_sim_register_types(void)
>> +{
>> +    type_register_static(&ipmi_sim_type);
>> +}
>> +
>> +type_init(ipmi_sim_register_types)
>>