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.
thanks
-- PMM
