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

2019-05-17 Thread Shameerali Kolothum Thodi 



> -Original Message-
> From: Igor Mammedov [mailto:imamm...@redhat.com]
> Sent: 17 May 2019 09:41
> To: Shameerali Kolothum Thodi 
> Cc: peter.mayd...@linaro.org; sa...@linux.intel.com;
> shannon.zha...@gmail.com; ard.biesheu...@linaro.org;
> qemu-devel@nongnu.org; Linuxarm ;
> eric.au...@redhat.com; qemu-...@nongnu.org; xuwei (O)
> ; sebastien.bo...@intel.com; ler...@redhat.com
> Subject: Re: [PATCH v4 3/8] hw/acpi: Add ACPI Generic Event Device Support
> 
> On Mon, 13 May 2019 17:00:13 +
> Shameerali Kolothum Thodi  wrote:
> 
> > > -Original Message-
> > > From: Igor Mammedov [mailto:imamm...@redhat.com]
> > > Sent: 13 May 2019 17:25
> > > To: Shameerali Kolothum Thodi 
> > > Subject: Re: [PATCH v4 3/8] hw/acpi: Add ACPI Generic Event Device
> > > Support
> > >
> > > On Mon, 13 May 2019 11:53:38 +
> > > Shameerali Kolothum Thodi 
> wrote:
> > >
> > > > Hi Igor,
> > > >
> > > > > -Original Message-
> > > > > From: Shameerali Kolothum Thodi
> > > > > Sent: 03 May 2019 13:46
> > > > > To: 'Igor Mammedov' 
> > > > > Cc: qemu-devel@nongnu.org; qemu-...@nongnu.org;
> > > > > eric.au...@redhat.com; peter.mayd...@linaro.org;
> > > > > shannon.zha...@gmail.com; sa...@linux.intel.com;
> > > > > sebastien.bo...@intel.com; xuwei (O) ;
> > > > > ler...@redhat.com; ard.biesheu...@linaro.org; Linuxarm
> > > > > 
> > > > > Subject: RE: [PATCH v4 3/8] hw/acpi: Add ACPI Generic Event
> > > > > Device
> > > Support
> > > > >
> > > > > Hi Igor,
> > > > >
> > > > > > -Original Message-
> > > > > > From: Igor Mammedov [mailto:imamm...@redhat.com]
> > > > > > Sent: 02 May 2019 17:13
> > > > > > To: Shameerali Kolothum Thodi
> > > 
> > > > > > Cc: qemu-devel@nongnu.org; qemu-...@nongnu.org;
> > > > > > eric.au...@redhat.com; peter.mayd...@linaro.org;
> > > > > > shannon.zha...@gmail.com; sa...@linux.intel.com;
> > > > > > sebastien.bo...@intel.com; xuwei (O) ;
> > > > > > ler...@redhat.com; ard.biesheu...@linaro.org; Linuxarm
> > > > > > 
> > > > > > Subject: Re: [PATCH v4 3/8] hw/acpi: Add ACPI Generic Event
> > > > > > Device
> > > Support
> > > > > >
> > > >
> > > > [...]
> > > >
> > > > > > > +}
> > > > > > > +
> > > > > > > +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.
> > > >
> > > > I attempted to remove _PROP_PTR for "ged-events" and use
> > > > _PROP_LINK
> > > and
> > > > _set_link(),
> > > >
> > > >
> > > > diff --git a/hw/acpi/generic_event_device.c
> > > b/hw/acpi/generic_event_device.c
> > > > index 856ca04c01..978c8e088e 100644
> > > > --- a/hw/acpi/generic_event_device.c
> > > > +++ b/hw/acpi/generic_event_device.c
> > > > @@ -268,7 +268,8 @@ static Property acpi_ged_properties[] = {
> > > >  DEFINE_PROP_PTR("gsi", AcpiGedState, gsi),
> > > >  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_LINK("ged-events", AcpiGedState, ged_events,
> > > TYPE_ACPI_GED,
> > > > + GedEvent *),
> > > >  DEFINE_PROP_UINT32("ged-events-size", AcpiGedState,
> > > ged_events_size, 0),
> > > >  DEFINE_PROP_END_OF_LIST(),
> > > >  };
> > > > diff --git a/hw/arm/virt.c b/hw/arm/virt.c index
> > > > 8179b3e511..c89b7b7120 100644
> > > > --- a/hw/arm/virt.c
> > > > +++ b/hw/arm/virt.c
> > > > @@ -537,7 +537,8 @@ static inline DeviceState
> > > *create_acpi_ged(VirtMachineState *vms)
> > > >  qdev_prop_set_ptr(dev, "gsi", vms->gsi);
> > > >  qdev_prop_set_uint64(dev, "ged-base",
> > > vms->memmap[VIRT_ACPI_GED].base);
> > > >  qdev_prop_set_uint32(dev, "ged-irq",
> vms->irqmap[VIRT_ACPI_GED]);
> > > > -qdev_prop_set_ptr(dev, "ged-events", ged_events);
> > > > +object_property_set_link(OBJECT(dev), OBJECT(ged_events),
> > > "ged-events",
> > > > + _abort);
> > > >  qdev_prop_set_uint32(dev, "ged-events-size",
> > > ARRAY_SIZE(ged_events));
> > > >
> > > >  object_property_add_child(qdev_get_machine(), "acpi-ged",
> > > > diff --git a/include/hw/acpi/generic_event_device.h
> > > b/include/hw/acpi/generic_event_device.h
> > > > index 9c840d8064..588f4ecfba 100644
> > > > ---

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

2019-05-17 Thread Igor Mammedov 
On Mon, 13 May 2019 17:00:13 +
Shameerali Kolothum Thodi  wrote:

> > -Original Message-
> > From: Igor Mammedov [mailto:imamm...@redhat.com]
> > Sent: 13 May 2019 17:25
> > To: Shameerali Kolothum Thodi 
> > Subject: Re: [PATCH v4 3/8] hw/acpi: Add ACPI Generic Event Device Support
> > 
> > On Mon, 13 May 2019 11:53:38 +
> > Shameerali Kolothum Thodi  wrote:
> > 
> > > Hi Igor,
> > >
> > > > -Original Message-
> > > > From: Shameerali Kolothum Thodi
> > > > Sent: 03 May 2019 13:46
> > > > To: 'Igor Mammedov' 
> > > > Cc: qemu-devel@nongnu.org; qemu-...@nongnu.org;
> > > > eric.au...@redhat.com; peter.mayd...@linaro.org;
> > > > shannon.zha...@gmail.com; sa...@linux.intel.com;
> > > > sebastien.bo...@intel.com; xuwei (O) ;
> > > > ler...@redhat.com; ard.biesheu...@linaro.org; Linuxarm
> > > > 
> > > > Subject: RE: [PATCH v4 3/8] hw/acpi: Add ACPI Generic Event Device
> > Support
> > > >
> > > > Hi Igor,
> > > >
> > > > > -Original Message-
> > > > > From: Igor Mammedov [mailto:imamm...@redhat.com]
> > > > > Sent: 02 May 2019 17:13
> > > > > To: Shameerali Kolothum Thodi
> > 
> > > > > Cc: qemu-devel@nongnu.org; qemu-...@nongnu.org;
> > > > > eric.au...@redhat.com; peter.mayd...@linaro.org;
> > > > > shannon.zha...@gmail.com; sa...@linux.intel.com;
> > > > > sebastien.bo...@intel.com; xuwei (O) ;
> > > > > ler...@redhat.com; ard.biesheu...@linaro.org; Linuxarm
> > > > > 
> > > > > Subject: Re: [PATCH v4 3/8] hw/acpi: Add ACPI Generic Event Device
> > Support
> > > > >
> > >
> > > [...]
> > >
> > > > > > +}
> > > > > > +
> > > > > > +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.
> > >
> > > I attempted to remove _PROP_PTR for "ged-events" and use _PROP_LINK
> > and
> > > _set_link(),
> > >
> > >
> > > diff --git a/hw/acpi/generic_event_device.c
> > b/hw/acpi/generic_event_device.c
> > > index 856ca04c01..978c8e088e 100644
> > > --- a/hw/acpi/generic_event_device.c
> > > +++ b/hw/acpi/generic_event_device.c
> > > @@ -268,7 +268,8 @@ static Property acpi_ged_properties[] = {
> > >  DEFINE_PROP_PTR("gsi", AcpiGedState, gsi),
> > >  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_LINK("ged-events", AcpiGedState, ged_events,
> > TYPE_ACPI_GED,
> > > + GedEvent *),
> > >  DEFINE_PROP_UINT32("ged-events-size", AcpiGedState,
> > ged_events_size, 0),
> > >  DEFINE_PROP_END_OF_LIST(),
> > >  };
> > > diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> > > index 8179b3e511..c89b7b7120 100644
> > > --- a/hw/arm/virt.c
> > > +++ b/hw/arm/virt.c
> > > @@ -537,7 +537,8 @@ static inline DeviceState
> > *create_acpi_ged(VirtMachineState *vms)
> > >  qdev_prop_set_ptr(dev, "gsi", vms->gsi);
> > >  qdev_prop_set_uint64(dev, "ged-base",
> > vms->memmap[VIRT_ACPI_GED].base);
> > >  qdev_prop_set_uint32(dev, "ged-irq", vms->irqmap[VIRT_ACPI_GED]);
> > > -qdev_prop_set_ptr(dev, "ged-events", ged_events);
> > > +object_property_set_link(OBJECT(dev), OBJECT(ged_events),
> > "ged-events",
> > > + _abort);
> > >  qdev_prop_set_uint32(dev, "ged-events-size",
> > ARRAY_SIZE(ged_events));
> > >
> > >  object_property_add_child(qdev_get_machine(), "acpi-ged",
> > > diff --git a/include/hw/acpi/generic_event_device.h
> > b/include/hw/acpi/generic_event_device.h
> > > index 9c840d8064..588f4ecfba 100644
> > > --- a/include/hw/acpi/generic_event_device.h
> > > +++ b/include/hw/acpi/generic_event_device.h
> > > @@ -111,7 +111,7 @@ typedef struct AcpiGedState {
> > >  hwaddr ged_base;
> > >  GEDState ged_state;
> > >  uint32_t ged_irq;
> > > -void *ged_events;
> > > +GedEvent *ged_events;
> > >  uint32_t ged_events_size;
> > >  } AcpiGedState;
> > >
> > >
> > > And with this I get,
> > >
> > > Segmentation fault  (core dumped) ./qemu-system-aarch64-ged-v5
> > > -machine virt, -cpu cortex-a57 -machine type=virt -nographic -smp 1 -m
> > > 4G,maxmem=8G,slots=10 -drive if=none,file=ubuntu-est-5.0,id=fs -device
> > > virtio-blk-device,drive=fs -kernel Image_memhp_remove -bios
> > > QEMU_EFI_Release.fd -object

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

2019-05-13 Thread Shameerali Kolothum Thodi 
> -Original Message-
> From: Igor Mammedov [mailto:imamm...@redhat.com]
> Sent: 13 May 2019 17:25
> To: Shameerali Kolothum Thodi 
> Subject: Re: [PATCH v4 3/8] hw/acpi: Add ACPI Generic Event Device Support
> 
> On Mon, 13 May 2019 11:53:38 +
> Shameerali Kolothum Thodi  wrote:
> 
> > Hi Igor,
> >
> > > -Original Message-
> > > From: Shameerali Kolothum Thodi
> > > Sent: 03 May 2019 13:46
> > > To: 'Igor Mammedov' 
> > > Cc: qemu-devel@nongnu.org; qemu-...@nongnu.org;
> > > eric.au...@redhat.com; peter.mayd...@linaro.org;
> > > shannon.zha...@gmail.com; sa...@linux.intel.com;
> > > sebastien.bo...@intel.com; xuwei (O) ;
> > > ler...@redhat.com; ard.biesheu...@linaro.org; Linuxarm
> > > 
> > > Subject: RE: [PATCH v4 3/8] hw/acpi: Add ACPI Generic Event Device
> Support
> > >
> > > Hi Igor,
> > >
> > > > -Original Message-
> > > > From: Igor Mammedov [mailto:imamm...@redhat.com]
> > > > Sent: 02 May 2019 17:13
> > > > To: Shameerali Kolothum Thodi
> 
> > > > Cc: qemu-devel@nongnu.org; qemu-...@nongnu.org;
> > > > eric.au...@redhat.com; peter.mayd...@linaro.org;
> > > > shannon.zha...@gmail.com; sa...@linux.intel.com;
> > > > sebastien.bo...@intel.com; xuwei (O) ;
> > > > ler...@redhat.com; ard.biesheu...@linaro.org; Linuxarm
> > > > 
> > > > Subject: Re: [PATCH v4 3/8] hw/acpi: Add ACPI Generic Event Device
> Support
> > > >
> >
> > [...]
> >
> > > > > +}
> > > > > +
> > > > > +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.
> >
> > I attempted to remove _PROP_PTR for "ged-events" and use _PROP_LINK
> and
> > _set_link(),
> >
> >
> > diff --git a/hw/acpi/generic_event_device.c
> b/hw/acpi/generic_event_device.c
> > index 856ca04c01..978c8e088e 100644
> > --- a/hw/acpi/generic_event_device.c
> > +++ b/hw/acpi/generic_event_device.c
> > @@ -268,7 +268,8 @@ static Property acpi_ged_properties[] = {
> >  DEFINE_PROP_PTR("gsi", AcpiGedState, gsi),
> >  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_LINK("ged-events", AcpiGedState, ged_events,
> TYPE_ACPI_GED,
> > + GedEvent *),
> >  DEFINE_PROP_UINT32("ged-events-size", AcpiGedState,
> ged_events_size, 0),
> >  DEFINE_PROP_END_OF_LIST(),
> >  };
> > diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> > index 8179b3e511..c89b7b7120 100644
> > --- a/hw/arm/virt.c
> > +++ b/hw/arm/virt.c
> > @@ -537,7 +537,8 @@ static inline DeviceState
> *create_acpi_ged(VirtMachineState *vms)
> >  qdev_prop_set_ptr(dev, "gsi", vms->gsi);
> >  qdev_prop_set_uint64(dev, "ged-base",
> vms->memmap[VIRT_ACPI_GED].base);
> >  qdev_prop_set_uint32(dev, "ged-irq", vms->irqmap[VIRT_ACPI_GED]);
> > -qdev_prop_set_ptr(dev, "ged-events", ged_events);
> > +object_property_set_link(OBJECT(dev), OBJECT(ged_events),
> "ged-events",
> > + _abort);
> >  qdev_prop_set_uint32(dev, "ged-events-size",
> ARRAY_SIZE(ged_events));
> >
> >  object_property_add_child(qdev_get_machine(), "acpi-ged",
> > diff --git a/include/hw/acpi/generic_event_device.h
> b/include/hw/acpi/generic_event_device.h
> > index 9c840d8064..588f4ecfba 100644
> > --- a/include/hw/acpi/generic_event_device.h
> > +++ b/include/hw/acpi/generic_event_device.h
> > @@ -111,7 +111,7 @@ typedef struct AcpiGedState {
> >  hwaddr ged_base;
> >  GEDState ged_state;
> >  uint32_t ged_irq;
> > -void *ged_events;
> > +GedEvent *ged_events;
> >  uint32_t ged_events_size;
> >  } AcpiGedState;
> >
> >
> > And with this I get,
> >
> > Segmentation fault  (core dumped) ./qemu-system-aarch64-ged-v5
> > -machine virt, -cpu cortex-a57 -machine type=virt -nographic -smp 1 -m
> > 4G,maxmem=8G,slots=10 -drive if=none,file=ubuntu-est-5.0,id=fs -device
> > virtio-blk-device,drive=fs -kernel Image_memhp_remove -bios
> > QEMU_EFI_Release.fd -object memory-backend-ram,id=mem1,size=1G
> -device
> > pc-dimm,id=dimm1,memdev=mem1 -numa node,nodeid=0 -append
> > "console=ttyAMA0 root=/dev/vda rw acpi=force movable_node"
> >
> > It looks like struct pointer cannot be used directly and has to make a QOM
> object
> > for DEFINE_PROP_LINK use. Not sure there is an easy way for setting ptr
> property
>

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

2019-05-13 Thread Shameerali Kolothum Thodi 
Hi Igor,

> -Original Message-
> From: Shameerali Kolothum Thodi
> Sent: 03 May 2019 13:46
> To: 'Igor Mammedov' 
> Cc: qemu-devel@nongnu.org; qemu-...@nongnu.org;
> eric.au...@redhat.com; peter.mayd...@linaro.org;
> shannon.zha...@gmail.com; sa...@linux.intel.com;
> sebastien.bo...@intel.com; xuwei (O) ;
> ler...@redhat.com; ard.biesheu...@linaro.org; Linuxarm
> 
> Subject: RE: [PATCH v4 3/8] hw/acpi: Add ACPI Generic Event Device Support
> 
> Hi Igor,
> 
> > -Original Message-
> > From: Igor Mammedov [mailto:imamm...@redhat.com]
> > Sent: 02 May 2019 17:13
> > To: Shameerali Kolothum Thodi 
> > Cc: qemu-devel@nongnu.org; qemu-...@nongnu.org;
> > eric.au...@redhat.com; peter.mayd...@linaro.org;
> > shannon.zha...@gmail.com; sa...@linux.intel.com;
> > sebastien.bo...@intel.com; xuwei (O) ;
> > ler...@redhat.com; ard.biesheu...@linaro.org; Linuxarm
> > 
> > Subject: Re: [PATCH v4 3/8] hw/acpi: Add ACPI Generic Event Device Support
> >

[...]

> > > +}
> > > +
> > > +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.

I attempted to remove _PROP_PTR for "ged-events" and use _PROP_LINK and
_set_link(),


diff --git a/hw/acpi/generic_event_device.c b/hw/acpi/generic_event_device.c
index 856ca04c01..978c8e088e 100644
--- a/hw/acpi/generic_event_device.c
+++ b/hw/acpi/generic_event_device.c
@@ -268,7 +268,8 @@ static Property acpi_ged_properties[] = {
 DEFINE_PROP_PTR("gsi", AcpiGedState, gsi),
 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_LINK("ged-events", AcpiGedState, ged_events, TYPE_ACPI_GED,
+ GedEvent *),
 DEFINE_PROP_UINT32("ged-events-size", AcpiGedState, ged_events_size, 0),
 DEFINE_PROP_END_OF_LIST(),
 };
diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index 8179b3e511..c89b7b7120 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -537,7 +537,8 @@ static inline DeviceState *create_acpi_ged(VirtMachineState 
*vms)
 qdev_prop_set_ptr(dev, "gsi", vms->gsi);
 qdev_prop_set_uint64(dev, "ged-base", vms->memmap[VIRT_ACPI_GED].base);
 qdev_prop_set_uint32(dev, "ged-irq", vms->irqmap[VIRT_ACPI_GED]);
-qdev_prop_set_ptr(dev, "ged-events", ged_events);
+object_property_set_link(OBJECT(dev), OBJECT(ged_events), "ged-events",
+ _abort);
 qdev_prop_set_uint32(dev, "ged-events-size", ARRAY_SIZE(ged_events));
 
 object_property_add_child(qdev_get_machine(), "acpi-ged",
diff --git a/include/hw/acpi/generic_event_device.h 
b/include/hw/acpi/generic_event_device.h
index 9c840d8064..588f4ecfba 100644
--- a/include/hw/acpi/generic_event_device.h
+++ b/include/hw/acpi/generic_event_device.h
@@ -111,7 +111,7 @@ typedef struct AcpiGedState {
 hwaddr ged_base;
 GEDState ged_state;
 uint32_t ged_irq;
-void *ged_events;
+GedEvent *ged_events;
 uint32_t ged_events_size;
 } AcpiGedState;


And with this I get,

Segmentation fault  (core dumped) ./qemu-system-aarch64-ged-v5
-machine virt, -cpu cortex-a57 -machine type=virt -nographic -smp 1 -m
4G,maxmem=8G,slots=10 -drive if=none,file=ubuntu-est-5.0,id=fs -device
virtio-blk-device,drive=fs -kernel Image_memhp_remove -bios
QEMU_EFI_Release.fd -object memory-backend-ram,id=mem1,size=1G -device
pc-dimm,id=dimm1,memdev=mem1 -numa node,nodeid=0 -append
"console=ttyAMA0 root=/dev/vda rw acpi=force movable_node"

It looks like struct pointer cannot be used directly and has to make a QOM 
object
for DEFINE_PROP_LINK use. Not sure there is an easy way for setting ptr property
using link() functions. Please let me know if there any reference 
implementation I
can take a look.

Appreciate your help,

Thanks,
Shameer

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

2019-05-09 Thread Igor Mammedov 
On Tue, 7 May 2019 09:01:43 +
Shameerali Kolothum Thodi  wrote:

> Hi Igor,
> 
> > -Original Message-
> > From: Igor Mammedov [mailto:imamm...@redhat.com]
> > Sent: 03 May 2019 16:10
> > To: Shameerali Kolothum Thodi 
> > Cc: qemu-devel@nongnu.org; qemu-...@nongnu.org;
> > eric.au...@redhat.com; peter.mayd...@linaro.org;
> > shannon.zha...@gmail.com; sa...@linux.intel.com;
> > sebastien.bo...@intel.com; xuwei (O) ;
> > ler...@redhat.com; ard.biesheu...@linaro.org; Linuxarm
> > 
> > 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(_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(_st->lock);
> > > > > +val = ged_st->sel;
> > > > > +ged_st->sel = 0;
> > > > > +qemu_mutex_unlock(_st->lock);
> > > > > +break;
> > > > > +default:
> > > > > +break;
> > > > > +}
> > > > > +
> > > > > +return val;
> > > > > +}
> > > > > +
> > > > > +/* Nothing is expected to be written to the GED memory region */
> > > > > +static void ged_write(void

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

2019-05-07 Thread Shameerali Kolothum Thodi 
Hi Igor,

> -Original Message-
> From: Igor Mammedov [mailto:imamm...@redhat.com]
> Sent: 03 May 2019 16:10
> To: Shameerali Kolothum Thodi 
> Cc: qemu-devel@nongnu.org; qemu-...@nongnu.org;
> eric.au...@redhat.com; peter.mayd...@linaro.org;
> shannon.zha...@gmail.com; sa...@linux.intel.com;
> sebastien.bo...@intel.com; xuwei (O) ;
> ler...@redhat.com; ard.biesheu...@linaro.org; Linuxarm
> 
> 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(_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(_st->lock);
> > > > +val = ged_st->sel;
> > > > +ged_st->sel = 0;
> > > > +qemu_mutex_unlock(_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,
> > > > +

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

2019-05-03 Thread Igor Mammedov 
On Fri, 3 May 2019 12:45:52 +
Shameerali Kolothum Thodi  wrote:

> Hi Igor,
> 
> > -Original Message-
> > From: Igor Mammedov [mailto:imamm...@redhat.com]
> > Sent: 02 May 2019 17:13
> > To: Shameerali Kolothum Thodi 
> > Cc: qemu-devel@nongnu.org; qemu-...@nongnu.org;
> > eric.au...@redhat.com; peter.mayd...@linaro.org;
> > shannon.zha...@gmail.com; sa...@linux.intel.com;
> > sebastien.bo...@intel.com; xuwei (O) ;
> > ler...@redhat.com; ard.biesheu...@linaro.org; Linuxarm
> > 
> > Subject: Re: [PATCH v4 3/8] hw/acpi: Add ACPI Generic Event Device Support
> > 
> > On Tue, 9 Apr 2019 11:29:30 +0100
> > Shameer Kolothum  wrote:
> > 
> > > From: Samuel Ortiz 
> > >
> > > 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 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 supports only memory hotplug for now.
> > >
> > > Signed-off-by: Samuel Ortiz 
> > > Signed-off-by: Sebastien Boeuf 
> > > Signed-off-by: Shameer Kolothum 
> > > ---
> > >  hw/acpi/Kconfig|   4 +
> > >  hw/acpi/Makefile.objs  |   1 +
> > >  hw/acpi/generic_event_device.c | 311
> > +
> > >  include/hw/acpi/generic_event_device.h | 121 +
> > >  4 files changed, 437 insertions(+)
> > >  create mode 100644 hw/acpi/generic_event_device.c
> > >  create mode 100644 include/hw/acpi/generic_event_device.h
> > >
> > > diff --git a/hw/acpi/Kconfig b/hw/acpi/Kconfig
> > > index eca3bee..01a8b41 100644
> > > --- a/hw/acpi/Kconfig
> > > +++ b/hw/acpi/Kconfig
> > > @@ -27,3 +27,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 2d46e37..b753232 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 000..856ca04
> > > --- /dev/null
> > > +++ b/hw/acpi/generic_event_device.c
> > > @@ -0,0 +1,311 @@
> > > +/*
> > > + *
> > > + * 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 .
> > > + */
> > > +
> > > +#include "qemu/osdep.h"
> > > +#include "qapi/error.h"
> > > +#include "exec/address-spaces.h"
> > > +#include "hw/sysbus.h"
> > > +#include "hw/acpi/acpi.h"
> > > +#include "hw/acpi/generic_event_device.h"
> > > +#include "hw/mem/pc-dimm.h"
> > > +
> > > +static Aml *ged_event_aml(const GedEvent *event)
> > > +{
> > > +
> > > +if (!event) {
> > In general, I prefer to check condition for calling something before doing 
> > call.
> > This way one can see in caller why and what is called, which is more clear.
> 
> Ok. I will move it then.
> 
> > 
> > > +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,
> >

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

2019-05-03 Thread Shameerali Kolothum Thodi 
Hi Igor,

> -Original Message-
> From: Igor Mammedov [mailto:imamm...@redhat.com]
> Sent: 02 May 2019 17:13
> To: Shameerali Kolothum Thodi 
> Cc: qemu-devel@nongnu.org; qemu-...@nongnu.org;
> eric.au...@redhat.com; peter.mayd...@linaro.org;
> shannon.zha...@gmail.com; sa...@linux.intel.com;
> sebastien.bo...@intel.com; xuwei (O) ;
> ler...@redhat.com; ard.biesheu...@linaro.org; Linuxarm
> 
> Subject: Re: [PATCH v4 3/8] hw/acpi: Add ACPI Generic Event Device Support
> 
> On Tue, 9 Apr 2019 11:29:30 +0100
> Shameer Kolothum  wrote:
> 
> > From: Samuel Ortiz 
> >
> > 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 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 supports only memory hotplug for now.
> >
> > Signed-off-by: Samuel Ortiz 
> > Signed-off-by: Sebastien Boeuf 
> > Signed-off-by: Shameer Kolothum 
> > ---
> >  hw/acpi/Kconfig|   4 +
> >  hw/acpi/Makefile.objs  |   1 +
> >  hw/acpi/generic_event_device.c | 311
> +
> >  include/hw/acpi/generic_event_device.h | 121 +
> >  4 files changed, 437 insertions(+)
> >  create mode 100644 hw/acpi/generic_event_device.c
> >  create mode 100644 include/hw/acpi/generic_event_device.h
> >
> > diff --git a/hw/acpi/Kconfig b/hw/acpi/Kconfig
> > index eca3bee..01a8b41 100644
> > --- a/hw/acpi/Kconfig
> > +++ b/hw/acpi/Kconfig
> > @@ -27,3 +27,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 2d46e37..b753232 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 000..856ca04
> > --- /dev/null
> > +++ b/hw/acpi/generic_event_device.c
> > @@ -0,0 +1,311 @@
> > +/*
> > + *
> > + * 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 .
> > + */
> > +
> > +#include "qemu/osdep.h"
> > +#include "qapi/error.h"
> > +#include "exec/address-spaces.h"
> > +#include "hw/sysbus.h"
> > +#include "hw/acpi/acpi.h"
> > +#include "hw/acpi/generic_event_device.h"
> > +#include "hw/mem/pc-dimm.h"
> > +
> > +static Aml *ged_event_aml(const GedEvent *event)
> > +{
> > +
> > +if (!event) {
> In general, I prefer to check condition for calling something before doing 
> call.
> This way one can see in caller why and what is called, which is more clear.

Ok. I will move it then.

> 
> > +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

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

2019-05-02 Thread Igor Mammedov 
On Tue, 9 Apr 2019 11:29:30 +0100
Shameer Kolothum  wrote:

> From: Samuel Ortiz 
> 
> 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 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 supports only memory hotplug for now.
> 
> Signed-off-by: Samuel Ortiz 
> Signed-off-by: Sebastien Boeuf 
> Signed-off-by: Shameer Kolothum 
> ---
>  hw/acpi/Kconfig|   4 +
>  hw/acpi/Makefile.objs  |   1 +
>  hw/acpi/generic_event_device.c | 311 
> +
>  include/hw/acpi/generic_event_device.h | 121 +
>  4 files changed, 437 insertions(+)
>  create mode 100644 hw/acpi/generic_event_device.c
>  create mode 100644 include/hw/acpi/generic_event_device.h
> 
> diff --git a/hw/acpi/Kconfig b/hw/acpi/Kconfig
> index eca3bee..01a8b41 100644
> --- a/hw/acpi/Kconfig
> +++ b/hw/acpi/Kconfig
> @@ -27,3 +27,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 2d46e37..b753232 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 000..856ca04
> --- /dev/null
> +++ b/hw/acpi/generic_event_device.c
> @@ -0,0 +1,311 @@
> +/*
> + *
> + * 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 .
> + */
> +
> +#include "qemu/osdep.h"
> +#include "qapi/error.h"
> +#include "exec/address-spaces.h"
> +#include "hw/sysbus.h"
> +#include "hw/acpi/acpi.h"
> +#include "hw/acpi/generic_event_device.h"
> +#include "hw/mem/pc-dimm.h"
> +
> +static Aml *ged_event_aml(const GedEvent *event)
> +{
> +
> +if (!event) {
In general, I prefer to check condition for calling something before doing call.
This way one can see in caller why and what is called, which is more clear.

> +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, HotplugHandler *hotplug_dev,
> +   uint32_t ged_irq, AmlRegionSpace rs)
> +{
> +AcpiGedState *s = ACPI_GED(hotplug_dev);
> +GedEvent *ged_events = s->ged_events;
> +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 (!s->ged_base || !ged_events || !s->ged_events_size) {
I'd move it up to the caller, it would be obvious right there when
function should be called.

> +return;
> +}
> +
> +/* _CRS interrupt */
> +

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

2019-05-02 Thread Igor Mammedov 
On Thu, 2 May 2019 09:22:35 +0200
Ard Biesheuvel  wrote:

> On Wed, 1 May 2019 at 13:25, Shameerali Kolothum Thodi
>  wrote:
> >
> > Hi Ard,
> >
> > > -Original Message-
> > > From: Ard Biesheuvel [mailto:ard.biesheu...@linaro.org]
> > > Sent: 01 May 2019 12:10
> > > To: Shameerali Kolothum Thodi 
> > > Cc: QEMU Developers ; qemu-arm
> > > ; Auger Eric ; Igor
> > > Mammedov ; Peter Maydell
> > > ; shannon.zha...@gmail.com;
> > > sa...@linux.intel.com; sebastien.bo...@intel.com; xuwei (O)
> > > ; Laszlo Ersek ; Linuxarm
> > > 
> > > Subject: Re: [PATCH v4 3/8] hw/acpi: Add ACPI Generic Event Device Support
> > >
> > > On Tue, 9 Apr 2019 at 12:31, Shameer Kolothum
> > >  wrote:
> > > >
> > > > From: Samuel Ortiz 
> > > >
> > > > 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 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 supports only memory hotplug for now.
> > > >
> > > > Signed-off-by: Samuel Ortiz 
> > > > Signed-off-by: Sebastien Boeuf 
> > > > Signed-off-by: Shameer Kolothum 
> > >
> > > Apologies if this question has been raised before, but do we really
> > > need a separate device for this? We already handle the power button
> > > via _AEI/_Exx on the GPIO device, and I think we should be able to add
> > > additional events using that interface, rather than have two event
> > > signalling methods/devices on the same platform.
> >
> > Right. The initial RFC was based on GPIO device[1] and later Igor commented
> > here[2] that,
> >
> > " ARM boards were first to use ACPI hw-reduced profile so they picked up
> > available back then GPIO based way to deliver hotplug event, later spec
> > introduced Generic Event Device for that means to use with hw-reduced
> > profile, which NEMU implemented[1], so I'd use that rather than ad-hoc
> > GPIO mapping. I'd guess it will more compatible with various contemporary
> > guests and we could reuse the same code for both x86/arm virt boards) "
> >
> 
> On mach-virt, we already use the GPIO controller for the ACPI event
> involving the power button, so by aligning with virt-x86, we end up in
> the opposite situation for mach-virt.
> 
> The problem with the GED device is that it only supports GSI
> interrupts, while the GPIO device supports arbitrary interrupts (such
> as GPIO signalled ones). This means that mach-virt will be stuck with
> having two different methods of signalling ACPI events, unless we
> rewire the power button not to use a GPIO interrupt but use a GSI
> directly.

we can rewire power button then.


> In general, I think the ACPI event delivery mechanism doesn't really
> have to be aligned: the ACPI event is ultimately converted into a AML
> notification to the right device, and how we end up there can remain
> platform specific.

Reasoning for using GED is to reduce code duplication with x86
and not creating zoo of different approached (if it could be avoided).

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

2019-05-02 Thread Ard Biesheuvel 
On Wed, 1 May 2019 at 13:25, Shameerali Kolothum Thodi
 wrote:
>
> Hi Ard,
>
> > -Original Message-
> > From: Ard Biesheuvel [mailto:ard.biesheu...@linaro.org]
> > Sent: 01 May 2019 12:10
> > To: Shameerali Kolothum Thodi 
> > Cc: QEMU Developers ; qemu-arm
> > ; Auger Eric ; Igor
> > Mammedov ; Peter Maydell
> > ; shannon.zha...@gmail.com;
> > sa...@linux.intel.com; sebastien.bo...@intel.com; xuwei (O)
> > ; Laszlo Ersek ; Linuxarm
> > 
> > Subject: Re: [PATCH v4 3/8] hw/acpi: Add ACPI Generic Event Device Support
> >
> > On Tue, 9 Apr 2019 at 12:31, Shameer Kolothum
> >  wrote:
> > >
> > > From: Samuel Ortiz 
> > >
> > > 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 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 supports only memory hotplug for now.
> > >
> > > Signed-off-by: Samuel Ortiz 
> > > Signed-off-by: Sebastien Boeuf 
> > > Signed-off-by: Shameer Kolothum 
> >
> > Apologies if this question has been raised before, but do we really
> > need a separate device for this? We already handle the power button
> > via _AEI/_Exx on the GPIO device, and I think we should be able to add
> > additional events using that interface, rather than have two event
> > signalling methods/devices on the same platform.
>
> Right. The initial RFC was based on GPIO device[1] and later Igor commented
> here[2] that,
>
> " ARM boards were first to use ACPI hw-reduced profile so they picked up
> available back then GPIO based way to deliver hotplug event, later spec
> introduced Generic Event Device for that means to use with hw-reduced
> profile, which NEMU implemented[1], so I'd use that rather than ad-hoc
> GPIO mapping. I'd guess it will more compatible with various contemporary
> guests and we could reuse the same code for both x86/arm virt boards) "
>

On mach-virt, we already use the GPIO controller for the ACPI event
involving the power button, so by aligning with virt-x86, we end up in
the opposite situation for mach-virt.

The problem with the GED device is that it only supports GSI
interrupts, while the GPIO device supports arbitrary interrupts (such
as GPIO signalled ones). This means that mach-virt will be stuck with
having two different methods of signalling ACPI events, unless we
rewire the power button not to use a GPIO interrupt but use a GSI
directly.

In general, I think the ACPI event delivery mechanism doesn't really
have to be aligned: the ACPI event is ultimately converted into a AML
notification to the right device, and how we end up there can remain
platform specific.

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

2019-05-02 Thread Auger Eric 
Hi Shameer,

On 5/1/19 12:40 PM, Shameerali Kolothum Thodi wrote:
> Hi Eric,
> 
>> -Original Message-
>> From: Auger Eric [mailto:eric.au...@redhat.com]
>> Sent: 30 April 2019 16:50
>> To: Shameerali Kolothum Thodi ;
>> qemu-devel@nongnu.org; qemu-...@nongnu.org; imamm...@redhat.com
>> Cc: peter.mayd...@linaro.org; shannon.zha...@gmail.com;
>> sa...@linux.intel.com; sebastien.bo...@intel.com; xuwei (O)
>> ; ler...@redhat.com; ard.biesheu...@linaro.org;
>> Linuxarm 
>> Subject: Re: [PATCH v4 3/8] hw/acpi: Add ACPI Generic Event Device Support
>>
>> Hi Shameer,
>>
>> On 4/9/19 12:29 PM, Shameer Kolothum wrote:
>>> From: Samuel Ortiz 
>>>
>>> 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 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 supports only memory hotplug for now.
>>>
>>> Signed-off-by: Samuel Ortiz 
>>> Signed-off-by: Sebastien Boeuf 
>>> Signed-off-by: Shameer Kolothum 
>>> ---
>>>  hw/acpi/Kconfig|   4 +
>>>  hw/acpi/Makefile.objs  |   1 +
>>>  hw/acpi/generic_event_device.c | 311
>> +
>>>  include/hw/acpi/generic_event_device.h | 121 +
>>>  4 files changed, 437 insertions(+)
>>>  create mode 100644 hw/acpi/generic_event_device.c
>>>  create mode 100644 include/hw/acpi/generic_event_device.h
>>>
>>> diff --git a/hw/acpi/Kconfig b/hw/acpi/Kconfig
>>> index eca3bee..01a8b41 100644
>>> --- a/hw/acpi/Kconfig
>>> +++ b/hw/acpi/Kconfig
>>> @@ -27,3 +27,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 2d46e37..b753232 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 000..856ca04
>>> --- /dev/null
>>> +++ b/hw/acpi/generic_event_device.c
>>> @@ -0,0 +1,311 @@
>>> +/*
>>> + *
>>> + * 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.
>> I am not sure we need below statements: see hw/misc/armsse-mhu.c for a
>> recent added file.
> 
> Ok. I will get rid of this.
>  
>>> + *
>>> + * 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 .
>>> + */
>>> +
>>> +#include "qemu/osdep.h"
>>> +#include "qapi/error.h"
>>> +#include "exec/address-spaces.h"
>>> +#include "hw/sysbus.h"
>>> +#include "hw/acpi/acpi.h"
>>> +#include "hw/acpi/generic_event_device.h"
>>> +#include "hw/mem/pc-dimm.h"
>>> +
>>> +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.

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

2019-05-01 Thread Shameerali Kolothum Thodi 
Hi Ard,

> -Original Message-
> From: Ard Biesheuvel [mailto:ard.biesheu...@linaro.org]
> Sent: 01 May 2019 12:10
> To: Shameerali Kolothum Thodi 
> Cc: QEMU Developers ; qemu-arm
> ; Auger Eric ; Igor
> Mammedov ; Peter Maydell
> ; shannon.zha...@gmail.com;
> sa...@linux.intel.com; sebastien.bo...@intel.com; xuwei (O)
> ; Laszlo Ersek ; Linuxarm
> 
> Subject: Re: [PATCH v4 3/8] hw/acpi: Add ACPI Generic Event Device Support
> 
> On Tue, 9 Apr 2019 at 12:31, Shameer Kolothum
>  wrote:
> >
> > From: Samuel Ortiz 
> >
> > 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 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 supports only memory hotplug for now.
> >
> > Signed-off-by: Samuel Ortiz 
> > Signed-off-by: Sebastien Boeuf 
> > Signed-off-by: Shameer Kolothum 
> 
> Apologies if this question has been raised before, but do we really
> need a separate device for this? We already handle the power button
> via _AEI/_Exx on the GPIO device, and I think we should be able to add
> additional events using that interface, rather than have two event
> signalling methods/devices on the same platform.

Right. The initial RFC was based on GPIO device[1] and later Igor commented
here[2] that,

" ARM boards were first to use ACPI hw-reduced profile so they picked up
available back then GPIO based way to deliver hotplug event, later spec
introduced Generic Event Device for that means to use with hw-reduced
profile, which NEMU implemented[1], so I'd use that rather than ad-hoc
GPIO mapping. I'd guess it will more compatible with various contemporary
guests and we could reuse the same code for both x86/arm virt boards) "

Thanks,
Shameer

[1]. https://patchwork.kernel.org/cover/10783589/
[2] http://patchwork.ozlabs.org/cover/1045604/

> 
> 
> > ---
> >  hw/acpi/Kconfig|   4 +
> >  hw/acpi/Makefile.objs  |   1 +
> >  hw/acpi/generic_event_device.c | 311
> +
> >  include/hw/acpi/generic_event_device.h | 121 +
> >  4 files changed, 437 insertions(+)
> >  create mode 100644 hw/acpi/generic_event_device.c
> >  create mode 100644 include/hw/acpi/generic_event_device.h
> >
> > diff --git a/hw/acpi/Kconfig b/hw/acpi/Kconfig
> > index eca3bee..01a8b41 100644
> > --- a/hw/acpi/Kconfig
> > +++ b/hw/acpi/Kconfig
> > @@ -27,3 +27,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 2d46e37..b753232 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 000..856ca04
> > --- /dev/null
> > +++ b/hw/acpi/generic_event_device.c
> > @@ -0,0 +1,311 @@
> > +/*
> > + *
> > + * 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 .
> > + */
> > +
> > +#include "qemu/osdep.h"
> > +#include "qapi/error.h"
> > +#include "exec/address-spaces.h"
> > +#include "hw/sysbus.h"
> > +#include "hw/acpi/acpi.h"
> > +#include "hw/acpi/generic_event_device.h"
> > +#include "hw/mem/pc-dimm.h"
> > +
> > +static Aml *ged_event_aml(const GedEvent *event)
> > +{
> > +
> > +if (!event) {
> > +return NULL;
> > +}
> > +
> > +switch (event->event) {
> > +case GED_MEMORY_HOTPLUG:
> > +/* We run a complete

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

2019-05-01 Thread Ard Biesheuvel 
On Tue, 9 Apr 2019 at 12:31, Shameer Kolothum
 wrote:
>
> From: Samuel Ortiz 
>
> 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 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 supports only memory hotplug for now.
>
> Signed-off-by: Samuel Ortiz 
> Signed-off-by: Sebastien Boeuf 
> Signed-off-by: Shameer Kolothum 

Apologies if this question has been raised before, but do we really
need a separate device for this? We already handle the power button
via _AEI/_Exx on the GPIO device, and I think we should be able to add
additional events using that interface, rather than have two event
signalling methods/devices on the same platform.



> ---
>  hw/acpi/Kconfig|   4 +
>  hw/acpi/Makefile.objs  |   1 +
>  hw/acpi/generic_event_device.c | 311 
> +
>  include/hw/acpi/generic_event_device.h | 121 +
>  4 files changed, 437 insertions(+)
>  create mode 100644 hw/acpi/generic_event_device.c
>  create mode 100644 include/hw/acpi/generic_event_device.h
>
> diff --git a/hw/acpi/Kconfig b/hw/acpi/Kconfig
> index eca3bee..01a8b41 100644
> --- a/hw/acpi/Kconfig
> +++ b/hw/acpi/Kconfig
> @@ -27,3 +27,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 2d46e37..b753232 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 000..856ca04
> --- /dev/null
> +++ b/hw/acpi/generic_event_device.c
> @@ -0,0 +1,311 @@
> +/*
> + *
> + * 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 .
> + */
> +
> +#include "qemu/osdep.h"
> +#include "qapi/error.h"
> +#include "exec/address-spaces.h"
> +#include "hw/sysbus.h"
> +#include "hw/acpi/acpi.h"
> +#include "hw/acpi/generic_event_device.h"
> +#include "hw/mem/pc-dimm.h"
> +
> +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, HotplugHandler *hotplug_dev,
> +   uint32_t ged_irq, AmlRegionSpace rs)
> +{
> +AcpiGedState *s = ACPI_GED(hotplug_dev);
> +GedEvent *ged_events = s->ged_events;
> +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 (!s->ged_base || !ged_events || !s->ged_events_size) {
> +

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

2019-05-01 Thread Shameerali Kolothum Thodi 
Hi Eric,

> -Original Message-
> From: Auger Eric [mailto:eric.au...@redhat.com]
> Sent: 30 April 2019 16:50
> To: Shameerali Kolothum Thodi ;
> qemu-devel@nongnu.org; qemu-...@nongnu.org; imamm...@redhat.com
> Cc: peter.mayd...@linaro.org; shannon.zha...@gmail.com;
> sa...@linux.intel.com; sebastien.bo...@intel.com; xuwei (O)
> ; ler...@redhat.com; ard.biesheu...@linaro.org;
> Linuxarm 
> Subject: Re: [PATCH v4 3/8] hw/acpi: Add ACPI Generic Event Device Support
> 
> Hi Shameer,
> 
> On 4/9/19 12:29 PM, Shameer Kolothum wrote:
> > From: Samuel Ortiz 
> >
> > 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 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 supports only memory hotplug for now.
> >
> > Signed-off-by: Samuel Ortiz 
> > Signed-off-by: Sebastien Boeuf 
> > Signed-off-by: Shameer Kolothum 
> > ---
> >  hw/acpi/Kconfig|   4 +
> >  hw/acpi/Makefile.objs  |   1 +
> >  hw/acpi/generic_event_device.c | 311
> +
> >  include/hw/acpi/generic_event_device.h | 121 +
> >  4 files changed, 437 insertions(+)
> >  create mode 100644 hw/acpi/generic_event_device.c
> >  create mode 100644 include/hw/acpi/generic_event_device.h
> >
> > diff --git a/hw/acpi/Kconfig b/hw/acpi/Kconfig
> > index eca3bee..01a8b41 100644
> > --- a/hw/acpi/Kconfig
> > +++ b/hw/acpi/Kconfig
> > @@ -27,3 +27,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 2d46e37..b753232 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 000..856ca04
> > --- /dev/null
> > +++ b/hw/acpi/generic_event_device.c
> > @@ -0,0 +1,311 @@
> > +/*
> > + *
> > + * 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.
> I am not sure we need below statements: see hw/misc/armsse-mhu.c for a
> recent added file.

Ok. I will get rid of this.
 
> > + *
> > + * 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 .
> > + */
> > +
> > +#include "qemu/osdep.h"
> > +#include "qapi/error.h"
> > +#include "exec/address-spaces.h"
> > +#include "hw/sysbus.h"
> > +#include "hw/acpi/acpi.h"
> > +#include "hw/acpi/generic_event_device.h"
> > +#include "hw/mem/pc-dimm.h"
> > +
> > +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

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

2019-04-30 Thread Auger Eric 
Hi Shameer,

On 4/9/19 12:29 PM, Shameer Kolothum wrote:
> From: Samuel Ortiz 
> 
> 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 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 supports only memory hotplug for now.
> 
> Signed-off-by: Samuel Ortiz 
> Signed-off-by: Sebastien Boeuf 
> Signed-off-by: Shameer Kolothum 
> ---
>  hw/acpi/Kconfig|   4 +
>  hw/acpi/Makefile.objs  |   1 +
>  hw/acpi/generic_event_device.c | 311 
> +
>  include/hw/acpi/generic_event_device.h | 121 +
>  4 files changed, 437 insertions(+)
>  create mode 100644 hw/acpi/generic_event_device.c
>  create mode 100644 include/hw/acpi/generic_event_device.h
> 
> diff --git a/hw/acpi/Kconfig b/hw/acpi/Kconfig
> index eca3bee..01a8b41 100644
> --- a/hw/acpi/Kconfig
> +++ b/hw/acpi/Kconfig
> @@ -27,3 +27,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 2d46e37..b753232 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 000..856ca04
> --- /dev/null
> +++ b/hw/acpi/generic_event_device.c
> @@ -0,0 +1,311 @@
> +/*
> + *
> + * 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.
I am not sure we need below statements: see hw/misc/armsse-mhu.c for a
recent added file.

> + *
> + * 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 .
> + */
> +
> +#include "qemu/osdep.h"
> +#include "qapi/error.h"
> +#include "exec/address-spaces.h"
> +#include "hw/sysbus.h"
> +#include "hw/acpi/acpi.h"
> +#include "hw/acpi/generic_event_device.h"
> +#include "hw/mem/pc-dimm.h"
> +
> +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, HotplugHandler *hotplug_dev,
> +   uint32_t ged_irq, AmlRegionSpace rs)
> +{
> +AcpiGedState *s = ACPI_GED(hotplug_dev);
> +GedEvent *ged_events = s->ged_events;
> +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 (!s->ged_base || !ged_events || !s->ged_events_size) {
> +return;
> +}
> +
> +/* _CRS interrupt */
> +aml_append(crs, aml_interrupt(AML_CONSUMER, AML_EDGE, AML_ACTIVE_HIGH,
> +  AML_EXCLUSIVE, _irq, 1));
> +/*
> + * For each GED