On Thu, 14 Mar 2019 16:55:36 +0000 Shameerali Kolothum Thodi <shameerali.kolothum.th...@huawei.com> wrote:
> 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. 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()? > 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) >
