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

[Igor Mammedov <imammedo@redhat.com>]

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