Re: SEV guest debugging support for Qemu

[Ashish Kalra]

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