| Message ID | 20250210084648.33798-1-david@redhat.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | physmem: teach cpu_memory_rw_debug() to write to more memory regions | expand |
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.
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 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!
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.
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(-)