Hello Paolo,
Thanks for your response.
On Fri, Sep 25, 2020 at 10:51:05AM +0200, Paolo Bonzini wrote:
> On 22/09/20 22:11, Ashish Kalra wrote:
> > This internally invokes the address_space_rw() accessor functions
> > which we had "fixed" internally (as part of the earlier patch) to
> > invoke memory region specific debug ops. In our earlier approach we
> > were adding debug ops/callbacks to memory regions and as per comments
> > on our earlier patches, Paolo was not happy with this debug API for
> > MemoryRegions and hence the SEV support for Qemu was merged without
> > the debug support.
>
> My complaint was only about hooking into address_space_read and
> address_space_write; I think the hook should not touch general-purpose
> (non-debug) code if possible, so something like this:
>
> typedef struct MemoryDebugOps {
> hwaddr (*translate)(CPUState *cpu, target_ulong addr,
> MemTxAttrs *attrs);
> MemTxResult (*read)(AddressSpace *as, hwaddr phys_addr,
> MemTxAttrs attrs, void *buf,
> hwaddr len);
> MemTxResult (*write)(AddressSpace *as, hwaddr phys_addr,
> MemTxAttrs attrs, const void *buf,
> hwaddr len);
> } MemoryDebugOps;
>
> These ops would be used only by cpu_memory_rw_debug and would default to
>
> static const MemoryDebugOps default_debug_ops = {
> .translate = cpu_get_phys_page_attrs_debug,
> .read = address_space_read,
> .write = address_space_write_rom
> };
>
> static const MemoryDebugOps *debug_ops = &default_debug_ops;
>
Yes, this looks like a good approach to proceed with.
I was also considering abstracting this vendor/SEV specific debug
interface via the CPUClass object, the CPUClass object aleady has cpu
specific methods for doing things like guest VA to GPA translations like the
get_phys_page_attrs_debug() method and it will be a simple and clean
approach to override this method with a SEV specific
get_phys_page_attrs_debug() if SEV guest is active and SEV debug policy
is allowed. This SEV specific method will then do guest page table walks
using the DBG_DECRYPT api and also clearing the c-bit bit on PxE copies.
One thought behind abstracting this vendor/SEV specific debug
interface via the CPUClass object is that the CPUClass object also has
methods for gdb register access such as
gdb_read_register()/gdb_write_register() which are invoked whenever
gdbstub does cpu register read/write.
As part of this debug interface we also want to consider cpu register
access for SEV-ES, etc., for instance on SEV-ES we need to reject the
register access. Again these gdb read/write methods in CPUClass object
can be overridden with SEV-ES specific variants which will then simply
return error when invoked.
I can probably add new interfaces/methods to this CPUClass object for
guest memory read/writes for debugging purpose and then invoke the same
from the generic cpu_memory_rw_debug() interface.
Let me know your thougts on abstracting this debug interface via the
CPUClass object ?
Or the other option is to introduce the new MemoryDebugOps you described
above and additionally apply SEV/SEV-ES considerations in CPUClass
methods such as gdb_read_register, gdb_write_register, etc.
Thanks,
Ashish
