On Mon, 5 Aug 2019 14:42:38 +0100
Peter Maydell <peter.mayd...@linaro.org> wrote:
> On Mon, 5 Aug 2019 at 14:30, Igor Mammedov <imamm...@redhat.com> wrote:
> >
> > On Thu, 1 Aug 2019 08:36:33 +0000
> > Shameerali Kolothum Thodi <shameerali.kolothum.th...@huawei.com> 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
