Hi, On 3/8/19 12:42 PM, Shameer Kolothum wrote: > From: Samuel Ortiz <sa...@linux.intel.com> > > The ACPI Generic Event Device (GED) is a hardware-reduced specific > device that handles all platform events, including the hotplug ones. > This patch generate the AML code that defines GEDs. s/generate/generates > Platforms need to specify their own GedEvent array to describe what kind > of events they want to support through GED. The build_ged_aml routine > takes a GedEvent array that maps a specific GED event to an IRQ number. > Then we use that array to build both the _CRS and the _EVT section > of the GED device. > > This is in preparation for making use of GED for ARM/virt > platform and for now supports only memory hotplug. > > Signed-off-by: Samuel Ortiz <sa...@linux.intel.com> > Signed-off-by: Sebastien Boeuf <sebastien.bo...@intel.com> > Signed-off-by: Shameer Kolothum <shameerali.kolothum.th...@huawei.com> > --- > hw/acpi/Makefile.objs | 1 + > hw/acpi/ged.c | 198 > ++++++++++++++++++++++++++++++++++++++++++++++++++ > include/hw/acpi/ged.h | 61 ++++++++++++++++ > 3 files changed, 260 insertions(+) > create mode 100644 hw/acpi/ged.c > create mode 100644 include/hw/acpi/ged.h > > diff --git a/hw/acpi/Makefile.objs b/hw/acpi/Makefile.objs > index 2d46e37..6cf572b 100644 > --- a/hw/acpi/Makefile.objs > +++ b/hw/acpi/Makefile.objs > @@ -6,6 +6,7 @@ common-obj-$(CONFIG_ACPI_MEMORY_HOTPLUG) += memory_hotplug.o > common-obj-$(CONFIG_ACPI_CPU_HOTPLUG) += cpu.o > common-obj-$(CONFIG_ACPI_NVDIMM) += nvdimm.o > common-obj-$(CONFIG_ACPI_VMGENID) += vmgenid.o > +common-obj-$(CONFIG_ACPI_HW_REDUCED) += ged.o > common-obj-$(call lnot,$(CONFIG_ACPI_X86)) += acpi-stub.o > > common-obj-y += acpi_interface.o > diff --git a/hw/acpi/ged.c b/hw/acpi/ged.c > new file mode 100644 > index 0000000..5076fbc > --- /dev/null > +++ b/hw/acpi/ged.c > @@ -0,0 +1,198 @@ > +/* > + * > + * Copyright (c) 2018 Intel Corporation > + * > + * This program is free software; you can redistribute it and/or modify it > + * under the terms and conditions of the GNU General Public License, > + * version 2 or later, as published by the Free Software Foundation. > + * > + * This program is distributed in the hope it will be useful, but WITHOUT > + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or > + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for > + * more details. > + * > + * You should have received a copy of the GNU General Public License along > with > + * this program. If not, see <http://www.gnu.org/licenses/>. > + */ > + > +#include "hw/acpi/ged.h" Add a comment giving a short description of what is the GED, available since 6.1 spec? > + > +static hwaddr ged_io_base; Couldn't we add a comment such as, "read by the GED _EVT AML dynamic method"? > +static uint64_t ged_read(void *opaque, hwaddr addr, unsigned size) > +{ > + uint64_t val = 0; > + GEDState *ged_st = opaque; > + > + switch (addr) { > + case ACPI_GED_IRQ_SEL_OFFSET: > + /* Read the selector value and reset it */ > + qemu_mutex_lock(&ged_st->lock); > + val = ged_st->sel; > + ged_st->sel = ACPI_GED_IRQ_SEL_INIT; using 0 instead of ACPI_GED_IRQ_SEL_INIT may be clearer? > + qemu_mutex_unlock(&ged_st->lock); > + break; > + default: > + break; > + } > + > + return val; > +} > + > +/* Nothing is expected to be written to the GED memory region */ > +static void ged_write(void *opaque, hwaddr addr, uint64_t data, > + unsigned int size) > +{ > +} > + > +static const MemoryRegionOps ged_ops = { > + .read = ged_read, > + .write = ged_write, > + .endianness = DEVICE_LITTLE_ENDIAN, > + .valid = { > + .min_access_size = 4, > + .max_access_size = 4, > + }, > +}; > + > +void acpi_ged_init(MemoryRegion *as, Object *owner, GEDState *ged_st, > + hwaddr base_addr, uint32_t ged_irq) Maybe directly pass the qemu_irq* and store it into the GEDState > +{ > + > + assert(!ged_io_base); > + > + ged_io_base = base_addr; > + ged_st->irq = ged_irq; > + qemu_mutex_init(&ged_st->lock); > + memory_region_init_io(&ged_st->io, owner, &ged_ops, ged_st, > + "acpi-ged-event", ACPI_GED_REG_LEN); > + memory_region_add_subregion(as, base_addr, &ged_st->io); > +} > + > +void acpi_ged_event(GEDState *ged_st, qemu_irq *irq, uint32_t ged_irq_sel) ... then you wouldn't need the irq parameter here? > +{ > + /* > + * Set the GED IRQ selector to the expected device type value. This > + * way, the ACPI method will be able to trigger the right code based > + * on a unique IRQ. > + */ > + qemu_mutex_lock(&ged_st->lock); > + ged_st->sel |= ged_irq_sel; is it allowed to have several events logged simultaneously? _EVT args is described as "EventNumber: An integer indicating the event number (GSIV number) of the current event" > + qemu_mutex_unlock(&ged_st->lock); > + > + /* Trigger the event by sending an interrupt to the guest. */ > + qemu_irq_pulse(irq[ged_st->irq]); Is it valid to consider the irq always is edge sensitive? I understand from the spec the irq mode may be platform dependent. > +} > + > +static Aml *ged_event_aml(GedEvent *event) > +{ > + > + if (!event) { > + return NULL; > + } > + > + switch (event->event) { > + case GED_MEMORY_HOTPLUG: > + /* We run a complete memory SCAN when getting a memory hotplug event > */ > + return aml_call0("\\_SB.MHPC." MEMORY_SLOT_SCAN_METHOD); use MEMORY_DEVICES_CONTAINER now exposed in include/hw/acpi/memory_hotplug.h? > + default: > + break; > + } > + > + return NULL; > +} > + > +void build_ged_aml(Aml *table, const char *name, uint32_t ged_irq, > + GedEvent *events, uint32_t events_size, > + AmlRegionSpace rs) > +{ > + Aml *crs = aml_resource_template(); > + Aml *evt, *field; > + Aml *zero = aml_int(0); > + Aml *dev = aml_device("%s", name); > + Aml *irq_sel = aml_local(0); > + Aml *isel = aml_name(AML_GED_IRQ_SEL); > + uint32_t i; > + > + if (!ged_io_base) { > + return; > + } > + > + /* _CRS interrupt */ > + aml_append(crs, aml_interrupt(AML_CONSUMER, AML_EDGE, AML_ACTIVE_HIGH, > + AML_EXCLUSIVE, &ged_irq, 1)); > + /* > + * For each GED event we: > + * - Add an interrupt to the CRS section. > + * - Add a conditional block for each event, inside a while loop. > + * This is semantically equivalent to a switch/case implementation. > + */ > + evt = aml_method("_EVT", 1, AML_SERIALIZED); > + { > + Aml *ged_aml; > + Aml *if_ctx; > + > + /* Local0 = ISEL */ > + aml_append(evt, aml_store(isel, irq_sel)); > + > + /* > + * Here we want to call a method for each supported GED event type. > + * The resulting ASL code looks like: > + * > + * Local0 = ISEL > + * If ((Local0 & irq0) == irq0) > + * { > + * MethodEvent0() > + * } > + * > + * If ((Local0 & irq1) == irq1) > + * { > + * MethodEvent1() > + * } > + * > + * If ((Local0 & irq2) == irq2) > + * { > + * MethodEvent2() > + * } > + */ > + > + for (i = 0; i < events_size; i++) { > + ged_aml = ged_event_aml(&events[i]); > + if (!ged_aml) { > + continue; > + } > + > + /* If ((Local1 == irq))*/ > + if_ctx = aml_if(aml_equal(aml_and(irq_sel, > aml_int(events[i].selector), NULL), aml_int(events[i].selector))); > + { > + /* AML for this specific type of event */ > + aml_append(if_ctx, ged_aml); > + } > + > + /* > + * We append the first if to the while context. We append the first "if" to the "while" context. > + * Other ifs will be elseifs. > + */ > + aml_append(evt, if_ctx); > + } > + } > + > + aml_append(dev, aml_name_decl("_HID", aml_string("ACPI0013"))); > + aml_append(dev, aml_name_decl("_UID", zero)); can't we use something different from 0 like a aml_string("GED")? > + aml_append(dev, aml_name_decl("_CRS", crs)); > + > + /* Append IO region */ > + aml_append(dev, aml_operation_region(AML_GED_IRQ_REG, rs, > + aml_int(ged_io_base + ACPI_GED_IRQ_SEL_OFFSET), > + ACPI_GED_IRQ_SEL_LEN)); > + field = aml_field(AML_GED_IRQ_REG, AML_DWORD_ACC, AML_NOLOCK, > + AML_WRITE_AS_ZEROS); > + aml_append(field, aml_named_field(AML_GED_IRQ_SEL, > + ACPI_GED_IRQ_SEL_LEN * 8)); > + aml_append(dev, field); > + > + /* Append _EVT method */ > + aml_append(dev, evt); > + > + aml_append(table, dev); > +} Spec says "The platform declare its support for the GED, and query whether an OS supports it, via the _OSC method". \_SB._OSC. I fail to see that method defined? > diff --git a/include/hw/acpi/ged.h b/include/hw/acpi/ged.h > new file mode 100644 > index 0000000..60689b0 > --- /dev/null > +++ b/include/hw/acpi/ged.h > @@ -0,0 +1,61 @@ > +/* > + * > + * Copyright (c) 2018 Intel Corporation > + * > + * This program is free software; you can redistribute it and/or modify it > + * under the terms and conditions of the GNU General Public License, > + * version 2 or later, as published by the Free Software Foundation. > + * > + * This program is distributed in the hope it will be useful, but WITHOUT > + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or > + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for > + * more details. > + * > + * You should have received a copy of the GNU General Public License along > with > + * this program. If not, see <http://www.gnu.org/licenses/>. > + */ > + > +#ifndef HW_ACPI_GED_H > +#define HW_ACPI_GED_H > + > +#include "qemu/osdep.h" > +#include "exec/memory.h" > +#include "hw/acpi/aml-build.h" > +#include "hw/acpi/memory_hotplug.h" > + > +#define ACPI_GED_IRQ_SEL_OFFSET 0x0 > +#define ACPI_GED_IRQ_SEL_LEN 0x4 > +#define ACPI_GED_IRQ_SEL_INIT 0x0 > +#define ACPI_GED_IRQ_SEL_MEM 0x1 > +#define ACPI_GED_REG_LEN 0x4 > + > +#define GED_DEVICE "GED" > +#define AML_GED_IRQ_REG "IREG" > +#define AML_GED_IRQ_SEL "ISEL" > + > +typedef struct Aml Aml; > + > +typedef enum { > + GED_MEMORY_HOTPLUG = 1, > +} GedEventType; > + > +typedef struct GedEvent { > + uint32_t selector;Add a comment saying this corresponds to GSIV > number? > + GedEventType event; > +} GedEvent; > + > +typedef struct GEDState { > + MemoryRegion io; > + uint32_t sel; > + uint32_t irq; why not storing the qemu_irq* directly? > + QemuMutex lock; > +} GEDState; > + > +void acpi_ged_init(MemoryRegion *as, Object *owner, GEDState *ged_st, > + hwaddr base_addr, uint32_t ged_irq); > +void acpi_ged_event(GEDState *ged_st, qemu_irq *irq, uint32_t ged_irq_sel); > +void build_ged_aml(Aml *table, const char* name, uint32_t ged_irq, > + GedEvent *events, uint32_t events_size, > + AmlRegionSpace rs); > + > +#endif >
Thanks Eric
