From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:42729) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1a20xV-0002Od-Kk for qemu-devel@nongnu.org; Thu, 26 Nov 2015 13:07:55 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1a20xS-000338-AA for qemu-devel@nongnu.org; Thu, 26 Nov 2015 13:07:53 -0500 Received: from e06smtp09.uk.ibm.com ([195.75.94.105]:40095) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1a20xR-00032t-T5 for qemu-devel@nongnu.org; Thu, 26 Nov 2015 13:07:50 -0500 Received: from localhost by e06smtp09.uk.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Thu, 26 Nov 2015 18:07:46 -0000 Received: from b06cxnps4075.portsmouth.uk.ibm.com (d06relay12.portsmouth.uk.ibm.com [9.149.109.197]) by d06dlp02.portsmouth.uk.ibm.com (Postfix) with ESMTP id 651A5219004D for ; Thu, 26 Nov 2015 18:07:27 +0000 (GMT) Received: from d06av07.portsmouth.uk.ibm.com (d06av07.portsmouth.uk.ibm.com [9.149.37.248]) by b06cxnps4075.portsmouth.uk.ibm.com (8.14.9/8.14.9/NCO v10.0) with ESMTP id tAQI7Xn810224094 for ; Thu, 26 Nov 2015 18:07:33 GMT Received: from d06av07.portsmouth.uk.ibm.com (localhost [127.0.0.1]) by d06av07.portsmouth.uk.ibm.com (8.14.4/8.14.4/NCO v10.0 AVout) with ESMTP id tAQI7XMM012552 for ; Thu, 26 Nov 2015 11:07:33 -0700 Message-ID: <56574A63.5020001@fr.ibm.com> Date: Thu, 26 Nov 2015 19:07:31 +0100 From: =?windows-1252?Q?C=E9dric_Le_Goater?= MIME-Version: 1.0 References: <1447354953-18893-1-git-send-email-minyard@acm.org> <1447354953-18893-4-git-send-email-minyard@acm.org> <5654669A.10301@fr.ibm.com> <5654BEA1.7090307@acm.org> In-Reply-To: <5654BEA1.7090307@acm.org> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 8bit Subject: Re: [Qemu-devel] [PATCH v4 03/17] ipmi: Add a local BMC simulation List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: minyard@acm.org, qemu-devel@nongnu.org, Paolo Bonzini , "Michael S. Tsirkin" , Igor Mammedov Cc: Corey Minyard 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 >>> >>> 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 >>> --- >>> 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 >>> +#include >>> +#include >>> +#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.