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

["Cédric Le Goater" <clg@fr.ibm.com>]

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, minyard@acm.org wrote:
>>> From: Corey Minyard <cminyard@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 <cminyard@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.