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

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

[Cédric Le Goater]

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>
---
 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)
+{
+    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

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

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 5937 bytes --]

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
> 
> 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>

This doesn't apply to ppc-for-5.0 for me.  I'm not sure which change
in there it's conflicting with, but there seems to be something.

> ---
>  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)
> +{
> +    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);
>  }

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[Cédric Le Goater]

On 19/11/2019 01:52, David Gibson 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
>>
>> 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>
> 
> This doesn't apply to ppc-for-5.0 for me.  I'm not sure which change
> in there it's conflicting with, but there seems to be something.

Sorry I should have been more precise.


This is because we need an IPMI patch to be merged first in 
Corey's tree :

  ipmi: Add support to customize OEM functions
  http://patchwork.ozlabs.org/patch/1185187/

and another one merged in the PPC tree: 

  ppc/pnv: Add HIOMAP commands
  http://patchwork.ozlabs.org/patch/1185185/


David, if Corey agrees, I think it would be simpler if you took 
them all. 

Thanks,

C. 

> 
>> ---
>>  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)
>> +{
>> +    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);
>>  }
> 

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

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 2461 bytes --]

On Tue, Nov 19, 2019 at 07:49:33AM +0100, Cédric Le Goater wrote:
> On 19/11/2019 01:52, David Gibson 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
> >>
> >> 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>
> > 
> > This doesn't apply to ppc-for-5.0 for me.  I'm not sure which change
> > in there it's conflicting with, but there seems to be something.
> 
> Sorry I should have been more precise.
> 
> 
> This is because we need an IPMI patch to be merged first in 
> Corey's tree :
> 
>   ipmi: Add support to customize OEM functions
>   http://patchwork.ozlabs.org/patch/1185187/
> 
> and another one merged in the PPC tree: 
> 
>   ppc/pnv: Add HIOMAP commands
>   http://patchwork.ozlabs.org/patch/1185185/
> 
> 
> David, if Corey agrees, I think it would be simpler if you took 
> them all.

Ok.  Corey, could I get an Acked-by from you?

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[Corey Minyard]

On Wed, Nov 20, 2019 at 09:05:07AM +1100, David Gibson wrote:
> On Tue, Nov 19, 2019 at 07:49:33AM +0100, Cédric Le Goater wrote:
> > On 19/11/2019 01:52, David Gibson 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
> > >>
> > >> 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>
> > > 
> > > This doesn't apply to ppc-for-5.0 for me.  I'm not sure which change
> > > in there it's conflicting with, but there seems to be something.
> > 
> > Sorry I should have been more precise.
> > 
> > 
> > This is because we need an IPMI patch to be merged first in 
> > Corey's tree :
> > 
> >   ipmi: Add support to customize OEM functions
> >   http://patchwork.ozlabs.org/patch/1185187/
> > 
> > and another one merged in the PPC tree: 
> > 
> >   ppc/pnv: Add HIOMAP commands
> >   http://patchwork.ozlabs.org/patch/1185185/
> > 
> > 
> > David, if Corey agrees, I think it would be simpler if you took 
> > them all.
> 
> Ok.  Corey, could I get an Acked-by from you?

Sure

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

> 
> -- 
> David Gibson			| I'll have my music baroque, and my code
> david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
> 				| _way_ _around_!
> http://www.ozlabs.org/~dgibson

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

[Corey Minyard]

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 <clg@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
> 

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 <clg@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?

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
>>

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

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 3152 bytes --]

On Thu, Nov 21, 2019 at 09:35:07AM -0600, Corey Minyard wrote:
> On Wed, Nov 20, 2019 at 09:05:07AM +1100, David Gibson wrote:
> > On Tue, Nov 19, 2019 at 07:49:33AM +0100, Cédric Le Goater wrote:
> > > On 19/11/2019 01:52, David Gibson 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
> > > >>
> > > >> 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>
> > > > 
> > > > This doesn't apply to ppc-for-5.0 for me.  I'm not sure which change
> > > > in there it's conflicting with, but there seems to be something.
> > > 
> > > Sorry I should have been more precise.
> > > 
> > > 
> > > This is because we need an IPMI patch to be merged first in 
> > > Corey's tree :
> > > 
> > >   ipmi: Add support to customize OEM functions
> > >   http://patchwork.ozlabs.org/patch/1185187/
> > > 
> > > and another one merged in the PPC tree: 
> > > 
> > >   ppc/pnv: Add HIOMAP commands
> > >   http://patchwork.ozlabs.org/patch/1185185/
> > > 
> > > 
> > > David, if Corey agrees, I think it would be simpler if you took 
> > > them all.
> > 
> > Ok.  Corey, could I get an Acked-by from you?
> 
> Sure
> 
> Acked-by: Corey Minyard <cminyard@mvista.com>

Sorry, I wasn't clear.  It's the prerequisite "ipmi: Add support to
customize OEM functions" patch that I'm after an Acked-by for, rather
than thos one - I generally like to have that if I'm taking a patch
through my tree that isn't in the area I'm tagged as maintainer for.

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]