Re: [Qemu-devel] [PATCH v2 04/11] hw/arm/virt: Add virtual ACPI device

[Shameerali Kolothum Thodi]

> -----Original Message-----
> From: Igor Mammedov [mailto:address@hidden
> Sent: 15 March 2019 08:42
> To: Shameerali Kolothum Thodi <address@hidden>
> Cc: address@hidden; address@hidden;
> address@hidden; address@hidden;
> address@hidden; address@hidden;
> address@hidden; Linuxarm <address@hidden>; xuwei (O)
> <address@hidden>
> Subject: Re: [PATCH v2 04/11] hw/arm/virt: Add virtual ACPI device
> 
> On Thu, 14 Mar 2019 16:55:36 +0000
> Shameerali Kolothum Thodi <address@hidden> wrote:
> 
> > Hi Igor,
> >
> > > -----Original Message-----
> > > From: Igor Mammedov [mailto:address@hidden
> > > Sent: 12 March 2019 15:48
> > > To: Shameerali Kolothum Thodi <address@hidden>
> > > Cc: address@hidden; address@hidden;
> > > address@hidden; address@hidden;
> > > address@hidden; address@hidden;
> > > address@hidden; Linuxarm <address@hidden>; xuwei (O)
> > > <address@hidden>
> > > Subject: Re: [PATCH v2 04/11] hw/arm/virt: Add virtual ACPI device
> > >
> > > On Fri, 8 Mar 2019 11:42:11 +0000
> > > Shameer Kolothum <address@hidden> wrote:
> > >
> > > > From: Samuel Ortiz <address@hidden>
> > > >
> > > > This is to provide an acpi device interface for Arm/virt.
> > > > This will be used by Arm/Virt to add hotplug support via
> > > > ACPI GED device.
> > > >
> > > > Signed-off-by: Samuel Ortiz <address@hidden>
> > > > Signed-off-by: Shameer Kolothum
> <address@hidden>
> > > > ---
> > > >  hw/arm/Makefile.objs  |   2 +-
> > > >  hw/arm/virt-acpi.c    | 111
> > > ++++++++++++++++++++++++++++++++++++++++++++++++++
> > >
> > > does it have to be ARM specific?
> > > could it be shared with future x86/virt? (if it could then it would be 
> > > better to
> put
> > > into hw/acpi/)
> > > also considering it implements vitual 'GED' device
> > > why not s/virt-acpi/generic-event-device/ ?
> >
> > I think that's a good idea to make it generic and I had a quick go at it.
> >
> > But the problem I am facing is as soon as sysbus_create_simple() is called
> from
> > virt_acpi_init(), the realize() function gets called before it sets the
> VirtAcpiState
> > members.
> 
> Lets look at it from the other side, what are the reasons to use inherit 
> device
> from TYPE_SYS_BUS_DEVICE and use sysbus_create_simple()?

Right. I think I can solve this by using qdev_create() and qdev_init_nofail().

Thanks,
Shameer
 
> 
> > This creates a problem as acpi_memory_hotplug_init() and acpi_ged_init()
> are
> > invoked from the realize() and it needs to have the base address, irq etc 
> > set
> based
> > on the platform.
> >
> > I tried using the qdev_prop_set_*() as well, but it also fails saying 
> > attempt to
> set
> > property after it was realized.
> >
> > Not sure what is the best way to solve this other than introducing a global
> struct to
> > hold the configuration.
> Globals are frowned upon, we shouldn't do add more.
> 
> >
> > Please let me know.
> >
> > Thanks,
> > Shameer
> >
> > >
> > > >  include/hw/arm/virt.h |   1 +
> > > >  3 files changed, 113 insertions(+), 1 deletion(-)
> > > >  create mode 100644 hw/arm/virt-acpi.c
> > > >
> > > > diff --git a/hw/arm/Makefile.objs b/hw/arm/Makefile.objs
> > > > index fa57c7c..e0db3cd 100644
> > > > --- a/hw/arm/Makefile.objs
> > > > +++ b/hw/arm/Makefile.objs
> > > > @@ -1,6 +1,6 @@
> > > >  obj-y += boot.o sysbus-fdt.o
> > > >  obj-$(CONFIG_ARM_VIRT) += virt.o
> > > > -obj-$(CONFIG_ACPI) += virt-acpi-build.o
> > > > +obj-$(CONFIG_ACPI) += virt-acpi-build.o virt-acpi.o
> > > >  obj-$(CONFIG_DIGIC) += digic_boards.o
> > > >  obj-$(CONFIG_EXYNOS4) += exynos4_boards.o
> > > >  obj-$(CONFIG_HIGHBANK) += highbank.o
> > > > diff --git a/hw/arm/virt-acpi.c b/hw/arm/virt-acpi.c
> > > > new file mode 100644
> > > > index 0000000..df8a02b
> > > > --- /dev/null
> > > > +++ b/hw/arm/virt-acpi.c
> > > > @@ -0,0 +1,111 @@
> > > > +/*
> > > > + *
> > > > + * 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 "qemu/range.h"
> > > > +#include "qapi/error.h"
> > > > +#include "exec/address-spaces.h"
> > > > +
> > > > +#include "hw/hw.h"
> > > > +#include "hw/hotplug.h"
> > > > +#include "hw/sysbus.h"
> > > > +#include "hw/arm/virt.h"
> > > > +
> > > > +#include "hw/acpi/acpi.h"
> > > > +
> > > > +typedef struct VirtAcpiState {
> > > > +    SysBusDevice parent_obj;
> > > > +} VirtAcpiState;
> > > > +
> > > > +#define TYPE_VIRT_ACPI "virt-acpi"
> > > > +#define VIRT_ACPI(obj) \
> > > > +    OBJECT_CHECK(VirtAcpiState, (obj), TYPE_VIRT_ACPI)
> > > > +
> > > > +static const VMStateDescription vmstate_acpi = {
> > > > +    .name = "virt_acpi",
> > > > +    .version_id = 1,
> > > > +    .minimum_version_id = 1,
> > > > +};
> > > > +
> > > > +static void virt_device_plug_cb(HotplugHandler *hotplug_dev,
> > > > +                                DeviceState *dev, Error **errp)
> > > > +{
> > > > +}
> > > > +
> > > > +static void virt_device_unplug_request_cb(HotplugHandler
> *hotplug_dev,
> > > > +                                          DeviceState *dev,
> Error
> > > **errp)
> > > > +{
> > > > +}
> > > > +
> > > > +static void virt_device_unplug_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)
> > > > +{
> > > > +}
> > > > +
> > > > +DeviceState *virt_acpi_init(void)
> > > > +{
> > > > +    return sysbus_create_simple(TYPE_VIRT_ACPI, -1, NULL);
> > > > +}
> > > > +
> > > > +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->vmsd = &vmstate_acpi;
> > > > +    dc->props = virt_acpi_properties;
> > > > +    dc->realize = virt_device_realize;
> > > > +
> > > > +    hc->plug = virt_device_plug_cb;
> > > > +    hc->unplug_request = virt_device_unplug_request_cb;
> > > > +    hc->unplug = virt_device_unplug_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/arm/virt.h b/include/hw/arm/virt.h
> > > > index 507517c..6076167 100644
> > > > --- a/include/hw/arm/virt.h
> > > > +++ b/include/hw/arm/virt.h
> > > > @@ -145,6 +145,7 @@ typedef struct {
> > > >      OBJECT_CLASS_CHECK(VirtMachineClass, klass,
> > > TYPE_VIRT_MACHINE)
> > > >
> > > >  void virt_acpi_setup(VirtMachineState *vms);
> > > > +DeviceState *virt_acpi_init(void);
> > > >
> > > >  /* Return the number of used redistributor regions  */
> > > >  static inline int virt_gicv3_redist_region_count(VirtMachineState *vms)
> >