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

[Igor Mammedov]

On Mon, 1 Apr 2019 14:21:40 +0000
Shameerali Kolothum Thodi <address@hidden> wrote:

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

from what I see that nemu uses GED only as ACPI aml code, while TYPE_VIRT_ACPI
actually implements hardware part of GED (i.e. initializes and owns MMIO/IRQs).
So it is GED device in practice.

If it's possible by ACPI spec to use GPIO with GED device, then I'd add it
later when there is actual usecase for it. Otherwise GPIO is just another
device with its own AML part to go with.

So I'd second Eric's suggestion to rename virt-acpi to acpi-ged

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