All of lore.kernel.org
 help / color / mirror / Atom feed
* [RFC PATCH] hw/sparc/sun4m: Use memdev backend for main RAM
@ 2020-05-10 11:35 Philippe Mathieu-Daudé
  2020-05-14 10:09 ` Igor Mammedov
  0 siblings, 1 reply; 6+ messages in thread
From: Philippe Mathieu-Daudé @ 2020-05-10 11:35 UTC (permalink / raw)
  To: qemu-devel
  Cc: Igor Mammedov, Mark Cave-Ayland, Philippe Mathieu-Daudé,
	Artyom Tarasenko

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.

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);
-- 
2.21.3



^ permalink raw reply related	[flat|nested] 6+ messages in thread

* Re: [RFC PATCH] hw/sparc/sun4m: Use memdev backend for main RAM
  2020-05-10 11:35 [RFC PATCH] hw/sparc/sun4m: Use memdev backend for main RAM Philippe Mathieu-Daudé
@ 2020-05-14 10:09 ` Igor Mammedov
  2020-05-14 12:13   ` Philippe Mathieu-Daudé
  0 siblings, 1 reply; 6+ messages in thread
From: Igor Mammedov @ 2020-05-14 10:09 UTC (permalink / raw)
  To: Philippe Mathieu-Daudé
  Cc: Mark Cave-Ayland, qemu-devel, Artyom Tarasenko

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).
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);



^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [RFC PATCH] hw/sparc/sun4m: Use memdev backend for main RAM
  2020-05-14 10:09 ` Igor Mammedov
@ 2020-05-14 12:13   ` Philippe Mathieu-Daudé
  2020-05-20 10:07     ` Igor Mammedow
  0 siblings, 1 reply; 6+ messages in thread
From: Philippe Mathieu-Daudé @ 2020-05-14 12:13 UTC (permalink / raw)
  To: Igor Mammedov; +Cc: Mark Cave-Ayland, qemu-devel, Artyom Tarasenko

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.

> 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);
> 
> 


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [RFC PATCH] hw/sparc/sun4m: Use memdev backend for main RAM
  2020-05-14 12:13   ` Philippe Mathieu-Daudé
@ 2020-05-20 10:07     ` Igor Mammedow
  2020-05-25  9:02       ` Mark Cave-Ayland
  0 siblings, 1 reply; 6+ messages in thread
From: Igor Mammedow @ 2020-05-20 10:07 UTC (permalink / raw)
  To: Philippe Mathieu-Daudé
  Cc: Mark Cave-Ayland, qemu-devel, Artyom Tarasenko

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);  
> > 
> >   
> 



^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [RFC PATCH] hw/sparc/sun4m: Use memdev backend for main RAM
  2020-05-20 10:07     ` Igor Mammedow
@ 2020-05-25  9:02       ` Mark Cave-Ayland
  2020-05-25 13:45         ` Igor Mammedow
  0 siblings, 1 reply; 6+ messages in thread
From: Mark Cave-Ayland @ 2020-05-25  9:02 UTC (permalink / raw)
  To: Igor Mammedow, Philippe Mathieu-Daudé; +Cc: qemu-devel, Artyom Tarasenko

On 20/05/2020 11:07, Igor Mammedow wrote:

> 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/

I don't feel too strongly about this, however if there is a standard way of doing
things then I would prefer to do that since it makes it easier for me as a maintainer
if there are existing (and up-to-date) examples that I can use as a reference.

Presumably it also means that if HMP/QMP commands are added in relation to machine
RAM then the SPARC machines would get that new functionality automatically...


ATB,

Mark.


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [RFC PATCH] hw/sparc/sun4m: Use memdev backend for main RAM
  2020-05-25  9:02       ` Mark Cave-Ayland
@ 2020-05-25 13:45         ` Igor Mammedow
  0 siblings, 0 replies; 6+ messages in thread
From: Igor Mammedow @ 2020-05-25 13:45 UTC (permalink / raw)
  To: Mark Cave-Ayland
  Cc: Philippe Mathieu-Daudé, Artyom Tarasenko, qemu-devel

On Mon, 25 May 2020 10:02:00 +0100
Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk> wrote:

> On 20/05/2020 11:07, Igor Mammedow wrote:
> 
> > 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/  
> 
> I don't feel too strongly about this, however if there is a standard
> way of doing things then I would prefer to do that since it makes it
> easier for me as a maintainer if there are existing (and up-to-date)
> examples that I can use as a reference.
> 
> Presumably it also means that if HMP/QMP commands are added in
> relation to machine RAM then the SPARC machines would get that new
> functionality automatically...
I'm afraid there is no standard way to do it yet.
Even for virt targeted boards we have a mix of
  1. explicit mapping of memory region (arm/virt, x86, spapr),
     -m/-machine memory-backend CLI options
          (with optional partitioning of it internally, using aliases
          (pc/q35, some other embed boards)) 
     which now maps auto-created or user provided hostmem back-end
     memory region 
     This area is quite a mess.
  2. device based RAM in guise of pc-dimm or nv-dimm, where user
     explicitly provides hostmem back-end and pairing it
     with [pc|nv]-dimm frontend on CLI (i.e. like on real hardware
     machines).

Theoretically you can reuse pc-dimm if whole dimm maps into VM address
space. It will get accounted in HMP info commands automatically. But
it hasn't been done for main RAM on any machine yet so thta's an area
to experiment with. which opens questions:
 * how to map -m/-machine memory-backend to frontend
 * or maybe ditch -m and force user to specify all DIMMs on CLI
   explicitly.

> 
> ATB,
> 
> Mark.
> 



^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2020-05-25 13:47 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-05-10 11:35 [RFC PATCH] hw/sparc/sun4m: Use memdev backend for main RAM Philippe Mathieu-Daudé
2020-05-14 10:09 ` Igor Mammedov
2020-05-14 12:13   ` Philippe Mathieu-Daudé
2020-05-20 10:07     ` Igor Mammedow
2020-05-25  9:02       ` Mark Cave-Ayland
2020-05-25 13:45         ` Igor Mammedow

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.