From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:58468) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1b5BeX-0001kd-Fo for qemu-devel@nongnu.org; Tue, 24 May 2016 08:41:42 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1b5BeR-0005bj-Qd for qemu-devel@nongnu.org; Tue, 24 May 2016 08:41:40 -0400 Received: from mail-pf0-x241.google.com ([2607:f8b0:400e:c00::241]:33265) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1b5BeR-0005ba-GV for qemu-devel@nongnu.org; Tue, 24 May 2016 08:41:35 -0400 Received: by mail-pf0-x241.google.com with SMTP id b124so1561942pfb.0 for ; Tue, 24 May 2016 05:41:35 -0700 (PDT) Sender: Corey Minyard Reply-To: minyard@acm.org References: <1464025217-24054-1-git-send-email-minyard@acm.org> <1464025217-24054-6-git-send-email-minyard@acm.org> <20160524094927.2f5d6d91@nial.brq.redhat.com> From: Corey Minyard Message-ID: <57444BF9.2080006@acm.org> Date: Tue, 24 May 2016 07:41:29 -0500 MIME-Version: 1.0 In-Reply-To: <20160524094927.2f5d6d91@nial.brq.redhat.com> Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH v6 5/6] acpi: Add IPMI table entries List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Igor Mammedov Cc: "Michael S . Tsirkin" , Paolo Bonzini , qemu-devel@nongnu.org, cminyard@mvista.com On 05/24/2016 02:49 AM, Igor Mammedov wrote: > On Mon, 23 May 2016 12:40:16 -0500 > minyard@acm.org wrote: > >> From: Corey Minyard >> >> Use the new ACPI table construction tools to create an ACPI >> entry for IPMI. This adds a function called from build_dsdt >> to add an DSDT entry for IPMI if IPMI is compiled in and has >> registered firmware. It also adds a dummy function if IPMI >> is not compiled in. >> >> This conforms to section "C3-2 Locating IPMI System Interfaces in >> ACPI Name Space" in the IPMI 2.0 specification. >> >> Signed-off-by: Corey Minyard >> --- >> hw/acpi/Makefile.objs | 2 + >> hw/acpi/ipmi.c | 105 +++++++++++++++++++++++++++++++++++++++++++++++++ >> hw/i386/acpi-build.c | 24 +++++++++-- >> hw/stubs/Makefile.objs | 1 + >> hw/stubs/acpi_ipmi.c | 14 +++++++ >> include/hw/acpi/ipmi.h | 22 +++++++++++ >> 6 files changed, 165 insertions(+), 3 deletions(-) >> create mode 100644 hw/acpi/ipmi.c >> create mode 100644 hw/stubs/acpi_ipmi.c >> create mode 100644 include/hw/acpi/ipmi.h >> >> diff --git a/hw/acpi/Makefile.objs b/hw/acpi/Makefile.objs >> index faee86c..95824e8 100644 >> --- a/hw/acpi/Makefile.objs >> +++ b/hw/acpi/Makefile.objs >> @@ -6,3 +6,5 @@ obj-$(CONFIG_ACPI_NVDIMM) += nvdimm.o >> common-obj-$(CONFIG_ACPI) += acpi_interface.o >> common-obj-$(CONFIG_ACPI) += bios-linker-loader.o >> common-obj-$(CONFIG_ACPI) += aml-build.o >> +common-obj-$(call land,$(CONFIG_ACPI),$(CONFIG_IPMI)) += ipmi.o >> + >> diff --git a/hw/acpi/ipmi.c b/hw/acpi/ipmi.c >> new file mode 100644 >> index 0000000..201f824 >> --- /dev/null >> +++ b/hw/acpi/ipmi.c >> @@ -0,0 +1,105 @@ >> +/* >> + * IPMI ACPI firmware handling >> + * >> + * Copyright (c) 2015 Corey Minyard, MontaVista Software, LLC >> + * >> + * This work is licensed under the terms of the GNU GPL, version 2 or later. >> + * See the COPYING file in the top-level directory. >> + */ >> + >> +#include "qemu/osdep.h" >> +#include "hw/ipmi/ipmi.h" >> +#include "hw/acpi/aml-build.h" >> +#include "hw/acpi/acpi.h" >> +#include "hw/acpi/ipmi.h" >> + >> +static Aml *aml_ipmi_crs(IPMIFwInfo *info) >> +{ >> + Aml *crs = aml_resource_template(); >> + uint8_t regspacing = info->register_spacing; > nit about naming, > why not s/regspacing|register_spacing/reg_alignment/ "Register spacing" comes from the SMBIOS definition in the IPMI spec. Since I did SMBIOS first, it won. There's no reason to have a local for this now (I think there was at some point). . . . >> + >> + if (!obj) { >> + continue; >> + } >> + >> + ii = IPMI_INTERFACE(obj); >> + iic = IPMI_INTERFACE_GET_CLASS(obj); >> + memset(&info, 0, sizeof(info)); > why not to put memset() inside iic->get_fwinfo(), > that way every caller won't have to do it (SMBIOS call site). I debated on that, but there are actually fewer call sites than implementers here. Well, they are equal now, but with SMBus there will be more implementers. >> + iic->get_fwinfo(ii, &info); >> + aml_append(scope, aml_ipmi_device(&info)); >> + } >> +} >> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c >> index 279f0d7..bf9253d 100644 >> --- a/hw/i386/acpi-build.c >> +++ b/hw/i386/acpi-build.c >> @@ -59,6 +59,8 @@ >> #include "qapi/qmp/qint.h" >> #include "qom/qom-qobject.h" >> >> +#include "hw/acpi/ipmi.h" >> + >> /* These are used to size the ACPI tables for -M pc-i440fx-1.7 and >> * -M pc-i440fx-2.0. Even if the actual amount of AML generated grows >> * a little bit, there should be plenty of free space since the DSDT >> @@ -1445,7 +1447,21 @@ static Aml *build_com_device_aml(uint8_t uid) >> return dev; >> } >> >> -static void build_isa_devices_aml(Aml *table) >> +static void build_isa_ipmi_aml(Aml *scope) >> +{ >> + bool ambiguous; >> + Object *obj = object_resolve_path_type("", TYPE_ISA_BUS, &ambiguous); >> + >> + if (ambiguous) { >> + error_report("Multiple ISA busses, unable to define IPMI ACPI data"); >> + } else if (!obj) { >> + error_report("No ISA bus, unable to define IPMI ACPI data"); >> + } else { >> + build_acpi_ipmi_devices(scope, BUS(obj)); >> + } >> +} > I'd fold this function into build_acpi_ipmi_devices() if there aren't any plans > for IPMI devices being present on another bus. Ok, it looked a little neater this way to me, but that's fine. >> + >> +static void build_isa_devices_aml(Aml *table, PCMachineState *pcms) > why is pcms is added here, it doesn't seem to be used? I forgot to remove it when I removed the isa_bus from PCMachineState. Thanks, -corey