Re: [PATCH v4 13/14] hw/mem/system-memory: add a memory sysbus device

[Igor Mammedov <imammedo@redhat.com>]

On Wed, 23 Feb 2022 11:19:49 +0100
Damien Hedde <damien.hedde@greensocs.com> wrote:

> On 2/23/22 10:44, Igor Mammedov wrote:
> > On Wed, 23 Feb 2022 10:07:05 +0100
> > Damien Hedde <damien.hedde@greensocs.com> wrote:
> >   
> >> This device can be used to create a memory wrapped into a
> >> sysbus device.
> >> This device has one property 'readonly' which allows
> >> to choose between a ram or a rom.
> >>  
> >   
> >> The purpose for this device is to be used with qapi command
> >> device_add.  
> > that's the way to add a device to QEMU but a don't actual
> > purpose described here, i.e. how board will use this
> > device/actual usecase and how it will be wired to board
> > and why it does have to be a sysbus device.
> >   
> Sorry, this was unclear.
> 
> It is a sysbus device in order to use it like any other sysbus device. 
> The memory region it contains is exposed as a sysbus mmio.

aside of that sysbus is legacy fictional bus (albeit widely used),
it doesn't scale to non sysbus devices (me thinking about buss-less
pc-dimm & co) since eventually we would like to create mainstream
machine types via QMP as well.

> I can replace the commit message by the following paragraph:
> 
> Boards instantiate memories by creating memory region objects which is 
> not possible using QAPI commands.

That's not entirely true, you can use object-add with hostmem backends
which do provide a means to allocate memory_region.
(there is no rom hostmem backend probably (i.e. one that return rom memory region)
but that could be added).
Another benefit of approach is that one can replace backing
memory any other backend (file/memfd/pmem...) without affecting
device model.

> To create a memory, the user can instantiate and map this device by 
> issuing the following commands:
> device_add driver=sysbus-memory size=0x1000 id=someram
> sysbus-mmio-map device=someram addr=0

I'd imagine more generic approach would look like:

object-add memory-backend-ram,id=mem1,size=0x1000,other_backend_twiks
device_add memdevice_frontend,memdev=mem1,addr=0x0

where [pre]plug hooks in machine can map device to
an appropriate address space/place at device realize time.
(see memory_device_[pre_]plug() for starters).


> >> Signed-off-by: Damien Hedde <damien.hedde@greensocs.com>
> >> ---
> >>   include/hw/mem/sysbus-memory.h | 28 ++++++++++++
> >>   hw/mem/sysbus-memory.c         | 80 ++++++++++++++++++++++++++++++++++
> >>   hw/mem/meson.build             |  2 +
> >>   3 files changed, 110 insertions(+)
> >>   create mode 100644 include/hw/mem/sysbus-memory.h
> >>   create mode 100644 hw/mem/sysbus-memory.c
> >>
> >> diff --git a/include/hw/mem/sysbus-memory.h b/include/hw/mem/sysbus-memory.h
> >> new file mode 100644
> >> index 0000000000..5c596f8b4f
> >> --- /dev/null
> >> +++ b/include/hw/mem/sysbus-memory.h
> >> @@ -0,0 +1,28 @@
> >> +/*
> >> + * SPDX-License-Identifier: GPL-2.0-or-later
> >> + *
> >> + * SysBusDevice Memory
> >> + *
> >> + * Copyright (c) 2021 Greensocs
> >> + */
> >> +
> >> +#ifndef HW_SYSBUS_MEMORY_H
> >> +#define HW_SYSBUS_MEMORY_H
> >> +
> >> +#include "hw/sysbus.h"
> >> +#include "qom/object.h"
> >> +
> >> +#define TYPE_SYSBUS_MEMORY "sysbus-memory"
> >> +OBJECT_DECLARE_SIMPLE_TYPE(SysBusMemoryState, SYSBUS_MEMORY)
> >> +
> >> +struct SysBusMemoryState {
> >> +    /* <private> */
> >> +    SysBusDevice parent_obj;
> >> +    uint64_t size;
> >> +    bool readonly;
> >> +
> >> +    /* <public> */
> >> +    MemoryRegion mem;
> >> +};
> >> +
> >> +#endif /* HW_SYSBUS_MEMORY_H */
> >> diff --git a/hw/mem/sysbus-memory.c b/hw/mem/sysbus-memory.c
> >> new file mode 100644
> >> index 0000000000..f1ad7ba7ec
> >> --- /dev/null
> >> +++ b/hw/mem/sysbus-memory.c
> >> @@ -0,0 +1,80 @@
> >> +/*
> >> + * SPDX-License-Identifier: GPL-2.0-or-later
> >> + *
> >> + * SysBusDevice Memory
> >> + *
> >> + * Copyright (c) 2021 Greensocs
> >> + */
> >> +
> >> +#include "qemu/osdep.h"
> >> +#include "hw/mem/sysbus-memory.h"
> >> +#include "hw/qdev-properties.h"
> >> +#include "qemu/log.h"
> >> +#include "qemu/module.h"
> >> +#include "qapi/error.h"
> >> +
> >> +static Property sysbus_memory_properties[] = {
> >> +    DEFINE_PROP_UINT64("size", SysBusMemoryState, size, 0),
> >> +    DEFINE_PROP_BOOL("readonly", SysBusMemoryState, readonly, false),
> >> +    DEFINE_PROP_END_OF_LIST(),
> >> +};
> >> +
> >> +static void sysbus_memory_realize(DeviceState *dev, Error **errp)
> >> +{
> >> +    SysBusMemoryState *s = SYSBUS_MEMORY(dev);
> >> +    gchar *name;
> >> +
> >> +    if (!s->size) {
> >> +        error_setg(errp, "'size' must be non-zero.");
> >> +        return;
> >> +    }
> >> +
> >> +    /*
> >> +     * We impose having an id (which is unique) because we need to generate
> >> +     * a unique name for the memory region.
> >> +     * memory_region_init_ram/rom() will abort() (in qemu_ram_set_idstr()
> >> +     * function if 2 system-memory devices are created with the same name
> >> +     * for the memory region).
> >> +     */
> >> +    if (!dev->id) {
> >> +        error_setg(errp, "system-memory device must have an id.");
> >> +        return;
> >> +    }
> >> +    name = g_strdup_printf("%s.region", dev->id);
> >> +
> >> +    if (s->readonly) {
> >> +        memory_region_init_rom(&s->mem, OBJECT(dev), name, s->size, errp);
> >> +    } else {
> >> +        memory_region_init_ram(&s->mem, OBJECT(dev), name, s->size, errp);
> >> +    }
> >> +
> >> +    g_free(name);
> >> +    if (*errp) {
> >> +        return;
> >> +    }
> >> +
> >> +    sysbus_init_mmio(SYS_BUS_DEVICE(s), &s->mem);
> >> +}
> >> +
> >> +static void sysbus_memory_class_init(ObjectClass *klass, void *data)
> >> +{
> >> +    DeviceClass *dc = DEVICE_CLASS(klass);
> >> +
> >> +    dc->user_creatable = true;
> >> +    dc->realize = sysbus_memory_realize;
> >> +    device_class_set_props(dc, sysbus_memory_properties);
> >> +}
> >> +
> >> +static const TypeInfo sysbus_memory_info = {
> >> +    .name          = TYPE_SYSBUS_MEMORY,
> >> +    .parent        = TYPE_SYS_BUS_DEVICE,
> >> +    .instance_size = sizeof(SysBusMemoryState),
> >> +    .class_init    = sysbus_memory_class_init,
> >> +};
> >> +
> >> +static void sysbus_memory_register_types(void)
> >> +{
> >> +    type_register_static(&sysbus_memory_info);
> >> +}
> >> +
> >> +type_init(sysbus_memory_register_types)
> >> diff --git a/hw/mem/meson.build b/hw/mem/meson.build
> >> index 82f86d117e..04c74e12f2 100644
> >> --- a/hw/mem/meson.build
> >> +++ b/hw/mem/meson.build
> >> @@ -7,3 +7,5 @@ mem_ss.add(when: 'CONFIG_NVDIMM', if_true: files('nvdimm.c'))
> >>   softmmu_ss.add_all(when: 'CONFIG_MEM_DEVICE', if_true: mem_ss)
> >>   
> >>   softmmu_ss.add(when: 'CONFIG_SPARSE_MEM', if_true: files('sparse-mem.c'))
> >> +
> >> +softmmu_ss.add(files('sysbus-memory.c'))  
> >   
>