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

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?

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

[David Hildenbrand]

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

-- 
Cheers,

David / dhildenb

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

[David Hildenbrand]

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.

-- 
Cheers,

David / dhildenb

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

[Philippe Mathieu-Daudé]

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).

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

[David Hildenbrand]

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.

-- 
Cheers,

David / dhildenb

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

[David Hildenbrand]

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

Re: [PATCH v1 3/4] hmp: use cpu_get_phys_page_debug() in hmp_gva2gpa()

[Philippe Mathieu-Daudé]

On 20/1/25 12:15, David Hildenbrand wrote:
> We don't need the MemTxAttrs, so let's simply use the simpler function
> variant.
> 
> Signed-off-by: David Hildenbrand <david@redhat.com>
> ---
>   monitor/hmp-cmds-target.c | 3 +--
>   1 file changed, 1 insertion(+), 2 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>