Re: [Qemu-devel] [PATCH-for-4.2 v8 3/9] hw/acpi: Add ACPI Generic Event Device Support

[Igor Mammedov]

On Mon, 5 Aug 2019 14:42:38 +0100
Peter Maydell <address@hidden> wrote:

> On Mon, 5 Aug 2019 at 14:30, Igor Mammedov <address@hidden> wrote:
> >
> > On Thu, 1 Aug 2019 08:36:33 +0000
> > Shameerali Kolothum Thodi <address@hidden> wrote:
> >  
> > > Hi Igor,
> > >  
> > > > -----Original Message-----  
> > > > > +static void acpi_ged_device_realize(DeviceState *dev, Error **errp)
> > > > > +{
> > > > > +    AcpiGedState *s = ACPI_GED(dev);
> > > > > +
> > > > > +    assert(s->ged_base);
> > > > > +    acpi_ged_init(get_system_memory(), dev, &s->ged_state);  
> > > >
> > > > calling get_system_memory() from device code used to be a reason for
> > > > rejecting patch,
> > > > I'm not sure what suggest though.
> > > >
> > > > Maybe Paolo could suggest something.  
> > >
> > > How about using object_property_set_link()? Something like below.  
> > I'm afraid it doesn't help much. Issue here is that we are letting
> > device to manage whole address space (which should be managed by machine)
> > So I'd just keep get_system_memory() as is for now if there aren't any
> > objections.  
> 
> What are we trying to do with this device, and what does it need
> the system memory region for?
> 
> In this case, we seem to do:
> 
> +static void acpi_ged_init(MemoryRegion *as, DeviceState *dev, GEDState 
> *ged_st)
> +{
> +    AcpiGedState *s = ACPI_GED(dev);
> +
> +    memory_region_init_io(&ged_st->io, OBJECT(dev), &ged_ops, ged_st,
> +                          TYPE_ACPI_GED, ACPI_GED_EVT_SEL_LEN);
> +    memory_region_add_subregion(as, s->ged_base, &ged_st->io);
> +    qdev_init_gpio_out_named(DEVICE(s), &s->irq, "ged-irq", 1);
> +}
> 
> 
> This is definitely a bad idea -- devices should not add their
> own memory regions to the system memory MR. They should
> expose their MRs (by being a sysbus-device) and let the board
> code do the wiring up of the MRs into the right memory space
> at the right address.

it's not the only place in GED that is trying to add to system address
space, optionally if called acpi_memory_hotplug_init() will do the same,
then later we could add cpu hotplug memory region over there.

Perhaps we could use bus-less device plug code path,
in that case memory_region_init_io()/qdev_init_gpio_out_named()
should be moved to ged_initfn() and mapping part into specialized helper
(similar to pc_dimm_plug() ) that's called by board (from 
virt_machine_device_plug_cb)
callback during completing device realize stage, it would be something like:

virt.c:
   virt_machine_device_plug_cb()
      if dev == GED_TYPE
        machine_ged_plug_helper(system_memory)

generic_event_device.c:
   machine_ged_plug_helper(as, irq) // similar to sysbus_mmio_map() but ged 
specialized
      connect_irq()
      memory_region_add_subregion(as, ged->ged_base, &ged->io)
      if ged->memory-hotplug-support
          memory_region_add_subregion(as, ged->memhp_base , 
&ged->memhp_state.memhp_io)


in this case addresses could be normally hard-codded in board code if device is 
not optional
(as in patch 6/9: create_acpi_ged() )
or potentially they could come from CLI as -device parameters
(/me thinking about building blocks that allow to create machine from config)

sysbus device might be fine as shortcut if we are thinking about
only creating device during machine_init (although I have a reservations towards
sysbus interface (ex: caller of sysbus_mmio_map() has no clue when mapping N-th
region at some address)).


> thanks
> -- PMM