Re: [Qemu-devel] [PATCH v3 03/10] hw/arm/virt: Add virtual ACPI device

[Shameerali Kolothum Thodi]

Hi Igor,

> -----Original Message-----
> From: Igor Mammedov [mailto:address@hidden
> Sent: 01 April 2019 14:09
> To: Shameerali Kolothum Thodi <address@hidden>
> Cc: Auger Eric <address@hidden>; address@hidden;
> address@hidden; address@hidden;
> address@hidden; address@hidden;
> address@hidden; Linuxarm <address@hidden>; xuwei (O)
> <address@hidden>
> Subject: Re: [Qemu-devel] [PATCH v3 03/10] hw/arm/virt: Add virtual ACPI
> device
> 
> On Fri, 29 Mar 2019 11:22:02 +0000
> Shameerali Kolothum Thodi <address@hidden> wrote:
> 
> > > -----Original Message-----
> > > From: Auger Eric [mailto:address@hidden
> > > Sent: 28 March 2019 14:15
> > > To: Shameerali Kolothum Thodi
> <address@hidden>;
> > > address@hidden; address@hidden;
> address@hidden;
> > > address@hidden; address@hidden;
> > > address@hidden; address@hidden
> > > Cc: Linuxarm <address@hidden>; xuwei (O) <address@hidden>
> > > Subject: Re: [PATCH v3 03/10] hw/arm/virt: Add virtual ACPI device
> > >
> > > Hi Shameer,
> > >
> > > On 3/21/19 11:47 AM, Shameer Kolothum wrote:
> > > > From: Samuel Ortiz <address@hidden>
> > > >
> > > > This adds the skeleton to support an acpi device interface for
> > > > HW-reduced acpi platforms via ACPI GED - Generic Event Device (ACPI
> > > > v6.1 5.6.9).
> > > >
> > > > This will be used by Arm/Virt to add hotplug support.
> > > >
> > > > Signed-off-by: Samuel Ortiz <address@hidden>
> > > > Signed-off-by: Shameer Kolothum
> <address@hidden>
> > > > ---
> > > >  hw/acpi/Kconfig                        |  4 ++
> > > >  hw/acpi/Makefile.objs                  |  1 +
> > > >  hw/acpi/generic_event_device.c         | 72
> > > ++++++++++++++++++++++++++++++++++
> > > >  include/hw/acpi/generic_event_device.h | 29 ++++++++++++++
> > > >  4 files changed, 106 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
> > > > 0000000..b21a551
> > > > --- /dev/null
> > > > +++ b/hw/acpi/generic_event_device.c
> > > > @@ -0,0 +1,72 @@
> > > > +/*
> > > > + *
> > > > + * 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 <http://www.gnu.org/licenses/>.
> > > > + */
> > > > +
> > > > +#include "qemu/osdep.h"
> > > > +#include "hw/sysbus.h"
> > > > +#include "hw/acpi/acpi.h"
> > > > +#include "hw/acpi/generic_event_device.h"
> > > the files are named generic_event_device.c/h while the device is named
> > > "virt-acpi". I would suggest to use the same naming as in nemu ie. ged or
> > > acpi_ged.
> >
> > Agree. The naming is a bit confusing. In nemu they have a separate virt-acpi
> > dev which makes use of GED. Here, we are rolling those two into one. I am
> > still not very sure whether we should leave it as virt-acpi, because the 
> > actual
> > device on which this is implemented can be changed eg, GED vs GPIO.
> 
> I probably lacking context here, could you clarify and maybe compare
> differences between x86 and ARM implementations and why it should be
> different devices?
> 

Right. I was not comparing against x86, but just pointing out how Nemu has
done this. They seems to have a virt-acpi dev specific to virt platforms
(hw/i386/virt/acpi.c) and then moved all GED related code in a separate file
(hw/acpi/ged.c) [1].

I was just thinking whether that approach makes any sense going forward where
there are cases where platforms support GED or GPIO for hotplug support and
virt-acpi dev can be configured to use either of those. May be not.

Thanks,
Shameer

[1]https://github.com/intel/nemu/commit/bcff7ee8588f7049cd919ee8b349f219a873ec41#diff-82ce92e28467c5894c90311f0e6a75fb

> 
> > > If think you should clarify what is the exact scope of this device. The 
> > > patch
> title
> > > make think this is bound to be used only in machvirt (+ the virt prefix 
> > > used in
> > > numerous functions?). Is it also bound to be used by other architectures?
> > > > +
> > > > +static void virt_device_plug_cb(HotplugHandler *hotplug_dev,
> > > > +                                DeviceState *dev, Error **errp)
> { }
> > > > +
> > > > +static void virt_send_ged(AcpiDeviceIf *adev, AcpiEventStatusBits ev)
> > > > +{ }
> > > > +
> > > > +static void virt_device_realize(DeviceState *dev, Error **errp) { }
> > > > +
> > > > +static Property virt_acpi_properties[] = {
> > > > +    DEFINE_PROP_END_OF_LIST(),
> > > > +};
> > > > +
> > > > +static void virt_acpi_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";
> > > > +    dc->props = virt_acpi_properties;
> > > > +    dc->realize = virt_device_realize;
> > > > +
> > > > +    hc->plug = virt_device_plug_cb;
> > > > +
> > > > +    adevc->send_event = virt_send_ged; }
> > > > +
> > > > +static const TypeInfo virt_acpi_info = {
> > > > +    .name          = TYPE_VIRT_ACPI,
> > > > +    .parent        = TYPE_SYS_BUS_DEVICE,
> > > > +    .instance_size = sizeof(VirtAcpiState),
> > > > +    .class_init    = virt_acpi_class_init,
> > > > +    .interfaces = (InterfaceInfo[]) {
> > > > +        { TYPE_HOTPLUG_HANDLER },
> > > > +        { TYPE_ACPI_DEVICE_IF },
> > > > +        { }
> > > > +    }
> > > > +};
> > > > +
> > > > +static void virt_acpi_register_types(void) {
> > > > +    type_register_static(&virt_acpi_info);
> > > > +}
> > > > +
> > > > +type_init(virt_acpi_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 0000000..f314515
> > > > --- /dev/null
> > > > +++ b/include/hw/acpi/generic_event_device.h
> > > > @@ -0,0 +1,29 @@
> > > > +/*
> > > > + *
> > > > + * 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 <http://www.gnu.org/licenses/>.
> > > > + */
> > > Add a comment in the header introducing what is the role of this device?
> > > link to GED spec? Explain the subset of the interfaces being implemented 
> > > by
> > > the device.
> >
> > Ok. I have added comments to that effect in patch #10, but I think I will 
> > make
> it
> > clear here as well.
> >
> > Cheers,
> > Shameer
> >
> > > > +
> > > > +#ifndef HW_ACPI_GED_H
> > > > +#define HW_ACPI_GED_H
> > > > +
> > > > +#define TYPE_VIRT_ACPI "virt-acpi"
> > > > +#define VIRT_ACPI(obj) \
> > > > +    OBJECT_CHECK(VirtAcpiState, (obj), TYPE_VIRT_ACPI)
> > > > +
> > > > +typedef struct VirtAcpiState {
> > > > +    SysBusDevice parent_obj;
> > > > +} VirtAcpiState;
> > > > +
> > > > +#endif
> > > >