On Mon, 5 Aug 2019 16:54:06 +0100 Peter Maydell <peter.mayd...@linaro.org> wrote:
> On Mon, 5 Aug 2019 at 16:47, Igor Mammedov <imamm...@redhat.com> wrote: > > On Mon, 5 Aug 2019 14:42:38 +0100 > > Peter Maydell <peter.mayd...@linaro.org> 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
