diff mbox series

[PATCH-for-4.2,v8,3/9] hw/acpi: Add ACPI Generic Event Device Support

Message ID 20190726104519.23812-4-shameerali.kolothum.thodi@huawei.com (mailing list archive)
State New, archived
Headers show
Series ARM virt: ACPI memory hotplug support | expand

Commit Message

Shameerali Kolothum Thodi July 26, 2019, 10:45 a.m. UTC
From: Samuel Ortiz <sameo@linux.intel.com>

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. This patch generates the AML code that
defines GEDs.

Platforms need to specify their own GED Event bitmap 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 supports only memory hotplug for now.

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>
Reviewed-by: Eric Auger <eric.auger@redhat.com>
---
v7 --> v8.
 -Removed qemu_mutex_lock() across the ged state selector access.
 -Rephrased comments section in acpi_ged_send_event().
 -Moved acpi_ged_event() code into acpi_ged_send_event().
 -Added check for memhp_base and ged_base in realize().

---
 hw/acpi/Kconfig                        |   4 +
 hw/acpi/Makefile.objs                  |   1 +
 hw/acpi/generic_event_device.c         | 322 +++++++++++++++++++++++++
 include/hw/acpi/generic_event_device.h | 100 ++++++++
 4 files changed, 427 insertions(+)
 create mode 100644 hw/acpi/generic_event_device.c
 create mode 100644 include/hw/acpi/generic_event_device.h

Comments

Igor Mammedov July 30, 2019, 3:25 p.m. UTC | #1
On Fri, 26 Jul 2019 11:45:13 +0100
Shameer Kolothum <shameerali.kolothum.thodi@huawei.com> wrote:

> From: Samuel Ortiz <sameo@linux.intel.com>
> 
> 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. This patch generates the AML code that
> defines GEDs.
> 
> Platforms need to specify their own GED Event bitmap 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 supports only memory hotplug for now.
> 

> diff --git a/hw/acpi/generic_event_device.c b/hw/acpi/generic_event_device.c
> new file mode 100644
> index 0000000000..7902e9d706
> --- /dev/null
> +++ b/hw/acpi/generic_event_device.c
[...]
> +void build_ged_aml(Aml *table, const char *name, HotplugHandler *hotplug_dev,
> +                   uint32_t ged_irq, AmlRegionSpace rs)
> +{
[...]
> +
> +        if (ged_events) {
> +            error_report("GED: Unsupported events specified");
> +            exit(1);
I'd use error_abort instead, since it's programing error, if you have to respin series.

> +        }
> +    }
> +
> +    /* Append _EVT method */
> +    aml_append(dev, evt);
> +
> +    aml_append(table, dev);
> +}
> +
[...]
> +static void acpi_ged_device_realize(DeviceState *dev, Error **errp)
> +{
> +    AcpiGedState *s = ACPI_GED(dev);
> +
> +    assert(s->ged_base);
> +    acpi_ged_init(get_system_memory(), dev, &s->ged_state);

calling get_system_memory() from device code used to be a reason for rejecting patch,
I'm not sure what suggest though. 

Maybe Paolo could suggest something.

> +
> +    if (s->memhp_state.is_enabled) {
> +        assert(s->memhp_base);
> +        acpi_memory_hotplug_init(get_system_memory(), OBJECT(dev),
                                     ^^^^
ditto

> +                                 &s->memhp_state,
> +                                 s->memhp_base);
> +    }
> +}
[...]

> +
> +#endif
Shameerali Kolothum Thodi Aug. 1, 2019, 8:36 a.m. UTC | #2
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: 30 July 2019 16:25
> To: Shameerali Kolothum Thodi <shameerali.kolothum.thodi@huawei.com>
> Cc: peter.maydell@linaro.org; sameo@linux.intel.com;
> ard.biesheuvel@linaro.org; shannon.zhaosl@gmail.com;
> qemu-devel@nongnu.org; xuwei (O) <xuwei5@huawei.com>; Linuxarm
> <linuxarm@huawei.com>; eric.auger@redhat.com; qemu-arm@nongnu.org;
> Paolo Bonzini <pbonzini@redhat.com>; sebastien.boeuf@intel.com;
> lersek@redhat.com
> Subject: Re: [Qemu-devel] [PATCH-for-4.2 v8 3/9] hw/acpi: Add ACPI Generic
> Event Device Support
> 
> On Fri, 26 Jul 2019 11:45:13 +0100
> Shameer Kolothum <shameerali.kolothum.thodi@huawei.com> wrote:
> 
> > From: Samuel Ortiz <sameo@linux.intel.com>
> >
> > 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. This patch generates the AML code that
> > defines GEDs.
> >
> > Platforms need to specify their own GED Event bitmap 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 supports only memory hotplug for now.
> >
> 
> > diff --git a/hw/acpi/generic_event_device.c
> b/hw/acpi/generic_event_device.c
> > new file mode 100644
> > index 0000000000..7902e9d706
> > --- /dev/null
> > +++ b/hw/acpi/generic_event_device.c
> [...]
> > +void build_ged_aml(Aml *table, const char *name, HotplugHandler
> *hotplug_dev,
> > +                   uint32_t ged_irq, AmlRegionSpace rs)
> > +{
> [...]
> > +
> > +        if (ged_events) {
> > +            error_report("GED: Unsupported events specified");
> > +            exit(1);
> I'd use error_abort instead, since it's programing error, if you have to respin
> series.

Ok.

> > +        }
> > +    }
> > +
> > +    /* Append _EVT method */
> > +    aml_append(dev, evt);
> > +
> > +    aml_append(table, dev);
> > +}
> > +
> [...]
> > +static void acpi_ged_device_realize(DeviceState *dev, Error **errp)
> > +{
> > +    AcpiGedState *s = ACPI_GED(dev);
> > +
> > +    assert(s->ged_base);
> > +    acpi_ged_init(get_system_memory(), dev, &s->ged_state);
> 
> calling get_system_memory() from device code used to be a reason for
> rejecting patch,
> I'm not sure what suggest though.
> 
> Maybe Paolo could suggest something.

How about using object_property_set_link()? Something like below.

------8-----
diff --git a/hw/acpi/generic_event_device.c b/hw/acpi/generic_event_device.c
index f00b0ab14b..eb1ed38f4a 100644
--- a/hw/acpi/generic_event_device.c
+++ b/hw/acpi/generic_event_device.c
@@ -229,11 +229,12 @@ static void acpi_ged_device_realize(DeviceState *dev, Error **errp)
     AcpiGedState *s = ACPI_GED(dev);
 
     assert(s->ged_base);
-    acpi_ged_init(get_system_memory(), dev, &s->ged_state);
+    assert(s->sys_mem);
+    acpi_ged_init(s->sys_mem, dev, &s->ged_state);
 
     if (s->memhp_state.is_enabled) {
         assert(s->memhp_base);
-        acpi_memory_hotplug_init(get_system_memory(), OBJECT(dev),
+        acpi_memory_hotplug_init(s->sys_mem, OBJECT(dev),
                                  &s->memhp_state,
                                  s->memhp_base);
     }
@@ -245,6 +246,8 @@ static Property acpi_ged_properties[] = {
      * because GED handles memory hotplug event and acpi-mem-hotplug
      * memory region gets initialized when GED device is realized.
      */
+    DEFINE_PROP_LINK("memory", AcpiGedState, sys_mem, TYPE_MEMORY_REGION,
+                     MemoryRegion *),
     DEFINE_PROP_UINT64("memhp-base", AcpiGedState, memhp_base, 0),
     DEFINE_PROP_BOOL("memory-hotplug-support", AcpiGedState,
                      memhp_state.is_enabled, true),
diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index 73a758d9a9..0cbaf6c6e1 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -529,8 +529,12 @@ static inline DeviceState *create_acpi_ged(VirtMachineState *vms, qemu_irq *pic)
     DeviceState *dev;
     int irq = vms->irqmap[VIRT_ACPI_GED];
     uint32_t event = ACPI_GED_MEM_HOTPLUG_EVT | ACPI_GED_PWR_DOWN_EVT;
+    MemoryRegion *sys_mem = get_system_memory();
 
     dev = DEVICE(object_new(TYPE_ACPI_GED));
+
+    object_property_set_link(OBJECT(dev), OBJECT(sys_mem),
+                             "memory", &error_abort);
     qdev_prop_set_uint64(dev, "memhp-base",
                          vms->memmap[VIRT_PCDIMM_ACPI].base);
     qdev_prop_set_uint64(dev, "ged-base", vms->memmap[VIRT_ACPI_GED].base);
diff --git a/include/hw/acpi/generic_event_device.h b/include/hw/acpi/generic_event_device.h
index 63104f1344..f1f2f337f7 100644
--- a/include/hw/acpi/generic_event_device.h
+++ b/include/hw/acpi/generic_event_device.h
@@ -89,6 +89,7 @@ typedef struct GEDState {
 
 typedef struct AcpiGedState {
     DeviceClass parent_obj;
+    MemoryRegion *sys_mem;
     MemHotplugState memhp_state;
     hwaddr memhp_base;
     hwaddr ged_base;

---8----
Igor Mammedov Aug. 5, 2019, 1:30 p.m. UTC | #3
On Thu, 1 Aug 2019 08:36:33 +0000
Shameerali Kolothum Thodi <shameerali.kolothum.thodi@huawei.com> 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: 30 July 2019 16:25
> > To: Shameerali Kolothum Thodi <shameerali.kolothum.thodi@huawei.com>
> > Cc: peter.maydell@linaro.org; sameo@linux.intel.com;
> > ard.biesheuvel@linaro.org; shannon.zhaosl@gmail.com;
> > qemu-devel@nongnu.org; xuwei (O) <xuwei5@huawei.com>; Linuxarm
> > <linuxarm@huawei.com>; eric.auger@redhat.com; qemu-arm@nongnu.org;
> > Paolo Bonzini <pbonzini@redhat.com>; sebastien.boeuf@intel.com;
> > lersek@redhat.com
> > Subject: Re: [Qemu-devel] [PATCH-for-4.2 v8 3/9] hw/acpi: Add ACPI Generic
> > Event Device Support
> > 
> > On Fri, 26 Jul 2019 11:45:13 +0100
> > Shameer Kolothum <shameerali.kolothum.thodi@huawei.com> wrote:
> >   
> > > From: Samuel Ortiz <sameo@linux.intel.com>
> > >
> > > 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. This patch generates the AML code that
> > > defines GEDs.
> > >
> > > Platforms need to specify their own GED Event bitmap 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 supports only memory hotplug for now.
> > >  
> >   
> > > diff --git a/hw/acpi/generic_event_device.c  
> > b/hw/acpi/generic_event_device.c  
> > > new file mode 100644
> > > index 0000000000..7902e9d706
> > > --- /dev/null
> > > +++ b/hw/acpi/generic_event_device.c  
> > [...]  
> > > +void build_ged_aml(Aml *table, const char *name, HotplugHandler  
> > *hotplug_dev,  
> > > +                   uint32_t ged_irq, AmlRegionSpace rs)
> > > +{  
> > [...]  
> > > +
> > > +        if (ged_events) {
> > > +            error_report("GED: Unsupported events specified");
> > > +            exit(1);  
> > I'd use error_abort instead, since it's programing error, if you have to respin
> > series.  
> 
> Ok.
> 
> > > +        }
> > > +    }
> > > +
> > > +    /* Append _EVT method */
> > > +    aml_append(dev, evt);
> > > +
> > > +    aml_append(table, dev);
> > > +}
> > > +  
> > [...]  
> > > +static void acpi_ged_device_realize(DeviceState *dev, Error **errp)
> > > +{
> > > +    AcpiGedState *s = ACPI_GED(dev);
> > > +
> > > +    assert(s->ged_base);
> > > +    acpi_ged_init(get_system_memory(), dev, &s->ged_state);  
> > 
> > calling get_system_memory() from device code used to be a reason for
> > rejecting patch,
> > I'm not sure what suggest though.
> > 
> > Maybe Paolo could suggest something.  
> 
> How about using object_property_set_link()? Something like below.
I'm afraid it doesn't help much. Issue here is that we are letting
device to manage whole address space (which should be managed by machine)
So I'd just keep get_system_memory() as is for now if there aren't any
objections.

> ------8-----
> diff --git a/hw/acpi/generic_event_device.c b/hw/acpi/generic_event_device.c
> index f00b0ab14b..eb1ed38f4a 100644
> --- a/hw/acpi/generic_event_device.c
> +++ b/hw/acpi/generic_event_device.c
> @@ -229,11 +229,12 @@ static void acpi_ged_device_realize(DeviceState *dev, Error **errp)
>      AcpiGedState *s = ACPI_GED(dev);
>  
>      assert(s->ged_base);
> -    acpi_ged_init(get_system_memory(), dev, &s->ged_state);
> +    assert(s->sys_mem);
> +    acpi_ged_init(s->sys_mem, dev, &s->ged_state);
>  
>      if (s->memhp_state.is_enabled) {
>          assert(s->memhp_base);
> -        acpi_memory_hotplug_init(get_system_memory(), OBJECT(dev),
> +        acpi_memory_hotplug_init(s->sys_mem, OBJECT(dev),
>                                   &s->memhp_state,
>                                   s->memhp_base);
>      }
> @@ -245,6 +246,8 @@ static Property acpi_ged_properties[] = {
>       * because GED handles memory hotplug event and acpi-mem-hotplug
>       * memory region gets initialized when GED device is realized.
>       */
> +    DEFINE_PROP_LINK("memory", AcpiGedState, sys_mem, TYPE_MEMORY_REGION,
> +                     MemoryRegion *),
>      DEFINE_PROP_UINT64("memhp-base", AcpiGedState, memhp_base, 0),
>      DEFINE_PROP_BOOL("memory-hotplug-support", AcpiGedState,
>                       memhp_state.is_enabled, true),
> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> index 73a758d9a9..0cbaf6c6e1 100644
> --- a/hw/arm/virt.c
> +++ b/hw/arm/virt.c
> @@ -529,8 +529,12 @@ static inline DeviceState *create_acpi_ged(VirtMachineState *vms, qemu_irq *pic)
>      DeviceState *dev;
>      int irq = vms->irqmap[VIRT_ACPI_GED];
>      uint32_t event = ACPI_GED_MEM_HOTPLUG_EVT | ACPI_GED_PWR_DOWN_EVT;
> +    MemoryRegion *sys_mem = get_system_memory();
>  
>      dev = DEVICE(object_new(TYPE_ACPI_GED));
> +
> +    object_property_set_link(OBJECT(dev), OBJECT(sys_mem),
> +                             "memory", &error_abort);
>      qdev_prop_set_uint64(dev, "memhp-base",
>                           vms->memmap[VIRT_PCDIMM_ACPI].base);
>      qdev_prop_set_uint64(dev, "ged-base", vms->memmap[VIRT_ACPI_GED].base);
> diff --git a/include/hw/acpi/generic_event_device.h b/include/hw/acpi/generic_event_device.h
> index 63104f1344..f1f2f337f7 100644
> --- a/include/hw/acpi/generic_event_device.h
> +++ b/include/hw/acpi/generic_event_device.h
> @@ -89,6 +89,7 @@ typedef struct GEDState {
>  
>  typedef struct AcpiGedState {
>      DeviceClass parent_obj;
> +    MemoryRegion *sys_mem;
>      MemHotplugState memhp_state;
>      hwaddr memhp_base;
>      hwaddr ged_base;
> 
> ---8----
>
Peter Maydell Aug. 5, 2019, 1:42 p.m. UTC | #4
On Mon, 5 Aug 2019 at 14:30, Igor Mammedov <imammedo@redhat.com> wrote:
>
> On Thu, 1 Aug 2019 08:36:33 +0000
> Shameerali Kolothum Thodi <shameerali.kolothum.thodi@huawei.com> wrote:
>
> > Hi Igor,
> >
> > > -----Original Message-----
> > > > +static void acpi_ged_device_realize(DeviceState *dev, Error **errp)
> > > > +{
> > > > +    AcpiGedState *s = ACPI_GED(dev);
> > > > +
> > > > +    assert(s->ged_base);
> > > > +    acpi_ged_init(get_system_memory(), dev, &s->ged_state);
> > >
> > > calling get_system_memory() from device code used to be a reason for
> > > rejecting patch,
> > > I'm not sure what suggest though.
> > >
> > > Maybe Paolo could suggest something.
> >
> > How about using object_property_set_link()? Something like below.
> I'm afraid it doesn't help much. Issue here is that we are letting
> device to manage whole address space (which should be managed by machine)
> So I'd just keep get_system_memory() as is for now if there aren't any
> objections.

What are we trying to do with this device, and what does it need
the system memory region for?

In this case, we seem to do:

+static void acpi_ged_init(MemoryRegion *as, DeviceState *dev, GEDState *ged_st)
+{
+    AcpiGedState *s = ACPI_GED(dev);
+
+    memory_region_init_io(&ged_st->io, OBJECT(dev), &ged_ops, ged_st,
+                          TYPE_ACPI_GED, ACPI_GED_EVT_SEL_LEN);
+    memory_region_add_subregion(as, s->ged_base, &ged_st->io);
+    qdev_init_gpio_out_named(DEVICE(s), &s->irq, "ged-irq", 1);
+}


This is definitely a bad idea -- devices should not add their
own memory regions to the system memory MR. They should
expose their MRs (by being a sysbus-device) and let the board
code do the wiring up of the MRs into the right memory space
at the right address.

thanks
-- PMM
Igor Mammedov Aug. 5, 2019, 3:46 p.m. UTC | #5
On Mon, 5 Aug 2019 14:42:38 +0100
Peter Maydell <peter.maydell@linaro.org> wrote:

> On Mon, 5 Aug 2019 at 14:30, Igor Mammedov <imammedo@redhat.com> wrote:
> >
> > On Thu, 1 Aug 2019 08:36:33 +0000
> > Shameerali Kolothum Thodi <shameerali.kolothum.thodi@huawei.com> wrote:
> >  
> > > Hi Igor,
> > >  
> > > > -----Original Message-----  
> > > > > +static void acpi_ged_device_realize(DeviceState *dev, Error **errp)
> > > > > +{
> > > > > +    AcpiGedState *s = ACPI_GED(dev);
> > > > > +
> > > > > +    assert(s->ged_base);
> > > > > +    acpi_ged_init(get_system_memory(), dev, &s->ged_state);  
> > > >
> > > > calling get_system_memory() from device code used to be a reason for
> > > > rejecting patch,
> > > > I'm not sure what suggest though.
> > > >
> > > > Maybe Paolo could suggest something.  
> > >
> > > How about using object_property_set_link()? Something like below.  
> > I'm afraid it doesn't help much. Issue here is that we are letting
> > device to manage whole address space (which should be managed by machine)
> > So I'd just keep get_system_memory() as is for now if there aren't any
> > objections.  
> 
> What are we trying to do with this device, and what does it need
> the system memory region for?
> 
> In this case, we seem to do:
> 
> +static void acpi_ged_init(MemoryRegion *as, DeviceState *dev, GEDState *ged_st)
> +{
> +    AcpiGedState *s = ACPI_GED(dev);
> +
> +    memory_region_init_io(&ged_st->io, OBJECT(dev), &ged_ops, ged_st,
> +                          TYPE_ACPI_GED, ACPI_GED_EVT_SEL_LEN);
> +    memory_region_add_subregion(as, s->ged_base, &ged_st->io);
> +    qdev_init_gpio_out_named(DEVICE(s), &s->irq, "ged-irq", 1);
> +}
> 
> 
> This is definitely a bad idea -- devices should not add their
> own memory regions to the system memory MR. They should
> expose their MRs (by being a sysbus-device) and let the board
> code do the wiring up of the MRs into the right memory space
> at the right address.

it's not the only place in GED that is trying to add to system address
space, optionally if called acpi_memory_hotplug_init() will do the same,
then later we could add cpu hotplug memory region over there.

Perhaps we could use bus-less device plug code path,
in that case memory_region_init_io()/qdev_init_gpio_out_named()
should be moved to ged_initfn() and mapping part into specialized helper
(similar to pc_dimm_plug() ) that's called by board (from virt_machine_device_plug_cb)
callback during completing device realize stage, it would be something like:

virt.c:
   virt_machine_device_plug_cb()
      if dev == GED_TYPE
        machine_ged_plug_helper(system_memory)

generic_event_device.c:
   machine_ged_plug_helper(as, irq) // similar to sysbus_mmio_map() but ged specialized
      connect_irq()
      memory_region_add_subregion(as, ged->ged_base, &ged->io)
      if ged->memory-hotplug-support
          memory_region_add_subregion(as, ged->memhp_base , &ged->memhp_state.memhp_io)


in this case addresses could be normally hard-codded in board code if device is not optional
(as in patch 6/9: create_acpi_ged() )
or potentially they could come from CLI as -device parameters
(/me thinking about building blocks that allow to create machine from config)

sysbus device might be fine as shortcut if we are thinking about
only creating device during machine_init (although I have a reservations towards
sysbus interface (ex: caller of sysbus_mmio_map() has no clue when mapping N-th
region at some address)).


> thanks
> -- PMM
Peter Maydell Aug. 5, 2019, 3:54 p.m. UTC | #6
On Mon, 5 Aug 2019 at 16:47, Igor Mammedov <imammedo@redhat.com> wrote:
> On Mon, 5 Aug 2019 14:42:38 +0100
> Peter Maydell <peter.maydell@linaro.org> wrote:
> > This is definitely a bad idea -- devices should not add their
> > own memory regions to the system memory MR. They should
> > expose their MRs (by being a sysbus-device) and let the board
> > code do the wiring up of the MRs into the right memory space
> > at the right address.
>
> it's not the only place in GED that is trying to add to system address
> space, optionally if called acpi_memory_hotplug_init() will do the same,
> then later we could add cpu hotplug memory region over there.
>
> Perhaps we could use bus-less device plug code path,
> in that case memory_region_init_io()/qdev_init_gpio_out_named()
> should be moved to ged_initfn() and mapping part into specialized helper
> (similar to pc_dimm_plug() ) that's called by board (from virt_machine_device_plug_cb)
> callback during completing device realize stage, it would be something like:
>
> virt.c:
>    virt_machine_device_plug_cb()
>       if dev == GED_TYPE
>         machine_ged_plug_helper(system_memory)
>
> generic_event_device.c:
>    machine_ged_plug_helper(as, irq) // similar to sysbus_mmio_map() but ged specialized
>       connect_irq()
>       memory_region_add_subregion(as, ged->ged_base, &ged->io)
>       if ged->memory-hotplug-support
>           memory_region_add_subregion(as, ged->memhp_base , &ged->memhp_state.memhp_io)

I don't really understand why we want to do this complicated
thing, rather than just doing the normal thing for devices
that live at particular addresses, ie make them sysbus devices
and have the board map their memory regions in the right place.

> in this case addresses could be normally hard-codded in board code if device is not optional
> (as in patch 6/9: create_acpi_ged() )
> or potentially they could come from CLI as -device parameters
> (/me thinking about building blocks that allow to create machine from config)

I don't think we want to do this. The user should not have to
know anything about addresses or have to specify them on the
command line. (This is why you can't create sysbus devices
with -device except for some odd special cases to do with passthrough
of hardware.)

> sysbus device might be fine as shortcut if we are thinking about
> only creating device during machine_init (although I have a reservations towards
> sysbus interface (ex: caller of sysbus_mmio_map() has no clue when mapping N-th
> region at some address)).

Not sure entirely what you have in mind here? (though yes, the
sysbus device API has its awkward corners, some of which are
just down to how old it is.)

thanks
-- PMM
Igor Mammedov Aug. 6, 2019, 9:47 a.m. UTC | #7
On Mon, 5 Aug 2019 16:54:06 +0100
Peter Maydell <peter.maydell@linaro.org> wrote:

> On Mon, 5 Aug 2019 at 16:47, Igor Mammedov <imammedo@redhat.com> wrote:
> > On Mon, 5 Aug 2019 14:42:38 +0100
> > Peter Maydell <peter.maydell@linaro.org> wrote:  
> > > This is definitely a bad idea -- devices should not add their
> > > own memory regions to the system memory MR. They should
> > > expose their MRs (by being a sysbus-device) and let the board
> > > code do the wiring up of the MRs into the right memory space
> > > at the right address.  
> >
> > it's not the only place in GED that is trying to add to system address
> > space, optionally if called acpi_memory_hotplug_init() will do the same,
> > then later we could add cpu hotplug memory region over there.
> >
> > Perhaps we could use bus-less device plug code path,
> > in that case memory_region_init_io()/qdev_init_gpio_out_named()
> > should be moved to ged_initfn() and mapping part into specialized helper
> > (similar to pc_dimm_plug() ) that's called by board (from virt_machine_device_plug_cb)
> > callback during completing device realize stage, it would be something like:
> >
> > virt.c:
> >    virt_machine_device_plug_cb()
> >       if dev == GED_TYPE
> >         machine_ged_plug_helper(system_memory)
> >
> > generic_event_device.c:
> >    machine_ged_plug_helper(as, irq) // similar to sysbus_mmio_map() but ged specialized
> >       connect_irq()
> >       memory_region_add_subregion(as, ged->ged_base, &ged->io)
> >       if ged->memory-hotplug-support
> >           memory_region_add_subregion(as, ged->memhp_base , &ged->memhp_state.memhp_io)  
> 
> I don't really understand why we want to do this complicated
> thing, rather than just doing the normal thing for devices
> that live at particular addresses, ie make them sysbus devices
> and have the board map their memory regions in the right place.

hotplug path is basically the same as sysbus the only difference is
that it uses machine's (pre_)plug handler to wire up devices and more
flexible than sysbus.
 
> > in this case addresses could be normally hard-codded in board code if device is not optional
> > (as in patch 6/9: create_acpi_ged() )
> > or potentially they could come from CLI as -device parameters
> > (/me thinking about building blocks that allow to create machine from config)  
> 
> I don't think we want to do this. The user should not have to
> know anything about addresses or have to specify them on the
> command line. (This is why you can't create sysbus devices
> with -device except for some odd special cases to do with passthrough
> of hardware.)
> 
> > sysbus device might be fine as shortcut if we are thinking about
> > only creating device during machine_init (although I have a reservations towards
> > sysbus interface (ex: caller of sysbus_mmio_map() has no clue when mapping N-th
> > region at some address)).  
> 
> Not sure entirely what you have in mind here? (though yes, the
> sysbus device API has its awkward corners, some of which are
> just down to how old it is.)
since it's a fixed device I don't mind using sysbus either,
lets do it this way.

> thanks
> -- PMM
diff mbox series

Patch

diff --git a/hw/acpi/Kconfig b/hw/acpi/Kconfig
index 7c59cf900b..12e3f1e86e 100644
--- a/hw/acpi/Kconfig
+++ b/hw/acpi/Kconfig
@@ -31,3 +31,7 @@  config ACPI_VMGENID
     bool
     default y
     depends on PC
+
+config ACPI_HW_REDUCED
+    bool
+    depends on ACPI
diff --git a/hw/acpi/Makefile.objs b/hw/acpi/Makefile.objs
index 9bb2101e3b..655a9c1973 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) += generic_event_device.o
 common-obj-$(call lnot,$(CONFIG_ACPI_X86)) += acpi-stub.o
 
 common-obj-y += acpi_interface.o
diff --git a/hw/acpi/generic_event_device.c b/hw/acpi/generic_event_device.c
new file mode 100644
index 0000000000..7902e9d706
--- /dev/null
+++ b/hw/acpi/generic_event_device.c
@@ -0,0 +1,322 @@ 
+/*
+ *
+ * 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.
+ */
+
+#include "qemu/osdep.h"
+#include "qapi/error.h"
+#include "exec/address-spaces.h"
+#include "hw/acpi/acpi.h"
+#include "hw/acpi/generic_event_device.h"
+#include "hw/mem/pc-dimm.h"
+#include "qemu/error-report.h"
+
+static const uint32_t ged_supported_events[] = {
+    ACPI_GED_MEM_HOTPLUG_EVT,
+};
+
+/*
+ * 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
+ * GED Event bitmap 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, HotplugHandler *hotplug_dev,
+                   uint32_t ged_irq, AmlRegionSpace rs)
+{
+    AcpiGedState *s = ACPI_GED(hotplug_dev);
+    Aml *crs = aml_resource_template();
+    Aml *evt, *field;
+    Aml *dev = aml_device("%s", name);
+    Aml *evt_sel = aml_local(0);
+    Aml *esel = aml_name(AML_GED_EVT_SEL);
+
+    assert(s->ged_base);
+
+    /* _CRS interrupt */
+    aml_append(crs, aml_interrupt(AML_CONSUMER, AML_EDGE, AML_ACTIVE_HIGH,
+                                  AML_EXCLUSIVE, &ged_irq, 1));
+
+    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_EVT_REG, rs,
+               aml_int(s->ged_base + ACPI_GED_EVT_SEL_OFFSET),
+               ACPI_GED_EVT_SEL_LEN));
+    field = aml_field(AML_GED_EVT_REG, AML_DWORD_ACC, AML_NOLOCK,
+                      AML_WRITE_AS_ZEROS);
+    aml_append(field, aml_named_field(AML_GED_EVT_SEL,
+                                      ACPI_GED_EVT_SEL_LEN * BITS_PER_BYTE));
+    aml_append(dev, field);
+
+    /*
+     * For each GED event we:
+     * - Add a conditional block for each event, inside a loop.
+     * - Call a method for each supported GED event type.
+     *
+     * The resulting ASL code looks like:
+     *
+     * Local0 = ESEL
+     * If ((Local0 & One) == One)
+     * {
+     *     MethodEvent0()
+     * }
+     *
+     * If ((Local0 & 0x2) == 0x2)
+     * {
+     *     MethodEvent1()
+     * }
+     * ...
+     */
+    evt = aml_method("_EVT", 1, AML_SERIALIZED);
+    {
+        Aml *if_ctx;
+        uint32_t i;
+        uint32_t ged_events = ctpop32(s->ged_event_bitmap);
+
+        /* Local0 = ESEL */
+        aml_append(evt, aml_store(esel, evt_sel));
+
+        for (i = 0; i < ARRAY_SIZE(ged_supported_events) && ged_events; i++) {
+            uint32_t event = s->ged_event_bitmap & ged_supported_events[i];
+
+            if (!event) {
+                continue;
+            }
+
+            if_ctx = aml_if(aml_equal(aml_and(evt_sel, aml_int(event), NULL),
+                                      aml_int(event)));
+            switch (event) {
+            case ACPI_GED_MEM_HOTPLUG_EVT:
+                aml_append(if_ctx, aml_call0(MEMORY_DEVICES_CONTAINER "."
+                                             MEMORY_SLOT_SCAN_METHOD));
+                break;
+            default:
+                /*
+                 * Please make sure all the events in ged_supported_events[]
+                 * are handled above.
+                 */
+                g_assert_not_reached();
+            }
+
+            aml_append(evt, if_ctx);
+            ged_events--;
+        }
+
+        if (ged_events) {
+            error_report("GED: Unsupported events specified");
+            exit(1);
+        }
+    }
+
+    /* 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_EVT_SEL_OFFSET:
+        /* Read the selector value and reset it */
+        val = ged_st->sel;
+        ged_st->sel = 0;
+        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_init(MemoryRegion *as, DeviceState *dev, GEDState *ged_st)
+{
+    AcpiGedState *s = ACPI_GED(dev);
+
+    memory_region_init_io(&ged_st->io, OBJECT(dev), &ged_ops, ged_st,
+                          TYPE_ACPI_GED, ACPI_GED_EVT_SEL_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);
+    GEDState *ged_st = &s->ged_state;
+    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;
+    }
+
+    /*
+     * Set the GED selector field to communicate the event type.
+     * This will be read by GED aml code to select the appropriate
+     * event method.
+     */
+    ged_st->sel |= sel;
+
+    /* Trigger the event by sending an interrupt to the guest. */
+    qemu_irq_pulse(s->irq);
+}
+
+static void acpi_ged_device_realize(DeviceState *dev, Error **errp)
+{
+    AcpiGedState *s = ACPI_GED(dev);
+
+    assert(s->ged_base);
+    acpi_ged_init(get_system_memory(), dev, &s->ged_state);
+
+    if (s->memhp_state.is_enabled) {
+        assert(s->memhp_base);
+        acpi_memory_hotplug_init(get_system_memory(), OBJECT(dev),
+                                 &s->memhp_state,
+                                 s->memhp_base);
+    }
+}
+
+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),
+    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..f0152b0018
--- /dev/null
+++ b/include/hw/acpi/generic_event_device.h
@@ -0,0 +1,100 @@ 
+/*
+ *
+ * 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 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;
+} 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