Re: [PATCH v2 0/7] physmem: teach cpu_memory_rw_debug() to write to more memory regions

[David Hildenbrand <david@redhat.com>]

On 03.02.25 17:49, Peter Xu wrote:
> On Fri, Jan 24, 2025 at 04:45:25PM +0100, David Hildenbrand wrote:
>> This is a follow-up to [1], implementing it by avoiding the use of
>> address_space_write_rom() in cpu_memory_rw_debug() completely, and
>> teaching address_space_write() about debug access instead, the can also
>> write to ROM.
>>
>> The goal is to let GDB via cpu_memory_rw_debug() to also properly write to
>> MMIO device regions, not just RAM/ROM.
>>
>> It's worth noting that other users of address_space_write_rom() are
>> left unchanged. Maybe hw/core/loader.c and friends could now be converted
>> to to a debug access via address_space_write() instead?
>>
>> Survives a basic gitlab CI build/check.
>>
>> [1] https://lore.kernel.org/all/20241220195923.314208-1-git@zabka.it/
>>
>> v1 -> v2:
>> * Split up "physmem: disallow direct access to RAM DEVICE in
>>    address_space_write_rom()" into 4 patches
>>
>> Cc: Paolo Bonzini <pbonzini@redhat.com>
>> Cc: Peter Xu <peterx@redhat.com>
>> Cc: Philippe Mathieu-Daudé <philmd@linaro.org>
>> Cc: Peter Maydell <peter.maydell@linaro.org>
>> Cc: Alex Bennée <alex.bennee@linaro.org>
>> Cc: Alex Williamson <alex.williamson@redhat.com>
>> Cc: Eduardo Habkost <eduardo@habkost.net>
>> Cc: Marcel Apfelbaum <marcel.apfelbaum@gmail.com>
>> Cc: Elena Ufimtseva <elena.ufimtseva@oracle.com>
>> Cc: Jagannathan Raman <jag.raman@oracle.com>
>> Cc: "Dr. David Alan Gilbert" <dave@treblig.org>
>> Cc: Stefan Zabka <git@zabka.it>
>>
>> David Hildenbrand (7):
>>    physmem: factor out memory_region_is_ram_device() check in
>>      memory_access_is_direct()
>>    physmem: factor out RAM/ROMD check in memory_access_is_direct()
>>    physmem: factor out direct access check into
>>      memory_region_supports_direct_access()
>>    physmem: disallow direct access to RAM DEVICE in
>>      address_space_write_rom()
> 
> IIUC the last patch will stop using this for debug path anyway, so I'm not
> sure whether this one is still needed.  The hope is it's only used to
> modify real ROMs?

There are still some remaining (other) non-debug users of 
address_space_write_rom(), such as hw/core/loader.c.

We could likely remove address_space_write_rom() by adding another 
"ignore-non-rom" tx flag, or allow for writing non-rom.

I'm not doing that as part of this series, though.

> 
>>    memory: pass MemTxAttrs to memory_access_is_direct()
>>    hmp: use cpu_get_phys_page_debug() in hmp_gva2gpa()
>>    physmem: teach cpu_memory_rw_debug() to write to more memory regions
> 
> Still, I can't think of anything harmful of patch 4.  So nothing I see
> wrong..
> 
> Reviewed-by: Peter Xu <peterx@redhat.com>

Thanks!

-- 
Cheers,

David / dhildenb