From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:59789) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aVbI6-00025b-Hf for qemu-devel@nongnu.org; Tue, 16 Feb 2016 03:47:28 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1aVbI1-0005X5-9o for qemu-devel@nongnu.org; Tue, 16 Feb 2016 03:47:26 -0500 Received: from e06smtp11.uk.ibm.com ([195.75.94.107]:44724) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aVbI1-0005WX-0K for qemu-devel@nongnu.org; Tue, 16 Feb 2016 03:47:21 -0500 Received: from localhost by e06smtp11.uk.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Tue, 16 Feb 2016 08:47:19 -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 EE2B32190061 for ; Tue, 16 Feb 2016 08:47:01 +0000 (GMT) Received: from d06av08.portsmouth.uk.ibm.com (d06av08.portsmouth.uk.ibm.com [9.149.37.249]) by b06cxnps4075.portsmouth.uk.ibm.com (8.14.9/8.14.9/NCO v10.0) with ESMTP id u1G8lGi645088888 for ; Tue, 16 Feb 2016 08:47:16 GMT Received: from d06av08.portsmouth.uk.ibm.com (localhost [127.0.0.1]) by d06av08.portsmouth.uk.ibm.com (8.14.4/8.14.4/NCO v10.0 AVout) with ESMTP id u1G8lGax011796 for ; Tue, 16 Feb 2016 01:47:16 -0700 References: <1455020010-17532-1-git-send-email-clg@fr.ibm.com> <1455020010-17532-7-git-send-email-clg@fr.ibm.com> <56C04819.6050604@gmail.com> <56C2081E.6080104@fr.ibm.com> <56C21BAE.2040104@redhat.com> <56C299BD.4020004@mvista.com> From: =?UTF-8?Q?C=c3=a9dric_Le_Goater?= Message-ID: <56C2E212.5020605@fr.ibm.com> Date: Tue, 16 Feb 2016 09:47:14 +0100 MIME-Version: 1.0 In-Reply-To: <56C299BD.4020004@mvista.com> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8bit Subject: Re: [Qemu-devel] [PATCH 6/8] ipmi: provide support for FRUs List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Corey Minyard , Marcel Apfelbaum Cc: "Michael S. Tsirkin" , qemu-devel@nongnu.org, Greg Kurz On 02/16/2016 04:38 AM, Corey Minyard wrote: > On 02/15/2016 12:40 PM, Marcel Apfelbaum wrote: >> On 02/15/2016 07:17 PM, Cédric Le Goater wrote: >>> On 02/14/2016 10:25 AM, Marcel Apfelbaum wrote: >>>> On 02/09/2016 02:13 PM, Cédric Le Goater wrote: >>>>> This patch provides a simple FRU support for the BMC simulator. FRUs >>>>> are loaded from a file which name is specified in the object >>>>> properties, each entry having a fixed size, also specified in the >>>>> properties. If the file is unknown or not accessible for some reason, >>>>> a unique entry of 1024 bytes is created as a default. Just enough to >>>>> start some simulation. >>>>> >>>>> Signed-off-by: Cédric Le Goater >>>>> --- >>>>> hw/ipmi/ipmi_bmc_sim.c | 140 +++++++++++++++++++++++++++++++++++++++++++++++++ >>>>> 1 file changed, 140 insertions(+) >>>>> >>>>> diff --git a/hw/ipmi/ipmi_bmc_sim.c b/hw/ipmi/ipmi_bmc_sim.c >>>>> index 69318eb6b556..b0754893fc08 100644 >>>>> --- a/hw/ipmi/ipmi_bmc_sim.c >>>>> +++ b/hw/ipmi/ipmi_bmc_sim.c >>>>> @@ -80,6 +80,9 @@ >>>>> #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_FRU_AREA_INFO 0x10 >>>>> +#define IPMI_CMD_READ_FRU_DATA 0x11 >>>>> +#define IPMI_CMD_WRITE_FRU_DATA 0x12 >>>>> #define IPMI_CMD_GET_SEL_INFO 0x40 >>>>> #define IPMI_CMD_GET_SEL_ALLOC_INFO 0x41 >>>>> #define IPMI_CMD_RESERVE_SEL 0x42 >>>>> @@ -122,6 +125,13 @@ typedef struct IPMISdr { >>>>> uint8_t overflow; >>>>> } IPMISdr; >>>>> >>>>> +typedef struct IPMIFru { >>>>> + char *filename; >>>>> + unsigned int nentries; >>>>> + uint16_t size; >>>>> + uint8_t *data; >>>>> +} IPMIFru; >>>>> + >>>>> typedef struct IPMISensor { >>>>> uint8_t status; >>>>> uint8_t reading; >>>>> @@ -208,6 +218,7 @@ struct IPMIBmcSim { >>>>> >>>>> IPMISel sel; >>>>> IPMISdr sdr; >>>>> + IPMIFru fru; >>>>> IPMISensor sensors[MAX_SENSORS]; >>>>> char *sdr_filename; >>>>> >>>>> @@ -1314,6 +1325,103 @@ static void get_sel_info(IPMIBmcSim *ibs, >>>>> IPMI_ADD_RSP_DATA((ibs->sel.overflow << 7) | 0x02); >>>>> } >>>>> >>>>> +static void get_fru_area_info(IPMIBmcSim *ibs, >>>>> + uint8_t *cmd, unsigned int cmd_len, >>>>> + uint8_t *rsp, unsigned int *rsp_len, >>>>> + unsigned int max_rsp_len) >>>>> +{ >>>>> + uint8_t fruid; >>>>> + uint16_t fru_entry_size; >>>>> + >>>>> + IPMI_CHECK_CMD_LEN(3); >>>> >>>> Hi, >>>> >>>> This is little awkward for me. The cmd_len and rsp >>>> parameters of the macro are implied. >>> >>> hmm, I am not sure what you mean. Are you concerned by that fact >>> we could overflow rsp and cmd ? >> >> Not really, no. Something more simple: >> >> IPMI_CHECK_CMD_LEN(3) should be actually IPMI_CHECK_CMD_LEN(3, cmd_len, rsp). >> What bothers me is that both cmd_len and rsp are implied by the macro. > > I would tend to agree. The hidden stuff in these macros was really a bad idea in > hindsight. IPMI_CHECK_CMD_LEN() could be replaced by an array of constants. I like the way IPMI_ADD_RSP_DATA() pushes bytes in the response buffer, it makes the code quite readable from the spec perspective. Maybe we could replace rsp and rsp_len with a simple stack-like struct and use one helper to push response bytes. It should not be too complex to cleanup. I will add a proposal in the next round. Thanks, C. > -corey > >> >> In other words, we don't know what parameters IPMI_CHECK_CMD_LEN really has. >> "3" is for sure not enough, so we need to guess or look a the macro definition >> to see what it uses. >> >> But again, maybe is only me. >> >> Thanks, >> Marcel >> >> >>> >>> I guess we could probably move the IPMI_CHECK_CMD_LEN in the caller >>> of these commands and use an array of expected argument lengths. >>> For rsp, IPMI_ADD_RSP_DATA needs to be used to check that we are not >>> exceeding max_rsp_len >>> >>>> Am I the only one this bothers? >>>> >>>>> + >>>>> + fruid = cmd[2]; >>>>> + >>>>> + if (fruid >= ibs->fru.nentries) { >>>>> + rsp[2] = IPMI_CC_INVALID_DATA_FIELD; >>>>> + return; >>>>> + } >>>>> + >>>>> + fru_entry_size = ibs->fru.size; >>>>> + >>>>> + IPMI_ADD_RSP_DATA(fru_entry_size & 0xff); >>>>> + IPMI_ADD_RSP_DATA(fru_entry_size >> 8 & 0xff); >>>>> + IPMI_ADD_RSP_DATA(0x0); >>>> >>>> Same here. By the way, do you have some spec for the above or >>>> is an ad-hoc encoding of the fields? If yes, you could >>>> add a reference for the spec.(This is also for the other functions in this patch) >>> >>> Yes I will add the reference. >>> >>> Thanks, >>> >>> C. >>> >>> >>>> Thanks, >>>> Marcel >>>> >>>>> +} >>>>> + >>>>> +#define min(x, y) ((x) < (y) ? (x) : (y)) >>>>> +#define max(x, y) ((x) > (y) ? (x) : (y)) >>>>> + >>>>> +static void read_fru_data(IPMIBmcSim *ibs, >>>>> + uint8_t *cmd, unsigned int cmd_len, >>>>> + uint8_t *rsp, unsigned int *rsp_len, >>>>> + unsigned int max_rsp_len) >>>>> +{ >>>>> + uint8_t fruid; >>>>> + uint16_t offset; >>>>> + int i; >>>>> + uint8_t *fru_entry; >>>>> + unsigned int count; >>>>> + >>>>> + IPMI_CHECK_CMD_LEN(5); >>>>> + >>>>> + fruid = cmd[2]; >>>>> + offset = (cmd[3] | cmd[4] << 8); >>>>> + >>>>> + if (fruid >= ibs->fru.nentries) { >>>>> + rsp[2] = IPMI_CC_INVALID_DATA_FIELD; >>>>> + return; >>>>> + } >>>>> + >>>>> + if (offset >= ibs->fru.size - 1) { >>>>> + rsp[2] = IPMI_CC_INVALID_DATA_FIELD; >>>>> + return; >>>>> + } >>>>> + >>>>> + fru_entry = &ibs->fru.data[fruid * ibs->fru.size]; >>>>> + >>>>> + count = min(cmd[5], ibs->fru.size - offset); >>>>> + >>>>> + IPMI_ADD_RSP_DATA(count & 0xff); >>>>> + for (i = 0; i < count; i++) { >>>>> + IPMI_ADD_RSP_DATA(fru_entry[offset + i]); >>>>> + } >>>>> +} >>>>> + >>>>> +static void write_fru_data(IPMIBmcSim *ibs, >>>>> + uint8_t *cmd, unsigned int cmd_len, >>>>> + uint8_t *rsp, unsigned int *rsp_len, >>>>> + unsigned int max_rsp_len) >>>>> +{ >>>>> + uint8_t fruid; >>>>> + uint16_t offset; >>>>> + uint8_t *fru_entry; >>>>> + unsigned int count; >>>>> + >>>>> + IPMI_CHECK_CMD_LEN(5); >>>>> + >>>>> + fruid = cmd[2]; >>>>> + offset = (cmd[3] | cmd[4] << 8); >>>>> + >>>>> + if (fruid >= ibs->fru.nentries) { >>>>> + rsp[2] = IPMI_CC_INVALID_DATA_FIELD; >>>>> + return; >>>>> + } >>>>> + >>>>> + if (offset >= ibs->fru.size - 1) { >>>>> + rsp[2] = IPMI_CC_INVALID_DATA_FIELD; >>>>> + return; >>>>> + } >>>>> + >>>>> + fru_entry = &ibs->fru.data[fruid * ibs->fru.size]; >>>>> + >>>>> + count = min(cmd_len - 5, ibs->fru.size - offset); >>>>> + >>>>> + memcpy(fru_entry + offset, cmd + 5, count); >>>>> + >>>>> + IPMI_ADD_RSP_DATA(count & 0xff); >>>>> +} >>>>> + >>>>> static void reserve_sel(IPMIBmcSim *ibs, >>>>> uint8_t *cmd, unsigned int cmd_len, >>>>> uint8_t *rsp, unsigned int *rsp_len, >>>>> @@ -1667,6 +1775,9 @@ static const IPMINetfn app_netfn = { >>>>> }; >>>>> >>>>> static const IPMICmdHandler storage_cmds[] = { >>>>> + [IPMI_CMD_GET_FRU_AREA_INFO] = get_fru_area_info, >>>>> + [IPMI_CMD_READ_FRU_DATA] = read_fru_data, >>>>> + [IPMI_CMD_WRITE_FRU_DATA] = write_fru_data, >>>>> [IPMI_CMD_GET_SDR_REP_INFO] = get_sdr_rep_info, >>>>> [IPMI_CMD_RESERVE_SDR_REP] = reserve_sdr_rep, >>>>> [IPMI_CMD_GET_SDR] = get_sdr, >>>>> @@ -1766,6 +1877,31 @@ static const VMStateDescription vmstate_ipmi_sim = { >>>>> } >>>>> }; >>>>> >>>>> +static void ipmi_fru_init(IPMIFru *fru) >>>>> +{ >>>>> + int fsize; >>>>> + int size = 0; >>>>> + >>>>> + fsize = get_image_size(fru->filename); >>>>> + if (fsize > 0) { >>>>> + size = QEMU_ALIGN_UP(fsize, fru->size); >>>>> + fru->data = g_malloc0(size); >>>>> + if (load_image_size(fru->filename, fru->data, fsize) != fsize) { >>>>> + error_report("Could not load file '%s'", fru->filename); >>>>> + g_free(fru->data); >>>>> + fru->data = NULL; >>>>> + } >>>>> + } >>>>> + >>>>> + if (!fru->data) { >>>>> + /* give one default FRU */ >>>>> + size = fru->size; >>>>> + fru->data = g_malloc0(size); >>>>> + } >>>>> + >>>>> + fru->nentries = size / fru->size; >>>>> +} >>>>> + >>>>> static void ipmi_sim_realize(DeviceState *dev, Error **errp) >>>> >>>> >>>>> { >>>>> IPMIBmc *b = IPMI_BMC(dev); >>>>> @@ -1788,6 +1924,8 @@ static void ipmi_sim_realize(DeviceState *dev, Error **errp) >>>>> >>>>> ipmi_sdr_init(ibs); >>>>> >>>>> + ipmi_fru_init(&ibs->fru); >>>>> + >>>>> ibs->acpi_power_state[0] = 0; >>>>> ibs->acpi_power_state[1] = 0; >>>>> >>>>> @@ -1806,6 +1944,8 @@ static void ipmi_sim_realize(DeviceState *dev, Error **errp) >>>>> } >>>>> >>>>> static Property ipmi_sim_properties[] = { >>>>> + DEFINE_PROP_UINT16("frusize", IPMIBmcSim, fru.size, 1024), >>>>> + DEFINE_PROP_STRING("frufile", IPMIBmcSim, fru.filename), >>>>> DEFINE_PROP_STRING("sdr", IPMIBmcSim, sdr_filename), >>>>> DEFINE_PROP_END_OF_LIST(), >>>>> }; >>>>> >>>> >>> >> >