On Thu, 14 May 2020 14:13:11 +0200
Philippe Mathieu-Daudé <f4...@amsat.org> wrote:
> On 5/14/20 12:09 PM, Igor Mammedov wrote:
> > On Sun, 10 May 2020 13:35:05 +0200
> > Philippe Mathieu-Daudé <f4...@amsat.org> wrote:
> >
> >> Since commit 82b911aaff3, machine_run_board_init() checks for
> >> ram_memdev_id and consume it. As TYPE_SUN4M_MEMORY is no more
> >> needed, replace it by the generic memdev allocated MemoryRegion
> >> and remove it. This completes commit b2554752b1da7c8f.
> >
> > I don't get justification here.
> > You are removing 'frontend' device model that has little/nothing
> > to do with how backend is instantiated.
> >
> > TYPE_SUN4M_MEMORY is analog to pc-dimm, only for builtin RAM
> > (not really useful but could serve as an example).
>
> I have no idea about the benefits of using memory frontend/backend
> with emulation. Is there documentation and examples? I'm seeing this
> code as a complicated way of doing a simple thing, but I guess I'm
> missing the big picture here.
Examples are pc-dimm and nv-dimm which thanks to separation easily
reusable across pc/spapr/virt-arm.
Having frontend is also useful for mgmt purposes, where HMP/QMP
just has to enumerate all RAM devices, otherwise boards would have
to provide a callback to describe their custom mappings.
But for embed boards with a single blob of RAM nailed down,
having frontend is probably overkill (at least ATM).
So I'm fine with this patch /with amended commit message/
>
> > It's fine to drop it as it doesn't accually do much
> > and map memory region directly like we do elsewhere for builtin RAM
> > and get rid of TYPE_SUN4M_MEMORY boiler-plate code.
> >
> > with such reasoning, I'd Ack it, but the final say belongs to board
> > maintainers
> >
> >
> >>
> >> Signed-off-by: Philippe Mathieu-Daudé <f4...@amsat.org>
> >> ---
> >> hw/sparc/sun4m.c | 54
> >> ++---------------------------------------------- 1 file changed, 2
> >> insertions(+), 52 deletions(-)
> >>
> >> diff --git a/hw/sparc/sun4m.c b/hw/sparc/sun4m.c
> >> index 36ee1a0a3d..9838c5a183 100644
> >> --- a/hw/sparc/sun4m.c
> >> +++ b/hw/sparc/sun4m.c
> >> @@ -772,50 +772,6 @@ static const TypeInfo prom_info = {
> >> .class_init = prom_class_init,
> >> };
> >>
> >> -#define TYPE_SUN4M_MEMORY "memory"
> >> -#define SUN4M_RAM(obj) OBJECT_CHECK(RamDevice, (obj),
> >> TYPE_SUN4M_MEMORY) -
> >> -typedef struct RamDevice {
> >> - SysBusDevice parent_obj;
> >> - HostMemoryBackend *memdev;
> >> -} RamDevice;
> >> -
> >> -/* System RAM */
> >> -static void ram_realize(DeviceState *dev, Error **errp)
> >> -{
> >> - RamDevice *d = SUN4M_RAM(dev);
> >> - MemoryRegion *ram = host_memory_backend_get_memory(d->memdev);
> >> -
> >> - sysbus_init_mmio(SYS_BUS_DEVICE(dev), ram);
> >> -}
> >> -
> >> -static void ram_initfn(Object *obj)
> >> -{
> >> - RamDevice *d = SUN4M_RAM(obj);
> >> - object_property_add_link(obj, "memdev", TYPE_MEMORY_BACKEND,
> >> - (Object **)&d->memdev,
> >> - object_property_allow_set_link,
> >> - OBJ_PROP_LINK_STRONG, &error_abort);
> >> - object_property_set_description(obj, "memdev", "Set RAM
> >> backend"
> >> - "Valid value is ID of a
> >> hostmem backend",
> >> - &error_abort);
> >> -}
> >> -
> >> -static void ram_class_init(ObjectClass *klass, void *data)
> >> -{
> >> - DeviceClass *dc = DEVICE_CLASS(klass);
> >> -
> >> - dc->realize = ram_realize;
> >> -}
> >> -
> >> -static const TypeInfo ram_info = {
> >> - .name = TYPE_SUN4M_MEMORY,
> >> - .parent = TYPE_SYS_BUS_DEVICE,
> >> - .instance_size = sizeof(RamDevice),
> >> - .instance_init = ram_initfn,
> >> - .class_init = ram_class_init,
> >> -};
> >> -
> >> static void cpu_devinit(const char *cpu_type, unsigned int id,
> >> uint64_t prom_addr, qemu_irq **cpu_irqs)
> >> {
> >> @@ -858,8 +814,6 @@ static void sun4m_hw_init(const struct
> >> sun4m_hwdef *hwdef, SysBusDevice *s;
> >> unsigned int smp_cpus = machine->smp.cpus;
> >> unsigned int max_cpus = machine->smp.max_cpus;
> >> - Object *ram_memdev =
> >> object_resolve_path_type(machine->ram_memdev_id,
> >> -
> >> TYPE_MEMORY_BACKEND, NULL);
> >> if (machine->ram_size > hwdef->max_mem) {
> >> error_report("Too much memory for this machine: %"
> >> PRId64 "," @@ -876,11 +830,8 @@ static void sun4m_hw_init(const
> >> struct sun4m_hwdef *hwdef, for (i = smp_cpus; i < MAX_CPUS; i++)
> >> cpu_irqs[i] = qemu_allocate_irqs(dummy_cpu_set_irq,
> >> NULL, MAX_PILS);
> >> - /* Create and map RAM frontend */
> >> - dev = qdev_create(NULL, "memory");
> >> - object_property_set_link(OBJECT(dev), ram_memdev, "memdev",
> >> &error_fatal);
> >> - qdev_init_nofail(dev);
> >> - sysbus_mmio_map(SYS_BUS_DEVICE(dev), 0, 0);
> >> + /* RAM */
> >> + memory_region_add_subregion(get_system_memory(), 0,
> >> machine->ram);
> >> /* models without ECC don't trap when missing ram is
> >> accessed */ if (!hwdef->ecc_base) {
> >> @@ -1575,7 +1526,6 @@ static void sun4m_register_types(void)
> >> type_register_static(&idreg_info);
> >> type_register_static(&afx_info);
> >> type_register_static(&prom_info);
> >> - type_register_static(&ram_info);
> >>
> >> type_register_static(&ss5_type);
> >> type_register_static(&ss10_type);
> >
> >
>
