Re: [PATCH v4 12/14] add sysbus-mmio-map qapi command

["Philippe Mathieu-Daudé" <philippe.mathieu.daude@gmail.com>]

On 23/2/22 10:07, Damien Hedde 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 qapi 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 command.

"sysbus-mmio-unmap" typo I presume.

> This command is still experimental (hence the 'unstable' feature),
> as it is related to the sysbus device creation through qapi commands.
> 
> This command is required to be able to dynamically build a machine
> from scratch as there is no qapi-way of doing a memory mapping.
> 
> Signed-off-by: Damien Hedde <damien.hedde@greensocs.com>
> ---
> Cc: Alistair Francis <alistair.francis@wdc.com>
> 
> v4:
>   + integrate priority parameter
>   + use 'unstable' feature flag instead of 'x-' prefix
>   + bump version to 7.0
>   + dropped Alistair's reviewed-by as a consequence
> ---
>   qapi/qdev.json   | 31 ++++++++++++++++++++++++++++++
>   hw/core/sysbus.c | 49 ++++++++++++++++++++++++++++++++++++++++++++++++
>   2 files changed, 80 insertions(+)
> 
> diff --git a/qapi/qdev.json b/qapi/qdev.json
> index 2e2de41499..4830e87a90 100644
> --- a/qapi/qdev.json
> +++ b/qapi/qdev.json
> @@ -160,3 +160,34 @@
>   ##
>   { 'event': 'DEVICE_UNPLUG_GUEST_ERROR',
>     'data': { '*device': 'str', 'path': 'str' } }
> +
> +##
> +# @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.
> +#
> +# @priority: The priority of the mapping (defaults to 0).
> +#
> +# Features:
> +# @unstable: Command is meant to map sysbus devices
> +#            while in preconfig mode.
> +#
> +# Since: 7.0
> +#
> +# Returns: Nothing on success
> +#
> +##
> +
> +{ 'command': 'sysbus-mmio-map',
> +  'data': { 'device': 'str',
> +            '*mmio': 'uint8',
> +            'addr': 'uint64',
> +            '*priority': 'int32' },

I wonder if not providing the explicit parent container now could
be problematic later, and if we shouldn't start with a QOM MR path
(default to 'system_memory'). Anyway, sysbus are currently
restricted to system_memory so as you described, this mimics well
sysbus_mmio_map().

> +  'features': ['unstable'],
> +  'allow-preconfig' : true }
> diff --git a/hw/core/sysbus.c b/hw/core/sysbus.c
> index 05c1da3d31..df1f1f43a5 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,54 @@ static void sysbus_mmio_map_common(SysBusDevice *dev, int n, hwaddr addr,
>       }
>   }
>   
> +void qmp_sysbus_mmio_map(const char *device,
> +                         bool has_mmio, uint8_t mmio,
> +                         uint64_t addr,
> +                         bool has_priority, int32_t priority,
> +                         Error **errp)
> +{
> +    Object *obj = object_resolve_path_type(device, TYPE_SYS_BUS_DEVICE, NULL);
> +    SysBusDevice *dev;
> +
> +    if (phase_get() != PHASE_MACHINE_INITIALIZED) {
> +        error_setg(errp, "The command is permitted only when "
> +                         "the machine is in initialized phase");

"command only permitted during the " #PHASE_MACHINE_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 (!has_priority) {
> +        priority = 0;
> +    }
> +
> +    if (mmio >= dev->num_mmio) {
> +        error_setg(errp, "MMIO index '%u' does not exist in '%s'",
> +                   mmio, device);
> +        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, priority,
> +                                         errp)) {
> +        return;
> +    }
> +
> +    dev->mmio[mmio].addr = addr;
> +}

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>