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

[Corey Minyard <corey@minyard.net>]

On Fri, Apr 04, 2025 at 03:21:09PM +0200, Philippe Mathieu-Daudé wrote:
> 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 <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().
> 
> 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.

Yeah, I was guessing that, and I also just found that the
vmstate_register() was deprecated.  I'll work on switching to the new
API.

Thanks,

-corey

> 
> See this thread:
> https://lore.kernel.org/qemu-devel/87czq0l2mn.fsf@dusky.pond.sub.org/
> 
> > 
> > -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.
> > > 
>