Re: [RFC PATCH] hw/sparc/sun4m: Use memdev backend for main RAM

[Igor Mammedow <imammedo@redhat.com>]

On Thu, 14 May 2020 14:13:11 +0200
Philippe Mathieu-Daudé <f4bug@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é <f4bug@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é <f4bug@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);  
> > 
> >   
>