[v3,09/10] hw/acpi: Add ACPI Generic Event Device Support
diff mbox series

Message ID 20190321104745.28068-10-shameerali.kolothum.thodi@huawei.com
State New
Headers show
Series
  • ARM virt: ACPI memory hotplug support
Related show

Commit Message

Shameer Kolothum March 21, 2019, 10:47 a.m. UTC
From: Samuel Ortiz <sameo@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 generates 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.  Also this uses a
a single interrupt for the  GED device, relying on IO memory region
to communicate the type of device affected by the interrupt. This
way, we can support up to 32 events with a unique interrupt.

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 <sameo@linux.intel.com>
Signed-off-by: Sebastien Boeuf <sebastien.boeuf@intel.com>
Signed-off-by: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com>
---
 hw/acpi/generic_event_device.c         | 200 +++++++++++++++++++++++++++++++++
 include/hw/acpi/generic_event_device.h |  34 ++++++
 2 files changed, 234 insertions(+)

Comments

Auger Eric March 29, 2019, 1:09 p.m. UTC | #1
Hi Shameer,

On 3/21/19 11:47 AM, Shameer Kolothum wrote:
> From: Samuel Ortiz <sameo@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 generates 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.  Also this uses a
> a single interrupt for the  GED device, relying on IO memory region
> to communicate the type of device affected by the interrupt. This
> way, we can support up to 32 events with a unique interrupt.
> 
> This is in preparation for making use of GED for ARM/virt
> platform and for now supports only memory hotplug.

Personally I would squash this with PATCH 3.
> 
> Signed-off-by: Samuel Ortiz <sameo@linux.intel.com>
> Signed-off-by: Sebastien Boeuf <sebastien.boeuf@intel.com>
> Signed-off-by: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com>
> ---
>  hw/acpi/generic_event_device.c         | 200 +++++++++++++++++++++++++++++++++
>  include/hw/acpi/generic_event_device.h |  34 ++++++
>  2 files changed, 234 insertions(+)
> 
> diff --git a/hw/acpi/generic_event_device.c b/hw/acpi/generic_event_device.c
> index 0b32fc9..9deaa33 100644
> --- a/hw/acpi/generic_event_device.c
> +++ b/hw/acpi/generic_event_device.c
> @@ -23,6 +23,183 @@
>  #include "hw/acpi/generic_event_device.h"
>  #include "hw/mem/pc-dimm.h"
>  
> +static hwaddr ged_io_base;
> +static GedEvent *ged_events;
> +static uint32_t ged_events_size;
> +
> +static Aml *ged_event_aml(const 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(MEMORY_DEVICES_CONTAINER "." MEMORY_SLOT_SCAN_METHOD);
> +    default:
> +        break;
> +    }
> +
> +    return NULL;
> +}
> +
> +/*
> + * The ACPI Generic Event Device (GED) is a hardware-reduced specific
> + * device[ACPI v6.1 Section 5.6.9] that handles all platform events,
> + * including the hotplug ones. Platforms need to specify their own
> + * GedEvent array to describe what kind of events they want to support
> + * through GED. This routine uses a single interrupt for the GED device,
> + * relying on IO memory region to communicate the type of device
> + * affected by the interrupt. This way, we can support up to 32 events
> + * with a unique interrupt.

During last review I asked the question herefater. I may have missed
your answer but just in case.

5.6.9.1 says:
"
The platform declare its support for the GED, and query whether an OS
supports it, via the _OSC method
"
Is it something done?
> + */
> +void build_ged_aml(Aml *table, const char *name, uint32_t ged_irq,
> +                   AmlRegionSpace rs)
> +{
> +    Aml *crs = aml_resource_template();
> +    Aml *evt, *field;
> +    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 || !ged_events || !ged_events_size) {
> +        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()
> +         * }
I think we could have stopped here ;-) with a ../..
> +         *
> +         * If ((Local0 & irq2) == irq2)
> +         * {
> +         *     MethodEvent2()
> +         * }
> +         */
> +
> +        for (i = 0; i < ged_events_size; i++) {
> +            ged_aml = ged_event_aml(&ged_events[i]);
> +            if (!ged_aml) {
> +                continue;
> +            }
> +
> +            /* If ((Local1 == irq))*/
> +            if_ctx = aml_if(aml_equal(aml_and(irq_sel, aml_int(ged_events[i].selector), NULL), aml_int(ged_events[i].selector)));
doesn't check_patch complain here?
> +            {
> +                /* 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", aml_string(GED_DEVICE)));
> +    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);
> +}
> +
> +/* Memory 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 = 0;
> +        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,
> +    },
> +};
> +
> +static void acpi_ged_event(GEDState *ged_st, 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(ged_st->gsi[ged_st->irq]);
I don't get this. The devices uses a single irq, right?

Why can't we do like other sysbus devices, sysbus_init_irq(dev, &s->irq);
and use
sysbus_connect_irq(SYS_BUS_DEVICE(dev), i, pic); in virt?
> +}
> +
>  static void virt_device_plug_cb(HotplugHandler *hotplug_dev,
>                                  DeviceState *dev, Error **errp)
>  {
> @@ -40,6 +217,21 @@ static void virt_device_plug_cb(HotplugHandler *hotplug_dev,
>  
>  static void virt_send_ged(AcpiDeviceIf *adev, AcpiEventStatusBits ev)
>  {
> +    VirtAcpiState *s = VIRT_ACPI(adev);
> +    uint32_t sel;
> +
> +    if (ev & ACPI_MEMORY_HOTPLUG_STATUS) {
> +        sel = ACPI_GED_IRQ_SEL_MEM;
> +    } else {
> +        /* Unknown event. Return without generating interrupt. */
> +        return;
> +    }
> +
> +    /*
> +     * We inject the hotplug interrupt. The IRQ selector will make
> +     * the difference from the ACPI table.
> +     */
> +    acpi_ged_event(&s->ged_state, sel);
>  }
>  
>  static void virt_device_realize(DeviceState *dev, Error **errp)
> @@ -57,6 +249,11 @@ static Property virt_acpi_properties[] = {
>      DEFINE_PROP_UINT64("memhp_base", VirtAcpiState, memhp_base, 0),
>      DEFINE_PROP_BOOL("memory-hotplug-support", VirtAcpiState,
>                       memhp_state.is_enabled, true),
It may be worth to explain why the GED device owns the
MEMORY_HOTPLUG_MMIO region. This was and still is confusing me I
acknowledge.
> +    DEFINE_PROP_PTR("gsi", VirtAcpiState, gsi),
see the comment abour irq above.
> +    DEFINE_PROP_UINT64("ged_base", VirtAcpiState, ged_base, 0),
> +    DEFINE_PROP_UINT32("ged_irq", VirtAcpiState, ged_irq, 0),
> +    DEFINE_PROP_PTR("ged_events", VirtAcpiState, ged_events),
> +    DEFINE_PROP_UINT32("ged_events_size", VirtAcpiState, ged_events_size, 0),
>      DEFINE_PROP_END_OF_LIST(),
>  };
>  
> @@ -70,6 +267,9 @@ static void virt_acpi_class_init(ObjectClass *class, void *data)
>      dc->props = virt_acpi_properties;
>      dc->realize = virt_device_realize;
>  
> +    /* Reason: pointer properties "gsi" and "gde_events" */
ged_events
> +    dc->user_creatable = false;
> +
>      hc->plug = virt_device_plug_cb;
>  
>      adevc->send_event = virt_send_ged;
> diff --git a/include/hw/acpi/generic_event_device.h b/include/hw/acpi/generic_event_device.h
> index 262ca7d..7f130f3 100644
> --- a/include/hw/acpi/generic_event_device.h
> +++ b/include/hw/acpi/generic_event_device.h
> @@ -24,11 +24,45 @@
>  #define VIRT_ACPI(obj) \
>      OBJECT_CHECK(VirtAcpiState, (obj), TYPE_VIRT_ACPI)
>  
> +#define ACPI_GED_IRQ_SEL_OFFSET 0x0
> +#define ACPI_GED_IRQ_SEL_LEN    0x4
> +#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 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;
> +    qemu_irq     *gsi;
> +    QemuMutex    lock;
> +} GEDState;
> +
>  typedef struct VirtAcpiState {
>      SysBusDevice parent_obj;
>      MemHotplugState memhp_state;
>      hwaddr memhp_base;
> +    void *gsi;
> +    hwaddr ged_base;
> +    GEDState ged_state;
> +    uint32_t ged_irq;
> +    void *ged_events;
> +    uint32_t ged_events_size;
>  } VirtAcpiState;
>  
> +void build_ged_aml(Aml *table, const char* name, uint32_t ged_irq,
> +                   AmlRegionSpace rs);
>  
>  #endif
> 

Thanks

Eric
Shameer Kolothum March 29, 2019, 1:44 p.m. UTC | #2
Hi Eric,

> -----Original Message-----
> From: Auger Eric [mailto:eric.auger@redhat.com]
> Sent: 29 March 2019 13:09
> To: Shameerali Kolothum Thodi <shameerali.kolothum.thodi@huawei.com>;
> qemu-devel@nongnu.org; qemu-arm@nongnu.org; imammedo@redhat.com;
> peter.maydell@linaro.org; shannon.zhaosl@gmail.com;
> sameo@linux.intel.com; sebastien.boeuf@intel.com
> Cc: Linuxarm <linuxarm@huawei.com>; xuwei (O) <xuwei5@huawei.com>
> Subject: Re: [PATCH v3 09/10] hw/acpi: Add ACPI Generic Event Device Support
> 
> Hi Shameer,
> 
> On 3/21/19 11:47 AM, Shameer Kolothum wrote:
> > From: Samuel Ortiz <sameo@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 generates 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.  Also this uses a
> > a single interrupt for the  GED device, relying on IO memory region
> > to communicate the type of device affected by the interrupt. This
> > way, we can support up to 32 events with a unique interrupt.
> >
> > This is in preparation for making use of GED for ARM/virt
> > platform and for now supports only memory hotplug.
> 
> Personally I would squash this with PATCH 3.

Ok.

> > Signed-off-by: Samuel Ortiz <sameo@linux.intel.com>
> > Signed-off-by: Sebastien Boeuf <sebastien.boeuf@intel.com>
> > Signed-off-by: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com>
> > ---
> >  hw/acpi/generic_event_device.c         | 200
> +++++++++++++++++++++++++++++++++
> >  include/hw/acpi/generic_event_device.h |  34 ++++++
> >  2 files changed, 234 insertions(+)
> >
> > diff --git a/hw/acpi/generic_event_device.c
> b/hw/acpi/generic_event_device.c
> > index 0b32fc9..9deaa33 100644
> > --- a/hw/acpi/generic_event_device.c
> > +++ b/hw/acpi/generic_event_device.c
> > @@ -23,6 +23,183 @@
> >  #include "hw/acpi/generic_event_device.h"
> >  #include "hw/mem/pc-dimm.h"
> >
> > +static hwaddr ged_io_base;
> > +static GedEvent *ged_events;
> > +static uint32_t ged_events_size;
> > +
> > +static Aml *ged_event_aml(const 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(MEMORY_DEVICES_CONTAINER "."
> MEMORY_SLOT_SCAN_METHOD);
> > +    default:
> > +        break;
> > +    }
> > +
> > +    return NULL;
> > +}
> > +
> > +/*
> > + * The ACPI Generic Event Device (GED) is a hardware-reduced specific
> > + * device[ACPI v6.1 Section 5.6.9] that handles all platform events,
> > + * including the hotplug ones. Platforms need to specify their own
> > + * GedEvent array to describe what kind of events they want to support
> > + * through GED. This routine uses a single interrupt for the GED device,
> > + * relying on IO memory region to communicate the type of device
> > + * affected by the interrupt. This way, we can support up to 32 events
> > + * with a unique interrupt.
> 
> During last review I asked the question herefater. I may have missed
> your answer but just in case.
> 
> 5.6.9.1 says:
> "
> The platform declare its support for the GED, and query whether an OS
> supports it, via the _OSC method
> "
> Is it something done?

Yes you did raise this earlier and I had replied as well :).

https://patchwork.kernel.org/patch/10844557/

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

> > + */
> > +void build_ged_aml(Aml *table, const char *name, uint32_t ged_irq,
> > +                   AmlRegionSpace rs)
> > +{
> > +    Aml *crs = aml_resource_template();
> > +    Aml *evt, *field;
> > +    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 || !ged_events || !ged_events_size) {
> > +        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()
> > +         * }
> I think we could have stopped here ;-) with a ../..

Ok

> > +         *
> > +         * If ((Local0 & irq2) == irq2)
> > +         * {
> > +         *     MethodEvent2()
> > +         * }
> > +         */
> > +
> > +        for (i = 0; i < ged_events_size; i++) {
> > +            ged_aml = ged_event_aml(&ged_events[i]);
> > +            if (!ged_aml) {
> > +                continue;
> > +            }
> > +
> > +            /* If ((Local1 == irq))*/
> > +            if_ctx = aml_if(aml_equal(aml_and(irq_sel,
> aml_int(ged_events[i].selector), NULL), aml_int(ged_events[i].selector)));
> doesn't check_patch complain here?

It did warn but thought this has better readability. I will address this.

> > +            {
> > +                /* 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", aml_string(GED_DEVICE)));
> > +    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);
> > +}
> > +
> > +/* Memory 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 = 0;
> > +        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,
> > +    },
> > +};
> > +
> > +static void acpi_ged_event(GEDState *ged_st, 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(ged_st->gsi[ged_st->irq]);
> I don't get this. The devices uses a single irq, right?

Yes it uses single irq.

> Why can't we do like other sysbus devices, sysbus_init_irq(dev, &s->irq);
> and use
> sysbus_connect_irq(SYS_BUS_DEVICE(dev), i, pic); in virt?

I have to take a look at this.

> > +}
> > +
> >  static void virt_device_plug_cb(HotplugHandler *hotplug_dev,
> >                                  DeviceState *dev, Error **errp)
> >  {
> > @@ -40,6 +217,21 @@ static void virt_device_plug_cb(HotplugHandler
> *hotplug_dev,
> >
> >  static void virt_send_ged(AcpiDeviceIf *adev, AcpiEventStatusBits ev)
> >  {
> > +    VirtAcpiState *s = VIRT_ACPI(adev);
> > +    uint32_t sel;
> > +
> > +    if (ev & ACPI_MEMORY_HOTPLUG_STATUS) {
> > +        sel = ACPI_GED_IRQ_SEL_MEM;
> > +    } else {
> > +        /* Unknown event. Return without generating interrupt. */
> > +        return;
> > +    }
> > +
> > +    /*
> > +     * We inject the hotplug interrupt. The IRQ selector will make
> > +     * the difference from the ACPI table.
> > +     */
> > +    acpi_ged_event(&s->ged_state, sel);
> >  }
> >
> >  static void virt_device_realize(DeviceState *dev, Error **errp)
> > @@ -57,6 +249,11 @@ static Property virt_acpi_properties[] = {
> >      DEFINE_PROP_UINT64("memhp_base", VirtAcpiState, memhp_base,
> 0),
> >      DEFINE_PROP_BOOL("memory-hotplug-support", VirtAcpiState,
> >                       memhp_state.is_enabled, true),
> It may be worth to explain why the GED device owns the
> MEMORY_HOTPLUG_MMIO region. This was and still is confusing me I
> acknowledge.

I will try to add the comment here saying this is the hotplug handler/
acpi interface device and needs to initialize the memory hotplug base
region.

> > +    DEFINE_PROP_PTR("gsi", VirtAcpiState, gsi),
> see the comment abour irq above.
> > +    DEFINE_PROP_UINT64("ged_base", VirtAcpiState, ged_base, 0),
> > +    DEFINE_PROP_UINT32("ged_irq", VirtAcpiState, ged_irq, 0),
> > +    DEFINE_PROP_PTR("ged_events", VirtAcpiState, ged_events),
> > +    DEFINE_PROP_UINT32("ged_events_size", VirtAcpiState,
> ged_events_size, 0),
> >      DEFINE_PROP_END_OF_LIST(),
> >  };
> >
> > @@ -70,6 +267,9 @@ static void virt_acpi_class_init(ObjectClass *class,
> void *data)
> >      dc->props = virt_acpi_properties;
> >      dc->realize = virt_device_realize;
> >
> > +    /* Reason: pointer properties "gsi" and "gde_events" */
> ged_events

Thanks,
Shameer

> > +    dc->user_creatable = false;
> > +
> >      hc->plug = virt_device_plug_cb;
> >
> >      adevc->send_event = virt_send_ged;
> > diff --git a/include/hw/acpi/generic_event_device.h
> b/include/hw/acpi/generic_event_device.h
> > index 262ca7d..7f130f3 100644
> > --- a/include/hw/acpi/generic_event_device.h
> > +++ b/include/hw/acpi/generic_event_device.h
> > @@ -24,11 +24,45 @@
> >  #define VIRT_ACPI(obj) \
> >      OBJECT_CHECK(VirtAcpiState, (obj), TYPE_VIRT_ACPI)
> >
> > +#define ACPI_GED_IRQ_SEL_OFFSET 0x0
> > +#define ACPI_GED_IRQ_SEL_LEN    0x4
> > +#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 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;
> > +    qemu_irq     *gsi;
> > +    QemuMutex    lock;
> > +} GEDState;
> > +
> >  typedef struct VirtAcpiState {
> >      SysBusDevice parent_obj;
> >      MemHotplugState memhp_state;
> >      hwaddr memhp_base;
> > +    void *gsi;
> > +    hwaddr ged_base;
> > +    GEDState ged_state;
> > +    uint32_t ged_irq;
> > +    void *ged_events;
> > +    uint32_t ged_events_size;
> >  } VirtAcpiState;
> >
> > +void build_ged_aml(Aml *table, const char* name, uint32_t ged_irq,
> > +                   AmlRegionSpace rs);
> >
> >  #endif
> >
> 
> Thanks
> 
> Eric

Patch
diff mbox series

diff --git a/hw/acpi/generic_event_device.c b/hw/acpi/generic_event_device.c
index 0b32fc9..9deaa33 100644
--- a/hw/acpi/generic_event_device.c
+++ b/hw/acpi/generic_event_device.c
@@ -23,6 +23,183 @@ 
 #include "hw/acpi/generic_event_device.h"
 #include "hw/mem/pc-dimm.h"
 
+static hwaddr ged_io_base;
+static GedEvent *ged_events;
+static uint32_t ged_events_size;
+
+static Aml *ged_event_aml(const 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(MEMORY_DEVICES_CONTAINER "." MEMORY_SLOT_SCAN_METHOD);
+    default:
+        break;
+    }
+
+    return NULL;
+}
+
+/*
+ * The ACPI Generic Event Device (GED) is a hardware-reduced specific
+ * device[ACPI v6.1 Section 5.6.9] that handles all platform events,
+ * including the hotplug ones. Platforms need to specify their own
+ * GedEvent array to describe what kind of events they want to support
+ * through GED. This routine uses a single interrupt for the GED device,
+ * relying on IO memory region to communicate the type of device
+ * affected by the interrupt. This way, we can support up to 32 events
+ * with a unique interrupt.
+ */
+void build_ged_aml(Aml *table, const char *name, uint32_t ged_irq,
+                   AmlRegionSpace rs)
+{
+    Aml *crs = aml_resource_template();
+    Aml *evt, *field;
+    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 || !ged_events || !ged_events_size) {
+        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 < ged_events_size; i++) {
+            ged_aml = ged_event_aml(&ged_events[i]);
+            if (!ged_aml) {
+                continue;
+            }
+
+            /* If ((Local1 == irq))*/
+            if_ctx = aml_if(aml_equal(aml_and(irq_sel, aml_int(ged_events[i].selector), NULL), aml_int(ged_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", aml_string(GED_DEVICE)));
+    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);
+}
+
+/* Memory 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 = 0;
+        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,
+    },
+};
+
+static void acpi_ged_event(GEDState *ged_st, 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(ged_st->gsi[ged_st->irq]);
+}
+
 static void virt_device_plug_cb(HotplugHandler *hotplug_dev,
                                 DeviceState *dev, Error **errp)
 {
@@ -40,6 +217,21 @@  static void virt_device_plug_cb(HotplugHandler *hotplug_dev,
 
 static void virt_send_ged(AcpiDeviceIf *adev, AcpiEventStatusBits ev)
 {
+    VirtAcpiState *s = VIRT_ACPI(adev);
+    uint32_t sel;
+
+    if (ev & ACPI_MEMORY_HOTPLUG_STATUS) {
+        sel = ACPI_GED_IRQ_SEL_MEM;
+    } else {
+        /* Unknown event. Return without generating interrupt. */
+        return;
+    }
+
+    /*
+     * We inject the hotplug interrupt. The IRQ selector will make
+     * the difference from the ACPI table.
+     */
+    acpi_ged_event(&s->ged_state, sel);
 }
 
 static void virt_device_realize(DeviceState *dev, Error **errp)
@@ -57,6 +249,11 @@  static Property virt_acpi_properties[] = {
     DEFINE_PROP_UINT64("memhp_base", VirtAcpiState, memhp_base, 0),
     DEFINE_PROP_BOOL("memory-hotplug-support", VirtAcpiState,
                      memhp_state.is_enabled, true),
+    DEFINE_PROP_PTR("gsi", VirtAcpiState, gsi),
+    DEFINE_PROP_UINT64("ged_base", VirtAcpiState, ged_base, 0),
+    DEFINE_PROP_UINT32("ged_irq", VirtAcpiState, ged_irq, 0),
+    DEFINE_PROP_PTR("ged_events", VirtAcpiState, ged_events),
+    DEFINE_PROP_UINT32("ged_events_size", VirtAcpiState, ged_events_size, 0),
     DEFINE_PROP_END_OF_LIST(),
 };
 
@@ -70,6 +267,9 @@  static void virt_acpi_class_init(ObjectClass *class, void *data)
     dc->props = virt_acpi_properties;
     dc->realize = virt_device_realize;
 
+    /* Reason: pointer properties "gsi" and "gde_events" */
+    dc->user_creatable = false;
+
     hc->plug = virt_device_plug_cb;
 
     adevc->send_event = virt_send_ged;
diff --git a/include/hw/acpi/generic_event_device.h b/include/hw/acpi/generic_event_device.h
index 262ca7d..7f130f3 100644
--- a/include/hw/acpi/generic_event_device.h
+++ b/include/hw/acpi/generic_event_device.h
@@ -24,11 +24,45 @@ 
 #define VIRT_ACPI(obj) \
     OBJECT_CHECK(VirtAcpiState, (obj), TYPE_VIRT_ACPI)
 
+#define ACPI_GED_IRQ_SEL_OFFSET 0x0
+#define ACPI_GED_IRQ_SEL_LEN    0x4
+#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 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;
+    qemu_irq     *gsi;
+    QemuMutex    lock;
+} GEDState;
+
 typedef struct VirtAcpiState {
     SysBusDevice parent_obj;
     MemHotplugState memhp_state;
     hwaddr memhp_base;
+    void *gsi;
+    hwaddr ged_base;
+    GEDState ged_state;
+    uint32_t ged_irq;
+    void *ged_events;
+    uint32_t ged_events_size;
 } VirtAcpiState;
 
+void build_ged_aml(Aml *table, const char* name, uint32_t ged_irq,
+                   AmlRegionSpace rs);
 
 #endif