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?
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 <c...@kaod.org>
> ---
> 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?
-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
>
