The create_unimplemented_device() function is very useful to
register unimplemented devices to the SysBus 'absolute' address.

Some devices are modelled as container of memory subregions,
the subregions being mmio-mapped relatively to the container
base address.

With these container devices, the current create_unimplemented_device()
does not work.
Add a function to remove the current limitation, and allow
containerized devices to use the helpful UnimplementedDevice.

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
I don't like the function name much, I first tried with
create_unimplemented_device_mr() :( Would it be cleaner renaming current
create_unimplemented_device() -> create_unimplemented_sysbus_device()?

Also, is this useful to have this code inlined?
---
 include/hw/misc/unimp.h | 42 +++++++++++++++++++++++++++++++++++------
 1 file changed, 36 insertions(+), 6 deletions(-)

diff --git a/include/hw/misc/unimp.h b/include/hw/misc/unimp.h
index 2a291ca42d..7ae1ace885 100644
--- a/include/hw/misc/unimp.h
+++ b/include/hw/misc/unimp.h
@@ -23,9 +23,12 @@ typedef struct {
 } UnimplementedDeviceState;
 
 /**
- * create_unimplemented_device: create and map a dummy device
+ * create_unimplemented_subregion_device: create and map a dummy device
+ *
+ * @mr: the #MemoryRegion to contain the new device.
  * @name: name of the device for debug logging
- * @base: base address of the device's MMIO region
+ * @addr: base address of the device's MMIO region, or
+ *        offset relative to @mr where the device is added.
  * @size: size of the device's MMIO region
  *
  * This utility function creates and maps an instance of unimplemented-device,
@@ -35,9 +38,10 @@ typedef struct {
  * use it to cover a large region and then map other devices on top of it
  * if necessary.
  */
-static inline void create_unimplemented_device(const char *name,
-                                               hwaddr base,
-                                               hwaddr size)
+static inline void create_unimplemented_subregion_device(MemoryRegion *mr,
+                                                         const char *name,
+                                                         hwaddr addr,
+                                                         hwaddr size)
 {
     DeviceState *dev = qdev_create(NULL, TYPE_UNIMPLEMENTED_DEVICE);
 
@@ -45,7 +49,33 @@ static inline void create_unimplemented_device(const char *name,
     qdev_prop_set_uint64(dev, "size", size);
     qdev_init_nofail(dev);
 
-    sysbus_mmio_map_overlap(SYS_BUS_DEVICE(dev), 0, base, -1000);
+    if (mr) {
+        MemoryRegion *submr = sysbus_mmio_get_region(SYS_BUS_DEVICE(dev), 0);
+        memory_region_add_subregion_overlap(mr, addr, submr, -1000);
+    } else {
+        sysbus_mmio_map_overlap(SYS_BUS_DEVICE(dev), 0, addr, -1000);
+    }
+}
+
+/**
+ * create_unimplemented_device: create and map a dummy SysBus device
+ *
+ * @name: name of the device for debug logging
+ * @base: base address of the device's MMIO region
+ * @size: size of the device's MMIO region
+ *
+ * This utility function creates and maps an instance of unimplemented-device,
+ * which is a dummy device which simply logs all guest accesses to
+ * it via the qemu_log LOG_UNIMP debug log.
+ * The device is mapped at priority -1000, which means that you can
+ * use it to cover a large region and then map other devices on top of it
+ * if necessary.
+ */
+static inline void create_unimplemented_device(const char *name,
+                                               hwaddr base,
+                                               hwaddr size)
+{
+    create_unimplemented_subregion_device(NULL, name, base, size);
 }
 
 #endif
-- 
2.17.0

On 28 May 2018 at 07:11, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
> The create_unimplemented_device() function is very useful to
> register unimplemented devices to the SysBus 'absolute' address.
>
> Some devices are modelled as container of memory subregions,
> the subregions being mmio-mapped relatively to the container
> base address.
>
> With these container devices, the current create_unimplemented_device()
> does not work.
> Add a function to remove the current limitation, and allow
> containerized devices to use the helpful UnimplementedDevice.
>
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
> I don't like the function name much, I first tried with
> create_unimplemented_device_mr() :( Would it be cleaner renaming current
> create_unimplemented_device() -> create_unimplemented_sysbus_device()?
>
> Also, is this useful to have this code inlined?

My experience has been that devices that want to map the
unimplemented device into some other container probably
also want to do in-place initialization rather than using
qdev_create() -- eg hw/arm/iotkit.c which directly creates
TYPE_UNIMPLEMENTED_DEVICEs which are embedded in its state
struct.

It was always the intention that you can create a
TYPE_UNIMPLEMENTED_DEVICE directly; create_unimplemented_device()
is just a convenience wrapper.

I'm not opposed to having another convenience wrapper here;
but I'm not sure this is the right API for one (ie if it
would be used in very many places).

thanks
-- PMM

On 05/29/2018 12:08 PM, Peter Maydell wrote:
> On 28 May 2018 at 07:11, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>> The create_unimplemented_device() function is very useful to
>> register unimplemented devices to the SysBus 'absolute' address.
>>
>> Some devices are modelled as container of memory subregions,
>> the subregions being mmio-mapped relatively to the container
>> base address.
>>
>> With these container devices, the current create_unimplemented_device()
>> does not work.
>> Add a function to remove the current limitation, and allow
>> containerized devices to use the helpful UnimplementedDevice.
>>
>> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>> ---
>> I don't like the function name much, I first tried with
>> create_unimplemented_device_mr() :( Would it be cleaner renaming current
>> create_unimplemented_device() -> create_unimplemented_sysbus_device()?
>>
>> Also, is this useful to have this code inlined?
> 
> My experience has been that devices that want to map the
> unimplemented device into some other container probably
> also want to do in-place initialization rather than using
> qdev_create() -- eg hw/arm/iotkit.c which directly creates
> TYPE_UNIMPLEMENTED_DEVICEs which are embedded in its state
> struct.
> 
> It was always the intention that you can create a
> TYPE_UNIMPLEMENTED_DEVICE directly; create_unimplemented_device()
> is just a convenience wrapper.

OK.

> I'm not opposed to having another convenience wrapper here;
> but I'm not sure this is the right API for one (ie if it
> would be used in very many places).

Fair enough.

Thanks,

Phil.