Hi Igor, > -----Original Message----- > From: Igor Mammedov [mailto:imamm...@redhat.com] > Sent: 01 April 2019 14:09 > To: Shameerali Kolothum Thodi <shameerali.kolothum.th...@huawei.com> > Cc: Auger Eric <eric.au...@redhat.com>; qemu-devel@nongnu.org; > qemu-...@nongnu.org; peter.mayd...@linaro.org; > shannon.zha...@gmail.com; sa...@linux.intel.com; > sebastien.bo...@intel.com; Linuxarm <linux...@huawei.com>; xuwei (O) > <xuw...@huawei.com> > 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 <shameerali.kolothum.th...@huawei.com> wrote: > > > > -----Original Message----- > > > From: Auger Eric [mailto:eric.au...@redhat.com] > > > Sent: 28 March 2019 14:15 > > > To: Shameerali Kolothum Thodi > <shameerali.kolothum.th...@huawei.com>; > > > qemu-devel@nongnu.org; qemu-...@nongnu.org; > imamm...@redhat.com; > > > peter.mayd...@linaro.org; shannon.zha...@gmail.com; > > > sa...@linux.intel.com; sebastien.bo...@intel.com > > > Cc: Linuxarm <linux...@huawei.com>; xuwei (O) <xuw...@huawei.com> > > > 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 <sa...@linux.intel.com> > > > > > > > > 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 <sa...@linux.intel.com> > > > > Signed-off-by: Shameer Kolothum > <shameerali.kolothum.th...@huawei.com> > > > > --- > > > > 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 > > > >
