Re: [Qemu-devel] [PATCH v4 3/8] hw/acpi: Add ACPI Generic Event Device Support

[Shameerali Kolothum Thodi <shameerali.kolothum.thodi@huawei.com>]

Hi Igor,

> -----Original Message-----
> From: Shameerali Kolothum Thodi
> Sent: 03 May 2019 13:46
> To: 'Igor Mammedov' <imammedo@redhat.com>
> Cc: qemu-devel@nongnu.org; qemu-arm@nongnu.org;
> eric.auger@redhat.com; peter.maydell@linaro.org;
> shannon.zhaosl@gmail.com; sameo@linux.intel.com;
> sebastien.boeuf@intel.com; xuwei (O) <xuwei5@huawei.com>;
> lersek@redhat.com; ard.biesheuvel@linaro.org; Linuxarm
> <linuxarm@huawei.com>
> Subject: RE: [PATCH v4 3/8] hw/acpi: Add ACPI Generic Event Device Support
> 
> Hi Igor,
> 
> > -----Original Message-----
> > From: Igor Mammedov [mailto:imammedo@redhat.com]
> > Sent: 02 May 2019 17:13
> > To: Shameerali Kolothum Thodi <shameerali.kolothum.thodi@huawei.com>
> > Cc: qemu-devel@nongnu.org; qemu-arm@nongnu.org;
> > eric.auger@redhat.com; peter.maydell@linaro.org;
> > shannon.zhaosl@gmail.com; sameo@linux.intel.com;
> > sebastien.boeuf@intel.com; xuwei (O) <xuwei5@huawei.com>;
> > lersek@redhat.com; ard.biesheuvel@linaro.org; Linuxarm
> > <linuxarm@huawei.com>
> > Subject: Re: [PATCH v4 3/8] hw/acpi: Add ACPI Generic Event Device Support
> >

[...]

> > > +}
> > > +
> > > +static Property acpi_ged_properties[] = {
> > > +    /*
> > > +     * Memory hotplug base address is a property of GED here,
> > > +     * because GED handles memory hotplug event and
> > MEMORY_HOTPLUG_DEVICE
> > > +     * gets initialized when GED device is realized.
> > > +     */
> > > +    DEFINE_PROP_UINT64("memhp-base", AcpiGedState, memhp_base,
> > 0),
> > > +    DEFINE_PROP_BOOL("memory-hotplug-support", AcpiGedState,
> > > +                     memhp_state.is_enabled, true),
> > > +    DEFINE_PROP_PTR("gsi", AcpiGedState, gsi),
> >
> > PTR shouldn't be used in new code, look at object_property_add_link() & co
> 
> Ok. I will take a look at that.

I attempted to remove _PROP_PTR for "ged-events" and use _PROP_LINK and
_set_link(),


diff --git a/hw/acpi/generic_event_device.c b/hw/acpi/generic_event_device.c
index 856ca04c01..978c8e088e 100644
--- a/hw/acpi/generic_event_device.c
+++ b/hw/acpi/generic_event_device.c
@@ -268,7 +268,8 @@ static Property acpi_ged_properties[] = {
     DEFINE_PROP_PTR("gsi", AcpiGedState, gsi),
     DEFINE_PROP_UINT64("ged-base", AcpiGedState, ged_base, 0),
     DEFINE_PROP_UINT32("ged-irq", AcpiGedState, ged_irq, 0),
-    DEFINE_PROP_PTR("ged-events", AcpiGedState, ged_events),
+    DEFINE_PROP_LINK("ged-events", AcpiGedState, ged_events, TYPE_ACPI_GED,
+                     GedEvent *),
     DEFINE_PROP_UINT32("ged-events-size", AcpiGedState, ged_events_size, 0),
     DEFINE_PROP_END_OF_LIST(),
 };
diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index 8179b3e511..c89b7b7120 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -537,7 +537,8 @@ static inline DeviceState *create_acpi_ged(VirtMachineState *vms)
     qdev_prop_set_ptr(dev, "gsi", vms->gsi);
     qdev_prop_set_uint64(dev, "ged-base", vms->memmap[VIRT_ACPI_GED].base);
     qdev_prop_set_uint32(dev, "ged-irq", vms->irqmap[VIRT_ACPI_GED]);
-    qdev_prop_set_ptr(dev, "ged-events", ged_events);
+    object_property_set_link(OBJECT(dev), OBJECT(ged_events), "ged-events",
+                             &error_abort);
     qdev_prop_set_uint32(dev, "ged-events-size", ARRAY_SIZE(ged_events));
 
     object_property_add_child(qdev_get_machine(), "acpi-ged",
diff --git a/include/hw/acpi/generic_event_device.h b/include/hw/acpi/generic_event_device.h
index 9c840d8064..588f4ecfba 100644
--- a/include/hw/acpi/generic_event_device.h
+++ b/include/hw/acpi/generic_event_device.h
@@ -111,7 +111,7 @@ typedef struct AcpiGedState {
     hwaddr ged_base;
     GEDState ged_state;
     uint32_t ged_irq;
-    void *ged_events;
+    GedEvent *ged_events;
     uint32_t ged_events_size;
 } AcpiGedState;


And with this I get,

Segmentation fault      (core dumped) ./qemu-system-aarch64-ged-v5
-machine virt, -cpu cortex-a57 -machine type=virt -nographic -smp 1 -m
4G,maxmem=8G,slots=10 -drive if=none,file=ubuntu-est-5.0,id=fs -device
virtio-blk-device,drive=fs -kernel Image_memhp_remove -bios
QEMU_EFI_Release.fd -object memory-backend-ram,id=mem1,size=1G -device
pc-dimm,id=dimm1,memdev=mem1 -numa node,nodeid=0 -append
"console=ttyAMA0 root=/dev/vda rw acpi=force movable_node"

It looks like struct pointer cannot be used directly and has to make a QOM object
for DEFINE_PROP_LINK use. Not sure there is an easy way for setting ptr property
using link() functions. Please let me know if there any reference implementation I
can take a look.

Appreciate your help,

Thanks,
Shameer