Re: [PATCH 02/11] exec: Add new MemoryDebugOps.

[Peter Maydell]

On Tue, 1 Dec 2020 at 14:28, Ashish Kalra <ashish.kalra@amd.com> wrote:
> On Tue, Dec 01, 2020 at 11:48:23AM +0000, Peter Maydell wrote:
> > This seems like a weird place to insert these hooks. Not
> > all debug related accesses are going to go via
> > cpu_memory_rw_debug(). For instance when the gdb stub is in
> > "PhyMemMode" and all addresses from the debugger are treated as
> > physical rather than virtual, gdbstub.c will call
> > cpu_physical_memory_write()/_read().
> >
> > I would have expected the "oh, this is a debug access, do
> > something special" to be at a lower level, so that any
> > address_space_* access to the guest memory with the debug
> > attribute gets the magic treatment, whether that was done
> > as a direct "read this physaddr" or via cpu_memory_rw_debug()
> > doing the virt-to-phys conversion first.
> >
>
> Actually, the earlier patch-set used to do this at a lower level,
> i.e., at the address_space level, but then Paolo's feedback on that
> was that we don't want to add debug specific hooks into generic code
> such as address_space_* interfaces, hence, these hooks are introduced at
> a higher level so that we can do this "debug" abstraction at
> cpu_memory_rw_debug() and adding new interfaces for physical memory
> read/write debugging such as cpu_physical_memory_rw_debug().

This seems to be mixing two separate designs, then. Either
you want to try to provide separate "debug" functions like this,
or you want to have a MemTxAttrs "debug" attribute, but you don't
need both. Personally I prefer the MemTxAttrs approach (and disagree
with Paolo :-)), because otherwise you're going to end up duplicating
a lot of functions, and the handling of "this memory is encrypted
and needs special handling" ends up being dealt with in various
layers of the code rather than being only in one place where the
lowest layer says "oh, debug access to encrypted memory, this is
how to do that".

> This seems logical too as cpu_memory_rw_debug() is invoked via the
> debugger, i.e., either gdbstub or qemu monitor, so this interface seems
> to be the right place to add these hooks.

Except that as noted, although all uses of cpu_memory_rw_debug()
are debug related, not all debug related accesses are to
cpu_memory_rw_debug()... The interesting characteristics of
cpu_memory_rw_debug() are (1) it takes a virtual address rather
than physical (2) it writes to ROMs (3) it refuses to write to
devices.

thanks
-- PMM