Re: [Qemu-devel] [RFC PATCH v4 02/20] memattrs: add debug attribute

[Stefan Hajnoczi <stefanha@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 2324 bytes --]

On Thu, Mar 23, 2017 at 01:14:17PM -0500, Brijesh Singh wrote:
> Hi Stefan,
> 
> 
> On 03/23/2017 06:29 AM, Stefan Hajnoczi wrote:
> > On Wed, Mar 08, 2017 at 03:51:28PM -0500, Brijesh Singh wrote:
> > > Add a new debug attribute, the attribute should be set when guest memory
> > > accesses are performed for debug purposes.
> > > The attribute will be used in SEV guest, where we need to distinguish normal
> > > vs debug access to guest memory. In debug mode, we need to use SEV commands
> > > to access the guest memory.
> > > 
> > > Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>
> > > ---
> > >  include/exec/memattrs.h |    4 ++++
> > >  1 file changed, 4 insertions(+)
> > > 
> > > diff --git a/include/exec/memattrs.h b/include/exec/memattrs.h
> > > index e601061..b802073 100644
> > > --- a/include/exec/memattrs.h
> > > +++ b/include/exec/memattrs.h
> > > @@ -37,6 +37,8 @@ typedef struct MemTxAttrs {
> > >      unsigned int user:1;
> > >      /* Requester ID (for MSI for example) */
> > >      unsigned int requester_id:16;
> > > +    /* Memory access for debug purposes */
> > 
> > What does "debug purposes" mean?  gdbstub?  Can the guest initiate debug
> > memory accesses or is the purely QEMU-internal?
> > 
> 
> What I mean by that is, any access to the guest memory within Qemu internal functions
> (e.g gdbstub, qemu monitor's memory dump, info mem, info tlb etc). I have also introduced
> debug version of ldl_phys, ldq_phys, cpu_physical_memory_* [1] and have updated hmp monitor
> code to use the debug version api's when accessing the guest memory [2].
> 
> [1] http://marc.info/?l=qemu-devel&m=148900832814697&w=2
> [2] http://marc.info/?l=qemu-devel&m=148900831414693&w=2
> 
> 
> Note: SEV debug API's are meant to be used by hypervisor to decrypt/encrypt guest memory.
> But when you are inside the guest, the  guest will have access to decrypted data and does
> not need to call down to hypervisor for debug access.

People reading the code in memattrs.h aren't thinking about AMD SEV, so
"debug purposes" isn't meaningful in this context.

I suggest something like this instead:

  /* Debug memory access for AMD SEV */

That way it's clear this "debug" flag has a very specific meaning in the
context of memory encryption.

Stefan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]