Re: [PATCH] hw/ipmi: Allow multiple BMC instances

[Philippe Mathieu-Daudé]

On 4/4/25 15:04, Corey Minyard wrote:

On Fri, Apr 04, 2025 at 02:41:46PM +0200, Philippe Mathieu-Daudé wrote:

Hi Corey,

On 4/4/25 02:57, Corey Minyard wrote:

Allow a system to have multiple BMC connections to the same BMC and
multiple different BMCs.  This can happen on real systems, and is
useful for testing the IPMI driver on Linux.

Signed-off-by: Corey Minyard <co...@minyard.net>
---
I'm working on a fairly extensive test suite for IPMI, the Linux
driver and qemu, and this is necessary for some driver tests.

   hw/ipmi/ipmi.c            | 1 +
   hw/ipmi/ipmi_bmc_extern.c | 5 +++--
   hw/ipmi/ipmi_bmc_sim.c    | 2 +-
   include/hw/ipmi/ipmi.h    | 1 +
   qemu-options.hx           | 9 ++++++++-
   5 files changed, 14 insertions(+), 4 deletions(-)

diff --git a/hw/ipmi/ipmi.c b/hw/ipmi/ipmi.c
index fdeaa5269f..ffd972f78b 100644
--- a/hw/ipmi/ipmi.c
+++ b/hw/ipmi/ipmi.c
@@ -110,6 +110,7 @@ void ipmi_bmc_find_and_link(Object *obj, Object **bmc)
   static const Property ipmi_bmc_properties[] = {
       DEFINE_PROP_UINT8("slave_addr",  IPMIBmc, slave_addr, 0x20),
+    DEFINE_PROP_UINT8("instance",    IPMIBmc, instance, 0),

Can we use "id" instead of "instance"? The latter confuses me, but
maybe a matter of taste.

"id" means "identifier", not "instance".  The error log mentions
"instance", that that is what is passed to vmstate_register().

Note, vmstate_register() is a legacy API, with only 20 cases left to
update. See commit 6caf1571a97 ("include/migration: mark
vmstate_register() as a legacy function"):

    /**
     * vmstate_register() - legacy function to register state
     * serialisation description
     *
     * New code shouldn't be using this function as QOM-ified devices
     * have dc->vmsd to store the serialisation description.
     *
     * Returns: 0 on success, -1 on failure
     */

Maybe it's better to just have a global variable that increments and not
pass it in?  That way it would work automatically.

Global variables often hide suble problems. We have a list of some used
in qdev / qbus / pci that we plan to remove, because it makes command
line not reproducible.

See this thread:
https://lore.kernel.org/qemu-devel/87czq0l2mn....@dusky.pond.sub.org/

-corey

Preferably s/instance/id/:
Reviewed-by: Philippe Mathieu-Daudé <phi...@linaro.org>

   };
   static void bmc_class_init(ObjectClass *oc, void *data)
diff --git a/hw/ipmi/ipmi_bmc_extern.c b/hw/ipmi/ipmi_bmc_extern.c
index d015500254..11c28d03ab 100644
--- a/hw/ipmi/ipmi_bmc_extern.c
+++ b/hw/ipmi/ipmi_bmc_extern.c
@@ -488,7 +488,8 @@ static const VMStateDescription vmstate_ipmi_bmc_extern = {
   static void ipmi_bmc_extern_realize(DeviceState *dev, Error **errp)
   {
-    IPMIBmcExtern *ibe = IPMI_BMC_EXTERN(dev);
+    IPMIBmc *b = IPMI_BMC(dev);
+    IPMIBmcExtern *ibe = IPMI_BMC_EXTERN(b);
       if (!qemu_chr_fe_backend_connected(&ibe->chr)) {
           error_setg(errp, "IPMI external bmc requires chardev attribute");
@@ -498,7 +499,7 @@ static void ipmi_bmc_extern_realize(DeviceState *dev, Error 
**errp)
       qemu_chr_fe_set_handlers(&ibe->chr, can_receive, receive,
                                chr_event, NULL, ibe, NULL, true);
-    vmstate_register(NULL, 0, &vmstate_ipmi_bmc_extern, ibe);
+    vmstate_register(NULL, b->instance, &vmstate_ipmi_bmc_extern, ibe);
   }
   static void ipmi_bmc_extern_init(Object *obj)
diff --git a/hw/ipmi/ipmi_bmc_sim.c b/hw/ipmi/ipmi_bmc_sim.c
index 6157ac7120..c1b39dbdc5 100644
--- a/hw/ipmi/ipmi_bmc_sim.c
+++ b/hw/ipmi/ipmi_bmc_sim.c
@@ -2188,7 +2188,7 @@ static void ipmi_sim_realize(DeviceState *dev, Error 
**errp)
       ibs->timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, ipmi_timeout, ibs);
-    vmstate_register(NULL, 0, &vmstate_ipmi_sim, ibs);
+    vmstate_register(NULL, b->instance, &vmstate_ipmi_sim, ibs);
   }
   static const Property ipmi_sim_properties[] = {
diff --git a/include/hw/ipmi/ipmi.h b/include/hw/ipmi/ipmi.h
index 77a7213ed9..4436d70842 100644
--- a/include/hw/ipmi/ipmi.h
+++ b/include/hw/ipmi/ipmi.h
@@ -183,6 +183,7 @@ struct IPMIBmc {
       DeviceState parent;
       uint8_t slave_addr;
+    uint8_t instance;
       IPMIInterface *intf;
   };
diff --git a/qemu-options.hx b/qemu-options.hx
index dc694a99a3..186433ac13 100644
--- a/qemu-options.hx
+++ b/qemu-options.hx
@@ -1120,6 +1120,10 @@ SRST
       ``slave_addr=val``
           Define slave address to use for the BMC. The default is 0x20.
+    ``instance=val``
+        For more than one BMC on the same system, each instance needs
+       a unique number.  The default is 0.
+
       ``sdrfile=file``
           file containing raw Sensor Data Records (SDR) data. The default
           is none.
@@ -1137,7 +1141,7 @@ SRST
           is set, get "Get GUID" command to the BMC will return it.
           Otherwise "Get GUID" will return an error.
-``-device ipmi-bmc-extern,id=id,chardev=id[,slave_addr=val]``
+``-device ipmi-bmc-extern,id=id,chardev=id[,slave_addr=val][,instance=id]``
       Add a connection to an external IPMI BMC simulator. Instead of
       locally emulating the BMC like the above item, instead connect to an
       external entity that provides the IPMI services.
@@ -1151,6 +1155,9 @@ SRST
       simulator running on a secure port on localhost, so neither the
       simulator nor QEMU is exposed to any outside network.
+    You can have more than one external BMC connection with this, but
+    you must set a unique instance for each BMC.
+
       See the "lanserv/README.vm" file in the OpenIPMI library for more
       details on the external interface.