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

[Corey Minyard <minyard@acm.org>]

On 04/26/2015 03:58 AM, Michael S. Tsirkin wrote:
> On Thu, Apr 23, 2015 at 05:57:43PM -0500, minyard@acm.org wrote:
>> From: Corey Minyard <cminyard@mvista.com>
>>
>> This provides the base infrastructure to tie IPMI low-level
>> interfaces into a PC ISA bus.
>>
>> Signed-off-by: Corey Minyard <cminyard@mvista.com>
>> ---
>>  default-configs/i386-softmmu.mak   |   1 +
>>  default-configs/x86_64-softmmu.mak |   1 +
>>  hw/ipmi/Makefile.objs              |   1 +
>>  hw/ipmi/isa_ipmi.c                 | 144 +++++++++++++++++++++++++++++++++++++
>>  include/hw/nvram/fw_cfg.h          |  11 ++-
>>  5 files changed, 157 insertions(+), 1 deletion(-)
>>  create mode 100644 hw/ipmi/isa_ipmi.c
>>
>> diff --git a/default-configs/i386-softmmu.mak b/default-configs/i386-softmmu.mak
>> index ab1a552..1c3153b 100644
>> --- a/default-configs/i386-softmmu.mak
>> +++ b/default-configs/i386-softmmu.mak
>> @@ -9,6 +9,7 @@ CONFIG_VGA_CIRRUS=y
>>  CONFIG_VMWARE_VGA=y
>>  CONFIG_VMMOUSE=y
>>  CONFIG_IPMI=y
>> +CONFIG_ISA_IPMI=y
>>  CONFIG_SERIAL=y
>>  CONFIG_PARALLEL=y
>>  CONFIG_I8254=y
>> diff --git a/default-configs/x86_64-softmmu.mak b/default-configs/x86_64-softmmu.mak
>> index 82bafcc..6b57430 100644
>> --- a/default-configs/x86_64-softmmu.mak
>> +++ b/default-configs/x86_64-softmmu.mak
>> @@ -9,6 +9,7 @@ CONFIG_VGA_CIRRUS=y
>>  CONFIG_VMWARE_VGA=y
>>  CONFIG_VMMOUSE=y
>>  CONFIG_IPMI=y
>> +CONFIG_ISA_IPMI=y
>>  CONFIG_SERIAL=y
>>  CONFIG_PARALLEL=y
>>  CONFIG_I8254=y
>> diff --git a/hw/ipmi/Makefile.objs b/hw/ipmi/Makefile.objs
>> index 65bde11..1518216 100644
>> --- a/hw/ipmi/Makefile.objs
>> +++ b/hw/ipmi/Makefile.objs
>> @@ -1 +1,2 @@
>> +common-obj-$(CONFIG_ISA_IPMI) += isa_ipmi.o
>>  common-obj-$(CONFIG_IPMI) += ipmi.o
>> diff --git a/hw/ipmi/isa_ipmi.c b/hw/ipmi/isa_ipmi.c
>> new file mode 100644
>> index 0000000..1c1ab8d
>> --- /dev/null
>> +++ b/hw/ipmi/isa_ipmi.c
>> @@ -0,0 +1,144 @@
>> +/*
>> + * QEMU ISA IPMI emulation
>> + *
>> + * Copyright (c) 2012 Corey Minyard, MontaVista Software, LLC
>> + *
>> + * Permission is hereby granted, free of charge, to any person obtaining a copy
>> + * of this software and associated documentation files (the "Software"), to deal
>> + * in the Software without restriction, including without limitation the rights
>> + * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
>> + * copies of the Software, and to permit persons to whom the Software is
>> + * furnished to do so, subject to the following conditions:
>> + *
>> + * The above copyright notice and this permission notice shall be included in
>> + * all copies or substantial portions of the Software.
>> + *
>> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
>> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
>> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
>> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
>> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
>> + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
>> + * THE SOFTWARE.
>> + */
>> +#include "hw/hw.h"
>> +#include "hw/isa/isa.h"
>> +#include "hw/i386/pc.h"
>> +#include "qemu/timer.h"
>> +#include "sysemu/char.h"
>> +#include "sysemu/sysemu.h"
>> +#include "ipmi.h"
>> +
>> +/* This is the type the user specifies on the -device command line */
>> +#define TYPE_ISA_IPMI           "isa-ipmi"
>> +#define ISA_IPMI(obj) OBJECT_CHECK(ISAIPMIDevice, (obj), TYPE_ISA_IPMI)
>> +
>> +typedef struct ISAIPMIDevice {
>> +    ISADevice dev;
>> +    char *interface;
>> +    uint32_t iobase;
>> +    int32 isairq;
>> +    uint8_t slave_addr;
>> +    CharDriverState *chr;
>> +    IPMIInterface *intf;
>> +} ISAIPMIDevice;
>> +
>> +static void ipmi_isa_realizefn(DeviceState *dev, Error **errp)
>> +{
>> +    ISADevice *isadev = ISA_DEVICE(dev);
>> +    ISAIPMIDevice *ipmi = ISA_IPMI(dev);
>> +    char typename[20];
>> +    Object *intfobj;
>> +    IPMIInterface *intf;
>> +    Object *bmcobj;
>> +    IPMIBmc *bmc;
>> +
>> +    if (!ipmi->interface) {
>> +        ipmi->interface = g_strdup("kcs");
>> +    }
>> +
>> +    if (ipmi->chr) {
>> +        bmcobj = object_new(TYPE_IPMI_BMC_EXTERN);
>> +    } else {
>> +        bmcobj = object_new(TYPE_IPMI_BMC_SIMULATOR);
>> +    }
>> +    bmc = IPMI_BMC(bmcobj);
>> +    bmc->chr = ipmi->chr;
>> +    snprintf(typename, sizeof(typename),
>> +             TYPE_IPMI_INTERFACE_PREFIX "%s", ipmi->interface);
>> +    intfobj = object_new(typename);
>
> I wonder what Andreas thinks about this.
> There are only 3 legal types, correct?
> I think it would be cleaner to avoid the prefix trick,
> and just do a plain
> 	if (!ipmi->interface)) {
> 		typename = TYPE_IPMI_INTERFACE_KCS
> 	} else if (!strcmp(ipmi->interface, "kcs")) {
> 		typename = TYPE_IPMI_INTERFACE_KCS
> 	} else if ....
>
>
> etc

Well, I'm fine either way.  The way I had it seems clear enough to me,
but I wrote it :).

If Andreas or you want it changed, not a problem.  Just say so.

>
>
>
>> +    intf = IPMI_INTERFACE(intfobj);
>> +    bmc->intf = intf;
>> +    intf->bmc = bmc;
>> +    intf->io_base = ipmi->iobase;
>> +    intf->slave_addr = ipmi->slave_addr;
>> +    ipmi_interface_init(intf, errp);
>> +    if (*errp) {
>> +        return;
>> +    }
>> +    ipmi_bmc_init(bmc, errp);
>> +    if (*errp) {
>> +        return;
>> +    }
>> +
>> +    /* These may be set by the interface. */
>> +    ipmi->iobase = intf->io_base;
>> +    ipmi->slave_addr = intf->slave_addr;
>> +
>> +    if (ipmi->isairq > 0) {
>> +        isa_init_irq(isadev, &intf->irq, ipmi->isairq);
>> +        intf->use_irq = 1;
>> +    }
>> +
>> +    ipmi->intf = intf;
>> +    object_property_add_child(OBJECT(isadev), "intf", OBJECT(intf), errp);
>> +    if (*errp) {
>> +        return;
>> +    }
>> +    object_property_add_child(OBJECT(isadev), "bmc", OBJECT(bmc), errp);
>> +    if (*errp) {
>> +        return;
>> +    }
>> +
> Should the created object be destroyed before return?

Returning an error from the realize here appears to result in an error
being printed and qemu being terminated, as far as I can tell.  So it
shouldn't matter here, right?

-corey

>> +    qdev_set_legacy_instance_id(dev, intf->io_base, intf->io_length);
>> +
>> +    isa_register_ioport(isadev, &intf->io, intf->io_base);
>> +}
>> +
>> +static void ipmi_isa_reset(DeviceState *qdev)
>> +{
>> +    ISAIPMIDevice *isa = ISA_IPMI(qdev);
>> +
>> +    ipmi_interface_reset(isa->intf);
>> +}
>> +
>> +static Property ipmi_isa_properties[] = {
>> +    DEFINE_PROP_STRING("interface", ISAIPMIDevice, interface),
>> +    DEFINE_PROP_UINT32("iobase", ISAIPMIDevice, iobase,  0),
>> +    DEFINE_PROP_INT32("irq",   ISAIPMIDevice, isairq,  5),
>> +    DEFINE_PROP_UINT8("slave_addr", ISAIPMIDevice, slave_addr,  0),
>> +    DEFINE_PROP_CHR("chardev",  ISAIPMIDevice, chr),
>> +    DEFINE_PROP_END_OF_LIST(),
>> +};
>> +
>> +static void ipmi_isa_class_initfn(ObjectClass *klass, void *data)
>> +{
>> +    DeviceClass *dc = DEVICE_CLASS(klass);
>> +    dc->realize = ipmi_isa_realizefn;
>> +    dc->reset = ipmi_isa_reset;
>> +    dc->props = ipmi_isa_properties;
>> +}
>> +
>> +static const TypeInfo ipmi_isa_info = {
>> +    .name          = TYPE_ISA_IPMI,
>> +    .parent        = TYPE_ISA_DEVICE,
>> +    .instance_size = sizeof(ISAIPMIDevice),
>> +    .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/include/hw/nvram/fw_cfg.h b/include/hw/nvram/fw_cfg.h
>> index 6d8a8ac..cac3a34 100644
>> --- a/include/hw/nvram/fw_cfg.h
>> +++ b/include/hw/nvram/fw_cfg.h
>> @@ -38,7 +38,16 @@
>>  
>>  #define FW_CFG_FILE_FIRST       0x20
>>  #define FW_CFG_FILE_SLOTS       0x10
>> -#define FW_CFG_MAX_ENTRY        (FW_CFG_FILE_FIRST+FW_CFG_FILE_SLOTS)
>> +
>> +#define FW_CFG_IPMI_INTERFACE   0x30
>> +#define FW_CFG_IPMI_BASE_ADDR   0x31
>> +#define FW_CFG_IPMI_REG_SPACE   0x32
>> +#define FW_CFG_IPMI_REG_SPACING 0x33
>> +#define FW_CFG_IPMI_SLAVE_ADDR  0x34
>> +#define FW_CFG_IPMI_IRQ         0x35
>> +#define FW_CFG_IPMI_VERSION     0x36
>> +
>> +#define FW_CFG_MAX_ENTRY        (FW_CFG_IPMI_VERSION + 1)
>>  
>>  #define FW_CFG_WRITE_CHANNEL    0x4000
>>  #define FW_CFG_ARCH_LOCAL       0x8000
>> -- 
>> 1.8.3.1
>>