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: 17 May 2019 09:41
> To: Shameerali Kolothum Thodi <address@hidden>
> Cc: address@hidden; address@hidden;
> address@hidden; address@hidden;
> address@hidden; Linuxarm <address@hidden>;
> address@hidden; address@hidden; xuwei (O)
> <address@hidden>; address@hidden; address@hidden
> Subject: Re: [PATCH v4 3/8] hw/acpi: Add ACPI Generic Event Device Support
> 
> On Mon, 13 May 2019 17:00:13 +0000
> Shameerali Kolothum Thodi <address@hidden> wrote:
> 
> > > -----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 )
> 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]);
>     }
> 

Cool!. Just tried and it works. Thanks for that.

I am going through other comments as well and hopefully will be able to
post a v5 soon.

Cheers,
Shameer

> >
> > Thanks,
> > Shameer
> >
> > > >
> > > > Appreciate your help,
> > > >
> > > > Thanks,
> > > > Shameer
> > > >