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: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