From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:52054) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aVO4u-0006od-6R for qemu-devel@nongnu.org; Mon, 15 Feb 2016 13:40:57 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1aVO4q-0002PT-K8 for qemu-devel@nongnu.org; Mon, 15 Feb 2016 13:40:56 -0500 Received: from mx1.redhat.com ([209.132.183.28]:34902) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aVO4q-0002OZ-4w for qemu-devel@nongnu.org; Mon, 15 Feb 2016 13:40:52 -0500 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> From: Marcel Apfelbaum Message-ID: <56C21BAE.2040104@redhat.com> Date: Mon, 15 Feb 2016 20:40:46 +0200 MIME-Version: 1.0 In-Reply-To: <56C2081E.6080104@fr.ibm.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: quoted-printable 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: =?UTF-8?Q?C=c3=a9dric_Le_Goater?= , Corey Minyard Cc: "Michael S. Tsirkin" , qemu-devel@nongnu.org, Greg Kurz On 02/15/2016 07:17 PM, C=C3=A9dric Le Goater wrote: > On 02/14/2016 10:25 AM, Marcel Apfelbaum wrote: >> On 02/09/2016 02:13 PM, C=C3=A9dric 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=C3=A9dric 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, r= sp). What bothers me is that both cmd_len and rsp are implied by the macro. In other words, we don't know what parameters IPMI_CHECK_CMD_LEN really h= as. "3" is for sure not enough, so we need to guess or look a the macro defin= ition 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 =3D cmd[2]; >>> + >>> + if (fruid >=3D ibs->fru.nentries) { >>> + rsp[2] =3D IPMI_CC_INVALID_DATA_FIELD; >>> + return; >>> + } >>> + >>> + fru_entry_size =3D 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 =3D cmd[2]; >>> + offset =3D (cmd[3] | cmd[4] << 8); >>> + >>> + if (fruid >=3D ibs->fru.nentries) { >>> + rsp[2] =3D IPMI_CC_INVALID_DATA_FIELD; >>> + return; >>> + } >>> + >>> + if (offset >=3D ibs->fru.size - 1) { >>> + rsp[2] =3D IPMI_CC_INVALID_DATA_FIELD; >>> + return; >>> + } >>> + >>> + fru_entry =3D &ibs->fru.data[fruid * ibs->fru.size]; >>> + >>> + count =3D min(cmd[5], ibs->fru.size - offset); >>> + >>> + IPMI_ADD_RSP_DATA(count & 0xff); >>> + for (i =3D 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 =3D cmd[2]; >>> + offset =3D (cmd[3] | cmd[4] << 8); >>> + >>> + if (fruid >=3D ibs->fru.nentries) { >>> + rsp[2] =3D IPMI_CC_INVALID_DATA_FIELD; >>> + return; >>> + } >>> + >>> + if (offset >=3D ibs->fru.size - 1) { >>> + rsp[2] =3D IPMI_CC_INVALID_DATA_FIELD; >>> + return; >>> + } >>> + >>> + fru_entry =3D &ibs->fru.data[fruid * ibs->fru.size]; >>> + >>> + count =3D 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 =3D { >>> }; >>> >>> static const IPMICmdHandler storage_cmds[] =3D { >>> + [IPMI_CMD_GET_FRU_AREA_INFO] =3D get_fru_area_info, >>> + [IPMI_CMD_READ_FRU_DATA] =3D read_fru_data, >>> + [IPMI_CMD_WRITE_FRU_DATA] =3D write_fru_data, >>> [IPMI_CMD_GET_SDR_REP_INFO] =3D get_sdr_rep_info, >>> [IPMI_CMD_RESERVE_SDR_REP] =3D reserve_sdr_rep, >>> [IPMI_CMD_GET_SDR] =3D get_sdr, >>> @@ -1766,6 +1877,31 @@ static const VMStateDescription vmstate_ipmi_s= im =3D { >>> } >>> }; >>> >>> +static void ipmi_fru_init(IPMIFru *fru) >>> +{ >>> + int fsize; >>> + int size =3D 0; >>> + >>> + fsize =3D get_image_size(fru->filename); >>> + if (fsize > 0) { >>> + size =3D QEMU_ALIGN_UP(fsize, fru->size); >>> + fru->data =3D g_malloc0(size); >>> + if (load_image_size(fru->filename, fru->data, fsize) !=3D fs= ize) { >>> + error_report("Could not load file '%s'", fru->filename); >>> + g_free(fru->data); >>> + fru->data =3D NULL; >>> + } >>> + } >>> + >>> + if (!fru->data) { >>> + /* give one default FRU */ >>> + size =3D fru->size; >>> + fru->data =3D g_malloc0(size); >>> + } >>> + >>> + fru->nentries =3D size / fru->size; >>> +} >>> + >>> static void ipmi_sim_realize(DeviceState *dev, Error **errp) >> >> >>> { >>> IPMIBmc *b =3D 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] =3D 0; >>> ibs->acpi_power_state[1] =3D 0; >>> >>> @@ -1806,6 +1944,8 @@ static void ipmi_sim_realize(DeviceState *dev, = Error **errp) >>> } >>> >>> static Property ipmi_sim_properties[] =3D { >>> + 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(), >>> }; >>> >> >