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

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

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.

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