[v2,09/11] hw/acpi: Add ACPI Generic Event Device Support

Commit Message

Shameerali Kolothum Thodi March 8, 2019, 11:42 a.m. UTC

From: Samuel Ortiz <[e-mail]>

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.
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 <[e-mail]>
Signed-off-by: Sebastien Boeuf <[e-mail]>
Signed-off-by: Shameer Kolothum <[e-mail]>
---
 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

Comments

Eric Auger March 11, 2019, 8:23 p.m. UTC | #1

Hi,

On 3/8/19 12:42 PM, Shameer Kolothum wrote:
> From: Samuel Ortiz <[e-mail]>
> 
> 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 <[e-mail]>
> Signed-off-by: Sebastien Boeuf <[e-mail]>
> Signed-off-by: Shameer Kolothum <[e-mail]>
> ---
>  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

Shameerali Kolothum Thodi March 14, 2019, 11:33 a.m. UTC | #2

Hi Eric,

> -----Original Message-----
> From: Auger Eric [mailto:[e-mail]]
> Sent: 11 March 2019 20:24
> To: Shameerali Kolothum Thodi <[e-mail]>;
> [e-mail]; [e-mail]; [e-mail];
> [e-mail]; [e-mail];
> [e-mail]; [e-mail]
> Cc: Linuxarm <[e-mail]>; xuwei (O) <[e-mail]>
> Subject: Re: [PATCH v2 09/11] hw/acpi: Add ACPI Generic Event Device Support
> 
> Hi,
> 
> On 3/8/19 12:42 PM, Shameer Kolothum wrote:
> > From: Samuel Ortiz <[e-mail]>
> >
> > 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 <[e-mail]>
> > Signed-off-by: Sebastien Boeuf <[e-mail]>
> > Signed-off-by: Shameer Kolothum <[e-mail]>
> > ---
> >  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?

Sure.

> > +
> > +static hwaddr ged_io_base;
>  Couldn't we add a comment such as, "read by the GED _EVT AML dynamic
> method"?


Ok

> > +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?

I will check that.

> > +        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

Yes, I think that makes sense.

> > +{
> > +
> > +    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?

Right.

> > +{
> > +    /*
> > +     * 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"

Ok. Spec doesn’t say it can hold multiple events.

> > +    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.

I considered that. But using level mode means, interrupt needs to be reset
within the _EVT method. Do you see just sticking to one mode(Edge) will be
a problem?

" When the event fires, OSPM handles the interrupt according to its mode
and invokes the _EVT method, passing it the interrupt number of the event"

Doesn’t it mean OSPM handles it with respect to the mode we set?

> > +}
> > +
> > +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?

Ok.

> > +    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.

Sure.

> > +             * 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")?

Ok. Will check that.

> > +    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?

Right. _OSC is not defined and I don’t see Qemu ever defined platform wide 
OSPM capabilities. I can see that it does that for PCI/PCIe hierarchies. 

Moreover looking at the Linux code, it doesn’t seems to care about
GED definition either,
https://elixir.bootlin.com/linux/v5.0/source/include/linux/acpi.h#L490

I can try adding _OSC declaring just the GED bit for future, but not sure
It makes much difference as of now. 

Please let me know if there is a strong argument for it.

> > 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?

Ok.

> > +    GedEventType event;
> > +} GedEvent;
> > +
> > +typedef struct GEDState {
> > +    MemoryRegion io;
> > +    uint32_t     sel;
> > +    uint32_t     irq;
> why not storing the qemu_irq* directly?

Agree.

Thanks,
Shameer

> > +    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

Igor Mammedov March 18, 2019, 12:26 p.m. UTC | #3

On Fri, 8 Mar 2019 11:42:16 +0000
Shameer Kolothum <[e-mail]> wrote:

> From: Samuel Ortiz <[e-mail]>
> 
> 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.
> 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.

I'd this a part of "virtual ACPI 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 <[e-mail]>
> Signed-off-by: Sebastien Boeuf <[e-mail]>
> Signed-off-by: Shameer Kolothum <[e-mail]>
> ---
[...]

Shameerali Kolothum Thodi March 18, 2019, 3:04 p.m. UTC | #4

Hi Igor,

> -----Original Message-----
> From: Igor Mammedov [mailto:[e-mail]]
> Sent: 18 March 2019 12:26
> To: Shameerali Kolothum Thodi <[e-mail]>
> Cc: [e-mail]; [e-mail];
> [e-mail]; [e-mail];
> [e-mail]; [e-mail];
> [e-mail]; Linuxarm <[e-mail]>; xuwei (O)
> <[e-mail]>
> Subject: Re: [PATCH v2 09/11] hw/acpi: Add ACPI Generic Event Device Support
> 
> On Fri, 8 Mar 2019 11:42:16 +0000
> Shameer Kolothum <[e-mail]> wrote:
> 
> > From: Samuel Ortiz <[e-mail]>
> >
> > 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.
> > 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.
> 
> I'd this a part of "virtual ACPI device"

Just to confirm, you meant, instead of separate virt-acpi.c and ged.c, move the
contents of this patch into "virtual ACPI device" implementation (something like 
hw/acpi/generic-event-device.c) ?

Please let me know.

Thanks,
Shameer

> > 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 <[e-mail]>
> > Signed-off-by: Sebastien Boeuf <[e-mail]>
> > Signed-off-by: Shameer Kolothum <[e-mail]>
> > ---
> [...]

Igor Mammedov March 18, 2019, 5 p.m. UTC | #5

On Mon, 18 Mar 2019 15:04:28 +0000
Shameerali Kolothum Thodi <[e-mail]> wrote:

> Hi Igor,
> 
> > -----Original Message-----
> > From: Igor Mammedov [mailto:[e-mail]]
> > Sent: 18 March 2019 12:26
> > To: Shameerali Kolothum Thodi <[e-mail]>
> > Cc: [e-mail]; [e-mail];
> > [e-mail]; [e-mail];
> > [e-mail]; [e-mail];
> > [e-mail]; Linuxarm <[e-mail]>; xuwei (O)
> > <[e-mail]>
> > Subject: Re: [PATCH v2 09/11] hw/acpi: Add ACPI Generic Event Device Support
> > 
> > On Fri, 8 Mar 2019 11:42:16 +0000
> > Shameer Kolothum <[e-mail]> wrote:
> >   
> > > From: Samuel Ortiz <[e-mail]>
> > >
> > > 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.
> > > 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.  
> > 
> > I'd this a part of "virtual ACPI device"  
> 
> Just to confirm, you meant, instead of separate virt-acpi.c and ged.c, move the
> contents of this patch into "virtual ACPI device" implementation (something like 
> hw/acpi/generic-event-device.c) ?
yep, and only moving bit making hw related routines form virt-acpi.c
a part of ged device model where they are called at appropriate times 
like initfn/realize or property setting time.

Then ACPI functions (not part of device but could be in the same file)
would fetch all necessary data from ged device and generate AML code from it.

> 
> Please let me know.
> 
> Thanks,
> Shameer
> 
> > > 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 <[e-mail]>
> > > Signed-off-by: Sebastien Boeuf <[e-mail]>
> > > Signed-off-by: Shameer Kolothum <[e-mail]>
> > > ---  
> > [...]

Patch

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"
+
+static hwaddr ged_io_base;
+
+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;
+        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)
+{
+
+    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)
+{
+    /*
+     * 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;
+    qemu_mutex_unlock(&ged_st->lock);
+
+    /* Trigger the event by sending an interrupt to the guest. */
+    qemu_irq_pulse(irq[ged_st->irq]);
+}
+
+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);
+    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.
+             * 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));
+    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);
+}
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;
+    GedEventType event;
+} GedEvent;
+
+typedef struct GEDState {
+    MemoryRegion io;
+    uint32_t     sel;
+    uint32_t     irq;
+    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