Re: [PATCH v1 1/4] physmem: disallow direct access to RAM DEVICE in address_space_write_rom()

[Philippe Mathieu-Daudé]

Hi David,

On 20/1/25 12:14, David Hildenbrand wrote:
> As documented in commit 4a2e242bbb306 ("memory: Don't use memcpy for
> ram_device regions"), we disallow direct access to RAM DEVICE regions.
>
> Let's factor out the "supports direct access" check from
> memory_access_is_direct() so we can reuse it, and make it a bit easier to
> read.
>
> This change implies that address_space_write_rom() and
> cpu_memory_rw_debug() won't be able to write to RAM DEVICE regions. It
> will also affect cpu_flush_icache_range(), but it's only used by
> hw/core/loader.c after writing to ROM, so it is expected to not apply
> here with RAM DEVICE.
>
> This fixes direct access to these regions where we don't want direct
> access. We'll extend cpu_memory_rw_debug() next to also be able to write to
> these (and IO) regions.
>
> This is a preparation for further changes.
>
> Cc: Alex Williamson <alex.williamson@redhat.com>
> Signed-off-by: David Hildenbrand <david@redhat.com>
> ---
>   include/exec/memory.h | 30 ++++++++++++++++++++++++------
>   system/physmem.c      |  3 +--
>   2 files changed, 25 insertions(+), 8 deletions(-)
>
> diff --git a/include/exec/memory.h b/include/exec/memory.h
> index 3ee1901b52..bd0ddb9cdf 100644
> --- a/include/exec/memory.h
> +++ b/include/exec/memory.h
> @@ -2985,15 +2985,33 @@ MemTxResult 
> address_space_write_cached_slow(MemoryRegionCache *cache,
>   int memory_access_size(MemoryRegion *mr, unsigned l, hwaddr addr);
>   bool prepare_mmio_access(MemoryRegion *mr);
>   
> +static inline bool memory_region_supports_direct_access(MemoryRegion *mr)
> +{
> +    /* ROM DEVICE regions only allow direct access if in ROMD mode. */
> +    if (memory_region_is_romd(mr)) {
> +        return true;
> +    }
> +    if (!memory_region_is_ram(mr)) {
> +        return false;
> +    }
> +    /*
> +     * RAM DEVICE regions can be accessed directly using memcpy, but it might
> +     * be MMIO and access using mempy can be wrong (e.g., using instructions 
> not
> +     * intended for MMIO access). So we treat this as IO.
> +     */
> +    return !memory_region_is_ram_device(mr);
> +
> +}
> +
>   static inline bool memory_access_is_direct(MemoryRegion *mr, bool is_write)
>   {
> -    if (is_write) {
> -        return memory_region_is_ram(mr) && !mr->readonly &&
> -               !mr->rom_device && !memory_region_is_ram_device(mr);
> -    } else {
> -        return (memory_region_is_ram(mr) && !memory_region_is_ram_device(mr)) 
> ||

This patch is doing multiple things at once, and I'm having hard time
reviewing it.

> -               memory_region_is_romd(mr);
> +    if (!memory_region_supports_direct_access(mr)) {
> +        return false;
> +    }
> +    if (!is_write) {
> +        return true;
>       }
> +    return !mr->readonly && !mr->rom_device;
>   }

Trying to split.

1/ Extract starting with ram[_device]:
-- >8 --
diff --git a/include/exec/memory.h b/include/exec/memory.h
index 3ee1901b52c..5834a208618 100644
--- a/include/exec/memory.h
+++ b/include/exec/memory.h
@@ -2987,2 +2987,15 @@ bool prepare_mmio_access(MemoryRegion *mr);

+static inline bool memory_region_supports_direct_access(MemoryRegion *mr)
+{
+    if (!memory_region_is_ram(mr)) {
+        return false;
+    }
+    /*
+     * RAM DEVICE regions can be accessed directly using memcpy, but it 
might
+     * be MMIO and access using mempy can be wrong (e.g., using 
instructions not
+     * intended for MMIO access). So we treat this as IO.
+     */
+    return !memory_region_is_ram_device(mr);
+}
+
 static inline bool memory_access_is_direct(MemoryRegion *mr, bool 
is_write)
@@ -2990,6 +3003,6 @@ static inline bool 
memory_access_is_direct(MemoryRegion *mr, bool is_write)
     if (is_write) {
-        return memory_region_is_ram(mr) && !mr->readonly &&
-               !mr->rom_device && !memory_region_is_ram_device(mr);
+        return !mr->readonly && !mr->rom_device &&
+               !memory_region_supports_direct_access(mr);
     } else {
-        return (memory_region_is_ram(mr) && 
!memory_region_is_ram_device(mr)) ||
+        return memory_region_supports_direct_access(mr) ||
                memory_region_is_romd(mr);
---

2/ Call memory_region_supports_direct_access() once [dubious]
-- >8 --
diff --git a/include/exec/memory.h b/include/exec/memory.h
index 5834a208618..4c5c84059b7 100644
--- a/include/exec/memory.h
+++ b/include/exec/memory.h
@@ -3002,8 +3002,10 @@ static inline bool 
memory_access_is_direct(MemoryRegion *mr, bool is_write)
 {
+    if (!memory_region_supports_direct_access(mr)) {
+        return false;
+    }
+
     if (is_write) {
-        return !mr->readonly && !mr->rom_device &&
-               !memory_region_supports_direct_access(mr);
+        return !mr->readonly && !mr->rom_device;
     } else {
-        return memory_region_supports_direct_access(mr) ||
-               memory_region_is_romd(mr);
+        return memory_region_is_romd(mr);
     }
---

3/ Invert if ladders:
-- >8 --
diff --git a/include/exec/memory.h b/include/exec/memory.h
index 4c5c84059b7..e89cd2f10f0 100644
--- a/include/exec/memory.h
+++ b/include/exec/memory.h
@@ -3006,7 +3006,7 @@ static inline bool 
memory_access_is_direct(MemoryRegion *mr, bool is_write)

-    if (is_write) {
-        return !mr->readonly && !mr->rom_device;
-    } else {
+    if (!is_write) {
         return memory_region_is_romd(mr);
     }
+
+    return !mr->readonly && !mr->rom_device;
 }
---

4/ Check memory_region_is_romd() [dubious]
-- >8 --
diff --git a/include/exec/memory.h b/include/exec/memory.h
index e89cd2f10f0..2cdbf4b43d7 100644
--- a/include/exec/memory.h
+++ b/include/exec/memory.h
@@ -2989,2 +2989,6 @@ static inline bool 
memory_region_supports_direct_access(MemoryRegion *mr)
 {
+    /* ROM DEVICE regions only allow direct access if in ROMD mode. */
+    if (memory_region_is_romd(mr)) {
+        return true;
+    }
     if (!memory_region_is_ram(mr)) {
@@ -3007,3 +3011,3 @@ static inline bool 
memory_access_is_direct(MemoryRegion *mr, bool is_write)
     if (!is_write) {
-        return memory_region_is_romd(mr);
+        return true;
     }
---

Hmm maybe this isn't a change that can be easily split in logical steps?