Re: [RFC PATCH v2 12/16] add x-sysbus-mmio-map qmp command

[Alistair Francis <alistair23@gmail.com>]

On Thu, Sep 23, 2021 at 2:26 AM Damien Hedde <damien.hedde@greensocs.com> wrote:
>
> This command allows to map an mmio region of sysbus device onto
> the system memory. Its behavior mimics the sysbus_mmio_map()
> function apart from the automatic unmap (the C function unmaps
> the region if it is already mapped).
> For the qmp function we consider it is an error to try to map
> an already mapped function. If unmapping is required, it is
> probably better to add a sysbus-mmip-unmap function.
>
> This command is still experimental (hence the x prefix), as it
> is related to the sysbus device creation through qmp commands.
>
> In future, we expect to have to handle the overlap/priority
> parameter but also multiple mapping of one mmio. For some
> devices, one mmio is mapped several times at different addresses on
> the bus (which is not supported by sysbus_mmio_map() function and
> requires the use of memory region aliases).

I think as is this is a good start. This is a useful feature!

>
> Signed-off-by: Damien Hedde <damien.hedde@greensocs.com>

Acked-by: Alistair Francis <alistair.francis@wdc.com>

> ---
>
> Note: this qmp command is required to be able to build a machine from
> scratch as there is no qmp-way of doing a memory mapping today.
>
> We've added the command into qapi/qdev.json section. It does not seem to
> have any really adequate section yet. Any idea ? should we create for
> example a new one: qapi/sysbus.json or qapi/memory.json ?

I would say leave it in qdev. We don't really want many more sysbus
commands, so qapi/sysbus.json doesn't need it's own file.

Alistair

> ---
>  qapi/qdev.json   | 21 +++++++++++++++++++++
>  hw/core/sysbus.c | 41 +++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 62 insertions(+)
>
> diff --git a/qapi/qdev.json b/qapi/qdev.json
> index ad669ae175..dfc1104197 100644
> --- a/qapi/qdev.json
> +++ b/qapi/qdev.json
> @@ -125,3 +125,24 @@
>  ##
>  { 'event': 'DEVICE_DELETED',
>    'data': { '*device': 'str', 'path': 'str' } }
> +
> +##
> +# @x-sysbus-mmio-map:
> +#
> +# Map a sysbus device mmio onto the main system bus.
> +#
> +# @device: the device's QOM path
> +#
> +# @mmio: The mmio number to be mapped (defaults to 0).
> +#
> +# @addr: The base address for the mapping.
> +#
> +# Since: 6.2
> +#
> +# Returns: Nothing on success
> +#
> +##
> +
> +{ 'command': 'x-sysbus-mmio-map',
> +  'data': {'device': 'str', '*mmio': 'uint8', 'addr': 'uint64'},
> +  'allow-preconfig' : true }
> diff --git a/hw/core/sysbus.c b/hw/core/sysbus.c
> index aaae8e23cc..b0891f37b6 100644
> --- a/hw/core/sysbus.c
> +++ b/hw/core/sysbus.c
> @@ -23,6 +23,7 @@
>  #include "hw/sysbus.h"
>  #include "monitor/monitor.h"
>  #include "exec/address-spaces.h"
> +#include "qapi/qapi-commands-qdev.h"
>
>  static void sysbus_dev_print(Monitor *mon, DeviceState *dev, int indent);
>  static char *sysbus_get_fw_dev_path(DeviceState *dev);
> @@ -154,6 +155,46 @@ static void sysbus_mmio_map_common(SysBusDevice *dev, int n, hwaddr addr,
>      }
>  }
>
> +void qmp_x_sysbus_mmio_map(const char *device, bool has_mmio, uint8_t mmio,
> +                           uint64_t addr, Error **errp)
> +{
> +    Object *obj = object_resolve_path_type(device, TYPE_SYS_BUS_DEVICE, NULL);
> +    SysBusDevice *dev;
> +
> +    if (phase_get() != MACHINE_INIT_PHASE_INITIALIZED) {
> +        error_setg(errp, "The command is permitted only when "
> +                         "the machine is in initialized phase");
> +        return;
> +    }
> +
> +    if (obj == NULL) {
> +        error_setg(errp, "Device '%s' not found", device);
> +        return;
> +    }
> +
> +    dev = SYS_BUS_DEVICE(obj);
> +    if (!has_mmio) {
> +        mmio = 0;
> +    }
> +    if (mmio >= dev->num_mmio) {
> +        error_setg(errp, "MMIO index '%u' is out of range", mmio);
> +        return;
> +    }
> +
> +    if (dev->mmio[mmio].addr != (hwaddr)-1) {
> +        error_setg(errp, "MMIO index '%u' is already mapped", mmio);
> +        return;
> +    }
> +
> +    if (!memory_region_try_add_subregion(get_system_memory(), addr,
> +                                         dev->mmio[mmio].memory, 0,
> +                                         errp)) {
> +        return;
> +    }
> +
> +    dev->mmio[mmio].addr = addr;
> +}
> +
>  void sysbus_mmio_unmap(SysBusDevice *dev, int n)
>  {
>      assert(n >= 0 && n < dev->num_mmio);
> --
> 2.33.0
>
>