Re: [Qemu-devel] [PATCH 00/16] Add an IPMI device to qemu

[Paolo Bonzini]

On 12/12/2014 20:15, address@hidden wrote:
> This set of patches adds an IPMI device to qemu.  This is good for
> systems that require an IPMI device to work correctly, for simulating
> scenarios that require IPMI and testing software that uses IPMI, and
> of course, for the Linux IPMI driver maintainer to use to reproduce
> issues that could not be easily reproduced otherwise :).
> 
> For those that don't know, IPMI is a standard for doing sensor
> monitoring and basic machine maintenance.  It has local interfaces
> on the host system (four different types, SMIC, KCS, BT, and SSIF)
> that can be in I/O space, memory space, and PCI space, and in the
> case of SSIF, is on an I2C bus.  It can also have network interfaces
> for out of band maintenance, though that's not directly relevant
> here.  It is a message-based interface; all IPMI operations are
> done by sending command messages, results come back as response
> messages.
> 
> The maintenance actions are done by a small controller on the system
> called a Baseboard Management Controller (BMC).  These devices are
> always on when the system is plugged in, even if the system is off.
> 
> The first 9 patches add the device itself on an ISA interface.  This
> adds a KCS device and a BT device (two of the four standard IPMI
> interfaces).  It also adds software to have a small BMC simulated
> inside of QEMU, and a way to connected to an external BMC (like
> openipmi library's lanserv) over the network.  Those patches should
> be pretty straightforward, though I'm still not 100% sure about
> migration.

Indeed patches 1-8 should be pretty much done.  And 9 looks like it's in
good shape too, though I have not reviewed it carefully enough.

I haven't checked that the patches pass checkpatch.pl; also the
preferred style over this:

+        (IPMI_INTERFACE_GET_CLASS(s))->handle_if_event(s);

is to have a function:

    ipmi_interface_handle_if_event(s);

that is written like

void ipmi_interface_handle_if_event(IPMIInterface *s)
{
    IPMIInterfaceClass *sc = IPMI_INTERFACE_GET_CLASS(s);
    sc->handle_if_event(s);
}

Also, a function like ipmi_signal would be in the .c file.  There should
be no use of *_GET_CLASS outside the .c file.

For patches 10-15, there is already code in QEMU for ACPI table
generation.  Right now it uses iasl, but Igor Mammedov is also working
on an API similar to yours (the duplication is unfortunate, indeed).  So
it would probably be better to integrate table construction with
"regular" SSDT construction in hw/i386/acpi-build.c.  ACPI tables are
already sent to the firmware after device initialization; IIUC they are
built on demand, so there should be no change required for this anymore.

SMBIOS tables are also built in QEMU now, which simplified your work a
bit.  Moving the work to a machine_done_notifier is a good idea and
matches ACPI.  The only thing I'd change is using a NotifierList instead
of rolling your own smbios_device_handler struct.

BTW, any reason to choose ISA over PCI or I2C (which is supported for
i386 via the ACPI SMBus controller)?

Paolo