On 01/22/2016 11:56 AM, Greg Kurz wrote:
> On Thu, 21 Jan 2016 18:18:50 +0100
> Cédric Le Goater <[email protected]> wrote:
>> The IPMI BMC simulator populates the SDR table with a set of initial
>> SDRs. The length of each SDR is taken from the record itself (byte 4)
>> which does not include the size of the header. But, the full length
>> (header + data) is required by the sdr_add_entry() routine.
>>
>> Signed-off-by: Cédric Le Goater <[email protected]>
>
> The patch is good but IMHO it should come before patch 4 because this is
> bugfix
> that could be applied right away, while patch 4 is code cleanup that may need
> some more discussion.
OK. I am fine with that. It should be the patch from v1.
Thanks,
C.
>> ---
>> hw/ipmi/ipmi_bmc_sim.c | 18 +++++++++---------
>> include/hw/ipmi/ipmi.h | 1 +
>> 2 files changed, 10 insertions(+), 9 deletions(-)
>>
>> diff --git a/hw/ipmi/ipmi_bmc_sim.c b/hw/ipmi/ipmi_bmc_sim.c
>> index 31f990199154..803c7e5130c0 100644
>> --- a/hw/ipmi/ipmi_bmc_sim.c
>> +++ b/hw/ipmi/ipmi_bmc_sim.c
>> @@ -327,11 +327,11 @@ static int sdr_add_entry(IPMIBmcSim *ibs, const
>> uint8_t *entry,
>> struct ipmi_sdr_header *sdrh =
>> (struct ipmi_sdr_header *) &ibs->sdr.sdr[ibs->sdr.next_free];
>>
>> - if ((len < 5) || (len > 255)) {
>> + if ((len < IPMI_SDR_HEADER_SIZE) || (len > 255)) {
>> return 1;
>> }
>>
>> - if (sdrh_entry->rec_length != len - 5) {
>> + if (ipmi_sdr_length(sdrh_entry) != len) {
>> return 1;
>> }
>>
>> @@ -364,7 +364,7 @@ static int sdr_find_entry(IPMISdr *sdr, uint16_t recid,
>> struct ipmi_sdr_header *sdrh =
>> (struct ipmi_sdr_header *) &sdr->sdr[pos];
>> uint16_t trec = ipmi_sdr_recid(sdrh);
>> - unsigned int nextpos = pos + sdrh->rec_length;
>> + unsigned int nextpos = pos + ipmi_sdr_length(sdrh);
>>
>> if (trec == recid) {
>> if (nextrec) {
>> @@ -1179,7 +1179,7 @@ static void get_sdr(IPMIBmcSim *ibs,
>>
>> sdrh = (struct ipmi_sdr_header *) &ibs->sdr.sdr[pos];
>>
>> - if (cmd[6] > sdrh->rec_length) {
>> + if (cmd[6] > ipmi_sdr_length(sdrh)) {
>> rsp[2] = IPMI_CC_PARM_OUT_OF_RANGE;
>> return;
>> }
>> @@ -1188,7 +1188,7 @@ static void get_sdr(IPMIBmcSim *ibs,
>> IPMI_ADD_RSP_DATA((nextrec >> 8) & 0xff);
>>
>> if (cmd[7] == 0xff) {
>> - cmd[7] = sdrh->rec_length - cmd[6];
>> + cmd[7] = ipmi_sdr_length(sdrh) - cmd[6];
>> }
>>
>> if ((cmd[7] + *rsp_len) > max_rsp_len) {
>> @@ -1659,22 +1659,22 @@ static void ipmi_sim_init(Object *obj)
>> for (i = 0;;) {
>> struct ipmi_sdr_header *sdrh;
>> int len;
>> - if ((i + 5) > sizeof(init_sdrs)) {
>> + if ((i + IPMI_SDR_HEADER_SIZE) > sizeof(init_sdrs)) {
>> error_report("Problem with recid 0x%4.4x", i);
>> return;
>> }
>> sdrh = (struct ipmi_sdr_header *) &init_sdrs[i];
>> - len = sdrh->rec_length;
>> + len = ipmi_sdr_length(sdrh);
>> recid = ipmi_sdr_recid(sdrh);
>> if (recid == 0xffff) {
>> break;
>> }
>> - if ((i + len + 5) > sizeof(init_sdrs)) {
>> + if ((i + len) > sizeof(init_sdrs)) {
>> error_report("Problem with recid 0x%4.4x", i);
>> return;
>> }
>> sdr_add_entry(ibs, init_sdrs + i, len, NULL);
>> - i += len + 5;
>> + i += len;
>> }
>>
>> ipmi_init_sensors_from_sdrs(ibs);
>> diff --git a/include/hw/ipmi/ipmi.h b/include/hw/ipmi/ipmi.h
>> index 7e142e241dcb..74a2b5af9613 100644
>> --- a/include/hw/ipmi/ipmi.h
>> +++ b/include/hw/ipmi/ipmi.h
>> @@ -219,6 +219,7 @@ struct ipmi_sdr_header {
>> #define IPMI_SDR_HEADER_SIZE sizeof(struct ipmi_sdr_header)
>>
>> #define ipmi_sdr_recid(sdr) ((sdr)->rec_id[0] | ((sdr)->rec_id[1] << 8))
>> +#define ipmi_sdr_length(sdr) ((sdr)->rec_length + IPMI_SDR_HEADER_SIZE)
>>
>> /*
>> * 43.2 SDR Type 02h. Compact Sensor Record
>
