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

["Philippe Mathieu-Daudé" <philmd@linaro.org>]

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.

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