Hi Igor, > -----Original Message----- > From: Igor Mammedov [mailto:imamm...@redhat.com] > Sent: 12 March 2019 15:48 > To: Shameerali Kolothum Thodi <shameerali.kolothum.th...@huawei.com> > Cc: qemu-devel@nongnu.org; qemu-...@nongnu.org; > eric.au...@redhat.com; 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: [PATCH v2 04/11] hw/arm/virt: Add virtual ACPI device > > On Fri, 8 Mar 2019 11:42:11 +0000 > Shameer Kolothum <shameerali.kolothum.th...@huawei.com> wrote: > > > From: Samuel Ortiz <sa...@linux.intel.com> > > > > 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 <sa...@linux.intel.com> > > Signed-off-by: Shameer Kolothum <shameerali.kolothum.th...@huawei.com> > > --- > > 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. 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. 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)
