Re: [Qemu-devel] [PATCH 5/9] IPMI: Add a PC ISA type structure

[Paolo Bonzini <pbonzini@redhat.com>]

Il 09/07/2012 21:17, minyard@acm.org ha scritto:
> +
> +static Property ipmi_isa_properties[] = {
> +    DEFINE_PROP_HEX32("type", ISAIPMIState, type,  IPMI_KCS),

You can add an enum property.  There is one example called
LostTickPolicy in the tree.  Please do not use the generic "type" name;
use "interface" for example.

Start with an empty enum, and let each of the two patches 6/7 add an
item; same for the various switch statements on
IPMI_KCS/IPMI_BT/IPMI_SMIC.  (Actually if you do this the IPMI_SMIC
branches will disappear, right?)

> +    DEFINE_PROP_HEX32("iobase", ISAIPMIState, iobase,  -1),
> +    DEFINE_PROP_UINT32("irq",   ISAIPMIState, isairq,  0),
> +    DEFINE_PROP_UINT8("slave_addr", ISAIPMIState, slave_addr,  0),
> +    DEFINE_PROP_PTR("charopts",  ISAIPMIState, state.chropts),

Here, you should add a "normal" chardev property so that people can use
-chardev and -device to create the IPMI interface.  The device can be
created like this:

   -chardev ...,id=charipmi -device ipmi,interface=kcs,chardev=charipmi

If the chardev is absent, the local interface is used instead.

See docs/qdev-device-use.txt for more information.

> +    DEFINE_PROP_END_OF_LIST(),
> +};
> +
> +static const VMStateDescription vmstate_isa_ipmi = {
> +    .name = "isa-ipmi",
> +    .version_id = 3,
> +    .minimum_version_id = 3,
> +    .fields      = (VMStateField []) {
> +        VMSTATE_STRUCT(state, ISAIPMIState, 0, vmstate_isa_ipmi,
> +		       IPMIState),
> +        VMSTATE_END_OF_LIST()
> +    }
> +};
> +
> +static void ipmi_isa_class_initfn(ObjectClass *klass, void *data)
> +{
> +    DeviceClass *dc = DEVICE_CLASS(klass);
> +    ISADeviceClass *ic = ISA_DEVICE_CLASS(klass);
> +    ic->init = ipmi_isa_initfn;
> +    dc->vmsd = &vmstate_isa_ipmi;
> +    dc->props = ipmi_isa_properties;
> +}
> +
> +static TypeInfo ipmi_isa_info = {
> +    .name          = "isa-ipmi",
> +    .parent        = TYPE_ISA_DEVICE,
> +    .instance_size = sizeof(ISAIPMIState),
> +    .class_init    = ipmi_isa_class_initfn,
> +};
> +
> +static void ipmi_register_types(void)
> +{
> +    type_register_static(&ipmi_isa_info);
> +}
> +
> +type_init(ipmi_register_types)
> diff --git a/hw/pc.c b/hw/pc.c
> index c0acb6a..965e053 100644
> --- a/hw/pc.c
> +++ b/hw/pc.c
> @@ -1173,6 +1173,18 @@ void pc_basic_device_init(ISABus *isa_bus, qemu_irq *gsi,
>          fd[i] = drive_get(IF_FLOPPY, 0, i);
>      }
>      *floppy = fdctrl_init_isa(isa_bus, fd);
> +
> +    i = 0;
> +    if (do_local_ipmi) {
> +	ipmi_isa_init(isa_bus, isa_find_free_irq(isa_bus), ipmi_types[i], NULL);
> +	i++;
> +    }
> +    for(; i < MAX_IPMI_DEVICES; i++) {
> +	if (ipmi_hds[i]) {
> +	    ipmi_isa_init(isa_bus, isa_find_free_irq(isa_bus),
> +			  ipmi_types[i], ipmi_hds[i]);
> +	}
> +    }
>  }
>  
>  void pc_pci_device_init(PCIBus *pci_bus)
> diff --git a/hw/pc.h b/hw/pc.h
> index 33ab689..5b6d947 100644
> --- a/hw/pc.h
> +++ b/hw/pc.h
> @@ -223,6 +223,24 @@ static inline bool isa_ne2000_init(ISABus *bus, int base, int irq, NICInfo *nd)
>      return true;
>  }
>  
> +/* IPMI */
> +static inline bool ipmi_isa_init(ISABus *bus, int irq,
> +				 int type, QemuOpts *opts)
> +{
> +    ISADevice *dev;
> +
> +    dev = isa_try_create(bus, "isa-ipmi");
> +    if (!dev) {
> +        return false;
> +    }
> +    qdev_prop_set_uint32(&dev->qdev, "type", type);
> +    qdev_prop_set_uint32(&dev->qdev, "irq", irq);
> +    qdev_prop_set_ptr(&dev->qdev, "charopts", opts);
> +    if (qdev_init(&dev->qdev) < 0) {
> +        return false;
> +    }