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

[Igor Mammedov]

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]);   
 
    } 


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