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