[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH-for-4.2 v7 03/10] hw/acpi: Add ACPI Generic Even
From: |
Igor Mammedov |
Subject: |
Re: [Qemu-devel] [PATCH-for-4.2 v7 03/10] hw/acpi: Add ACPI Generic Event Device Support |
Date: |
Thu, 18 Jul 2019 14:30:34 +0200 |
On Thu, 18 Jul 2019 10:52:10 +0000
Shameerali Kolothum Thodi <address@hidden> wrote:
> Hi Igor,
>
> > -----Original Message-----
> > From: Qemu-devel
> > [mailto:qemu-devel-bounces+shameerali.kolothum.thodi=huawei.com@nongn
> > u.org] On Behalf Of Igor Mammedov
> > Sent: 17 July 2019 15:33
> > To: Shameerali Kolothum Thodi <address@hidden>
> > Cc: address@hidden; address@hidden;
> > address@hidden; address@hidden;
> > address@hidden; xuwei (O) <address@hidden>; Linuxarm
> > <address@hidden>; address@hidden; address@hidden;
> > address@hidden; address@hidden
> > Subject: Re: [Qemu-devel] [PATCH-for-4.2 v7 03/10] hw/acpi: Add ACPI Generic
> > Event Device Support
> >
> > On Tue, 16 Jul 2019 16:38:09 +0100
> > Shameer Kolothum <address@hidden> wrote:
>
> [...]
>
> > > +static void acpi_ged_event(AcpiGedState *s, uint32_t sel)
> > > +{
> > > + GEDState *ged_st = &s->ged_state;
> > > + /*
> > > + * 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.
> > comment isn't correct anymore, pls fix it
>
> True.
>
> >
> > > + */
> > > + qemu_mutex_lock(&ged_st->lock);
> > Is this lock really necessary?
> > (I thought that MMIO and monitor access is guarded by BQL)
>
> Hmm..I am not sure. This is to synchronize with the ged_st->sel update inside
> ged_read(). And also acpi_ged_event() gets called through
> _power_down_notifier()
> as well. BQL guard is in place for all the paths here?
power down command originates from HMP or QMP monitor, so you don't really
need a lock here.
>
> >
> > > + ged_st->sel |= sel;
> > > + qemu_mutex_unlock(&ged_st->lock);
> > > +
> > > + /* Trigger the event by sending an interrupt to the guest. */
> > > + qemu_irq_pulse(s->irq);
> > > +}
> > > +
> > > +static void acpi_ged_init(MemoryRegion *as, DeviceState *dev, GEDState
> > *ged_st)
> > > +{
> > > + AcpiGedState *s = ACPI_GED(dev);
> > > +
> > > + assert(s->ged_base);
> > > +
> > > + qemu_mutex_init(&ged_st->lock);
> > > + memory_region_init_io(&ged_st->io, OBJECT(dev), &ged_ops, ged_st,
> > > + TYPE_ACPI_GED, ACPI_GED_REG_LEN);
> > > + memory_region_add_subregion(as, s->ged_base, &ged_st->io);
> > > + qdev_init_gpio_out_named(DEVICE(s), &s->irq, "ged-irq", 1);
> > > +}
> > > +
> > > +static void acpi_ged_device_plug_cb(HotplugHandler *hotplug_dev,
> > > + DeviceState *dev, Error **errp)
> > > +{
> > > + AcpiGedState *s = ACPI_GED(hotplug_dev);
> > > +
> > > + if (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM)) {
> > > + if (s->memhp_state.is_enabled) {
> > > + acpi_memory_plug_cb(hotplug_dev, &s->memhp_state, dev,
> > errp);
> > > + } else {
> > > + error_setg(errp,
> > > + "memory hotplug is not
> > enabled: %s.memory-hotplug-support "
> > > + "is not set", object_get_typename(OBJECT(s)));
> > > + }
> > > + } 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_MEM_HOTPLUG_EVT;
> > > + } else {
> > > + /* Unknown event. Return without generating interrupt. */
> > > + warn_report("GED: Unsupported event %d. No irq injected", ev);
> > > + return;
> > > + }
> > > +
> > > + /*
> > > + * We inject the hotplug interrupt. The IRQ selector will make
> > > + * the difference from the ACPI table.
> > I don't get comment at all, pls rephrase/
>
> Ok. I think better to get rid of this comment here and update the one in
> acpi_ged_event()
> appropriately.
>
> >
> > > + */
> > > + acpi_ged_event(s, sel);
> > it seems to used only once and only here, suggest to drop acpi_ged_event()
> > and move it's code here.
>
> But patch #10 makes use of it from acpi_ged_pm_powerdown_req().
it looks like a valid shortcut but I'd make it follow
AcpiInterface->send_event()
path for consistency so only one call chain would exist.
>
> > > +}
> > > +
> > > +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 acpi-mem-hotplug
> > > + * memory region 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_UINT64("ged-base", AcpiGedState, ged_base, 0),
> >
> > I'd add checks to realize() to ensure that both foo_base are not left
> > uninitialized (i.e. at default values)
>
> Ok. ged_base has a check in acpi_ged_init(). I will add check for memhp_base
> and move the ged_base one to realize().
>
> >
> > > + DEFINE_PROP_UINT32("ged-event", AcpiGedState, ged_event_bitmap,
> > 0),
> > > + DEFINE_PROP_END_OF_LIST(),
> > > +};
> > > +
> > > +static bool vmstate_test_use_memhp(void *opaque)
> > > +{
> > > + AcpiGedState *s = opaque;
> > > + return s->memhp_state.is_enabled;
> > > +}
> > > +
> > > +static const VMStateDescription vmstate_memhp_state = {
> > > + .name = "acpi-ged/memhp",
> > > + .version_id = 1,
> > > + .minimum_version_id = 1,
> > > + .needed = vmstate_test_use_memhp,
> > > + .fields = (VMStateField[]) {
> > > + VMSTATE_MEMORY_HOTPLUG(memhp_state, AcpiGedState),
> > > + VMSTATE_END_OF_LIST()
> > > + }
> > > +};
> > > +
> > > +static const VMStateDescription vmstate_ged_state = {
> > > + .name = "acpi-ged-state",
> > > + .version_id = 1,
> > > + .minimum_version_id = 1,
> > > + .fields = (VMStateField[]) {
> > > + VMSTATE_UINT32(sel, GEDState),
> > > + VMSTATE_END_OF_LIST()
> > > + }
> > > +};
> > > +
> > > +static const VMStateDescription vmstate_acpi_ged = {
> > > + .name = "acpi-ged",
> > > + .version_id = 1,
> > > + .minimum_version_id = 1,
> > > + .fields = (VMStateField[]) {
> > > + VMSTATE_STRUCT(ged_state, AcpiGedState, 1,
> > vmstate_ged_state, GEDState),
> > > + VMSTATE_END_OF_LIST(),
> > > + },
> > > + .subsections = (const VMStateDescription * []) {
> > > + &vmstate_memhp_state,
> > > + NULL
> > > + }
> > > +};
> > > +
> > > +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 Generic Event Device";
> > > + dc->props = acpi_ged_properties;
> > > + dc->realize = acpi_ged_device_realize;
> > > + dc->vmsd = &vmstate_acpi_ged;
> > > +
> > > + 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 0000000000..f69d084c89
> > > --- /dev/null
> > > +++ b/include/hw/acpi/generic_event_device.h
> > > @@ -0,0 +1,102 @@
> > > +/*
> > > + *
> > > + * Copyright (c) 2018 Intel Corporation
> > > + * Copyright (c) 2019 Huawei Technologies R & D (UK) Ltd
> > > + * Written by Samuel Ortiz, Shameer Kolothum
> > > + *
> > > + * 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.
> > > + *
> > > + * 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 (EREG, SystemMemory, 0x09080000, 0x04)
> > > + * Field (EREG, DWordAcc, NoLock, WriteAsZeros)
> > > + * {
> > > + * ESEL, 32
> > > + * }
> > > + *
> > > + * Method (_EVT, 1, Serialized) // _EVT: Event
> > > + * {
> > > + * Local0 = ESEL // ESEL = IO memory region which specifies the
> > > + * // device type.
> > > + * If (((Local0 & One) == One))
> > > + * {
> > > + * MethodEvent1()
> > > + * }
> > > + * If ((Local0 & 0x2) == 0x2)
> > > + * {
> > > + * MethodEvent2()
> > > + * }
> > > + * ...
> > > + * }
> > > + * }
> > > + *
> > > + */
> > > +
> > > +#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_EVT_SEL_OFFSET 0x0
> >
> > > +#define ACPI_GED_EVT_SEL_LEN 0x4
> > > +#define ACPI_GED_REG_LEN 0x4
> > Didn't both macros refer to the same thing?
> > Why not use the same one in AML and in QEMU MMIO handler?
>
> Ok.
>
> Thanks,
> Shameer
>
> >
> > > +
> > > +#define GED_DEVICE "GED"
> > > +#define AML_GED_EVT_REG "EREG"
> > > +#define AML_GED_EVT_SEL "ESEL"
> > > +
> > > +/*
> > > + * Platforms need to specify the GED event bitmap
> > > + * to describe what kind of events they want to support
> > > + * through GED.
> > > + */
> > > +#define ACPI_GED_MEM_HOTPLUG_EVT 0x1
> > > +
> > > +typedef struct GEDState {
> > > + MemoryRegion io;
> > > + uint32_t sel;
> > > + QemuMutex lock;
> > > +} GEDState;
> > > +
> > > +typedef struct AcpiGedState {
> > > + DeviceClass parent_obj;
> > > + MemHotplugState memhp_state;
> > > + hwaddr memhp_base;
> > > + hwaddr ged_base;
> > > + GEDState ged_state;
> > > + uint32_t ged_event_bitmap;
> > > + qemu_irq irq;
> > > +} AcpiGedState;
> > > +
> > > +void build_ged_aml(Aml *table, const char* name, HotplugHandler
> > *hotplug_dev,
> > > + uint32_t ged_irq, AmlRegionSpace rs);
> > > +
> > > +#endif
> >
>
- Re: [Qemu-devel] [PATCH-for-4.2 v7 04/10] hw/arm/virt: Add memory hotplug framework, (continued)
- [Qemu-devel] [PATCH-for-4.2 v7 05/10] hw/arm/virt: Add 4.2 machine type, Shameer Kolothum, 2019/07/16
- [Qemu-devel] [PATCH-for-4.2 v7 02/10] hw/acpi: Do not create memory hotplug method when handler is not defined, Shameer Kolothum, 2019/07/16
- [Qemu-devel] [PATCH-for-4.2 v7 01/10] hw/acpi: Make ACPI IO address space configurable, Shameer Kolothum, 2019/07/16
- [Qemu-devel] [PATCH-for-4.2 v7 06/10] hw/arm/virt: Enable device memory cold/hot plug with ACPI boot, Shameer Kolothum, 2019/07/16
- [Qemu-devel] [PATCH-for-4.2 v7 03/10] hw/acpi: Add ACPI Generic Event Device Support, Shameer Kolothum, 2019/07/16
[Qemu-devel] [PATCH-for-4.2 v7 07/10] hw/arm/virt-acpi-build: Add PC-DIMM in SRAT, Shameer Kolothum, 2019/07/16
[Qemu-devel] [PATCH-for-4.2 v7 09/10] hw/arm: Use GED for system_powerdown event, Shameer Kolothum, 2019/07/16
[Qemu-devel] [PATCH-for-4.2 v7 10/10] tests: Update DSDT ACPI table for arm/virt board with PCDIMM related changes, Shameer Kolothum, 2019/07/16