Am 08.05.2015 um 23:16 schrieb Corey Minyard:
> On 04/26/2015 03:58 AM, Michael S. Tsirkin wrote:
>> On Thu, Apr 23, 2015 at 05:57:43PM -0500, miny...@acm.org wrote:
>>> From: Corey Minyard <cminy...@mvista.com>
>>>
>>> This provides the base infrastructure to tie IPMI low-level
>>> interfaces into a PC ISA bus.
>>>
>>> Signed-off-by: Corey Minyard <cminy...@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.
I do prefer mst's suggestion. You are right that your code is clear, but
it's duplicated and thus makes refactorings harder. For clarity I would
even propose to skip bmcobj in favor of bmc. Same for the other objects.
OBJECT() is cheap, unlike other casts.
Another problem is that you're using object_new() in realize at all,
which means that it's too late for any management interface to tweak
properties on the new device. One possible solution would be to create
the object in a property setter, before realizing the object. Did you
look at how some of the other backends are implemented, such as rng?
>>> + 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?
Negative, this is a property setter that has nothing to do with what its
callers do. For one, you cannot rely on errp != NULL, so you need to use
a local Error *err = NULL; and call error_propagate() when necessary.
Also keep in mind that this realized property is publicly accessible.
When set to false and re-set to true those will definitely fail as I see
no unrealize implementation ever deleting these properties. In that
case, with your guest running, terminating QEMU is not desired.
>>> + 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)
s/klass/oc/ preferred.
>>> +{
>>> + 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
Do we really need all those fw_cfg entries for an optional device?
>>> +
>>> +#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
Regards,
Andreas
--
SUSE Linux GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Felix Imendörffer, Jane Smithard, Dilip Upmanyu, Graham Norton; HRB
21284 (AG Nürnberg)
