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/

v2 -> v3:
* Rebased, only a minor conflict in the last patch.

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

Sorry for the delayed engagement, I failed to apply the patch set from 
the mailing list and had to remember that David had published this 
change set on GitHub.

Tested-by: Stefan Zabka <git@zabka.it>

This addresses my initial use case of being able to write to a single 
MMIO device. I have not set up a scenario with an interleaving of
MMIO and RAM/ROM regions to ensure that a single large write is 
correctly handled there.

Reviewed-by: Stefan Zabka <git@zabka.it>

I don't know if this counts for anything, but I've read through the 
entire patch series, tried to make sense of it and couldn't spot any 
issues. It should be noted that I am a terrible C programmer and have 
only written basic devices so far.

On 10/02/2025 09:46, 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/
> 
> v2 -> v3:
> * Rebased, only a minor conflict in the last patch.
> 
> 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(-)
> 

On Thu, Feb 13, 2025 at 12:26:42AM +0100, Stefan Zabka wrote:
> Sorry for the delayed engagement, I failed to apply the patch set from the
> mailing list and had to remember that David had published this change set on
> GitHub.
> 
> Tested-by: Stefan Zabka <git@zabka.it>
> 
> This addresses my initial use case of being able to write to a single MMIO
> device. I have not set up a scenario with an interleaving of
> MMIO and RAM/ROM regions to ensure that a single large write is correctly
> handled there.
> 
> Reviewed-by: Stefan Zabka <git@zabka.it>
> 
> I don't know if this counts for anything, but I've read through the entire
> patch series, tried to make sense of it and couldn't spot any issues. It
> should be noted that I am a terrible C programmer and have only written
> basic devices so far.

Thanks, that's always helpful. It's already in a pull, but if I'll need to
respin the pull I'll attach the tags.

-- 
Peter Xu

On 13.02.25 00:26, Stefan Zabka wrote:
> Sorry for the delayed engagement, I failed to apply the patch set from
> the mailing list and had to remember that David had published this
> change set on GitHub.
> 
> Tested-by: Stefan Zabka <git@zabka.it>
> 
> This addresses my initial use case of being able to write to a single
> MMIO device. I have not set up a scenario with an interleaving of
> MMIO and RAM/ROM regions to ensure that a single large write is
> correctly handled there.
> 
> Reviewed-by: Stefan Zabka <git@zabka.it>
> 

Thanks a bunch!

> I don't know if this counts for anything, but I've read through the
> entire patch series, tried to make sense of it and couldn't spot any
> issues. It should be noted that I am a terrible C programmer and have
> only written basic devices so far.
Hard to believe ("terrible C programmer") :) Any review is appreciated!

-- 
Cheers,

David / dhildenb

On Mon, Feb 10, 2025 at 09:46:41AM +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/
> 
> v2 -> v3:
> * Rebased, only a minor conflict in the last patch.
> 
> v1 -> v2:
>  * Split up "physmem: disallow direct access to RAM DEVICE in
>    address_space_write_rom()" into 4 patches

queued.

-- 
Peter Xu