> -----Original Message----- > From: Igor Mammedov [mailto:imamm...@redhat.com] > Sent: 13 May 2019 17:25 > To: Shameerali Kolothum Thodi <shameerali.kolothum.th...@huawei.com> > Subject: Re: [PATCH v4 3/8] hw/acpi: Add ACPI Generic Event Device Support > > On Mon, 13 May 2019 11:53:38 +0000 > Shameerali Kolothum Thodi <shameerali.kolothum.th...@huawei.com> wrote: > > > Hi Igor, > > > > > -----Original Message----- > > > From: Shameerali Kolothum Thodi > > > Sent: 03 May 2019 13:46 > > > To: 'Igor Mammedov' <imamm...@redhat.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; xuwei (O) <xuw...@huawei.com>; > > > ler...@redhat.com; ard.biesheu...@linaro.org; Linuxarm > > > <linux...@huawei.com> > > > Subject: RE: [PATCH v4 3/8] hw/acpi: Add ACPI Generic Event Device > Support > > > > > > Hi Igor, > > > > > > > -----Original Message----- > > > > From: Igor Mammedov [mailto:imamm...@redhat.com] > > > > Sent: 02 May 2019 17:13 > > > > 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; xuwei (O) <xuw...@huawei.com>; > > > > ler...@redhat.com; ard.biesheu...@linaro.org; Linuxarm > > > > <linux...@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. > > Simple 'struct' won't work with link, it has to be QOM object. > > Question is do we still need GedEvent array? > We handle only 1 irq and several event types, why not replace GedEvent > with with a 32bit bitmap (1 bit per event type) and pass that to > ged device from machine as 'int' property?
Right. That might solve the ged_events ptr issue. But we need to set the irq as well for the GED device. Is there a way to set the irq directly for a TYPE_DEVICE object? (I think Eric mentioned a way of setting it directly earlier but that time GED was a TYPE_SYS_BUS_DEVICE. May be that is a reason to go back to SYS_BUS_DEVICE ) Thanks, Shameer > > > > Appreciate your help, > > > > Thanks, > > Shameer > >
