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

[Ashish Kalra]

On Tue, Dec 01, 2020 at 11:48:23AM +0000, Peter Maydell wrote:
> On Mon, 16 Nov 2020 at 19:07, Ashish Kalra <Ashish.Kalra@amd.com> wrote:
> >
> > From: Ashish Kalra <ashish.kalra@amd.com>
> >
> > Introduce new MemoryDebugOps which hook into guest virtual and physical
> > memory debug interfaces such as cpu_memory_rw_debug, to allow vendor 
> > specific
> > assist/hooks for debugging and delegating accessing the guest memory.
> > This is required for example in case of AMD SEV platform where the guest
> > memory is encrypted and a SEV specific debug assist/hook will be required
> > to access the guest memory.
> >
> > The MemoryDebugOps are used by cpu_memory_rw_debug() and default to
> > address_space_read and address_space_write_rom.
> 
> 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 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.

Thanks,
Ashish