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

[David Hildenbrand <david@redhat.com>]

On 22.01.25 11:18, David Hildenbrand wrote:
> On 22.01.25 11:17, Philippe Mathieu-Daudé wrote:
>> On 22/1/25 11:13, David Hildenbrand wrote:
>>> On 22.01.25 11:10, David Hildenbrand wrote:
>>>> On 22.01.25 11:07, Philippe Mathieu-Daudé wrote:
>>>>> 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.
>>>>
>>>> I appreciate the review, but ... really?! :)
>>>>
>>>> 25 insertions(+), 8 deletions(-)
>>>
>>> FWIW, I'll try to split it up ... I thought the comments added to
>>> memory_region_supports_direct_access() and friends are pretty clear.
>>
>> No worry, I'll give it another try. (split still welcomed, but not
>> blocking).
> 
> I think unmangling the existing unreadable conditions in
> memory_access_is_direct() can be done separately; let me see what I can do.

The following should hopefully be easier to follow:


 From 89519beec0de96d9c9c243a844ccb698db759893 Mon Sep 17 00:00:00 2001
From: David Hildenbrand <david@redhat.com>
Date: Wed, 22 Jan 2025 11:23:00 +0100
Subject: [PATCH 1/4] physmem: factor out memory_region_is_ram_device() check
  in memory_access_is_direct()

As documented in commit 4a2e242bbb306 ("memory: Don't use memcpy for
ram_device regions"), we disallow direct access to RAM DEVICE regions.

Let's make this clearer to prepare for further changes. Note that ROMD
regions will never be RAM DEVICE at the same time.

Signed-off-by: David Hildenbrand <david@redhat.com>
---
  include/exec/memory.h | 13 ++++++++++---
  1 file changed, 10 insertions(+), 3 deletions(-)

diff --git a/include/exec/memory.h b/include/exec/memory.h
index 3ee1901b52..7931aba2ea 100644
--- a/include/exec/memory.h
+++ b/include/exec/memory.h
@@ -2987,12 +2987,19 @@ bool prepare_mmio_access(MemoryRegion *mr);
  
  static inline bool memory_access_is_direct(MemoryRegion *mr, bool is_write)
  {
+    /*
+     * 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.
+     */
+    if (memory_region_is_ram_device(mr)) {
+        return false;
+    }
      if (is_write) {
          return memory_region_is_ram(mr) && !mr->readonly &&
-               !mr->rom_device && !memory_region_is_ram_device(mr);
+               !mr->rom_device;
      } else {
-        return (memory_region_is_ram(mr) && !memory_region_is_ram_device(mr)) ||
-               memory_region_is_romd(mr);
+        return memory_region_is_ram(mr) || memory_region_is_romd(mr);
      }
  }
  
-- 
2.47.1


 From ba793917cf3e35bc3b39898524ea96a85cef768d Mon Sep 17 00:00:00 2001
From: David Hildenbrand <david@redhat.com>
Date: Wed, 22 Jan 2025 11:28:19 +0100
Subject: [PATCH 2/4] physmem: factor out RAM/ROMD check in
  memory_access_is_direct()

Let's factor more of the generic "is this directly accessible" check,
independent of the "write" condition out.

Note that the "!mr->rom_device" check in the write case essentially
disallows the memory_region_is_romd() condition again. Further note that
RAM DEVICE regions are also RAM regions, so we can check for RAM+ROMD
first.

This is a preparation for further changes.

Signed-off-by: David Hildenbrand <david@redhat.com>
---
  include/exec/memory.h | 10 ++++++----
  1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/include/exec/memory.h b/include/exec/memory.h
index 7931aba2ea..086dec5086 100644
--- a/include/exec/memory.h
+++ b/include/exec/memory.h
@@ -2987,6 +2987,10 @@ bool prepare_mmio_access(MemoryRegion *mr);
  
  static inline bool memory_access_is_direct(MemoryRegion *mr, bool is_write)
  {
+    /* ROM DEVICE regions only allow direct access if in ROMD mode. */
+    if (!memory_region_is_ram(mr) && !memory_region_is_romd(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
@@ -2996,11 +3000,9 @@ static inline bool memory_access_is_direct(MemoryRegion *mr, bool is_write)
          return false;
      }
      if (is_write) {
-        return memory_region_is_ram(mr) && !mr->readonly &&
-               !mr->rom_device;
-    } else {
-        return memory_region_is_ram(mr) || memory_region_is_romd(mr);
+        return !mr->readonly && !mr->rom_device;
      }
+    return true;
  }
  
  /**
-- 
2.47.1


 From 2ccd2af17bb9e1fd4922e1bd2dcc202de4bf8942 Mon Sep 17 00:00:00 2001
From: David Hildenbrand <david@redhat.com>
Date: Wed, 22 Jan 2025 11:34:00 +0100
Subject: [PATCH 3/4] physmem: factor out direct access check into
  memory_region_supports_direct_access()

Let's factor the complete "directly accessible" check independent of
the "write" condition out so we can reuse it next.

We can now split up the checks RAM and ROMD check, so we really only check
for RAM DEVICE in case of RAM -- ROM DEVICE is neither RAM not RAM DEVICE.

Signed-off-by: David Hildenbrand <david@redhat.com>
---
  include/exec/memory.h | 14 +++++++++++---
  1 file changed, 11 insertions(+), 3 deletions(-)

diff --git a/include/exec/memory.h b/include/exec/memory.h
index 086dec5086..3b4449e847 100644
--- a/include/exec/memory.h
+++ b/include/exec/memory.h
@@ -2985,10 +2985,13 @@ 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_access_is_direct(MemoryRegion *mr, bool is_write)
+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_ram(mr) && !memory_region_is_romd(mr)) {
+    if (memory_region_is_romd(mr)) {
+        return true;
+    }
+    if (!memory_region_is_ram(mr)) {
          return false;
      }
      /*
@@ -2996,7 +2999,12 @@ static inline bool memory_access_is_direct(MemoryRegion *mr, bool is_write)
       * be MMIO and access using mempy can be wrong (e.g., using instructions not
       * intended for MMIO access). So we treat this as IO.
       */
-    if (memory_region_is_ram_device(mr)) {
+    return !memory_region_is_ram_device(mr);
+}
+
+static inline bool memory_access_is_direct(MemoryRegion *mr, bool is_write)
+{
+    if (!memory_region_supports_direct_access(mr)) {
          return false;
      }
      if (is_write) {
-- 
2.47.1


 From b9deb2c212f8edbe526152529072b89d73e2258f Mon Sep 17 00:00:00 2001
From: David Hildenbrand <david@redhat.com>
Date: Wed, 22 Jan 2025 11:35:29 +0100
Subject: [PATCH 4/4] physmem: disallow direct access to RAM DEVICE in
  address_space_write_rom()

As documented in commit 4a2e242bbb306 ("memory: Don't use memcpy for
ram_device regions"), we disallow direct access to RAM DEVICE regions.

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>
---
  system/physmem.c | 3 +--
  1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/system/physmem.c b/system/physmem.c
index c76503aea8..2d4f8110e8 100644
--- a/system/physmem.c
+++ b/system/physmem.c
@@ -3029,8 +3029,7 @@ static inline MemTxResult address_space_write_rom_internal(AddressSpace *as,
          l = len;
          mr = address_space_translate(as, addr, &addr1, &l, true, attrs);
  
-        if (!(memory_region_is_ram(mr) ||
-              memory_region_is_romd(mr))) {
+        if (!memory_region_supports_direct_access(mr)) {
              l = memory_access_size(mr, l, addr1);
          } else {
              /* ROM/RAM case */
-- 
2.47.1



-- 
Cheers,

David / dhildenb