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

[Corey Minyard <corey@minyard.net>]

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 <corey@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().

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

-corey

> 
> Preferably s/instance/id/:
> Reviewed-by: Philippe Mathieu-Daudé <philmd@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.
>