| Message ID | 1429829878-26862-3-git-send-email-minyard@acm.org |
|---|---|
| State | New |
| Headers | show |
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 > + 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? > + 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 >
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); > + 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; > + } > + > + 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 Can IPMI use fw cfg file interface? This is, generally, preferred. > -- > 1.8.3.1 >
On Sun, Apr 26, 2015 at 10:58:56AM +0200, 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> BTW can you version patches please? recent git format-patch has a -v flag, older ones let you use --subject-prefix='PATCH vX'.
On 26/04/2015 11:05, Michael S. Tsirkin wrote: >> + >> +#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) Also, what is this used for? I haven't seen firmware patches, maybe it's obsoleted by SMBIOS support in QEMU? Paolo
On 04/26/2015 12:03 PM, Paolo Bonzini wrote: > > On 26/04/2015 11:05, Michael S. Tsirkin wrote: >>> + >>> +#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) > Also, what is this used for? I haven't seen firmware patches, maybe > it's obsoleted by SMBIOS support in QEMU? > > Paolo Finally back on this. This is bogus, it was left over from something else, and I missed it because it was at the end of the file. -corey
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 >>
On 08/05/2015 23:16, Corey Minyard wrote: >>> >> + 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? I would just use &error_abort and ignore error handling. Paolo
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, 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. 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
On 11/05/2015 19:26, Andreas Färber wrote: > 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? Note that this is not exactly a backend. It's a different guest-visible I/O interface. But there are no properties on the interface, so it's okay to create it at realize time. Paolo
On 05/11/2015 02:42 PM, Paolo Bonzini wrote: > > On 11/05/2015 19:26, Andreas Färber wrote: >> 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? > Note that this is not exactly a backend. It's a different guest-visible > I/O interface. But there are no properties on the interface, so it's > okay to create it at realize time. > > Paolo 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. Thanks, -corey
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
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 IPMIBmcInternal (subclass of IPMIBmc) An internal BMC IPMIBmcExternal (subclass of IPMIBmc) A connection to an external BMC 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 IPMIInterface (subclass of Interface) An Interface that connects between a BMC and a physical IPMI interface (BT, KCS, SMBus) 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 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 This seems rather complicated, but it seem like the only way to break this up. And I don't know if object_property_add_link() works on interfaces, but that should be easy to fix if it doesn't. Does this sound plausible? Thanks, -corey
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
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); + 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; + } + + 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