From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:50182) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YrrTK-0005DP-Pn for qemu-devel@nongnu.org; Mon, 11 May 2015 13:26:32 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1YrrTH-0001c9-NM for qemu-devel@nongnu.org; Mon, 11 May 2015 13:26:30 -0400 Received: from cantor2.suse.de ([195.135.220.15]:40674 helo=mx2.suse.de) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YrrTH-0001c3-DW for qemu-devel@nongnu.org; Mon, 11 May 2015 13:26:27 -0400 Message-ID: <5550E641.2060108@suse.de> Date: Mon, 11 May 2015 19:26:25 +0200 From: =?windows-1252?Q?Andreas_F=E4rber?= MIME-Version: 1.0 References: <1429829878-26862-1-git-send-email-minyard@acm.org> <1429829878-26862-3-git-send-email-minyard@acm.org> <20150426105502-mutt-send-email-mst@redhat.com> <554D2791.6070401@acm.org> In-Reply-To: <554D2791.6070401@acm.org> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH 02/17] ipmi: Add a PC ISA type structure List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: minyard@acm.org, "Michael S. Tsirkin" Cc: Corey Minyard , qemu-devel@nongnu.org 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 >>> >>> This provides the base infrastructure to tie IPMI low-level >>> interfaces into a PC ISA bus. >>> >>> Signed-off-by: Corey Minyard >>> --- >>> 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=3Dy >>> CONFIG_VMWARE_VGA=3Dy >>> CONFIG_VMMOUSE=3Dy >>> CONFIG_IPMI=3Dy >>> +CONFIG_ISA_IPMI=3Dy >>> CONFIG_SERIAL=3Dy >>> CONFIG_PARALLEL=3Dy >>> CONFIG_I8254=3Dy >>> 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=3Dy >>> CONFIG_VMWARE_VGA=3Dy >>> CONFIG_VMMOUSE=3Dy >>> CONFIG_IPMI=3Dy >>> +CONFIG_ISA_IPMI=3Dy >>> CONFIG_SERIAL=3Dy >>> CONFIG_PARALLEL=3Dy >>> CONFIG_I8254=3Dy >>> 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) +=3D isa_ipmi.o >>> common-obj-$(CONFIG_IPMI) +=3D 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 obtai= ning a copy >>> + * of this software and associated documentation files (the "Softwar= e"), 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 Softwar= e is >>> + * furnished to do so, subject to the following conditions: >>> + * >>> + * The above copyright notice and this permission notice shall be in= cluded in >>> + * all copies or substantial portions of the Software. >>> + * >>> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, E= XPRESS OR >>> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTA= BILITY, >>> + * 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, A= RISING FROM, >>> + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEA= LINGS 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_IP= MI) >>> + >>> +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 =3D ISA_DEVICE(dev); >>> + ISAIPMIDevice *ipmi =3D ISA_IPMI(dev); >>> + char typename[20]; >>> + Object *intfobj; >>> + IPMIInterface *intf; >>> + Object *bmcobj; >>> + IPMIBmc *bmc; >>> + >>> + if (!ipmi->interface) { >>> + ipmi->interface =3D g_strdup("kcs"); >>> + } >>> + >>> + if (ipmi->chr) { >>> + bmcobj =3D object_new(TYPE_IPMI_BMC_EXTERN); >>> + } else { >>> + bmcobj =3D object_new(TYPE_IPMI_BMC_SIMULATOR); >>> + } >>> + bmc =3D IPMI_BMC(bmcobj); >>> + bmc->chr =3D ipmi->chr; >>> + snprintf(typename, sizeof(typename), >>> + TYPE_IPMI_INTERFACE_PREFIX "%s", ipmi->interface); >>> + intfobj =3D 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 =3D TYPE_IPMI_INTERFACE_KCS >> } else if (!strcmp(ipmi->interface, "kcs")) { >> typename =3D TYPE_IPMI_INTERFACE_KCS >> } else if .... >> >> >> etc >=20 > Well, I'm fine either way. The way I had it seems clear enough to me, > but I wrote it :). >=20 > 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 =3D IPMI_INTERFACE(intfobj); >>> + bmc->intf =3D intf; >>> + intf->bmc =3D bmc; >>> + intf->io_base =3D ipmi->iobase; >>> + intf->slave_addr =3D 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 =3D intf->io_base; >>> + ipmi->slave_addr =3D intf->slave_addr; >>> + >>> + if (ipmi->isairq > 0) { >>> + isa_init_irq(isadev, &intf->irq, ipmi->isairq); >>> + intf->use_irq =3D 1; >>> + } >>> + >>> + ipmi->intf =3D intf; >>> + object_property_add_child(OBJECT(isadev), "intf", OBJECT(intf), = errp); >>> + if (*errp) { >>> + return; >>> + } >>> + object_property_add_child(OBJECT(isadev), "bmc", OBJECT(bmc), er= rp); >>> + if (*errp) { >>> + return; >>> + } >>> + >> Should the created object be destroyed before return? >=20 > 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 !=3D NULL, so you need to us= e a local Error *err =3D 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 =3D ISA_IPMI(qdev); >>> + >>> + ipmi_interface_reset(isa->intf); >>> +} >>> + >>> +static Property ipmi_isa_properties[] =3D { >>> + 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 =3D DEVICE_CLASS(klass); >>> + dc->realize =3D ipmi_isa_realizefn; >>> + dc->reset =3D ipmi_isa_reset; >>> + dc->props =3D ipmi_isa_properties; >>> +} >>> + >>> +static const TypeInfo ipmi_isa_info =3D { >>> + .name =3D TYPE_ISA_IPMI, >>> + .parent =3D TYPE_ISA_DEVICE, >>> + .instance_size =3D sizeof(ISAIPMIDevice), >>> + .class_init =3D 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 @@ >>> =20 >>> #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) >>> =20 >>> #define FW_CFG_WRITE_CHANNEL 0x4000 >>> #define FW_CFG_ARCH_LOCAL 0x8000 >>> --=20 >>> 1.8.3.1 Regards, Andreas --=20 SUSE Linux GmbH, Maxfeldstr. 5, 90409 N=FCrnberg, Germany GF: Felix Imend=F6rffer, Jane Smithard, Dilip Upmanyu, Graham Norton; HRB 21284 (AG N=FCrnberg)