Re: [Qemu-devel] [PATCH-for-4.2 v7 03/10] hw/acpi: Add ACPI Generic Event Device Support

[Shameerali Kolothum Thodi]

> -----Original Message-----
> From: Igor Mammedov [mailto:address@hidden]
> Sent: 18 July 2019 13:31
> To: Shameerali Kolothum Thodi <address@hidden>
> Cc: address@hidden; address@hidden;
> address@hidden; address@hidden;
> address@hidden; xuwei (O) <address@hidden>; Linuxarm
> <address@hidden>; address@hidden; address@hidden;
> address@hidden; address@hidden
> Subject: Re: [Qemu-devel] [PATCH-for-4.2 v7 03/10] hw/acpi: Add ACPI Generic
> Event Device Support
> 
> On Thu, 18 Jul 2019 10:52:10 +0000
> Shameerali Kolothum Thodi <address@hidden> wrote:
> 
> > Hi Igor,
> >
> > > -----Original Message-----
> > > From: Qemu-devel
> > >
> [mailto:qemu-devel-bounces+shameerali.kolothum.thodi=huawei.com@nongn
> > > u.org] On Behalf Of Igor Mammedov
> > > Sent: 17 July 2019 15:33
> > > To: Shameerali Kolothum Thodi <address@hidden>
> > > Cc: address@hidden; address@hidden;
> > > address@hidden; address@hidden;
> > > address@hidden; xuwei (O) <address@hidden>; Linuxarm
> > > <address@hidden>; address@hidden;
> address@hidden;
> > > address@hidden; address@hidden
> > > Subject: Re: [Qemu-devel] [PATCH-for-4.2 v7 03/10] hw/acpi: Add ACPI
> Generic
> > > Event Device Support
> > >
> > > On Tue, 16 Jul 2019 16:38:09 +0100
> > > Shameer Kolothum <address@hidden> wrote:
> >
> > [...]
> >
> > > > +static void acpi_ged_event(AcpiGedState *s, uint32_t sel)
> > > > +{
> > > > +    GEDState *ged_st = &s->ged_state;
> > > > +    /*
> > > > +     * Set the GED IRQ selector to the expected device type value. This
> > > > +     * way, the ACPI method will be able to trigger the right code 
> > > > based
> > > > +     * on a unique IRQ.
> > > comment isn't correct anymore, pls fix it
> >
> > True.
> >
> > >
> > > > +     */
> > > > +    qemu_mutex_lock(&ged_st->lock);
> > > Is this lock really necessary?
> > > (I thought that MMIO and monitor access is guarded by BQL)
> >
> > Hmm..I am not sure. This is to synchronize with the ged_st->sel update 
> > inside
> > ged_read(). And also acpi_ged_event() gets called through
> _power_down_notifier()
> > as well. BQL guard is in place for all the paths here?
> power down command originates from HMP or QMP monitor, so you don't
> really
> need a lock here.

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