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

[Corey Minyard <cminyard@mvista.com>]

On Thu, Nov 21, 2019 at 05:23:40PM +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
> 
> 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 <clg@kaod.org>

Acked-by: Corey Minyard <cminyard@mvista.com>

> ---
> 
>  Changes since v1:
> 
>  - renamed ipmi_bt_init() to pnv_ipmi_bt_init()
> 
>  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..f0adb06c8d65 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 pnv_ipmi_bt_init(ISABus *bus, IPMIBmc *bmc, uint32_t irq)
> +{
> +    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 */
> +    pnv_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
>