| Message ID | 1463671442-15631-6-git-send-email-minyard@acm.org |
|---|---|
| State | New |
| Headers | show |
On Thu, 19 May 2016 10:24:01 -0500 minyard@acm.org wrote: > From: Corey Minyard <cminyard@mvista.com> > > 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 <cminyard@mvista.com> > --- > hw/acpi/Makefile.objs | 2 + > hw/acpi/ipmi.c | 114 +++++++++++++++++++++++++++++++++++++++++++++++++ > hw/acpi/noipmi.c | 14 ++++++ > hw/i386/acpi-build.c | 10 +++-- > hw/i386/pc_piix.c | 1 + > hw/i386/pc_q35.c | 1 + > include/hw/acpi/ipmi.h | 22 ++++++++++ > include/hw/i386/pc.h | 1 + > 8 files changed, 162 insertions(+), 3 deletions(-) > create mode 100644 hw/acpi/ipmi.c > create mode 100644 hw/acpi/noipmi.c > create mode 100644 include/hw/acpi/ipmi.h > > diff --git a/hw/acpi/Makefile.objs b/hw/acpi/Makefile.objs > index faee86c..b8d5c84 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 > +common-obj-$(call land,$(CONFIG_ACPI),$(call lnot,$(CONFIG_IPMI))) += noipmi.o > diff --git a/hw/acpi/ipmi.c b/hw/acpi/ipmi.c > new file mode 100644 > index 0000000..3d61550 > --- /dev/null > +++ b/hw/acpi/ipmi.c > @@ -0,0 +1,114 @@ > +/* > + * 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, const char *scope) scope is unused inside this function, drop it? > +{ > + Aml *crs = aml_resource_template(); > + uint8_t regspacing = info->register_spacing; > + > + /* > + * The base address is fixed and cannot change. That may be different > + * if someone does PCI, but we aren't there yet. > + */ > + switch (info->memspace) { > + case IPMI_MEMSPACE_IO: > + aml_append(crs, aml_io(AML_DECODE16, info->base_address, > + info->base_address + info->register_length - 1, > + regspacing, info->register_length)); > + break; > + case IPMI_MEMSPACE_MEM32: > + aml_append(crs, > + aml_dword_memory(AML_POS_DECODE, > + AML_MIN_FIXED, AML_MAX_FIXED, > + AML_NON_CACHEABLE, AML_READ_WRITE, > + 0xffffffff, > + info->base_address, > + info->base_address + info->register_length - 1, > + regspacing, info->register_length)); > + break; > + case IPMI_MEMSPACE_MEM64: > + aml_append(crs, > + aml_qword_memory(AML_POS_DECODE, > + AML_MIN_FIXED, AML_MAX_FIXED, > + AML_NON_CACHEABLE, AML_READ_WRITE, > + 0xffffffffffffffffULL, > + info->base_address, > + info->base_address + info->register_length - 1, > + regspacing, info->register_length)); > + break; > + case IPMI_MEMSPACE_SMBUS: > + aml_append(crs, aml_return(aml_int(info->base_address))); > + break; > + default: > + abort(); > + } > + > + if (info->interrupt_number) { > + aml_append(crs, aml_irq_no_flags(info->interrupt_number)); > + } > + > + return crs; > +} > + > +static void ipmi_encode_one_acpi(Aml *table, IPMIFwInfo *info, > + const char *resource) it seems that 'resource' is used incorrectly in this patch, or more exactly it's not needed. s/ipmi_encode_one_acpi/acpi_ipmi_device/ drop 'table' and 'resource' args and make it return 'dev' it composes. > +{ > + Aml *dev, *method; > + uint16_t version = ((info->ipmi_spec_major_revision << 8) > + | (info->ipmi_spec_minor_revision << 4)); > + > + dev = aml_device("MI0"); > + aml_append(dev, aml_name_decl("_HID", aml_eisaid("IPI0001"))); > + aml_append(dev, aml_name_decl("_STR", aml_string("ipmi_%s", > + info->interface_name))); > + aml_append(dev, aml_name_decl("_UID", aml_int(info->uuid))); > + aml_append(dev, aml_name_decl("_CRS", aml_ipmi_crs(info, resource))); > + > + /* > + * The spec seems to require these to be methods. All the examples > + * show them this way and it doesn't seem to work if they are not. > + */ > + method = aml_method("_IFT", 0, AML_NOTSERIALIZED); > + aml_append(method, aml_return(aml_int(info->interface_type))); > + aml_append(dev, method); > + method = aml_method("_SRV", 0, AML_NOTSERIALIZED); > + aml_append(method, aml_return(aml_int(version))); > + aml_append(dev, method); replace these methods with aml_name_decl() as they do not contain any logic except of returning static value. > + > + aml_append(table, dev); > +} > + > +void acpi_add_ipmi(Aml *table, BusState *bus, const char *resource) s/acpi_add_ipmi/build_acpi_ipmi_devices/ drop 'resource' s/table/scope/ > +{ > + BusChild *kid; > + > + QTAILQ_FOREACH(kid, &bus->children, sibling) { > + DeviceState *dev = kid->child; > + Object *obj = object_dynamic_cast(OBJECT(dev), TYPE_IPMI_INTERFACE); > + IPMIInterface *ii; > + IPMIInterfaceClass *iic; > + IPMIFwInfo info; > + > + if (!obj) { > + continue; > + } > + > + ii = IPMI_INTERFACE(obj); > + iic = IPMI_INTERFACE_GET_CLASS(obj); > + memset(&info, 0, sizeof(info)); > + iic->get_fwinfo(ii, &info); > + ipmi_encode_one_acpi(table, &info, resource); aml_append(scope, my_ipmi_device) > + } > +} > diff --git a/hw/acpi/noipmi.c b/hw/acpi/noipmi.c > new file mode 100644 > index 0000000..dd7590d > --- /dev/null > +++ b/hw/acpi/noipmi.c move this to .a/stubs/ > @@ -0,0 +1,14 @@ > +/* > + * 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 "hw/acpi/ipmi.h" > + > +void acpi_add_ipmi(Aml *table, BusState *bus, const char *resource) > +{ > +} > diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c > index 279f0d7..101082d 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,7 @@ static Aml *build_com_device_aml(uint8_t uid) > return dev; > } > > -static void build_isa_devices_aml(Aml *table) > +static void build_isa_devices_aml(Aml *table, ISABus *isa_bus) > { > ISADevice *fdc = pc_find_fdc0(); > > @@ -1461,6 +1463,8 @@ static void build_isa_devices_aml(Aml *table) > aml_append(scope, build_com_device_aml(1)); > aml_append(scope, build_com_device_aml(2)); > > + acpi_add_ipmi(scope, BUS(isa_bus), "\\_SB.PCI0.ISA"); > + > aml_append(table, scope); > } > > @@ -2011,7 +2015,7 @@ build_dsdt(GArray *table_data, GArray *linker, > build_hpet_aml(dsdt); > build_piix4_pm(dsdt); > build_piix4_isa_bridge(dsdt); > - build_isa_devices_aml(dsdt); > + build_isa_devices_aml(dsdt, pcms->isa_bus); I'm not sure about adding 'isa_bus' field to PCMachineState, it might be better to find a ISA bus object internally in build_isa_devices_aml() and assert if found more than one, since code assumes that there is only one anyway. > build_piix4_pci_hotplug(dsdt); > build_piix4_pci0_int(dsdt); > } else { > @@ -2039,7 +2043,7 @@ build_dsdt(GArray *table_data, GArray *linker, > > build_hpet_aml(dsdt); > build_q35_isa_bridge(dsdt); > - build_isa_devices_aml(dsdt); > + build_isa_devices_aml(dsdt, pcms->isa_bus); > build_q35_pci0_int(dsdt); > } > > diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c > index 7f50116..2e5ff45 100644 > --- a/hw/i386/pc_piix.c > +++ b/hw/i386/pc_piix.c > @@ -185,6 +185,7 @@ static void pc_init1(MachineState *machine, > &error_abort); > no_hpet = 1; > } > + pcms->isa_bus = isa_bus; > isa_bus_irqs(isa_bus, gsi); > > if (kvm_pic_in_kernel()) { > diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c > index 04aae89..027655b 100644 > --- a/hw/i386/pc_q35.c > +++ b/hw/i386/pc_q35.c > @@ -190,6 +190,7 @@ static void pc_q35_init(MachineState *machine) > ICH9_LPC_NB_PIRQS); > pci_bus_set_route_irq_fn(host_bus, ich9_route_intx_pin_to_irq); > isa_bus = ich9_lpc->isa_bus; > + pcms->isa_bus = isa_bus; > > /*end early*/ > isa_bus_irqs(isa_bus, gsi); > diff --git a/include/hw/acpi/ipmi.h b/include/hw/acpi/ipmi.h > new file mode 100644 > index 0000000..67d6e2f > --- /dev/null > +++ b/include/hw/acpi/ipmi.h > @@ -0,0 +1,22 @@ > +/* > + * QEMU IPMI ACPI handling > + * > + * Copyright (c) 2015 Corey Minyard <cminyard@mvista.com> > + * > + * This work is licensed under the terms of the GNU GPL, version 2 or later. > + * See the COPYING file in the top-level directory. > + */ > +#ifndef HW_ACPI_IPMI_H > +#define HW_ACPI_IPMI_H > + > +#include "qemu/osdep.h" > +#include "hw/acpi/aml-build.h" > + > +/* > + * Add ACPI IPMI entries for all registered IPMI devices whose parent > + * bus matches the given bus. The resource is the ACPI resource that > + * contains the IPMI device, this is required for the I2C CRS. > + */ > +void acpi_add_ipmi(Aml *table, BusState *bus, const char *resource); > + > +#endif /* HW_ACPI_IPMI_H */ > diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h > index 96f0b66..aa8bc97 100644 > --- a/include/hw/i386/pc.h > +++ b/include/hw/i386/pc.h > @@ -51,6 +51,7 @@ struct PCMachineState { > HotplugHandler *acpi_dev; > ISADevice *rtc; > PCIBus *bus; > + ISABus *isa_bus; > FWCfgState *fw_cfg; > > /* Configuration options: */
Thanks for all the comments. I didn't know about stubs, as there's nothing that currently uses it in hw directory, but it's easy enough to add. I did have two comment below: On 05/20/2016 04:53 AM, Igor Mammedov wrote: > On Thu, 19 May 2016 10:24:01 -0500 > minyard@acm.org wrote: . . . > > + aml_append(dev, aml_name_decl("_STR", aml_string("ipmi_%s", > + info->interface_name))); > + aml_append(dev, aml_name_decl("_UID", aml_int(info->uuid))); > + aml_append(dev, aml_name_decl("_CRS", aml_ipmi_crs(info, resource))); > + > + /* > + * The spec seems to require these to be methods. All the examples > + * show them this way and it doesn't seem to work if they are not. > + */ > + method = aml_method("_IFT", 0, AML_NOTSERIALIZED); > + aml_append(method, aml_return(aml_int(info->interface_type))); > + aml_append(dev, method); > + method = aml_method("_SRV", 0, AML_NOTSERIALIZED); > + aml_append(method, aml_return(aml_int(version))); > + aml_append(dev, method); > replace these methods with aml_name_decl() as they do not contain any logic > except of returning static value. I'm not sure why, but what you ask doesn't work. These have to be methods, and that is show by the IPMI spec, as the comment above these says. . . . > > @@ -2011,7 +2015,7 @@ build_dsdt(GArray *table_data, GArray *linker, > build_hpet_aml(dsdt); > build_piix4_pm(dsdt);one > build_piix4_isa_bridge(dsdt); > - build_isa_devices_aml(dsdt); > + build_isa_devices_aml(dsdt, pcms->isa_bus); > I'm not sure about adding 'isa_bus' field to PCMachineState, > it might be better to find a ISA bus object internally in > build_isa_devices_aml() and assert if found more than one, > since code assumes that there is only one anyway. I don't see a clean way to find the ISA bus object. Well, I guess you can scan the PCI bus for an ISA bus object, is that what you mean? -corey
On 05/22/2016 03:28 AM, Corey Minyard wrote: > Thanks for all the comments. I didn't know about stubs, as > there's nothing that currently uses it in hw directory, but > it's easy enough to add. I did have two comment below: > > On 05/20/2016 04:53 AM, Igor Mammedov wrote: >> On Thu, 19 May 2016 10:24:01 -0500 >> minyard@acm.org wrote: > . > . > . >> >> + aml_append(dev, aml_name_decl("_STR", aml_string("ipmi_%s", >> + info->interface_name))); >> + aml_append(dev, aml_name_decl("_UID", aml_int(info->uuid))); >> + aml_append(dev, aml_name_decl("_CRS", aml_ipmi_crs(info, resource))); >> + >> + /* >> + * The spec seems to require these to be methods. All the examples >> + * show them this way and it doesn't seem to work if they are not. >> + */ >> + method = aml_method("_IFT", 0, AML_NOTSERIALIZED); >> + aml_append(method, aml_return(aml_int(info->interface_type))); >> + aml_append(dev, method); >> + method = aml_method("_SRV", 0, AML_NOTSERIALIZED); >> + aml_append(method, aml_return(aml_int(version))); >> + aml_append(dev, method); >> replace these methods with aml_name_decl() as they do not contain any logic >> except of returning static value. > > I'm not sure why, but what you ask doesn't work. These have to be > methods, and that is show by the IPMI spec, as the comment above > these says. > > . > . > . > >> @@ -2011,7 +2015,7 @@ build_dsdt(GArray *table_data, GArray *linker, >> build_hpet_aml(dsdt); >> build_piix4_pm(dsdt);one >> build_piix4_isa_bridge(dsdt); >> - build_isa_devices_aml(dsdt); >> + build_isa_devices_aml(dsdt, pcms->isa_bus); >> I'm not sure about adding 'isa_bus' field to PCMachineState, >> it might be better to find a ISA bus object internally in >> build_isa_devices_aml() and assert if found more than one, >> since code assumes that there is only one anyway. > I agree. > I don't see a clean way to find the ISA bus object. Well, I guess > you can scan the PCI bus for an ISA bus object, is that what you > mean? > You can run a QOM query. Please look for object_resolve_path function. Please let me know if you have difficulties with it. Thanks, Marcel > -corey >
On Sat, 21 May 2016 19:28:59 -0500 Corey Minyard <minyard@acm.org> wrote: > Thanks for all the comments. I didn't know about stubs, as > there's nothing that currently uses it in hw directory, but > it's easy enough to add. I did have two comment below: > > On 05/20/2016 04:53 AM, Igor Mammedov wrote: > > On Thu, 19 May 2016 10:24:01 -0500 > > minyard@acm.org wrote: > . > . > . > > > > + aml_append(dev, aml_name_decl("_STR", aml_string("ipmi_%s", > > + info->interface_name))); > > + aml_append(dev, aml_name_decl("_UID", aml_int(info->uuid))); > > + aml_append(dev, aml_name_decl("_CRS", aml_ipmi_crs(info, resource))); > > + > > + /* > > + * The spec seems to require these to be methods. All the examples > > + * show them this way and it doesn't seem to work if they are not. > > + */ > > + method = aml_method("_IFT", 0, AML_NOTSERIALIZED); > > + aml_append(method, aml_return(aml_int(info->interface_type))); > > + aml_append(dev, method); > > + method = aml_method("_SRV", 0, AML_NOTSERIALIZED); > > + aml_append(method, aml_return(aml_int(version))); > > + aml_append(dev, method); > > replace these methods with aml_name_decl() as they do not contain any logic > > except of returning static value. > > I'm not sure why, but what you ask doesn't work. These have to be > methods, and that is show by the IPMI spec, as the comment above > these says. on linux these methods are evaluated by ACPICA core and named constant is equivalent to a method without arguments that returns constant value. It might be worth to investigate why it doesn't work. > -corey
On 05/23/2016 05:01 AM, Igor Mammedov wrote: > On Sat, 21 May 2016 19:28:59 -0500 > Corey Minyard <minyard@acm.org> wrote: > >> Thanks for all the comments. I didn't know about stubs, as >> there's nothing that currently uses it in hw directory, but >> it's easy enough to add. I did have two comment below: >> >> On 05/20/2016 04:53 AM, Igor Mammedov wrote: >>> On Thu, 19 May 2016 10:24:01 -0500 >>> minyard@acm.org wrote: >> . >> . >> . >>> + aml_append(dev, aml_name_decl("_STR", aml_string("ipmi_%s", >>> + info->interface_name))); >>> + aml_append(dev, aml_name_decl("_UID", aml_int(info->uuid))); >>> + aml_append(dev, aml_name_decl("_CRS", aml_ipmi_crs(info, resource))); >>> + >>> + /* >>> + * The spec seems to require these to be methods. All the examples >>> + * show them this way and it doesn't seem to work if they are not. >>> + */ >>> + method = aml_method("_IFT", 0, AML_NOTSERIALIZED); >>> + aml_append(method, aml_return(aml_int(info->interface_type))); >>> + aml_append(dev, method); >>> + method = aml_method("_SRV", 0, AML_NOTSERIALIZED); >>> + aml_append(method, aml_return(aml_int(version))); >>> + aml_append(dev, method); >>> replace these methods with aml_name_decl() as they do not contain any logic >>> except of returning static value. >> I'm not sure why, but what you ask doesn't work. These have to be >> methods, and that is show by the IPMI spec, as the comment above >> these says. > on linux these methods are evaluated by ACPICA core and named constant > is equivalent to a method without arguments that returns constant value. > > It might be worth to investigate why it doesn't work. I just tried this again and it did work. I'm not sure why it didn't work before, if it was a change in Linux or my error. However, the latest IPMI spec has the following text: Note: _IFT and _SRV, following, have been reserved in ACPI 3.0 as names for control methods defined for SPMI Just because it works in Linux doesn't mean it will work on other OSes. Wouldn't it be safer to use a method here? -corey >> -corey
On Mon, 23 May 2016 07:42:32 -0500 Corey Minyard <minyard@acm.org> wrote: > On 05/23/2016 05:01 AM, Igor Mammedov wrote: > > On Sat, 21 May 2016 19:28:59 -0500 > > Corey Minyard <minyard@acm.org> wrote: > > > >> Thanks for all the comments. I didn't know about stubs, as > >> there's nothing that currently uses it in hw directory, but > >> it's easy enough to add. I did have two comment below: > >> > >> On 05/20/2016 04:53 AM, Igor Mammedov wrote: > >>> On Thu, 19 May 2016 10:24:01 -0500 > >>> minyard@acm.org wrote: > >> . > >> . > >> . > >>> + aml_append(dev, aml_name_decl("_STR", aml_string("ipmi_%s", > >>> + info->interface_name))); > >>> + aml_append(dev, aml_name_decl("_UID", aml_int(info->uuid))); > >>> + aml_append(dev, aml_name_decl("_CRS", aml_ipmi_crs(info, resource))); > >>> + > >>> + /* > >>> + * The spec seems to require these to be methods. All the examples > >>> + * show them this way and it doesn't seem to work if they are not. > >>> + */ > >>> + method = aml_method("_IFT", 0, AML_NOTSERIALIZED); > >>> + aml_append(method, aml_return(aml_int(info->interface_type))); > >>> + aml_append(dev, method); > >>> + method = aml_method("_SRV", 0, AML_NOTSERIALIZED); > >>> + aml_append(method, aml_return(aml_int(version))); > >>> + aml_append(dev, method); > >>> replace these methods with aml_name_decl() as they do not contain any logic > >>> except of returning static value. > >> I'm not sure why, but what you ask doesn't work. These have to be > >> methods, and that is show by the IPMI spec, as the comment above > >> these says. > > on linux these methods are evaluated by ACPICA core and named constant > > is equivalent to a method without arguments that returns constant value. > > > > It might be worth to investigate why it doesn't work. > > I just tried this again and it did work. I'm not sure why it didn't work > before, if it was a change in Linux or my error. > > However, the latest IPMI spec has the following text: > > Note: _IFT and _SRV, following, have been reserved in ACPI 3.0 as names for > control methods defined for SPMI > > Just because it works in Linux doesn't mean it will work on other OSes. > Wouldn't it be safer to use a method here? I think it should be safe to use named constant instead of method. Spec often uses terms method/object interchangeably for argument-less methods and we use it to a make simpler/smaller AML bytecode. So far there weren't any issues caused by it either on linux/windows guests. PS: ACPI6.0 spec uses named object instead of method for _IFT as an example. > -corey > > >> -corey >
On 05/23/2016 04:05 AM, Marcel Apfelbaum wrote: > On 05/22/2016 03:28 AM, Corey Minyard wrote: >> Thanks for all the comments. I didn't know about stubs, as >> there's nothing that currently uses it in hw directory, but >> it's easy enough to add. I did have two comment below: >> >> On 05/20/2016 04:53 AM, Igor Mammedov wrote: >>> On Thu, 19 May 2016 10:24:01 -0500 >>> minyard@acm.org wrote: >> . >> . >> . >>> >>> + aml_append(dev, aml_name_decl("_STR", aml_string("ipmi_%s", >>> + info->interface_name))); >>> + aml_append(dev, aml_name_decl("_UID", aml_int(info->uuid))); >>> + aml_append(dev, aml_name_decl("_CRS", aml_ipmi_crs(info, >>> resource))); >>> + >>> + /* >>> + * The spec seems to require these to be methods. All the >>> examples >>> + * show them this way and it doesn't seem to work if they are not. >>> + */ >>> + method = aml_method("_IFT", 0, AML_NOTSERIALIZED); >>> + aml_append(method, aml_return(aml_int(info->interface_type))); >>> + aml_append(dev, method); >>> + method = aml_method("_SRV", 0, AML_NOTSERIALIZED); >>> + aml_append(method, aml_return(aml_int(version))); >>> + aml_append(dev, method); >>> replace these methods with aml_name_decl() as they do not contain >>> any logic >>> except of returning static value. >> >> I'm not sure why, but what you ask doesn't work. These have to be >> methods, and that is show by the IPMI spec, as the comment above >> these says. >> >> . >> . >> . >> >>> @@ -2011,7 +2015,7 @@ build_dsdt(GArray *table_data, GArray *linker, >>> build_hpet_aml(dsdt); >>> build_piix4_pm(dsdt);one >>> build_piix4_isa_bridge(dsdt); >>> - build_isa_devices_aml(dsdt); >>> + build_isa_devices_aml(dsdt, pcms->isa_bus); >>> I'm not sure about adding 'isa_bus' field to PCMachineState, >>> it might be better to find a ISA bus object internally in >>> build_isa_devices_aml() and assert if found more than one, >>> since code assumes that there is only one anyway. >> > > I agree. > >> I don't see a clean way to find the ISA bus object. Well, I guess >> you can scan the PCI bus for an ISA bus object, is that what you >> mean? >> > > You can run a QOM query. Please look for object_resolve_path function. > Please let me know if you have difficulties with it. > > Thanks, > Marcel > Thanks for that info. I looked at that, and it works for a single object, but I don't see a way to make this work if there is more than one IPMI device defined. I changed to the following code, which seems to work ok. You pass in the ISA bus (or the SMBus in the I2C case). void build_acpi_ipmi_devices(Aml *scope, BusState *bus) { BusChild *kid; QTAILQ_FOREACH(kid, &bus->children, sibling) { DeviceState *dev = kid->child; Object *obj = object_dynamic_cast(OBJECT(dev), TYPE_IPMI_INTERFACE); IPMIInterface *ii; IPMIInterfaceClass *iic; IPMIFwInfo info; if (!obj) { continue; } ii = IPMI_INTERFACE(obj); iic = IPMI_INTERFACE_GET_CLASS(obj); memset(&info, 0, sizeof(info)); iic->get_fwinfo(ii, &info); aml_append(scope, acpi_ipmi_device(&info)); } }
On 05/23/2016 04:39 PM, Corey Minyard wrote: > On 05/23/2016 04:05 AM, Marcel Apfelbaum wrote: >> On 05/22/2016 03:28 AM, Corey Minyard wrote: >>> Thanks for all the comments. I didn't know about stubs, as >>> there's nothing that currently uses it in hw directory, but >>> it's easy enough to add. I did have two comment below: >>> >>> On 05/20/2016 04:53 AM, Igor Mammedov wrote: >>>> On Thu, 19 May 2016 10:24:01 -0500 >>>> minyard@acm.org wrote: >>> . >>> . >>> . >>>> >>>> + aml_append(dev, aml_name_decl("_STR", aml_string("ipmi_%s", >>>> + info->interface_name))); >>>> + aml_append(dev, aml_name_decl("_UID", aml_int(info->uuid))); >>>> + aml_append(dev, aml_name_decl("_CRS", aml_ipmi_crs(info, resource))); >>>> + >>>> + /* >>>> + * The spec seems to require these to be methods. All the examples >>>> + * show them this way and it doesn't seem to work if they are not. >>>> + */ >>>> + method = aml_method("_IFT", 0, AML_NOTSERIALIZED); >>>> + aml_append(method, aml_return(aml_int(info->interface_type))); >>>> + aml_append(dev, method); >>>> + method = aml_method("_SRV", 0, AML_NOTSERIALIZED); >>>> + aml_append(method, aml_return(aml_int(version))); >>>> + aml_append(dev, method); >>>> replace these methods with aml_name_decl() as they do not contain any logic >>>> except of returning static value. >>> >>> I'm not sure why, but what you ask doesn't work. These have to be >>> methods, and that is show by the IPMI spec, as the comment above >>> these says. >>> >>> . >>> . >>> . >>> >>>> @@ -2011,7 +2015,7 @@ build_dsdt(GArray *table_data, GArray *linker, >>>> build_hpet_aml(dsdt); >>>> build_piix4_pm(dsdt);one >>>> build_piix4_isa_bridge(dsdt); >>>> - build_isa_devices_aml(dsdt); >>>> + build_isa_devices_aml(dsdt, pcms->isa_bus); >>>> I'm not sure about adding 'isa_bus' field to PCMachineState, >>>> it might be better to find a ISA bus object internally in >>>> build_isa_devices_aml() and assert if found more than one, >>>> since code assumes that there is only one anyway. >>> >> >> I agree. >> >>> I don't see a clean way to find the ISA bus object. Well, I guess >>> you can scan the PCI bus for an ISA bus object, is that what you >>> mean? >>> >> >> You can run a QOM query. Please look for object_resolve_path function. >> Please let me know if you have difficulties with it. >> >> Thanks, >> Marcel >> > > Thanks for that info. > > I looked at that, and it works for a single object, but I don't see a way to > make this work if there is more than one IPMI device defined. I was under impression that you are looking for a way to find the isa_bus. Since we have only a single isa_bus you can use object_resolve_path instead of adding the isa_bus to the pc machine. I didn't refer to the IPMI device, sorry if I was not clear enough. Thanks, Marcel > > I changed to the following code, which seems to work ok. You pass in the > ISA bus (or the SMBus in the I2C case). > > void build_acpi_ipmi_devices(Aml *scope, BusState *bus) > { > BusChild *kid; > > QTAILQ_FOREACH(kid, &bus->children, sibling) { > DeviceState *dev = kid->child; > Object *obj = object_dynamic_cast(OBJECT(dev), TYPE_IPMI_INTERFACE); > IPMIInterface *ii; > IPMIInterfaceClass *iic; > IPMIFwInfo info; > > if (!obj) { > continue; > } > > ii = IPMI_INTERFACE(obj); > iic = IPMI_INTERFACE_GET_CLASS(obj); > memset(&info, 0, sizeof(info)); > iic->get_fwinfo(ii, &info); > aml_append(scope, acpi_ipmi_device(&info)); > } > }
On 05/23/2016 08:51 AM, Marcel Apfelbaum wrote: > On 05/23/2016 04:39 PM, Corey Minyard wrote: >> On 05/23/2016 04:05 AM, Marcel Apfelbaum wrote: >>> On 05/22/2016 03:28 AM, Corey Minyard wrote: >>>> Thanks for all the comments. I didn't know about stubs, as >>>> there's nothing that currently uses it in hw directory, but >>>> it's easy enough to add. I did have two comment below: >>>> >>>> On 05/20/2016 04:53 AM, Igor Mammedov wrote: >>>>> On Thu, 19 May 2016 10:24:01 -0500 >>>>> minyard@acm.org wrote: >>>> . >>>> . >>>> . >>>>> >>>>> + aml_append(dev, aml_name_decl("_STR", aml_string("ipmi_%s", >>>>> + info->interface_name))); >>>>> + aml_append(dev, aml_name_decl("_UID", aml_int(info->uuid))); >>>>> + aml_append(dev, aml_name_decl("_CRS", aml_ipmi_crs(info, >>>>> resource))); >>>>> + >>>>> + /* >>>>> + * The spec seems to require these to be methods. All the >>>>> examples >>>>> + * show them this way and it doesn't seem to work if they are >>>>> not. >>>>> + */ >>>>> + method = aml_method("_IFT", 0, AML_NOTSERIALIZED); >>>>> + aml_append(method, aml_return(aml_int(info->interface_type))); >>>>> + aml_append(dev, method); >>>>> + method = aml_method("_SRV", 0, AML_NOTSERIALIZED); >>>>> + aml_append(method, aml_return(aml_int(version))); >>>>> + aml_append(dev, method); >>>>> replace these methods with aml_name_decl() as they do not contain >>>>> any logic >>>>> except of returning static value. >>>> >>>> I'm not sure why, but what you ask doesn't work. These have to be >>>> methods, and that is show by the IPMI spec, as the comment above >>>> these says. >>>> >>>> . >>>> . >>>> . >>>> >>>>> @@ -2011,7 +2015,7 @@ build_dsdt(GArray *table_data, GArray *linker, >>>>> build_hpet_aml(dsdt); >>>>> build_piix4_pm(dsdt);one >>>>> build_piix4_isa_bridge(dsdt); >>>>> - build_isa_devices_aml(dsdt); >>>>> + build_isa_devices_aml(dsdt, pcms->isa_bus); >>>>> I'm not sure about adding 'isa_bus' field to PCMachineState, >>>>> it might be better to find a ISA bus object internally in >>>>> build_isa_devices_aml() and assert if found more than one, >>>>> since code assumes that there is only one anyway. >>>> >>> >>> I agree. >>> >>>> I don't see a clean way to find the ISA bus object. Well, I guess >>>> you can scan the PCI bus for an ISA bus object, is that what you >>>> mean? >>>> >>> >>> You can run a QOM query. Please look for object_resolve_path function. >>> Please let me know if you have difficulties with it. >>> >>> Thanks, >>> Marcel >>> >> >> Thanks for that info. >> >> I looked at that, and it works for a single object, but I don't see a >> way to >> make this work if there is more than one IPMI device defined. > > > I was under impression that you are looking for a way to find the > isa_bus. > Since we have only a single isa_bus you can use object_resolve_path > instead > of adding the isa_bus to the pc machine. > I didn't refer to the IPMI device, sorry if I was not clear enough. > Got it, thanks. I was over thinking it. -corey > Thanks, > Marcel > >> >> I changed to the following code, which seems to work ok. You pass in >> the >> ISA bus (or the SMBus in the I2C case). >> >> void build_acpi_ipmi_devices(Aml *scope, BusState *bus) >> { >> BusChild *kid; >> >> QTAILQ_FOREACH(kid, &bus->children, sibling) { >> DeviceState *dev = kid->child; >> Object *obj = object_dynamic_cast(OBJECT(dev), >> TYPE_IPMI_INTERFACE); >> IPMIInterface *ii; >> IPMIInterfaceClass *iic; >> IPMIFwInfo info; >> >> if (!obj) { >> continue; >> } >> >> ii = IPMI_INTERFACE(obj); >> iic = IPMI_INTERFACE_GET_CLASS(obj); >> memset(&info, 0, sizeof(info)); >> iic->get_fwinfo(ii, &info); >> aml_append(scope, acpi_ipmi_device(&info)); >> } >> } >
On 20/05/2016 11:53, Igor Mammedov wrote: >> > diff --git a/hw/acpi/noipmi.c b/hw/acpi/noipmi.c >> > new file mode 100644 >> > index 0000000..dd7590d >> > --- /dev/null >> > +++ b/hw/acpi/noipmi.c > move this to .a/stubs/ > Don't overuse stubs. The stubs library is mostly to share code between QEMU and the tools. What Corey did is more similar to what hw/ already does in other places. Unfortunately, weak symbols are not portable. :( Paolo
On 05/23/2016 01:06 PM, Paolo Bonzini wrote: > > On 20/05/2016 11:53, Igor Mammedov wrote: >>>> diff --git a/hw/acpi/noipmi.c b/hw/acpi/noipmi.c >>>> new file mode 100644 >>>> index 0000000..dd7590d >>>> --- /dev/null >>>> +++ b/hw/acpi/noipmi.c >> move this to .a/stubs/ >> > Don't overuse stubs. The stubs library is mostly to share code between > QEMU and the tools. What Corey did is more similar to what hw/ already > does in other places. Should I change it back? > Unfortunately, weak symbols are not portable. :( Yeah, that's really a pain. -corey > Paolo
On Mon, 23 May 2016 20:06:57 +0200 Paolo Bonzini <pbonzini@redhat.com> wrote: > On 20/05/2016 11:53, Igor Mammedov wrote: > >> > diff --git a/hw/acpi/noipmi.c b/hw/acpi/noipmi.c > >> > new file mode 100644 > >> > index 0000000..dd7590d > >> > --- /dev/null > >> > +++ b/hw/acpi/noipmi.c > > move this to .a/stubs/ > > > > Don't overuse stubs. The stubs library is mostly to share code between > QEMU and the tools. What Corey did is more similar to what hw/ already > does in other places. So what's the rule when one should use stubs or not? > > Unfortunately, weak symbols are not portable. :( > > Paolo
On 23/05/2016 21:25, Corey Minyard wrote: >> Don't overuse stubs. The stubs library is mostly to share code between >> QEMU and the tools. What Corey did is more similar to what hw/ already >> does in other places. > > Should I change it back? Yes, please. Sorry. Thanks, Paolo
On 24/05/2016 09:17, Igor Mammedov wrote: > > Don't overuse stubs. The stubs library is mostly to share code between > > QEMU and the tools. What Corey did is more similar to what hw/ already > > does in other places. > > So what's the rule when one should use stubs or not? If you have a config symbol to base your choice on, do not use stubs. Paolo
diff --git a/hw/acpi/Makefile.objs b/hw/acpi/Makefile.objs index faee86c..b8d5c84 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 +common-obj-$(call land,$(CONFIG_ACPI),$(call lnot,$(CONFIG_IPMI))) += noipmi.o diff --git a/hw/acpi/ipmi.c b/hw/acpi/ipmi.c new file mode 100644 index 0000000..3d61550 --- /dev/null +++ b/hw/acpi/ipmi.c @@ -0,0 +1,114 @@ +/* + * 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, const char *scope) +{ + Aml *crs = aml_resource_template(); + uint8_t regspacing = info->register_spacing; + + /* + * The base address is fixed and cannot change. That may be different + * if someone does PCI, but we aren't there yet. + */ + switch (info->memspace) { + case IPMI_MEMSPACE_IO: + aml_append(crs, aml_io(AML_DECODE16, info->base_address, + info->base_address + info->register_length - 1, + regspacing, info->register_length)); + break; + case IPMI_MEMSPACE_MEM32: + aml_append(crs, + aml_dword_memory(AML_POS_DECODE, + AML_MIN_FIXED, AML_MAX_FIXED, + AML_NON_CACHEABLE, AML_READ_WRITE, + 0xffffffff, + info->base_address, + info->base_address + info->register_length - 1, + regspacing, info->register_length)); + break; + case IPMI_MEMSPACE_MEM64: + aml_append(crs, + aml_qword_memory(AML_POS_DECODE, + AML_MIN_FIXED, AML_MAX_FIXED, + AML_NON_CACHEABLE, AML_READ_WRITE, + 0xffffffffffffffffULL, + info->base_address, + info->base_address + info->register_length - 1, + regspacing, info->register_length)); + break; + case IPMI_MEMSPACE_SMBUS: + aml_append(crs, aml_return(aml_int(info->base_address))); + break; + default: + abort(); + } + + if (info->interrupt_number) { + aml_append(crs, aml_irq_no_flags(info->interrupt_number)); + } + + return crs; +} + +static void ipmi_encode_one_acpi(Aml *table, IPMIFwInfo *info, + const char *resource) +{ + Aml *dev, *method; + uint16_t version = ((info->ipmi_spec_major_revision << 8) + | (info->ipmi_spec_minor_revision << 4)); + + dev = aml_device("MI0"); + aml_append(dev, aml_name_decl("_HID", aml_eisaid("IPI0001"))); + aml_append(dev, aml_name_decl("_STR", aml_string("ipmi_%s", + info->interface_name))); + aml_append(dev, aml_name_decl("_UID", aml_int(info->uuid))); + aml_append(dev, aml_name_decl("_CRS", aml_ipmi_crs(info, resource))); + + /* + * The spec seems to require these to be methods. All the examples + * show them this way and it doesn't seem to work if they are not. + */ + method = aml_method("_IFT", 0, AML_NOTSERIALIZED); + aml_append(method, aml_return(aml_int(info->interface_type))); + aml_append(dev, method); + method = aml_method("_SRV", 0, AML_NOTSERIALIZED); + aml_append(method, aml_return(aml_int(version))); + aml_append(dev, method); + + aml_append(table, dev); +} + +void acpi_add_ipmi(Aml *table, BusState *bus, const char *resource) +{ + BusChild *kid; + + QTAILQ_FOREACH(kid, &bus->children, sibling) { + DeviceState *dev = kid->child; + Object *obj = object_dynamic_cast(OBJECT(dev), TYPE_IPMI_INTERFACE); + IPMIInterface *ii; + IPMIInterfaceClass *iic; + IPMIFwInfo info; + + if (!obj) { + continue; + } + + ii = IPMI_INTERFACE(obj); + iic = IPMI_INTERFACE_GET_CLASS(obj); + memset(&info, 0, sizeof(info)); + iic->get_fwinfo(ii, &info); + ipmi_encode_one_acpi(table, &info, resource); + } +} diff --git a/hw/acpi/noipmi.c b/hw/acpi/noipmi.c new file mode 100644 index 0000000..dd7590d --- /dev/null +++ b/hw/acpi/noipmi.c @@ -0,0 +1,14 @@ +/* + * 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 "hw/acpi/ipmi.h" + +void acpi_add_ipmi(Aml *table, BusState *bus, const char *resource) +{ +} diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c index 279f0d7..101082d 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,7 @@ static Aml *build_com_device_aml(uint8_t uid) return dev; } -static void build_isa_devices_aml(Aml *table) +static void build_isa_devices_aml(Aml *table, ISABus *isa_bus) { ISADevice *fdc = pc_find_fdc0(); @@ -1461,6 +1463,8 @@ static void build_isa_devices_aml(Aml *table) aml_append(scope, build_com_device_aml(1)); aml_append(scope, build_com_device_aml(2)); + acpi_add_ipmi(scope, BUS(isa_bus), "\\_SB.PCI0.ISA"); + aml_append(table, scope); } @@ -2011,7 +2015,7 @@ build_dsdt(GArray *table_data, GArray *linker, build_hpet_aml(dsdt); build_piix4_pm(dsdt); build_piix4_isa_bridge(dsdt); - build_isa_devices_aml(dsdt); + build_isa_devices_aml(dsdt, pcms->isa_bus); build_piix4_pci_hotplug(dsdt); build_piix4_pci0_int(dsdt); } else { @@ -2039,7 +2043,7 @@ build_dsdt(GArray *table_data, GArray *linker, build_hpet_aml(dsdt); build_q35_isa_bridge(dsdt); - build_isa_devices_aml(dsdt); + build_isa_devices_aml(dsdt, pcms->isa_bus); build_q35_pci0_int(dsdt); } diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c index 7f50116..2e5ff45 100644 --- a/hw/i386/pc_piix.c +++ b/hw/i386/pc_piix.c @@ -185,6 +185,7 @@ static void pc_init1(MachineState *machine, &error_abort); no_hpet = 1; } + pcms->isa_bus = isa_bus; isa_bus_irqs(isa_bus, gsi); if (kvm_pic_in_kernel()) { diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c index 04aae89..027655b 100644 --- a/hw/i386/pc_q35.c +++ b/hw/i386/pc_q35.c @@ -190,6 +190,7 @@ static void pc_q35_init(MachineState *machine) ICH9_LPC_NB_PIRQS); pci_bus_set_route_irq_fn(host_bus, ich9_route_intx_pin_to_irq); isa_bus = ich9_lpc->isa_bus; + pcms->isa_bus = isa_bus; /*end early*/ isa_bus_irqs(isa_bus, gsi); diff --git a/include/hw/acpi/ipmi.h b/include/hw/acpi/ipmi.h new file mode 100644 index 0000000..67d6e2f --- /dev/null +++ b/include/hw/acpi/ipmi.h @@ -0,0 +1,22 @@ +/* + * QEMU IPMI ACPI handling + * + * Copyright (c) 2015 Corey Minyard <cminyard@mvista.com> + * + * This work is licensed under the terms of the GNU GPL, version 2 or later. + * See the COPYING file in the top-level directory. + */ +#ifndef HW_ACPI_IPMI_H +#define HW_ACPI_IPMI_H + +#include "qemu/osdep.h" +#include "hw/acpi/aml-build.h" + +/* + * Add ACPI IPMI entries for all registered IPMI devices whose parent + * bus matches the given bus. The resource is the ACPI resource that + * contains the IPMI device, this is required for the I2C CRS. + */ +void acpi_add_ipmi(Aml *table, BusState *bus, const char *resource); + +#endif /* HW_ACPI_IPMI_H */ diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h index 96f0b66..aa8bc97 100644 --- a/include/hw/i386/pc.h +++ b/include/hw/i386/pc.h @@ -51,6 +51,7 @@ struct PCMachineState { HotplugHandler *acpi_dev; ISADevice *rtc; PCIBus *bus; + ISABus *isa_bus; FWCfgState *fw_cfg; /* Configuration options: */