Re: [Qemu-arm] [Qemu-devel] [PATCH v4 3/8] hw/acpi: Add ACPI Generic Event Device Support

[Igor Mammedov]

On Tue, 7 May 2019 09:01:43 +0000
Shameerali Kolothum Thodi <address@hidden> wrote:

> Hi Igor,
> 
> > -----Original Message-----
> > From: Igor Mammedov [mailto:address@hidden
> > Sent: 03 May 2019 16:10
> > To: Shameerali Kolothum Thodi <address@hidden>
> > Cc: address@hidden; address@hidden;
> > address@hidden; address@hidden;
> > address@hidden; address@hidden;
> > address@hidden; xuwei (O) <address@hidden>;
> > address@hidden; address@hidden; Linuxarm
> > <address@hidden>
> > Subject: Re: [PATCH v4 3/8] hw/acpi: Add ACPI Generic Event Device Support
> 
> [...]
> 
> > > > type.
> > > > > +         * The resulting ASL code looks like:
> > > > > +         *
> > > > > +         * Local0 = ISEL
> > > > > +         * If ((Local0 & irq0) == irq0)
> > > > > +         * {
> > > > > +         *     MethodEvent0()
> > > > > +         * }
> > > > > +         *
> > > > > +         * If ((Local0 & irq1) == irq1)
> > > > > +         * {
> > > > > +         *     MethodEvent1()
> > > > > +         * }
> > > > > +         * ...
> > > > > +         */
> > > > Well, I'm confused.
> > > > do we actually use multiple IRQs or we use only one + MMIO for event
> > type?
> > >
> > > It is one irq + MMIO. I will change the comment block something like
> > > this,
> > 
> > change corresponding variable names as well
> 
> Ok.
> 
> > >
> > >     Local0 = ISEL
> > >     If ((Local0 & One) == One)
> > >     {
> > >         MethodEvent1()
> > >     }
> > >
> > >     If ((Local0 & 0x02) == 0x02)
> > >     {
> > >         MethodEvent2()
> > >     }
> > >     ...
> > >
> > > >
> > > > > +        for (i = 0; i < s->ged_events_size; i++) {
> > > >
> > > > > +            ged_aml = ged_event_aml(&ged_events[i]);
> > > > > +            if (!ged_aml) {
> > > > > +                continue;
> > > > > +            }
> > > > I'd get rid of ged_event_aml replace it with more 'switch':
> > > >    for (i,...)
> > > >        if_ctx = aml_if(...)
> > > >        switch (event)
> > > >           case GED_MEMORY_HOTPLUG:
> > > >                aml_append(if_ctx,
> > > > aml_call0(MEMORY_DEVICES_CONTAINER "."
> > > > MEMORY_SLOT_SCAN_METHOD))
> > > >                break
> > > >           default:
> > > >                about(); // make sure that a newly added events have
> > > > a handler
> > >
> > > Ok. I will change this.
> > >
> > > >
> > > > > +
> > > > > +            /* 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 "if"s will be "elseif"s.
> > > > > +             */
> > > > > +            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(s->ged_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);
> > > >
> > > > I'd move it up above EVT() method, so it would be clear from the
> > > > begging for what device we are building AML
> > >
> > > Ok.
> > >
> > > >
> > > > > +    /* 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;
> > > > what if there are 2 events with different sel value and 2nd event
> > > > happens before guesr read the first one?
> > >
> > > This was previously,
> > >    ged_st->sel |= ged_irq_sel;
> > >
> > > and was changed based on the assumption that there won't be any
> > > multiple events. But I think the scenario above is right. I will
> > > change it back so that we won't overwrite.
> > so it was bitmap, then handling it as a DWORD in AML could be wrong and AML
> > part probably should be changed to accommodate it.
> 
> Hmm..I am slightly confused as the AML code already uses the len as 32 bits 
> for this
> register field,
> 
>     /* Append IO region */
>     aml_append(dev, aml_operation_region(AML_GED_IRQ_REG, rs,
>                aml_int(s->ged_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);
> 
> and event check logic is, 
> 
>      if_ctx = aml_if(aml_equal(aml_and(irq_sel,
>                           aml_int(ged_events[i].selector), NULL),
>                           aml_int(ged_events[i].selector)));
> 
> Not sure what is the concern here. Please let me know. 

Looking at it second time, it seems I've been mistaken.
that 'if' should work.

one suggestion how to improve AML side,
instead of using  AML_GED_IRQ_REG DWORD + AND masking
you can use bit fields directly.
See how "aml_named_field(...,1)" is used for inspiration.


>             
> > >
> > > >
> > > > > +    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 acpi_ged_init(MemoryRegion *as, DeviceState *dev,
> > > > > +GEDState
> > > > *ged_st)
> > > > > +{
> > > > > +    AcpiGedState *s = ACPI_GED(dev);
> > > > > +
> > > > > +    assert(s->ged_base);
> > > > > +
> > > > > +    ged_st->irq = s->ged_irq;
> > > > > +    ged_st->gsi = s->gsi;
> > > > > +    qemu_mutex_init(&ged_st->lock);
> > > > > +    memory_region_init_io(&ged_st->io, OBJECT(dev), &ged_ops,
> > ged_st,
> > > > > +                          "acpi-ged-event", ACPI_GED_REG_LEN);
> > > > > +    memory_region_add_subregion(as, s->ged_base, &ged_st->io); }
> > > > > +
> > > > > +static void acpi_ged_device_plug_cb(HotplugHandler *hotplug_dev,
> > > > > +                                    DeviceState *dev, Error
> > > > > +**errp) {
> > > > > +    AcpiGedState *s = ACPI_GED(hotplug_dev);
> > > > > +
> > > > > +    if (s->memhp_state.is_enabled &&
> > > > > +        object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM)) {
> > > > > +            acpi_memory_plug_cb(hotplug_dev, &s->memhp_state,
> > > > > +                                dev, errp);
> > > > > +    } else {
> > > > > +        error_setg(errp, "virt: device plug request for
> > > > > + unsupported
> > > > device"
> > > > > +                   " type: %s",
> > object_get_typename(OBJECT(dev)));
> > > > > +    }
> > > > > +}
> > > > > +
> > > > > +static void acpi_ged_send_event(AcpiDeviceIf *adev,
> > > > > +AcpiEventStatusBits
> > > > ev)
> > > > > +{
> > > > > +    AcpiGedState *s = ACPI_GED(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 acpi_ged_device_realize(DeviceState *dev, Error
> > > > > +**errp) {
> > > > > +    AcpiGedState *s = ACPI_GED(dev);
> > > > > +
> > > > > +    if (s->memhp_state.is_enabled) {
> > > > > +        acpi_memory_hotplug_init(get_system_memory(),
> > OBJECT(dev),
> > > > > +                                 &s->memhp_state,
> > > > > +                                 s->memhp_base);
> > > > > +        acpi_ged_init(get_system_memory(), dev, &s->ged_state);
> > > > > +    }
> > > > > +}
> > > > > +
> > > > > +static Property acpi_ged_properties[] = {
> > > > > +    /*
> > > > > +     * Memory hotplug base address is a property of GED here,
> > > > > +     * because GED handles memory hotplug event and
> > > > MEMORY_HOTPLUG_DEVICE
> > > > > +     * gets initialized when GED device is realized.
> > > > > +     */
> > > > > +    DEFINE_PROP_UINT64("memhp-base", AcpiGedState,
> > memhp_base,
> > > > 0),
> > > > > +    DEFINE_PROP_BOOL("memory-hotplug-support", AcpiGedState,
> > > > > +                     memhp_state.is_enabled, true),
> > > > > +    DEFINE_PROP_PTR("gsi", AcpiGedState, gsi),
> > > >
> > > > PTR shouldn't be used in new code, look at
> > > > object_property_add_link() & co
> > >
> > > Ok. I will take a look at that.
> > >
> > > >
> > > > > +    DEFINE_PROP_UINT64("ged-base", AcpiGedState, ged_base, 0),
> > > > > +    DEFINE_PROP_UINT32("ged-irq", AcpiGedState, ged_irq, 0),
> > > > > +    DEFINE_PROP_PTR("ged-events", AcpiGedState, ged_events),
> > > > > +    DEFINE_PROP_UINT32("ged-events-size", AcpiGedState,
> > > > ged_events_size, 0),
> > > > > +    DEFINE_PROP_END_OF_LIST(),
> > > > > +};
> > > > > +
> > > > > +static void acpi_ged_class_init(ObjectClass *class, void *data) {
> > > > > +    DeviceClass *dc = DEVICE_CLASS(class);
> > > > > +    HotplugHandlerClass *hc = HOTPLUG_HANDLER_CLASS(class);
> > > > > +    AcpiDeviceIfClass *adevc = ACPI_DEVICE_IF_CLASS(class);
> > > > > +
> > > > > +    dc->desc = "ACPI";
> > > > > +    dc->props = acpi_ged_properties;
> > > > > +    dc->realize = acpi_ged_device_realize;
> > > > > +
> > > > > +    /* Reason: pointer properties "gsi" and "ged_events" */
> > > > > +    dc->user_creatable = false;
> > > > > +
> > > > > +    hc->plug = acpi_ged_device_plug_cb;
> > > > > +
> > > > > +    adevc->send_event = acpi_ged_send_event; }
> > > > > +
> > > > > +static const TypeInfo acpi_ged_info = {
> > > > > +    .name          = TYPE_ACPI_GED,
> > > > > +    .parent        = TYPE_DEVICE,
> > > > > +    .instance_size = sizeof(AcpiGedState),
> > > > > +    .class_init    = acpi_ged_class_init,
> > > > > +    .interfaces = (InterfaceInfo[]) {
> > > > > +        { TYPE_HOTPLUG_HANDLER },
> > > > > +        { TYPE_ACPI_DEVICE_IF },
> > > > > +        { }
> > > > > +    }
> > > > > +};
> > > > > +
> > > > > +static void acpi_ged_register_types(void) {
> > > > > +    type_register_static(&acpi_ged_info);
> > > > > +}
> > > > > +
> > > > > +type_init(acpi_ged_register_types)
> > > > > diff --git a/include/hw/acpi/generic_event_device.h
> > > > b/include/hw/acpi/generic_event_device.h
> > > > > new file mode 100644
> > > > > index 0000000..9c840d8
> > > > > --- /dev/null
> > > > > +++ b/include/hw/acpi/generic_event_device.h
> > > > > @@ -0,0 +1,121 @@
> > > > > +/*
> > > > > + *
> > > > > + * 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/>.
> > > > > + *
> > > > > + * 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. Generic Event Device allows
> > > > > + platforms
> > > > > + * to handle interrupts in ACPI ASL statements. It follows a very
> > > > > + * similar approach like the _EVT method from GPIO events. All
> > > > > + * interrupts are listed in  _CRS and the handler is written in
> > > > > + _EVT
> > > > > + * method. Here, we use 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.
> > > > > + *
> > > > > + * Here is an example.
> > > > > + *
> > > > > + * Device (\_SB.GED)
> > > > > + * {
> > > > > + *     Name (_HID, "ACPI0013")
> > > > > + *     Name (_UID, Zero)
> > > > > + *     Name (_CRS, ResourceTemplate ()
> > > > > + *     {
> > > > > + *         Interrupt (ResourceConsumer, Edge, ActiveHigh,
> > Exclusive, ,, )
> > > > > + *         {
> > > > > + *              0x00000029,
> > > > > + *         }
> > > > > + *     })
> > > > > + *     OperationRegion (IREG, SystemMemory, 0x09080000, 0x04)
> > > > > + *     Field (IREG, DWordAcc, NoLock, WriteAsZeros)
> > > > > + *     {
> > > > > + *         ISEL,   32
> > > > > + *     }
> > > > > + *
> > > > > + *     Method (_EVT, 1, Serialized)  // _EVT: Event
> > > > > + *     {
> > > > > + *         Local0 = ISEL // ISEL = IO memory region which specifies 
> > > > > the
> > > > > + *                       // device type.
> > > > > + *         If (((Local0 & irq0) == irq0))
> > > > > + *         {
> > > > > + *             MethodEvent0()
> > > > > + *         }
> > > > > + *         ElseIf ((Local0 & irq1) == irq1)
> > > > > + *         {
> > > > > + *             MethodEvent1()
> > > > > + *         }
> > > > > + *         ...
> > > > > + *     }
> > > > > + * }
> > > > > + *
> > > > > + */
> > > > > +
> > > > > +#ifndef HW_ACPI_GED_H
> > > > > +#define HW_ACPI_GED_H
> > > > > +
> > > > > +#include "hw/acpi/memory_hotplug.h"
> > > > > +
> > > > > +#define TYPE_ACPI_GED "acpi-ged"
> > > > > +#define ACPI_GED(obj) \
> > > > > +    OBJECT_CHECK(AcpiGedState, (obj), TYPE_ACPI_GED)
> > > > > +
> > > > > +#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;
> > > > > +
> > > > > +/*
> > > > > + * Platforms need to specify their own GedEvent array
> > > > > + * to describe what kind of events they want to support
> > > > > + * through GED.
> > > > > + */
> > > > > +typedef struct GedEvent {
> > > > > +    uint32_t     selector;
> > > > > +    GedEventType event;
> > > > > +} GedEvent;
> > > > > +
> > > > > +typedef struct GEDState {
> > > > > +    MemoryRegion io;
> > > > > +    uint32_t     sel;
> > > >
> > > > do we need to migrate this during migration?
> > >
> > > TBH, I don't know and currently this series doesn't address migration
> > > as we don't have any VMStateDescription and friends. Is this something
> > > we can sort later?
> > 
> > It probably should be implemented, otherwise we will have to deal with hard 
> > to
> > debug bug reports when users will try to migrate.
> > 
> > Alternative hack could be that, enabling memory hotplug should put migration
> > blocker, but that's probably the same effort as adding proper
> > VMStateDescription
> 
> Ok. I will take a look into VMStateDescription then.
> 
> Thanks,
> Shameer
> 
> > >
> > > Thanks,
> > > Shameer
> > >
> > > > > +    uint32_t     irq;
> > > > > +    qemu_irq     *gsi;
> > > > > +    QemuMutex    lock;
> > > > > +} GEDState;
> > > > > +
> > > > > +
> > > > > +typedef struct AcpiGedState {
> > > > > +    DeviceClass 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;
> > > > > +} AcpiGedState;
> > > > > +
> > > > > +void build_ged_aml(Aml *table, const char* name, HotplugHandler
> > > > *hotplug_dev,
> > > > > +                   uint32_t ged_irq, AmlRegionSpace rs);
> > > > > +
> > > > > +#endif
> > >
> 
>