On Mon, 13 May 2019 17:00:13 +0000 Shameerali Kolothum Thodi <shameerali.kolothum.th...@huawei.com> wrote:
> > -----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 ) It's probably not necessary, I think I've found what you are looking for. Pls, take a loot at hw/i386/pc_q35.c: for (i = 0; i < GSI_NUM_PINS; i++) { qdev_connect_gpio_out_named(lpc_dev, ICH9_GPIO_GSI, i, pcms->gsi[i]); } > > Thanks, > Shameer > > > > > > > Appreciate your help, > > > > > > Thanks, > > > Shameer > > >
