On 11/24/2015 08:46 PM, Corey Minyard wrote: > 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.
Yes. I am trying to add the few extra commands the opal firmware and the kernel are using when running on an open power platform. It is currently booting fine but there are a few errors and still a few gaps to fill to make it look like a BMC was in charge. FRU would be nice also but we will see how it goes. >> On 11/12/2015 08:02 PM, miny...@acm.org wrote: >>> From: Corey Minyard <cminy...@mvista.com> >>> >>> 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 <cminy...@mvista.com> >>> --- >>> 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? Yes. The Open Power systems use this command to set the "System Progress Sensor (0F)" and another one "Boot Count (C3) " which is an OEM one. Looking at the code, I think the message we send as an issue with the spec, or the receiver has non specified expectations : https://github.com/open-power/skiboot/blob/master/hw/ipmi/ipmi-sensor.c request.sensor_number = fw_sensor_num; request.operation = 0xa0; /* Set event data bytes, assertion bits */ request.assertion_mask[0] = 0x04; /* Firmware progress offset */ request.event_data[1] = state; Should not that be : request.sensor_number = fw_sensor_num; request.operation = 0xa0; /* Set event data bytes, assertion bits */ request.assertion_mask[0] = 0x04; /* Firmware progress offset (2) */ request.event_data[1] = 0xc2; /* event extension in byte2, event offset 2 */ request.event_data[2] = state; ? > If so, it will take a little work but won't be too hard. Well, the event data operations should be interesting ! Should we generate an event for each command SET_SENSOR_READING we receive ? Thanks, C.
