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

 hw/core/cpu-system.c      | 13 +++++++++----
 hw/core/loader.c          |  2 +-
 hw/remote/vfio-user-obj.c |  2 +-
 include/exec/memattrs.h   |  5 ++++-
 include/exec/memory.h     | 35 +++++++++++++++++++++++++++--------
 monitor/hmp-cmds-target.c |  3 +--
 system/memory_ldst.c.inc  | 18 +++++++++---------
 system/physmem.c          | 24 +++++++++---------------
 8 files changed, 61 insertions(+), 41 deletions(-)

-- 
2.47.1

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

David, I think it doesn't apply on master, would you rebase and repost?

Stefan, it'll always be good to get an ack from you to show this at least
works for you - I'd expect that but an explicit email or Tested-by at the
last patch would be great (either this or a new version).

Thanks,

-- 
Peter Xu

On 04.02.25 15:46, 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()
>>    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
> 
> David, I think it doesn't apply on master, would you rebase and repost?

Sure, can do.

-- 
Cheers,

David / dhildenb

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?

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

-- 
Peter Xu

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