Re: [PATCH v2 73/86] sparc:sun4m: use memdev for RAM

[Mark Cave-Ayland]

On 15/01/2020 15:07, Igor Mammedov wrote:

> memory_region_allocate_system_memory() API is going away, so
> replace it with memdev allocated MemoryRegion. The later is
> initialized by generic code, so board only needs to opt in
> to memdev scheme by providing
>   MachineClass::default_ram_id
> and using MachineState::ram instead of manually initializing
> RAM memory region.
> 
> Patch moves ram size check into sun4m_hw_init() and drops
> ram_init() moving remainder to sun4m_hw_init() as well,
> as it was the only place that called it.
> 
> Also it rewrites impl. of RamDevice a little bit, which
> could serve as an example of frontend device for RAM backend.
> (Caveats are:
>   1. it doesn't check for memdev backend being mapped
>      since it's been  usurped by generic machine to handle
>      majority of machines which don't have RAM frontend device
>   2. it still lacks 'addr' property and still has hardcoded
>      sysbus_mmio_map() in board init. If done right, board should
>      set 'addr' property and bus/machine plug handler should map
>      it during device realize time.
> )
> Further improvements were left as exercise for the future,
> since frontends are out scope of RAM conversion to memdev.
> 
> Signed-off-by: Igor Mammedov <address@hidden>
> ---
> CC: address@hidden
> CC: address@hidden
> ---
>  hw/sparc/sun4m.c | 73 
> ++++++++++++++++++++++++++++----------------------------
>  1 file changed, 36 insertions(+), 37 deletions(-)
> 
> diff --git a/hw/sparc/sun4m.c b/hw/sparc/sun4m.c
> index 2aaa5bf..834ad2a 100644
> --- a/hw/sparc/sun4m.c
> +++ b/hw/sparc/sun4m.c
> @@ -777,63 +777,42 @@ static const TypeInfo prom_info = {
>  
>  typedef struct RamDevice {
>      SysBusDevice parent_obj;
> -
> -    MemoryRegion ram;
> -    uint64_t size;
> +    HostMemoryBackend *memdev;
>  } RamDevice;
>  
>  /* System RAM */
>  static void ram_realize(DeviceState *dev, Error **errp)
>  {
>      RamDevice *d = SUN4M_RAM(dev);
> -    SysBusDevice *sbd = SYS_BUS_DEVICE(dev);
> +    MemoryRegion *ram = host_memory_backend_get_memory(d->memdev);
>  
> -    memory_region_allocate_system_memory(&d->ram, OBJECT(d), "sun4m.ram",
> -                                         d->size);
> -    sysbus_init_mmio(sbd, &d->ram);
> +    sysbus_init_mmio(SYS_BUS_DEVICE(dev), ram);
>  }
>  
> -static void ram_init(hwaddr addr, ram_addr_t RAM_size,
> -                     uint64_t max_mem)
> +static void ram_initfn(Object *obj)
>  {
> -    DeviceState *dev;
> -    SysBusDevice *s;
> -    RamDevice *d;
> -
> -    /* allocate RAM */
> -    if ((uint64_t)RAM_size > max_mem) {
> -        error_report("Too much memory for this machine: %" PRId64 ","
> -                     " maximum %" PRId64,
> -                     RAM_size / MiB, max_mem / MiB);
> -        exit(1);
> -    }
> -    dev = qdev_create(NULL, "memory");
> -    s = SYS_BUS_DEVICE(dev);
> -
> -    d = SUN4M_RAM(dev);
> -    d->size = RAM_size;
> -    qdev_init_nofail(dev);
> -
> -    sysbus_mmio_map(s, 0, addr);
> +    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 Property ram_properties[] = {
> -    DEFINE_PROP_UINT64("size", RamDevice, size, 0),
> -    DEFINE_PROP_END_OF_LIST(),
> -};
> -
>  static void ram_class_init(ObjectClass *klass, void *data)
>  {
>      DeviceClass *dc = DEVICE_CLASS(klass);
>  
>      dc->realize = ram_realize;
> -    dc->props = ram_properties;
>  }
>  
>  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,
>  };
>  
> @@ -880,6 +859,13 @@ static void sun4m_hw_init(const struct sun4m_hwdef 
> *hwdef,
>      unsigned int smp_cpus = machine->smp.cpus;
>      unsigned int max_cpus = machine->smp.max_cpus;
>  
> +    if (machine->ram_size > hwdef->max_mem) {
> +        error_report("Too much memory for this machine: %" PRId64 ","
> +                     " maximum %" PRId64,
> +                     machine->ram_size / MiB, hwdef->max_mem / MiB);
> +        exit(1);
> +    }
> +
>      /* init CPUs */
>      for(i = 0; i < smp_cpus; i++) {
>          cpu_devinit(machine->cpu_type, i, hwdef->slavio_base, &cpu_irqs[i]);
> @@ -888,9 +874,13 @@ 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), OBJECT(machine->ram_memdev),
> +                             "memdev", &error_fatal);
> +    qdev_init_nofail(dev);
> +    sysbus_mmio_map(SYS_BUS_DEVICE(dev), 0, 0);
>  
> -    /* set up devices */
> -    ram_init(0, machine->ram_size, hwdef->max_mem);
>      /* models without ECC don't trap when missing ram is accessed */
>      if (!hwdef->ecc_base) {
>          empty_slot_init(machine->ram_size, hwdef->max_mem - 
> machine->ram_size);
> @@ -1078,7 +1068,7 @@ static void sun4m_hw_init(const struct sun4m_hwdef 
> *hwdef,
>  
>      fw_cfg_add_i16(fw_cfg, FW_CFG_NB_CPUS, (uint16_t)smp_cpus);
>      fw_cfg_add_i16(fw_cfg, FW_CFG_MAX_CPUS, (uint16_t)max_cpus);
> -    fw_cfg_add_i64(fw_cfg, FW_CFG_RAM_SIZE, (uint64_t)ram_size);
> +    fw_cfg_add_i64(fw_cfg, FW_CFG_RAM_SIZE, (uint64_t)machine->ram_size);
>      fw_cfg_add_i16(fw_cfg, FW_CFG_MACHINE_ID, hwdef->machine_id);
>      fw_cfg_add_i16(fw_cfg, FW_CFG_SUN4M_DEPTH, graphic_depth);
>      fw_cfg_add_i16(fw_cfg, FW_CFG_SUN4M_WIDTH, graphic_width);
> @@ -1415,6 +1405,7 @@ static void ss5_class_init(ObjectClass *oc, void *data)
>      mc->default_boot_order = "c";
>      mc->default_cpu_type = SPARC_CPU_TYPE_NAME("Fujitsu-MB86904");
>      mc->default_display = "tcx";
> +    mc->default_ram_id = "sun4m.ram";
>  }
>  
>  static const TypeInfo ss5_type = {
> @@ -1434,6 +1425,7 @@ static void ss10_class_init(ObjectClass *oc, void *data)
>      mc->default_boot_order = "c";
>      mc->default_cpu_type = SPARC_CPU_TYPE_NAME("TI-SuperSparc-II");
>      mc->default_display = "tcx";
> +    mc->default_ram_id = "sun4m.ram";
>  }
>  
>  static const TypeInfo ss10_type = {
> @@ -1453,6 +1445,7 @@ static void ss600mp_class_init(ObjectClass *oc, void 
> *data)
>      mc->default_boot_order = "c";
>      mc->default_cpu_type = SPARC_CPU_TYPE_NAME("TI-SuperSparc-II");
>      mc->default_display = "tcx";
> +    mc->default_ram_id = "sun4m.ram";
>  }
>  
>  static const TypeInfo ss600mp_type = {
> @@ -1472,6 +1465,7 @@ static void ss20_class_init(ObjectClass *oc, void *data)
>      mc->default_boot_order = "c";
>      mc->default_cpu_type = SPARC_CPU_TYPE_NAME("TI-SuperSparc-II");
>      mc->default_display = "tcx";
> +    mc->default_ram_id = "sun4m.ram";
>  }
>  
>  static const TypeInfo ss20_type = {
> @@ -1490,6 +1484,7 @@ static void voyager_class_init(ObjectClass *oc, void 
> *data)
>      mc->default_boot_order = "c";
>      mc->default_cpu_type = SPARC_CPU_TYPE_NAME("Fujitsu-MB86904");
>      mc->default_display = "tcx";
> +    mc->default_ram_id = "sun4m.ram";
>  }
>  
>  static const TypeInfo voyager_type = {
> @@ -1508,6 +1503,7 @@ static void ss_lx_class_init(ObjectClass *oc, void 
> *data)
>      mc->default_boot_order = "c";
>      mc->default_cpu_type = SPARC_CPU_TYPE_NAME("TI-MicroSparc-I");
>      mc->default_display = "tcx";
> +    mc->default_ram_id = "sun4m.ram";
>  }
>  
>  static const TypeInfo ss_lx_type = {
> @@ -1526,6 +1522,7 @@ static void ss4_class_init(ObjectClass *oc, void *data)
>      mc->default_boot_order = "c";
>      mc->default_cpu_type = SPARC_CPU_TYPE_NAME("Fujitsu-MB86904");
>      mc->default_display = "tcx";
> +    mc->default_ram_id = "sun4m.ram";
>  }
>  
>  static const TypeInfo ss4_type = {
> @@ -1544,6 +1541,7 @@ static void scls_class_init(ObjectClass *oc, void *data)
>      mc->default_boot_order = "c";
>      mc->default_cpu_type = SPARC_CPU_TYPE_NAME("TI-MicroSparc-I");
>      mc->default_display = "tcx";
> +    mc->default_ram_id = "sun4m.ram";
>  }
>  
>  static const TypeInfo scls_type = {
> @@ -1562,6 +1560,7 @@ static void sbook_class_init(ObjectClass *oc, void 
> *data)
>      mc->default_boot_order = "c";
>      mc->default_cpu_type = SPARC_CPU_TYPE_NAME("TI-MicroSparc-I");
>      mc->default_display = "tcx";
> +    mc->default_ram_id = "sun4m.ram";
>  }
>  
>  static const TypeInfo sbook_type = {

As you say it's a little bit more complicated here, however it looks sensible 
to me.

Acked-by: Mark Cave-Ayland <address@hidden>


ATB,

Mark.