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