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

[Ashish Kalra <ashish.kalra@amd.com>]

On Tue, Dec 01, 2020 at 02:38:30PM +0000, Peter Maydell wrote:
> 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".
> 

I agree that we end up duplicating a lot of functions, but doesn't that
keep this whole debugging stuff separate and clean and also isolated
from generic code ? 

Thanks,
Ashish

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