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

Commit Message

Damien Hedde Feb. 23, 2022, 9:07 a.m. UTC

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.

Signed-off-by: Damien Hedde <[e-mail]>
---
 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

Comments

Igor Mammedov Feb. 23, 2022, 9:44 a.m. UTC | #1

On Wed, 23 Feb 2022 10:07:05 +0100
Damien Hedde <[e-mail]> 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.

> Signed-off-by: Damien Hedde <[e-mail]>
> ---
>  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'))

Damien Hedde Feb. 23, 2022, 10:19 a.m. UTC | #2

On 2/23/22 10:44, Igor Mammedov wrote:
> On Wed, 23 Feb 2022 10:07:05 +0100
> Damien Hedde <[e-mail]> 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.

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

>> Signed-off-by: Damien Hedde <[e-mail]>
>> ---
>>   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'))
>

Igor Mammedov Feb. 24, 2022, 9:55 a.m. UTC | #3

On Wed, 23 Feb 2022 11:19:49 +0100
Damien Hedde <[e-mail]> wrote:

> On 2/23/22 10:44, Igor Mammedov wrote:
> > On Wed, 23 Feb 2022 10:07:05 +0100
> > Damien Hedde <[e-mail]> 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 <[e-mail]>
> >> ---
> >>   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'))  
> >   
>

Damien Hedde Feb. 24, 2022, 11:43 a.m. UTC | #4

On 2/24/22 10:55, Igor Mammedov wrote:
> On Wed, 23 Feb 2022 11:19:49 +0100
> Damien Hedde <[e-mail]> wrote:
> 
>> On 2/23/22 10:44, Igor Mammedov wrote:
>>> On Wed, 23 Feb 2022 10:07:05 +0100
>>> Damien Hedde <[e-mail]> 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.

I'm not familiar with memory backends. I need to take a look at them.
> 
>> 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).
> 

We cannot rely on hooks the machine would define, because we start
from an empty machine. So anything must come from qapi and we would
need to do something like that I suppose:
object-add memory-backend-ram,id=mem1,size=0x1000,other_backend_twiks
device_add sysbus-memory-frontend,memdev=mem1,id=memdev_fe
sysbus-mmio-map device=memdev_fe addr=0

Thanks,
--
Damien

Igor Mammedov Feb. 25, 2022, 11:38 a.m. UTC | #5

On Thu, 24 Feb 2022 12:43:21 +0100
Damien Hedde <[e-mail]> wrote:

> On 2/24/22 10:55, Igor Mammedov wrote:
> > On Wed, 23 Feb 2022 11:19:49 +0100
> > Damien Hedde <[e-mail]> wrote:
> >   
> >> On 2/23/22 10:44, Igor Mammedov wrote:  
> >>> On Wed, 23 Feb 2022 10:07:05 +0100
> >>> Damien Hedde <[e-mail]> 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.  
> 
> I'm not familiar with memory backends. I need to take a look at them.
> >   
> >> 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).
> >   
> 
> We cannot rely on hooks the machine would define, because we start
> from an empty machine. So anything must come from qapi and we would
> need to do something like that I suppose:
> object-add memory-backend-ram,id=mem1,size=0x1000,other_backend_twiks
> device_add sysbus-memory-frontend,memdev=mem1,id=memdev_fe
> sysbus-mmio-map device=memdev_fe addr=0

As I pointed out using legacy sysbus doesn't scale, also I'd avoid
spawning more new device based on it if it can be helped.

with bus-less design, machine is still empty, in advance prepared
plug callbacks, is practically meta-data saying which device class
map into which address space which is quite generic. It helps
to avoid having extra QMP command for mapping.
However if prebuilt mapping is problematic, maybe have an alternative
QMP command that would do mapping, just not limited to sysbus,
something like 

   map_at_as device=1 as={parent_mr_name,system,io} [addr=x overlap=y prio=z]

which should give you full control where and how to map device.
 
> Thanks,
> --
> Damien
> 
>

Damien Hedde Feb. 25, 2022, 3:31 p.m. UTC | #6

On 2/25/22 12:38, Igor Mammedov wrote:
> On Thu, 24 Feb 2022 12:43:21 +0100
> Damien Hedde <[e-mail]> wrote:
> 
>> On 2/24/22 10:55, Igor Mammedov wrote:
>>> On Wed, 23 Feb 2022 11:19:49 +0100
>>> Damien Hedde <[e-mail]> wrote:
>>>    
>>>> On 2/23/22 10:44, Igor Mammedov wrote:
>>>>> On Wed, 23 Feb 2022 10:07:05 +0100
>>>>> Damien Hedde <[e-mail]> 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.
>>
>> I'm not familiar with memory backends. I need to take a look at them.
>>>    
>>>> 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).
>>>    
>>
>> We cannot rely on hooks the machine would define, because we start
>> from an empty machine. So anything must come from qapi and we would
>> need to do something like that I suppose:
>> object-add memory-backend-ram,id=mem1,size=0x1000,other_backend_twiks
>> device_add sysbus-memory-frontend,memdev=mem1,id=memdev_fe
>> sysbus-mmio-map device=memdev_fe addr=0
> 
> As I pointed out using legacy sysbus doesn't scale, also I'd avoid
> spawning more new device based on it if it can be helped.

I'm not sure to get the issue with adding sysbus devices, there is a lot 
of them. And for most them, they'll never be put on anything else than a 
simple memory bus. This one is trivial.
Right now there is a sysbus and the whole buses tree starts from it, it 
propagates reset. Everything is based on it.

> 
> with bus-less design, machine is still empty, in advance prepared
> plug callbacks, is practically meta-data saying which device class
> map into which address space which is quite generic. It helps
> to avoid having extra QMP command for mapping.

AFAIK the sysbus is the only bus type, on which we cannot specify the 
mapping/addresses with some device_add command parameter (this is 
probably doable, but hard). That's why I proposed to add sysbus-mmio-map 
several months ago. I didn't look again since, it's probably easier now 
with the modification done to device_add.

> However if prebuilt mapping is problematic, maybe have an alternative
> QMP command that would do mapping, just not limited to sysbus,
> something like
> 
>     map_at_as device=1 as={parent_mr_name,system,io} [addr=x overlap=y prio=z]
> 
> which should give you full control where and how to map device.

sysbus-mmio-map is not introduced by this series just for this memory 
device. It is here to solve mapping of any existing sysbus devices.

By bus-less. You mean mapping a sysbus device on another address space 
than the main one exposed by the sysbus ? We can support this by adding 
an 'as' parameter to the mapping function.

You mentioned non sysbus devices above. I don't think we need to try to 
do super-commands to solve all use cases.
I think there is a need to map a sysbus device on a sysbus.
Maybe there is also a need to map a non-sysbus device (a memory region 
then ?) to an address space.

--
Damien

Philippe Mathieu-Daudé March 3, 2022, 3:16 p.m. UTC | #7

+Mark / Daniel / Markus / Alex for design.

On 25/2/22 16:31, Damien Hedde wrote:
> On 2/25/22 12:38, Igor Mammedov wrote:
>> On Thu, 24 Feb 2022 12:43:21 +0100
>> Damien Hedde <[e-mail]> wrote:
>>> On 2/24/22 10:55, Igor Mammedov wrote:
>>>> On Wed, 23 Feb 2022 11:19:49 +0100
>>>> Damien Hedde <[e-mail]> wrote:
>>>>> On 2/23/22 10:44, Igor Mammedov wrote:
>>>>>> On Wed, 23 Feb 2022 10:07:05 +0100
>>>>>> Damien Hedde <[e-mail]> 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.
>>>
>>> I'm not familiar with memory backends. I need to take a look at them.
>>>>> 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).
>>>
>>> We cannot rely on hooks the machine would define, because we start
>>> from an empty machine. So anything must come from qapi and we would
>>> need to do something like that I suppose:
>>> object-add memory-backend-ram,id=mem1,size=0x1000,other_backend_twiks
>>> device_add sysbus-memory-frontend,memdev=mem1,id=memdev_fe
>>> sysbus-mmio-map device=memdev_fe addr=0
>>
>> As I pointed out using legacy sysbus doesn't scale, also I'd avoid
>> spawning more new device based on it if it can be helped.
> 
> I'm not sure to get the issue with adding sysbus devices, there is a lot 
> of them. And for most them, they'll never be put on anything else than a 
> simple memory bus. This one is trivial.
> Right now there is a sysbus and the whole buses tree starts from it, it 
> propagates reset. Everything is based on it.
> 
>>
>> with bus-less design, machine is still empty, in advance prepared
>> plug callbacks, is practically meta-data saying which device class
>> map into which address space which is quite generic. It helps
>> to avoid having extra QMP command for mapping.
> 
> AFAIK the sysbus is the only bus type, on which we cannot specify the 
> mapping/addresses with some device_add command parameter (this is 
> probably doable, but hard). That's why I proposed to add sysbus-mmio-map 
> several months ago. I didn't look again since, it's probably easier now 
> with the modification done to device_add.
> 
>> However if prebuilt mapping is problematic, maybe have an alternative
>> QMP command that would do mapping, just not limited to sysbus,
>> something like
>>
>>     map_at_as device=1 as={parent_mr_name,system,io} [addr=x overlap=y 
>> prio=z]
>>
>> which should give you full control where and how to map device.

I agree with Igor (this is what I mentioned I was worried about when
reviewing patch #12 "add sysbus-mmio-map qapi command").

Sysbus devices are limited to a single main bus, and it is limiting us.
Instead of growing the sysbus API, we should add such feature to the
qdev API, so it could benefit any device (sysbus devices being a corner
case of qdev devices).

> sysbus-mmio-map is not introduced by this series just for this memory 
> device. It is here to solve mapping of any existing sysbus devices.
> 
> By bus-less. You mean mapping a sysbus device on another address space 
> than the main one exposed by the sysbus ? We can support this by adding 
> an 'as' parameter to the mapping function.

Yes.

> You mentioned non sysbus devices above. I don't think we need to try to 
> do super-commands to solve all use cases.

I tend to disagree, we want to build any machine, being able to use all
of our devices, not only the sysbus ones. How do you plan to create a
Clock object for example? IIRC some machines don't use sysbus at all;
I don't think the raspi3b do for example.

> I think there is a need to map a sysbus device on a sysbus.
> Maybe there is also a need to map a non-sysbus device (a memory region 
> then ?) to an address space.

Certainly.

Jim Shu May 24, 2022, 8:10 p.m. UTC | #8

Tested-by: Jim Shu <[e-mail]>

On Wed, Feb 23, 2022 at 5:14 PM Damien Hedde <[e-mail]>
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.
>
> Signed-off-by: Damien Hedde <[e-mail]>
> ---
>  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'))
> --
> 2.35.1
>
>
>

Patch

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