From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:52386) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aVWTJ-0006F2-QC for qemu-devel@nongnu.org; Mon, 15 Feb 2016 22:38:43 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1aVWTI-0000SK-9S for qemu-devel@nongnu.org; Mon, 15 Feb 2016 22:38:41 -0500 Received: from mail-ob0-x22a.google.com ([2607:f8b0:4003:c01::22a]:34424) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aVWTI-0000RM-07 for qemu-devel@nongnu.org; Mon, 15 Feb 2016 22:38:40 -0500 Received: by mail-ob0-x22a.google.com with SMTP id wb13so240685086obb.1 for ; Mon, 15 Feb 2016 19:38:39 -0800 (PST) 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> From: Corey Minyard Message-ID: <56C299BD.4020004@mvista.com> Date: Mon, 15 Feb 2016 21:38:37 -0600 MIME-Version: 1.0 In-Reply-To: <56C21BAE.2040104@redhat.com> Content-Type: text/plain; charset=utf-8; format=flowed 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: Marcel Apfelbaum , =?UTF-8?Q?C=c3=a9dric_Le_Goater?= Cc: "Michael S. Tsirkin" , qemu-devel@nongnu.org, Greg Kurz 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. -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(), >>>> }; >>>> >>> >> >