Re: [Qemu-devel] [PATCH 02/17] ipmi: Add a PC ISA type structure

[Paolo Bonzini]

On 16/05/2015 03:48, Corey Minyard wrote:
> On 05/13/2015 09:52 AM, Paolo Bonzini wrote:
>>
>> On 11/05/2015 21:58, Corey Minyard wrote:
>>> I've debated this in my mind since I've been learning more about
>>> qemu. Some of the bmc properties are being passed in to the interface
>>> and passed on to the bmc. Plus some IPMI systems have multiple
>>> interfaces that point to the same bmc.  It might be best to have the
>>> user create a bmc device then tie an interface device to it.
>>>
>>> If I do this, what is the acceptable way to look up another object
>>> this way?  I hunted a bit and didn't come up with anything clean.
>> Yes, you're right indeed!!!  I think you want something like
>>
>>    -object ipmi-bmc-extern,id=bmc0,chardev=foo
>>    -device isa-ipmi-kcs,bmc=bmc0
>>
>> vs.
>>
>>    -object ipmi-bmc,id=bmc0
>>    -device isa-ipmi-bt,bmc=bmc0
>>
>> ipmi-bmc would be a subclass of Object like the one that you have, but
>> it needs to implement the UserCreatable interface; see backends/rng.c
>> for an example.
>>
>> Then ipmi-isa-kcs would be your usual ISA device, so a subclass of
>> TYPE_ISA_DEVICE; however it would implement IPMIInterface, which would
>> be an interface rather than an abstract class.  For an example of
>> interface boilerplate, see hw/core/hotplug.c.  For an example of how to
>> implement the "bmc" property, see hw/mem/pc-dimm.c.
>>
>> Paolo
> I've been trying to piece this together, and the problem is that BT and
> KCS can sit on different kinds of busses, not just ISA.  There are PCI
> and memory based implementations.  I'd prefer to have one implementation
> for all, so I'm trying to figure out a way to piece all these things
> together.
> 
> What I've come up with is the following class structure:
> 
> IPMIBmc (abstract subclass of Device)
>   Implements the base BMC handling

This is actually a subclass of Object that implements UserCreatable.
But that's a detail.

> IPMIBmcInternal (subclass of IPMIBmc)
>   An internal BMC
> 
> IPMIBmcExternal (subclass of IPMIBmc)
>   A connection to an external BMC

These are good.

> BusDevice (subclass of Interface)
>   An interface where a device can connect to a bus and do I/O and
> interrupts.
> 
> BusDeviceISA (subclass of ISADevice, implements BusDevice)
>   An ISA interface for BusDevice

See below...

> IPMIInterface (subclass of Interface)
>   An Interface that connects between a BMC and a physical IPMI interface
>   (BT, KCS, SMBus)

This is okay.

> IPMIDevice (abstract subclass of Device, implements IPMIInterface)
>   The base class for the various IPMI devices.  This code finds the IPMIBmc
>   and the BusDevice objects and plugs them in to the subclasses of this
>   class.
> 
> IPMIDevKCS (subclass of IPMIDevice)
>   KCS interface
> 
> IPMIDevBT (subclass of IPMIDevice)
>   BT interface

For now I would just make IPMIDevKCS and IPMIDevBT two ISADevice
subclasses.  Any code reuse between them can be placed in the
implementation of IPMIInterface: interface methods need not be abstract,
they can have a default implementation.

Regarding BusDevice, if a PCI interface is added in the future we can
use C composition (structs :)) to group the common KCS and BT code.
There's no need for an explicit class hierarchy; for an example see the
AHCI and EHCI devices.

The SMBus interface can be added already (through a subclass of I2CSlave
that implements IPMIInterface.

> So on the command line, you would say:
> 
>   -device isadev,irq=5,id=ipmiisadev,addr=0xca2 -device
> ipmi-bmc-internal,id=bmc1
>   -device ipmi-kcs,bmc=bmc1,busdev=ipmiisadev

In my proposal this would be

        -object ipmi-bmc-internal,id=bmc1
        -device isa-ipmi-kcs,bmc=bmc1,irq=5,iobase=0xca2

(the irq and iobase can be given suitable defaults of course; iobase is
a more standard name for ISA I/O ports).

> This seems rather complicated, but it seem like the only way to break
> this up.

If you want a pure QOM solution it is.  But we can use plain struct
composition too, in order to keep the implementation simple.  The same
is done in the kernel, which uses structs or kobjects depending on the
use case.

> And I don't know if object_property_add_link() works on interfaces

Yes, it does.

> Does this sound plausible?

It did---does what I wrote also sound plausible? :)

Paolo