Re: [Qemu-devel] [PATCH 15/21] ppc/pnv: add initial IPMI sensors for the BMC simulator

[Cédric Le Goater]

On 04/10/2017 07:31 AM, David Gibson wrote:
> On Wed, Apr 05, 2017 at 02:41:40PM +0200, Cédric Le Goater wrote:
>> Skiboot, the firmware for the PowerNV platform, expects the BMC to
>> provide some specific IPMI sensors. These sensors are exposed in the
>> device tree and their values are updated by the firmware at boot time.
>>
>> Sensors of interest are :
>>
>>      "FW Boot Progress"
>>      "Boot Count"
>>
>> As such a device is defined on the command line, we can only detect
>> its presence at reset time.
>>
>> Signed-off-by: Cédric Le Goater <address@hidden>
>> ---
>>  hw/ppc/Makefile.objs |  2 +-
>>  hw/ppc/pnv.c         | 17 +++++++++++
>>  hw/ppc/pnv_bmc.c     | 81 
>> ++++++++++++++++++++++++++++++++++++++++++++++++++++
>>  include/hw/ppc/pnv.h |  7 +++++
>>  4 files changed, 106 insertions(+), 1 deletion(-)
>>  create mode 100644 hw/ppc/pnv_bmc.c
>>
>> diff --git a/hw/ppc/Makefile.objs b/hw/ppc/Makefile.objs
>> index ef67ea820158..7efc68674819 100644
>> --- a/hw/ppc/Makefile.objs
>> +++ b/hw/ppc/Makefile.objs
>> @@ -6,7 +6,7 @@ obj-$(CONFIG_PSERIES) += spapr_hcall.o spapr_iommu.o 
>> spapr_rtas.o
>>  obj-$(CONFIG_PSERIES) += spapr_pci.o spapr_rtc.o spapr_drc.o spapr_rng.o
>>  obj-$(CONFIG_PSERIES) += spapr_cpu_core.o spapr_ovec.o
>>  # IBM PowerNV
>> -obj-$(CONFIG_POWERNV) += pnv.o pnv_xscom.o pnv_core.o pnv_lpc.o pnv_psi.o 
>> pnv_occ.o
>> +obj-$(CONFIG_POWERNV) += pnv.o pnv_xscom.o pnv_core.o pnv_lpc.o pnv_psi.o 
>> pnv_occ.o pnv_bmc.o
>>  ifeq ($(CONFIG_PCI)$(CONFIG_PSERIES)$(CONFIG_LINUX), yyy)
>>  obj-y += spapr_pci_vfio.o
>>  endif
>> diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
>> index dfa1cf849b35..c7caecad0aa6 100644
>> --- a/hw/ppc/pnv.c
>> +++ b/hw/ppc/pnv.c
>> @@ -35,6 +35,7 @@
>>  #include "qapi/visitor.h"
>>  #include "monitor/monitor.h"
>>  #include "hw/intc/intc.h"
>> +#include "hw/ipmi/ipmi.h"
>>  
>>  #include "hw/ppc/xics.h"
>>  #include "hw/ppc/pnv_xscom.h"
>> @@ -458,16 +459,32 @@ static void *powernv_create_fdt(MachineState *machine)
>>      }
>>  
>>      powernv_populate_isa(pnv->isa_bus, fdt);
>> +
>> +    if (pnv->bmc) {
>> +        pnv_bmc_populate_sensors(pnv->bmc, fdt);
>> +    }
>> +
>>      return fdt;
>>  }
>>  
>>  static void ppc_powernv_reset(void)
>>  {
>>      MachineState *machine = MACHINE(qdev_get_machine());
>> +    PnvMachineState *pnv = POWERNV_MACHINE(machine);
>>      void *fdt;
>>  
>>      qemu_devices_reset();
>>  
>> +    /* OpenPOWER systems have a BMC, which can be defined on the
>> +     * command line with:
>> +     *
>> +     *   -device ipmi-bmc-sim,id=bmc0
>> +     *
>> +     * This is the internal simulator but it could also be an external
>> +     * BMC.
>> +     */
>> +    pnv->bmc = object_resolve_path_type("", TYPE_IPMI_BMC, NULL);
> 
> I'd suggest typing the pointer and doing the cast/test here, rather
> than in the functions that need to manipulate it.

OK.
> 
>>      fdt = powernv_create_fdt(machine);
>>  
>>      /* Pack resulting tree */
>> diff --git a/hw/ppc/pnv_bmc.c b/hw/ppc/pnv_bmc.c
>> new file mode 100644
>> index 000000000000..2998530dc4c0
>> --- /dev/null
>> +++ b/hw/ppc/pnv_bmc.c
>> @@ -0,0 +1,81 @@
>> +/*
>> + * QEMU PowerNV, BMC related functions
>> + *
>> + * Copyright (c) 2016-2017, IBM Corporation.
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License, version 2, as
>> + * published by the Free Software Foundation.
>> + *
>> + * This program is distributed in the hope that it will be useful,
>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>> + * GNU General Public License for more details.
>> + *
>> + * You should have received a copy of the GNU General Public License
>> + * along with this program; if not, see <http://www.gnu.org/licenses/>.
>> + */
>> +
>> +#include "qemu/osdep.h"
>> +#include "hw/hw.h"
>> +#include "sysemu/sysemu.h"
>> +#include "target/ppc/cpu.h"
>> +#include "qapi/error.h"
>> +#include "qemu/log.h"
>> +#include "hw/ipmi/ipmi.h"
>> +#include "hw/ppc/fdt.h"
>> +
>> +#include "hw/ppc/pnv.h"
>> +
>> +#include <libfdt.h>
>> +
>> +/* TODO: include definition in ipmi.h */
>> +#define IPMI_SDR_FULL_TYPE 1
>> +
>> +void pnv_bmc_populate_sensors(Object *bmc, void *fdt)
>> +{
>> +    int offset;
>> +    int i;
>> +    const struct ipmi_sdr_compact *sdr;
>> +    uint16_t nextrec;
>> +
>> +    offset = fdt_add_subnode(fdt, 0, "/bmc");
>> +    _FDT(offset);
>> +
>> +    _FDT((fdt_setprop_string(fdt, offset, "name", "bmc")));
>> +    _FDT((fdt_setprop_cell(fdt, offset, "#address-cells", 0x1)));
>> +    _FDT((fdt_setprop_cell(fdt, offset, "#size-cells", 0x0)));
>> +
>> +    offset = fdt_add_subnode(fdt, offset, "sensors");
>> +    _FDT(offset);
>> +
>> +    _FDT((fdt_setprop_cell(fdt, offset, "#address-cells", 0x1)));
>> +    _FDT((fdt_setprop_cell(fdt, offset, "#size-cells", 0x0)));
>> +
>> +    for (i = 0; !ipmi_bmc_sdr_find(IPMI_BMC(bmc), i, &sdr, &nextrec); i++) {
>> +        int off;
>> +        char *name;
>> +
>> +        if (sdr->header.rec_type != IPMI_SDR_COMPACT_TYPE ||
>> +            sdr->header.rec_type != IPMI_SDR_FULL_TYPE) {
> 
> Um.. the condition above is always true.  It's only false if rec_type
> is equal to both FULL_TYPE and COMPACT_TYPE at the same time...

yes. that is a last minute bug ... It should be using "&&".

Thanks,

C.

> 
>> +            continue;
>> +        }
>> +
>> +        name = g_strdup_printf("address@hidden", sdr->sensor_owner_number);
>> +        off = fdt_add_subnode(fdt, offset, name);
>> +        _FDT(off);
>> +        g_free(name);
>> +
>> +        _FDT((fdt_setprop_cell(fdt, off, "reg", sdr->sensor_owner_number)));
>> +        _FDT((fdt_setprop_string(fdt, off, "name", "sensor")));
>> +        _FDT((fdt_setprop_string(fdt, off, "compatible", 
>> "ibm,ipmi-sensor")));
>> +        _FDT((fdt_setprop_cell(fdt, off, "ipmi-sensor-reading-type",
>> +                               sdr->reading_type)));
>> +        _FDT((fdt_setprop_cell(fdt, off, "ipmi-entity-id",
>> +                               sdr->entity_id)));
>> +        _FDT((fdt_setprop_cell(fdt, off, "ipmi-entity-instance",
>> +                               sdr->entity_instance)));
>> +        _FDT((fdt_setprop_cell(fdt, off, "ipmi-sensor-type",
>> +                               sdr->sensor_type)));
>> +    }
>> +}
>> diff --git a/include/hw/ppc/pnv.h b/include/hw/ppc/pnv.h
>> index b45a0d91c813..050763f6cf54 100644
>> --- a/include/hw/ppc/pnv.h
>> +++ b/include/hw/ppc/pnv.h
>> @@ -130,12 +130,19 @@ typedef struct PnvMachineState {
>>  
>>      ISABus       *isa_bus;
>>      uint32_t     cpld_irqstate;
>> +
>> +    Object       *bmc;
>>  } PnvMachineState;
>>  
>>  #define PNV_FDT_ADDR          0x01000000
>>  #define PNV_TIMEBASE_FREQ     512000000ULL
>>  
>>  /*
>> + * BMC helpers
>> + */
>> +void pnv_bmc_populate_sensors(Object *objbmc, void *fdt);
>> +
>> +/*
>>   * POWER8 MMIO base addresses
>>   */
>>  #define PNV_XSCOM_SIZE        0x800000000ull
>