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

[Shameerali Kolothum Thodi]

> -----Original Message-----
> From: Igor Mammedov [mailto:address@hidden
> Sent: 13 May 2019 17:25
> To: Shameerali Kolothum Thodi <address@hidden>
> 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 <address@hidden> wrote:
> 
> > Hi Igor,
> >
> > > -----Original Message-----
> > > From: Shameerali Kolothum Thodi
> > > Sent: 03 May 2019 13:46
> > > To: 'Igor Mammedov' <address@hidden>
> > > Cc: address@hidden; address@hidden;
> > > address@hidden; address@hidden;
> > > address@hidden; address@hidden;
> > > address@hidden; xuwei (O) <address@hidden>;
> > > address@hidden; address@hidden; Linuxarm
> > > <address@hidden>
> > > Subject: RE: [PATCH v4 3/8] hw/acpi: Add ACPI Generic Event Device
> Support
> > >
> > > Hi Igor,
> > >
> > > > -----Original Message-----
> > > > From: Igor Mammedov [mailto:address@hidden
> > > > Sent: 02 May 2019 17:13
> > > > To: Shameerali Kolothum Thodi
> <address@hidden>
> > > > Cc: address@hidden; address@hidden;
> > > > address@hidden; address@hidden;
> > > > address@hidden; address@hidden;
> > > > address@hidden; xuwei (O) <address@hidden>;
> > > > address@hidden; address@hidden; Linuxarm
> > > > <address@hidden>
> > > > 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
> >