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 16:54:06 +0100
Peter Maydell <address@hidden> wrote:

> On Mon, 5 Aug 2019 at 16:47, Igor Mammedov <address@hidden> wrote:
> > On Mon, 5 Aug 2019 14:42:38 +0100
> > Peter Maydell <address@hidden> wrote:  
> > > 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)  
> 
> I don't really understand why we want to do this complicated
> thing, rather than just doing the normal thing for devices
> that live at particular addresses, ie make them sysbus devices
> and have the board map their memory regions in the right place.

hotplug path is basically the same as sysbus the only difference is
that it uses machine's (pre_)plug handler to wire up devices and more
flexible than sysbus.
 
> > 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)  
> 
> I don't think we want to do this. The user should not have to
> know anything about addresses or have to specify them on the
> command line. (This is why you can't create sysbus devices
> with -device except for some odd special cases to do with passthrough
> of hardware.)
> 
> > 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)).  
> 
> Not sure entirely what you have in mind here? (though yes, the
> sysbus device API has its awkward corners, some of which are
> just down to how old it is.)
since it's a fixed device I don't mind using sysbus either,
lets do it this way.

> thanks
> -- PMM