Re: [PATCH] ppc/pnv: Create BMC devices at machine init

[Cédric Le Goater]

On 21/11/2019 16:36, Corey Minyard wrote:
> On Mon, Nov 18, 2019 at 10:22:22AM +0100, Cédric Le Goater wrote:
>> The BMC of the OpenPOWER systems monitors the machine state using
>> sensors, controls the power and controls the access to the PNOR flash
>> device containing the firmware image required to boot the host.
>>
>> QEMU models the power cycle process, access to the sensors and access
>> to the PNOR device. But, for these features to be available, the QEMU
>> PowerNV machine needs two extras devices on the command line, an IPMI
>> BT device for communication and a BMC backend device:
>>
>>   -device ipmi-bmc-sim,id=bmc0 -device isa-ipmi-bt,bmc=bmc0,irq=10
> 
> With these changes, the above line will no longer be necessary, right?

Yes. 

I would have liked to find a way to introduce some kind of lazy binding
between the two devices and still be able to change the IPMI backend on 
the command line but it seemed too complex.

> Minor nit inline ...
> 
>>
>> The BMC properties are then defined accordingly in the device tree and
>> OPAL self adapts. If a BMC device and an IPMI BT device are not
>> available, OPAL does not try to communicate with the BMC in any
>> manner. This is not how real systems behave.
>>
>> To be closer to the default behavior, create an IPMI BMC simulator
>> device and an IPMI BT device at machine initialization time. We loose
>> the ability to define an external BMC device but there are benefits:
>>
>>   - a better match with real systems,
>>   - a better test coverage of the OPAL code,
>>   - system powerdown and reset commands that work,
>>   - a QEMU device tree compliant with the specifications (*).
>>
>> (*) Still needs a MBOX device.
>>
>> Signed-off-by: Cédric Le Goater <address@hidden>
>> ---
>>  include/hw/ppc/pnv.h |  2 +-
>>  hw/ppc/pnv.c         | 33 ++++++++++++++++-----------------
>>  hw/ppc/pnv_bmc.c     | 20 +++++++++++++++++---
>>  3 files changed, 34 insertions(+), 21 deletions(-)
>>
>> diff --git a/include/hw/ppc/pnv.h b/include/hw/ppc/pnv.h
>> index 07c56c05ad30..90f1343ed07c 100644
>> --- a/include/hw/ppc/pnv.h
>> +++ b/include/hw/ppc/pnv.h
>> @@ -198,7 +198,7 @@ static inline bool pnv_is_power9(PnvMachineState *pnv)
>>   */
>>  void pnv_dt_bmc_sensors(IPMIBmc *bmc, void *fdt);
>>  void pnv_bmc_powerdown(IPMIBmc *bmc);
>> -int pnv_bmc_hiomap(IPMIBmc *bmc);
>> +IPMIBmc *pnv_bmc_create(void);
>>  
>>  /*
>>   * POWER8 MMIO base addresses
>> diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
>> index c3ac0d6d5b4a..2117d879895c 100644
>> --- a/hw/ppc/pnv.c
>> +++ b/hw/ppc/pnv.c
>> @@ -551,27 +551,10 @@ static void pnv_powerdown_notify(Notifier *n, void 
>> *opaque)
>>  
>>  static void pnv_reset(MachineState *machine)
>>  {
>> -    PnvMachineState *pnv = PNV_MACHINE(machine);
>>      void *fdt;
>> -    Object *obj;
>>  
>>      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.
>> -     */
>> -    obj = object_resolve_path_type("", "ipmi-bmc-sim", NULL);
>> -    if (obj) {
>> -        pnv->bmc = IPMI_BMC(obj);
>> -        pnv_bmc_hiomap(pnv->bmc);
>> -    }
>> -
>>      fdt = pnv_dt_create(machine);
>>  
>>      /* Pack resulting tree */
>> @@ -629,6 +612,16 @@ static bool pnv_match_cpu(const char *default_type, 
>> const char *cpu_type)
>>      return ppc_default->pvr_match(ppc_default, ppc->pvr);
>>  }
>>  
>> +static void ipmi_bt_init(ISABus *bus, IPMIBmc *bmc, uint32_t irq)
> 
> All the other names have pnv_ in front of them, could you add that to
> this, too?

Sure. I will resend a v2 with this change and your acked-by.

C. 


> 
> -corey
> 
>> +{
>> +    Object *obj;
>> +
>> +    obj = OBJECT(isa_create(bus, "isa-ipmi-bt"));
>> +    object_property_set_link(obj, OBJECT(bmc), "bmc", &error_fatal);
>> +    object_property_set_int(obj, irq, "irq", &error_fatal);
>> +    object_property_set_bool(obj, true, "realized", &error_fatal);
>> +}
>> +
>>  static void pnv_init(MachineState *machine)
>>  {
>>      PnvMachineState *pnv = PNV_MACHINE(machine);
>> @@ -751,6 +744,9 @@ static void pnv_init(MachineState *machine)
>>      }
>>      g_free(chip_typename);
>>  
>> +    /* Create the machine BMC simulator */
>> +    pnv->bmc = pnv_bmc_create();
>> +
>>      /* Instantiate ISA bus on chip 0 */
>>      pnv->isa_bus = pnv_isa_create(pnv->chips[0], &error_fatal);
>>  
>> @@ -760,6 +756,9 @@ static void pnv_init(MachineState *machine)
>>      /* Create an RTC ISA device too */
>>      mc146818_rtc_init(pnv->isa_bus, 2000, NULL);
>>  
>> +    /* Create the IPMI BT device for communication with the BMC */
>> +    ipmi_bt_init(pnv->isa_bus, pnv->bmc, 10);
>> +
>>      /*
>>       * OpenPOWER systems use a IPMI SEL Event message to notify the
>>       * host to powerdown
>> diff --git a/hw/ppc/pnv_bmc.c b/hw/ppc/pnv_bmc.c
>> index aa5c89586c63..07fa1e1c7e45 100644
>> --- a/hw/ppc/pnv_bmc.c
>> +++ b/hw/ppc/pnv_bmc.c
>> @@ -17,6 +17,8 @@
>>   */
>>  
>>  #include "qemu/osdep.h"
>> +#include "qemu-common.h"
>> +#include "qapi/error.h"
>>  #include "target/ppc/cpu.h"
>>  #include "qemu/log.h"
>>  #include "hw/ipmi/ipmi.h"
>> @@ -211,8 +213,20 @@ static const IPMINetfn hiomap_netfn = {
>>      .cmd_handlers = hiomap_cmds
>>  };
>>  
>> -int pnv_bmc_hiomap(IPMIBmc *bmc)
>> +/*
>> + * Instantiate the machine BMC. PowerNV uses the QEMU internal
>> + * simulator but it could also be external.
>> + */
>> +IPMIBmc *pnv_bmc_create(void)
>>  {
>> -    return ipmi_sim_register_netfn(IPMI_BMC_SIMULATOR(bmc),
>> -                                   IPMI_NETFN_OEM, &hiomap_netfn);
>> +    Object *obj;
>> +
>> +    obj = object_new(TYPE_IPMI_BMC_SIMULATOR);
>> +    object_property_set_bool(obj, true, "realized", &error_fatal);
>> +
>> +    /* Install the HIOMAP protocol handlers to access the PNOR */
>> +    ipmi_sim_register_netfn(IPMI_BMC_SIMULATOR(obj), IPMI_NETFN_OEM,
>> +                            &hiomap_netfn);
>> +
>> +    return IPMI_BMC(obj);
>>  }
>> -- 
>> 2.21.0
>>