Re: [Qemu-devel] [PATCH v3 09/10] hw/acpi: Add ACPI Generic Event Device Support

[Auger Eric]

Hi Shameer,

On 3/21/19 11:47 AM, Shameer Kolothum wrote:
> From: Samuel Ortiz <address@hidden>
> 
> 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 <address@hidden>
> Signed-off-by: Sebastien Boeuf <address@hidden>
> Signed-off-by: Shameer Kolothum <address@hidden>
> ---
>  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