diff mbox series

[v4,12/14] add sysbus-mmio-map qapi command

Message ID 20220223090706.4888-13-damien.hedde@greensocs.com (mailing list archive)
State New, archived
Headers show
Series Initial support for machine creation via QMP | expand

Commit Message

Damien Hedde Feb. 23, 2022, 9:07 a.m. UTC
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.

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(+)

Comments

Philippe Mathieu-Daudé March 3, 2022, 2:59 p.m. UTC | #1
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>
Damien Hedde March 4, 2022, 10:42 a.m. UTC | #2
On 3/3/22 15:59, Philippe Mathieu-Daudé wrote:
> 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().

I think we ended-up adding such a parameter to handle complex xilinx 
machines having several cpu clusters. I wanted to stay simple in this 
series here as there are probably several way to address this issue. (we 
could also add a bus parameter, and create more sysbus).
We can still add the parameter later, with an appropriate default value 
(or even make the parameter mandatory).

If everybody agree to go for the bus-less approach. I can add the MR 
parameter right now.

CCing Igor

>> +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"?

What do you mean by '#', this is not a macro parameter. 
PHASE_MACHINE_INITIALIZED is just an enum value and there is no 
human/qapi exposed name.
"when the machine is initialized/initializing" ?
I think all the machine phase error message are bit like that (not 
mentioning the phase).
Jim Shu May 24, 2022, 8:09 p.m. UTC | #3
Tested-by: Jim Shu <jim.shu@sifive.com>

On Wed, Feb 23, 2022 at 5:37 PM 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 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.
>
> 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' },
> +  '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");
> +        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;
> +}
> +
>  void sysbus_mmio_unmap(SysBusDevice *dev, int n)
>  {
>      assert(n >= 0 && n < dev->num_mmio);
> --
> 2.35.1
>
>
>
diff mbox series

Patch

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' },
+  '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");
+        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;
+}
+
 void sysbus_mmio_unmap(SysBusDevice *dev, int n)
 {
     assert(n >= 0 && n < dev->num_mmio);