Re: [PATCH] docs/devel: memory: Document MemoryRegionOps requirement

["Philippe Mathieu-Daudé" <philmd@redhat.com>]

On 9/6/21 2:20 PM, Bin Meng wrote:
> It's been a requirement that at least one function pointer for read
> and one for write are provided ever since the MemoryRegion APIs were
> introduced in 2012.
> 
> Signed-off-by: Bin Meng <bmeng.cn@gmail.com>
> ---
> 
>  docs/devel/memory.rst | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/docs/devel/memory.rst b/docs/devel/memory.rst
> index 5dc8a12682..7b589b21d2 100644
> --- a/docs/devel/memory.rst
> +++ b/docs/devel/memory.rst
> @@ -344,6 +344,11 @@ based on the attributes used for the memory transaction, or need
>  to be able to respond that the access should provoke a bus error
>  rather than completing successfully; those devices can use the
>  ->read_with_attrs() and ->write_with_attrs() callbacks instead.
> +The requirement for a device's MemoryRegionOps is that at least
> +one callback for read and one for write are provided. If both
> +->read() and ->read_with_attrs() are provided, the plain ->read()
> +version takes precedence over the with_attrs() version. So does
> +the write callback.

What about also adding a runtime check?

-- >8 --
diff --git a/softmmu/memory.c b/softmmu/memory.c
index bfedaf9c4df..8ab602d3379 100644
--- a/softmmu/memory.c
+++ b/softmmu/memory.c
@@ -1516,6 +1516,17 @@ MemTxResult
memory_region_dispatch_write(MemoryRegion *mr,
     }
 }

+static void memory_region_set_ops(MemoryRegion *mr, const
MemoryRegionOps *ops)
+{
+    if (ops) {
+        assert(ops->valid.accepts || (ops->read || ops->read_with_attrs));
+        assert(ops->valid.accepts || (ops->write ||
ops->write_with_attrs));
+        mr->ops = ops;
+    } else {
+        mr->ops = &unassigned_mem_ops;
+    }
+}
+
 void memory_region_init_io(MemoryRegion *mr,
                            Object *owner,
                            const MemoryRegionOps *ops,
@@ -1524,7 +1535,7 @@ void memory_region_init_io(MemoryRegion *mr,
                            uint64_t size)
 {
     memory_region_init(mr, owner, name, size);
-    mr->ops = ops ? ops : &unassigned_mem_ops;
+    memory_region_set_ops(mr, ops);
     mr->opaque = opaque;
     mr->terminates = true;
 }
@@ -1701,7 +1712,7 @@ void
memory_region_init_rom_device_nomigrate(MemoryRegion *mr,
     Error *err = NULL;
     assert(ops);
     memory_region_init(mr, owner, name, size);
-    mr->ops = ops;
+    memory_region_set_ops(mr, ops);
     mr->opaque = opaque;
     mr->terminates = true;
     mr->rom_device = true;
---