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 <[email protected]> > > > > --- > > > > 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/[email protected]/ > > > > > -corey > > > > > > > > Preferably s/instance/id/: > > > Reviewed-by: Philippe Mathieu-Daudé <[email protected]> > > > > > > > }; > > > > 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. > > > >
