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