> -----Original Message-----
> From: Igor Mammedov [mailto:imamm...@redhat.com]
> Sent: 18 July 2019 13:31
> To: Shameerali Kolothum Thodi <shameerali.kolothum.th...@huawei.com>
> Cc: peter.mayd...@linaro.org; sa...@linux.intel.com;
> shannon.zha...@gmail.com; ard.biesheu...@linaro.org;
> qemu-devel@nongnu.org; xuwei (O) <xuw...@huawei.com>; Linuxarm
> <linux...@huawei.com>; eric.au...@redhat.com; qemu-...@nongnu.org;
> sebastien.bo...@intel.com; ler...@redhat.com
> Subject: Re: [Qemu-devel] [PATCH-for-4.2 v7 03/10] hw/acpi: Add ACPI Generic
> Event Device Support
>
> On Thu, 18 Jul 2019 10:52:10 +0000
> Shameerali Kolothum Thodi <shameerali.kolothum.th...@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: 17 July 2019 15:33
> > > To: Shameerali Kolothum Thodi <shameerali.kolothum.th...@huawei.com>
> > > Cc: peter.mayd...@linaro.org; sa...@linux.intel.com;
> > > shannon.zha...@gmail.com; ard.biesheu...@linaro.org;
> > > qemu-devel@nongnu.org; xuwei (O) <xuw...@huawei.com>; Linuxarm
> > > <linux...@huawei.com>; eric.au...@redhat.com;
> qemu-...@nongnu.org;
> > > sebastien.bo...@intel.com; ler...@redhat.com
> > > Subject: Re: [Qemu-devel] [PATCH-for-4.2 v7 03/10] hw/acpi: Add ACPI
> Generic
> > > Event Device Support
> > >
> > > On Tue, 16 Jul 2019 16:38:09 +0100
> > > Shameer Kolothum <shameerali.kolothum.th...@huawei.com> wrote:
> >
> > [...]
> >
> > > > +static void acpi_ged_event(AcpiGedState *s, uint32_t sel)
> > > > +{
> > > > + GEDState *ged_st = &s->ged_state;
> > > > + /*
> > > > + * Set the GED IRQ selector to the expected device type value. This
> > > > + * way, the ACPI method will be able to trigger the right code
> > > > based
> > > > + * on a unique IRQ.
> > > comment isn't correct anymore, pls fix it
> >
> > True.
> >
> > >
> > > > + */
> > > > + qemu_mutex_lock(&ged_st->lock);
> > > Is this lock really necessary?
> > > (I thought that MMIO and monitor access is guarded by BQL)
> >
> > Hmm..I am not sure. This is to synchronize with the ged_st->sel update
> > inside
> > ged_read(). And also acpi_ged_event() gets called through
> _power_down_notifier()
> > as well. BQL guard is in place for all the paths here?
> power down command originates from HMP or QMP monitor, so you don't
> really
> need a lock here.
Ok. I will get rid of it then.
> >
> > >
> > > > + ged_st->sel |= sel;
> > > > + qemu_mutex_unlock(&ged_st->lock);
> > > > +
> > > > + /* Trigger the event by sending an interrupt to the guest. */
> > > > + qemu_irq_pulse(s->irq);
> > > > +}
> > > > +
> > > > +static void acpi_ged_init(MemoryRegion *as, DeviceState *dev,
> GEDState
> > > *ged_st)
> > > > +{
> > > > + AcpiGedState *s = ACPI_GED(dev);
> > > > +
> > > > + assert(s->ged_base);
> > > > +
> > > > + qemu_mutex_init(&ged_st->lock);
> > > > + memory_region_init_io(&ged_st->io, OBJECT(dev), &ged_ops,
> ged_st,
> > > > + TYPE_ACPI_GED, ACPI_GED_REG_LEN);
> > > > + memory_region_add_subregion(as, s->ged_base, &ged_st->io);
> > > > + qdev_init_gpio_out_named(DEVICE(s), &s->irq, "ged-irq", 1);
> > > > +}
> > > > +
> > > > +static void acpi_ged_device_plug_cb(HotplugHandler *hotplug_dev,
> > > > + DeviceState *dev, Error
> **errp)
> > > > +{
> > > > + AcpiGedState *s = ACPI_GED(hotplug_dev);
> > > > +
> > > > + if (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM)) {
> > > > + if (s->memhp_state.is_enabled) {
> > > > + acpi_memory_plug_cb(hotplug_dev, &s->memhp_state,
> dev,
> > > errp);
> > > > + } else {
> > > > + error_setg(errp,
> > > > + "memory hotplug is not
> > > enabled: %s.memory-hotplug-support "
> > > > + "is not set", object_get_typename(OBJECT(s)));
> > > > + }
> > > > + } else {
> > > > + error_setg(errp, "virt: device plug request for unsupported
> > > device"
> > > > + " type: %s",
> object_get_typename(OBJECT(dev)));
> > > > + }
> > > > +}
> > > > +
> > > > +static void acpi_ged_send_event(AcpiDeviceIf *adev,
> AcpiEventStatusBits
> > > ev)
> > > > +{
> > > > + AcpiGedState *s = ACPI_GED(adev);
> > > > + uint32_t sel;
> > > > +
> > > > + if (ev & ACPI_MEMORY_HOTPLUG_STATUS) {
> > > > + sel = ACPI_GED_MEM_HOTPLUG_EVT;
> > > > + } else {
> > > > + /* Unknown event. Return without generating interrupt. */
> > > > + warn_report("GED: Unsupported event %d. No irq injected",
> ev);
> > > > + return;
> > > > + }
> > > > +
> > > > + /*
> > > > + * We inject the hotplug interrupt. The IRQ selector will make
> > > > + * the difference from the ACPI table.
> > > I don't get comment at all, pls rephrase/
> >
> > Ok. I think better to get rid of this comment here and update the one in
> acpi_ged_event()
> > appropriately.
> >
> > >
> > > > + */
> > > > + acpi_ged_event(s, sel);
> > > it seems to used only once and only here, suggest to drop acpi_ged_event()
> > > and move it's code here.
> >
> > But patch #10 makes use of it from acpi_ged_pm_powerdown_req().
> it looks like a valid shortcut but I'd make it follow
> AcpiInterface->send_event()
> path for consistency so only one call chain would exist.
Agree.
Thanks,
Shameer
