[PATCH 00/11] Add QEMU debug support for SEV guests

[PATCH 00/11] Add QEMU debug support for SEV guests

[Ashish Kalra]

From: Ashish Kalra <ashish.kalra@amd.com>

This patchset adds QEMU debug support for SEV guests. Debug requires access to the guest pages, which is encrypted when SEV is enabled.

KVM_SEV_DBG_DECRYPT and KVM_SEV_DBG_ENCRYPT commands are available to decrypt/encrypt the guest pages, if the guest policy allows for debugging.

Changes are made to the guest page table walker since SEV guest pte entries will have the C-bit set.

Also introduces 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 used 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 as described below.

typedef struct MemoryDebugOps {
    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;

Ashish Kalra (3):
  exec: Add new MemoryDebugOps.
  exec: Add address_space_read and address_space_write debug helpers.
  sev/i386: add SEV specific MemoryDebugOps.

Brijesh Singh (8):
  memattrs: add debug attribute
  exec: add ram_debug_ops support
  exec: add debug version of physical memory read and write API
  monitor/i386: use debug APIs when accessing guest memory
  kvm: introduce debug memory encryption API
  sev/i386: add debug encrypt and decrypt commands
  hw/i386: set ram_debug_ops when memory encryption is enabled
  target/i386: clear C-bit when walking SEV guest page table

 accel/kvm/kvm-all.c       |  22 ++++
 accel/kvm/sev-stub.c      |   8 ++
 accel/stubs/kvm-stub.c    |   8 ++
 hw/i386/pc.c              |   9 ++
 hw/i386/pc_sysfw.c        |   6 +
 include/exec/cpu-common.h |  18 +++
 include/exec/memattrs.h   |   2 +
 include/exec/memory.h     |  49 ++++++++
 include/sysemu/kvm.h      |  15 +++
 include/sysemu/sev.h      |  12 ++
 monitor/misc.c            |   4 +-
 softmmu/cpus.c            |   2 +-
 softmmu/physmem.c         | 170 +++++++++++++++++++++++++-
 target/i386/kvm.c         |   4 +
 target/i386/monitor.c     | 124 +++++++++++--------
 target/i386/sev.c         | 244 ++++++++++++++++++++++++++++++++++++++
 target/i386/trace-events  |   1 +
 17 files changed, 642 insertions(+), 56 deletions(-)

-- 
2.17.1

[PATCH 01/11] memattrs: add debug attribute

[Ashish Kalra]

From: Brijesh Singh <brijesh.singh@amd.com>

From: Brijesh Singh <brijesh.singh@amd.com>

Extend the MemTxAttrs to include a 'debug' flag. The flag can be used as
general indicator that operation was triggered by the debugger.

A subsequent patch will set the debug=1 when issuing a memory access
from the gdbstub or HMP commands. This is a prerequisite to support
debugging an encrypted guest. When a request with debug=1 is seen, the
encryption APIs will be used to access the guest memory.

Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>
Signed-off-by: Ashish Kalra <ashish.kalra@amd.com>
---
 include/exec/memattrs.h | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/include/exec/memattrs.h b/include/exec/memattrs.h
index 95f2d20d55..c8b56389d6 100644
--- a/include/exec/memattrs.h
+++ b/include/exec/memattrs.h
@@ -49,6 +49,8 @@ typedef struct MemTxAttrs {
     unsigned int target_tlb_bit0 : 1;
     unsigned int target_tlb_bit1 : 1;
     unsigned int target_tlb_bit2 : 1;
+    /* Memory access request from the debugger */
+    unsigned int debug:1;
 } MemTxAttrs;
 
 /* Bus masters which don't specify any attributes will get this,
-- 
2.17.1

Re: [PATCH 01/11] memattrs: add debug attribute

[Dr. David Alan Gilbert]

* Ashish Kalra (Ashish.Kalra@amd.com) wrote:
> From: Brijesh Singh <brijesh.singh@amd.com>
> 
> From: Brijesh Singh <brijesh.singh@amd.com>
> 
> Extend the MemTxAttrs to include a 'debug' flag. The flag can be used as
> general indicator that operation was triggered by the debugger.
> 
> A subsequent patch will set the debug=1 when issuing a memory access
> from the gdbstub or HMP commands. This is a prerequisite to support
> debugging an encrypted guest. When a request with debug=1 is seen, the
> encryption APIs will be used to access the guest memory.

Is this also the flag that would be used for memory dumping?

> Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>
> Signed-off-by: Ashish Kalra <ashish.kalra@amd.com>
> ---
>  include/exec/memattrs.h | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/include/exec/memattrs.h b/include/exec/memattrs.h
> index 95f2d20d55..c8b56389d6 100644
> --- a/include/exec/memattrs.h
> +++ b/include/exec/memattrs.h
> @@ -49,6 +49,8 @@ typedef struct MemTxAttrs {
>      unsigned int target_tlb_bit0 : 1;
>      unsigned int target_tlb_bit1 : 1;
>      unsigned int target_tlb_bit2 : 1;
> +    /* Memory access request from the debugger */
> +    unsigned int debug:1;

It might be good to clarify that this is for QEMU debug features, not
guest side debug features (e.g. CPU debug facilities/registers)

Dave

>  } MemTxAttrs;
>  
>  /* Bus masters which don't specify any attributes will get this,
> -- 
> 2.17.1
> 
-- 
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [PATCH 01/11] memattrs: add debug attribute

[Peter Maydell]

On Mon, 16 Nov 2020 at 19:28, Ashish Kalra <Ashish.Kalra@amd.com> wrote:
>
> From: Brijesh Singh <brijesh.singh@amd.com>
>
> From: Brijesh Singh <brijesh.singh@amd.com>
>
> Extend the MemTxAttrs to include a 'debug' flag. The flag can be used as
> general indicator that operation was triggered by the debugger.
>
> A subsequent patch will set the debug=1 when issuing a memory access
> from the gdbstub or HMP commands. This is a prerequisite to support
> debugging an encrypted guest. When a request with debug=1 is seen, the
> encryption APIs will be used to access the guest memory.

So, what counts as "debug" here, and why are debug requests
special? If "debug=1" means "can actually get at the guest memory",
why wouldn't every device model want to use it?

thanks
-- PMM

Re: [PATCH 01/11] memattrs: add debug attribute

[Dr. David Alan Gilbert]

* Peter Maydell (peter.maydell@linaro.org) wrote:
> On Mon, 16 Nov 2020 at 19:28, Ashish Kalra <Ashish.Kalra@amd.com> wrote:
> >
> > From: Brijesh Singh <brijesh.singh@amd.com>
> >
> > From: Brijesh Singh <brijesh.singh@amd.com>
> >
> > Extend the MemTxAttrs to include a 'debug' flag. The flag can be used as
> > general indicator that operation was triggered by the debugger.
> >
> > A subsequent patch will set the debug=1 when issuing a memory access
> > from the gdbstub or HMP commands. This is a prerequisite to support
> > debugging an encrypted guest. When a request with debug=1 is seen, the
> > encryption APIs will be used to access the guest memory.
> 
> So, what counts as "debug" here, and why are debug requests
> special? If "debug=1" means "can actually get at the guest memory",
> why wouldn't every device model want to use it?

SEV has a flag that the guest-owner can set on a VM to enable debug;
it's rare for it to be enabled; so it's not suitable for use by normal
devices.  It's only there for debug if the guest owner allows you to.

Dave

> thanks
> -- PMM
> 
-- 
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [PATCH 01/11] memattrs: add debug attribute

[Peter Maydell]

On Tue, 1 Dec 2020 at 11:51, Dr. David Alan Gilbert <dgilbert@redhat.com> wrote:
>
> * Peter Maydell (peter.maydell@linaro.org) wrote:
> > On Mon, 16 Nov 2020 at 19:28, Ashish Kalra <Ashish.Kalra@amd.com> wrote:
> > >
> > > From: Brijesh Singh <brijesh.singh@amd.com>
> > >
> > > From: Brijesh Singh <brijesh.singh@amd.com>
> > >
> > > Extend the MemTxAttrs to include a 'debug' flag. The flag can be used as
> > > general indicator that operation was triggered by the debugger.
> > >
> > > A subsequent patch will set the debug=1 when issuing a memory access
> > > from the gdbstub or HMP commands. This is a prerequisite to support
> > > debugging an encrypted guest. When a request with debug=1 is seen, the
> > > encryption APIs will be used to access the guest memory.
> >
> > So, what counts as "debug" here, and why are debug requests
> > special? If "debug=1" means "can actually get at the guest memory",
> > why wouldn't every device model want to use it?
>
> SEV has a flag that the guest-owner can set on a VM to enable debug;
> it's rare for it to be enabled; so it's not suitable for use by normal
> devices.  It's only there for debug if the guest owner allows you to.

So if I do a memory transaction with debug=1 then I should expect
that it might come back with a failure status (meaning "this VM
doesn't permit debug") and I should handle that error ?

thanks
-- PMM

Re: [PATCH 01/11] memattrs: add debug attribute

[Dr. David Alan Gilbert]

* Peter Maydell (peter.maydell@linaro.org) wrote:
> On Tue, 1 Dec 2020 at 11:51, Dr. David Alan Gilbert <dgilbert@redhat.com> wrote:
> >
> > * Peter Maydell (peter.maydell@linaro.org) wrote:
> > > On Mon, 16 Nov 2020 at 19:28, Ashish Kalra <Ashish.Kalra@amd.com> wrote:
> > > >
> > > > From: Brijesh Singh <brijesh.singh@amd.com>
> > > >
> > > > From: Brijesh Singh <brijesh.singh@amd.com>
> > > >
> > > > Extend the MemTxAttrs to include a 'debug' flag. The flag can be used as
> > > > general indicator that operation was triggered by the debugger.
> > > >
> > > > A subsequent patch will set the debug=1 when issuing a memory access
> > > > from the gdbstub or HMP commands. This is a prerequisite to support
> > > > debugging an encrypted guest. When a request with debug=1 is seen, the
> > > > encryption APIs will be used to access the guest memory.
> > >
> > > So, what counts as "debug" here, and why are debug requests
> > > special? If "debug=1" means "can actually get at the guest memory",
> > > why wouldn't every device model want to use it?
> >
> > SEV has a flag that the guest-owner can set on a VM to enable debug;
> > it's rare for it to be enabled; so it's not suitable for use by normal
> > devices.  It's only there for debug if the guest owner allows you to.
> 
> So if I do a memory transaction with debug=1 then I should expect
> that it might come back with a failure status (meaning "this VM
> doesn't permit debug") and I should handle that error ?

I think that's probably true.

Dave

> thanks
> -- PMM
> 
-- 
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

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

[Ashish Kalra]

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.

Signed-off-by: Ashish Kalra <ashish.kalra@amd.com>
---
 include/exec/memory.h | 11 +++++++++++
 softmmu/physmem.c     | 24 ++++++++++++++++++++----
 2 files changed, 31 insertions(+), 4 deletions(-)

diff --git a/include/exec/memory.h b/include/exec/memory.h
index aff6ef7605..73deb4b456 100644
--- a/include/exec/memory.h
+++ b/include/exec/memory.h
@@ -2394,6 +2394,17 @@ MemTxResult address_space_write_cached_slow(MemoryRegionCache *cache,
                                             hwaddr addr, const void *buf,
                                             hwaddr len);
 
+typedef struct MemoryDebugOps {
+    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;
+
+void address_space_set_debug_ops(const MemoryDebugOps *ops);
+
 static inline bool memory_access_is_direct(MemoryRegion *mr, bool is_write)
 {
     if (is_write) {
diff --git a/softmmu/physmem.c b/softmmu/physmem.c
index a9adedb9f8..057d6d4ce1 100644
--- a/softmmu/physmem.c
+++ b/softmmu/physmem.c
@@ -166,6 +166,18 @@ struct DirtyBitmapSnapshot {
     unsigned long dirty[];
 };
 
+static const MemoryDebugOps default_debug_ops = {
+    .read = address_space_read,
+    .write = address_space_write_rom
+};
+
+static const MemoryDebugOps *debug_ops = &default_debug_ops;
+
+void address_space_set_debug_ops(const MemoryDebugOps *ops)
+{
+    debug_ops = ops;
+}
+
 static void phys_map_node_reserve(PhysPageMap *map, unsigned nodes)
 {
     static unsigned alloc_hint = 16;
@@ -3407,6 +3419,10 @@ int cpu_memory_rw_debug(CPUState *cpu, target_ulong addr,
         page = addr & TARGET_PAGE_MASK;
         phys_addr = cpu_get_phys_page_attrs_debug(cpu, page, &attrs);
         asidx = cpu_asidx_from_attrs(cpu, attrs);
+
+        /* set debug attrs to indicate memory access is from the debugger */
+        attrs.debug = 1;
+
         /* if no physical page mapped, return an error */
         if (phys_addr == -1)
             return -1;
@@ -3415,11 +3431,11 @@ int cpu_memory_rw_debug(CPUState *cpu, target_ulong addr,
             l = len;
         phys_addr += (addr & ~TARGET_PAGE_MASK);
         if (is_write) {
-            res = address_space_write_rom(cpu->cpu_ases[asidx].as, phys_addr,
-                                          attrs, buf, l);
+            res = debug_ops->write(cpu->cpu_ases[asidx].as, phys_addr,
+                                   attrs, buf, l);
         } else {
-            res = address_space_read(cpu->cpu_ases[asidx].as, phys_addr,
-                                     attrs, buf, l);
+            res = debug_ops->read(cpu->cpu_ases[asidx].as, phys_addr,
+                                  attrs, buf, l);
         }
         if (res != MEMTX_OK) {
             return -1;
-- 
2.17.1

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

[Dr. David Alan Gilbert]

* 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.
> 
> Signed-off-by: Ashish Kalra <ashish.kalra@amd.com>

Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>

> ---
>  include/exec/memory.h | 11 +++++++++++
>  softmmu/physmem.c     | 24 ++++++++++++++++++++----
>  2 files changed, 31 insertions(+), 4 deletions(-)
> 
> diff --git a/include/exec/memory.h b/include/exec/memory.h
> index aff6ef7605..73deb4b456 100644
> --- a/include/exec/memory.h
> +++ b/include/exec/memory.h
> @@ -2394,6 +2394,17 @@ MemTxResult address_space_write_cached_slow(MemoryRegionCache *cache,
>                                              hwaddr addr, const void *buf,
>                                              hwaddr len);
>  
> +typedef struct MemoryDebugOps {
> +    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;
> +
> +void address_space_set_debug_ops(const MemoryDebugOps *ops);
> +
>  static inline bool memory_access_is_direct(MemoryRegion *mr, bool is_write)
>  {
>      if (is_write) {
> diff --git a/softmmu/physmem.c b/softmmu/physmem.c
> index a9adedb9f8..057d6d4ce1 100644
> --- a/softmmu/physmem.c
> +++ b/softmmu/physmem.c
> @@ -166,6 +166,18 @@ struct DirtyBitmapSnapshot {
>      unsigned long dirty[];
>  };
>  
> +static const MemoryDebugOps default_debug_ops = {
> +    .read = address_space_read,
> +    .write = address_space_write_rom
> +};
> +
> +static const MemoryDebugOps *debug_ops = &default_debug_ops;
> +
> +void address_space_set_debug_ops(const MemoryDebugOps *ops)
> +{
> +    debug_ops = ops;
> +}
> +
>  static void phys_map_node_reserve(PhysPageMap *map, unsigned nodes)
>  {
>      static unsigned alloc_hint = 16;
> @@ -3407,6 +3419,10 @@ int cpu_memory_rw_debug(CPUState *cpu, target_ulong addr,
>          page = addr & TARGET_PAGE_MASK;
>          phys_addr = cpu_get_phys_page_attrs_debug(cpu, page, &attrs);
>          asidx = cpu_asidx_from_attrs(cpu, attrs);
> +
> +        /* set debug attrs to indicate memory access is from the debugger */
> +        attrs.debug = 1;
> +
>          /* if no physical page mapped, return an error */
>          if (phys_addr == -1)
>              return -1;
> @@ -3415,11 +3431,11 @@ int cpu_memory_rw_debug(CPUState *cpu, target_ulong addr,
>              l = len;
>          phys_addr += (addr & ~TARGET_PAGE_MASK);
>          if (is_write) {
> -            res = address_space_write_rom(cpu->cpu_ases[asidx].as, phys_addr,
> -                                          attrs, buf, l);
> +            res = debug_ops->write(cpu->cpu_ases[asidx].as, phys_addr,
> +                                   attrs, buf, l);
>          } else {
> -            res = address_space_read(cpu->cpu_ases[asidx].as, phys_addr,
> -                                     attrs, buf, l);
> +            res = debug_ops->read(cpu->cpu_ases[asidx].as, phys_addr,
> +                                  attrs, buf, l);
>          }
>          if (res != MEMTX_OK) {
>              return -1;
> -- 
> 2.17.1
> 
-- 
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

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

[Peter Maydell]

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.

thanks
-- PMM

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

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

[Peter Maydell]

On Tue, 1 Dec 2020 at 14:28, Ashish Kalra <ashish.kalra@amd.com> 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".

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

thanks
-- PMM

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

[Ashish Kalra]

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@amd.com> 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.
> 

[PATCH 03/11] exec: add ram_debug_ops support

[Ashish Kalra]

From: Brijesh Singh <brijesh.singh@amd.com>

From: Brijesh Singh <brijesh.singh@amd.com>

Currently, guest memory access for debugging purposes is performed using
memcpy(). Extend the 'struct MemoryRegion' to include new callbacks that
can be used to override the use of memcpy() with something else.

The new callbacks can be used to display the guest memory of an SEV guest
by registering callbacks to the SEV memory encryption/decryption APIs.

Typical usage:

mem_read(uint8_t *dst, uint8_t *src, uint32_t len, MemTxAttrs *attrs);
mem_write(uint8_t *dst, uint8_t *src, uint32_t len, MemTxAttrs *attrs);

MemoryRegionRAMReadWriteOps ops;
ops.read = mem_read;
ops.write = mem_write;

memory_region_init_ram(mem, NULL, "memory", size, NULL);
memory_region_set_ram_debug_ops(mem, ops);

Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>
Signed-off-by: Ashish Kalra <ashish.kalra@amd.com>
---
 include/exec/memory.h | 27 +++++++++++++++++++++++++++
 1 file changed, 27 insertions(+)

diff --git a/include/exec/memory.h b/include/exec/memory.h
index 73deb4b456..2fb4193358 100644
--- a/include/exec/memory.h
+++ b/include/exec/memory.h
@@ -402,6 +402,18 @@ struct IOMMUMemoryRegionClass {
 typedef struct CoalescedMemoryRange CoalescedMemoryRange;
 typedef struct MemoryRegionIoeventfd MemoryRegionIoeventfd;
 
+/* Memory Region RAM debug callback */
+typedef struct MemoryRegionRAMReadWriteOps MemoryRegionRAMReadWriteOps;
+
+struct MemoryRegionRAMReadWriteOps {
+    /* Write data into guest memory */
+    int (*write) (uint8_t *dest, const uint8_t *src,
+                  uint32_t len, MemTxAttrs attrs);
+    /* Read data from guest memory */
+    int (*read) (uint8_t *dest, const uint8_t *src,
+                 uint32_t len, MemTxAttrs attrs);
+};
+
 /** MemoryRegion:
  *
  * A struct representing a memory region.
@@ -445,6 +457,7 @@ struct MemoryRegion {
     const char *name;
     unsigned ioeventfd_nb;
     MemoryRegionIoeventfd *ioeventfds;
+    const MemoryRegionRAMReadWriteOps *ram_debug_ops;
 };
 
 struct IOMMUMemoryRegion {
@@ -1060,6 +1073,20 @@ void memory_region_init_rom_nomigrate(MemoryRegion *mr,
                                       uint64_t size,
                                       Error **errp);
 
+/**
+ * memory_region_set_ram_debug_ops: Set access ops for a give memory region.
+ *
+ * @mr: the #MemoryRegion to be initialized
+ * @ops: a function that will be used when accessing @target region during
+ *       debug
+ */
+static inline void
+memory_region_set_ram_debug_ops(MemoryRegion *mr,
+                                const MemoryRegionRAMReadWriteOps *ops)
+{
+    mr->ram_debug_ops = ops;
+}
+
 /**
  * memory_region_init_rom_device_nomigrate:  Initialize a ROM memory region.
  *                                 Writes are handled via callbacks.
-- 
2.17.1

Re: [PATCH 03/11] exec: add ram_debug_ops support

[Peter Maydell]

On Mon, 16 Nov 2020 at 19:19, Ashish Kalra <Ashish.Kalra@amd.com> wrote:
>
> From: Brijesh Singh <brijesh.singh@amd.com>
>
> From: Brijesh Singh <brijesh.singh@amd.com>
>
> Currently, guest memory access for debugging purposes is performed using
> memcpy(). Extend the 'struct MemoryRegion' to include new callbacks that
> can be used to override the use of memcpy() with something else.
>
> The new callbacks can be used to display the guest memory of an SEV guest
> by registering callbacks to the SEV memory encryption/decryption APIs.
>
> Typical usage:
>
> mem_read(uint8_t *dst, uint8_t *src, uint32_t len, MemTxAttrs *attrs);
> mem_write(uint8_t *dst, uint8_t *src, uint32_t len, MemTxAttrs *attrs);

We already have a function prototype for "I need to call a function
to do this read or write":
    MemTxResult (*read_with_attrs)(void *opaque,
                                   hwaddr addr,
                                   uint64_t *data,
                                   unsigned size,
                                   MemTxAttrs attrs);
    MemTxResult (*write_with_attrs)(void *opaque,
                                    hwaddr addr,
                                    uint64_t data,
                                    unsigned size,
                                    MemTxAttrs attrs);

Do the prototypes for accessing guest RAM that needs decryption
really need to be different from that?

thanks
-- PMM

Re: [PATCH 03/11] exec: add ram_debug_ops support

[Ashish Kalra]

On Tue, Dec 01, 2020 at 12:08:28PM +0000, Peter Maydell wrote:
> On Mon, 16 Nov 2020 at 19:19, Ashish Kalra <Ashish.Kalra@amd.com> wrote:
> >
> > From: Brijesh Singh <brijesh.singh@amd.com>
> >
> > From: Brijesh Singh <brijesh.singh@amd.com>
> >
> > Currently, guest memory access for debugging purposes is performed using
> > memcpy(). Extend the 'struct MemoryRegion' to include new callbacks that
> > can be used to override the use of memcpy() with something else.
> >
> > The new callbacks can be used to display the guest memory of an SEV guest
> > by registering callbacks to the SEV memory encryption/decryption APIs.
> >
> > Typical usage:
> >
> > mem_read(uint8_t *dst, uint8_t *src, uint32_t len, MemTxAttrs *attrs);
> > mem_write(uint8_t *dst, uint8_t *src, uint32_t len, MemTxAttrs *attrs);
> 
> We already have a function prototype for "I need to call a function
> to do this read or write":
>     MemTxResult (*read_with_attrs)(void *opaque,
>                                    hwaddr addr,
>                                    uint64_t *data,
>                                    unsigned size,
>                                    MemTxAttrs attrs);
>     MemTxResult (*write_with_attrs)(void *opaque,
>                                     hwaddr addr,
>                                     uint64_t data,
>                                     unsigned size,
>                                     MemTxAttrs attrs);
> 
> Do the prototypes for accessing guest RAM that needs decryption
> really need to be different from that?
> 

This again falls back to the same thought process, to keep the debug
specific code separate from the generic code. If the above
MemoryRegionOps are used, then again we are using generic code to invoke
debug specific stuff.

Thanks,
Ashish

[PATCH 04/11] exec: Add address_space_read and address_space_write debug helpers.

[Ashish Kalra]

From: Ashish Kalra <ashish.kalra@amd.com>

Add new address_space_read and address_space_write debug helper
interfaces which can be invoked by vendor specific guest memory
debug assist/hooks to do guest RAM memory accesses using the
added MemoryRegion callbacks.

Signed-off-by: Ashish Kalra <ashish.kalra@amd.com>
---
 include/exec/memory.h | 10 +++++
 softmmu/physmem.c     | 88 +++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 98 insertions(+)

diff --git a/include/exec/memory.h b/include/exec/memory.h
index 2fb4193358..74f2dcec00 100644
--- a/include/exec/memory.h
+++ b/include/exec/memory.h
@@ -2432,6 +2432,16 @@ typedef struct MemoryDebugOps {
 
 void address_space_set_debug_ops(const MemoryDebugOps *ops);
 
+MemTxResult address_space_write_rom_debug(AddressSpace *as,
+                                          hwaddr addr,
+                                          MemTxAttrs attrs,
+                                          const void *ptr,
+                                          hwaddr len);
+
+MemTxResult address_space_read_debug(AddressSpace *as, hwaddr addr,
+                                     MemTxAttrs attrs, void *buf,
+                                     hwaddr len);
+
 static inline bool memory_access_is_direct(MemoryRegion *mr, bool is_write)
 {
     if (is_write) {
diff --git a/softmmu/physmem.c b/softmmu/physmem.c
index 057d6d4ce1..2c08624ca8 100644
--- a/softmmu/physmem.c
+++ b/softmmu/physmem.c
@@ -3266,6 +3266,94 @@ void cpu_physical_memory_unmap(void *buffer, hwaddr len,
 #define RCU_READ_UNLOCK(...)     rcu_read_unlock()
 #include "memory_ldst.c.inc"
 
+MemTxResult address_space_read_debug(AddressSpace *as, hwaddr addr,
+                                     MemTxAttrs attrs, void *ptr,
+                                     hwaddr len)
+{
+    uint64_t val;
+    MemoryRegion *mr;
+    hwaddr l = len;
+    hwaddr addr1;
+    MemTxResult result = MEMTX_OK;
+    bool release_lock = false;
+    uint8_t *buf = ptr;
+    uint8_t *ram_ptr;
+
+    for (;;) {
+        RCU_READ_LOCK_GUARD();
+        mr = address_space_translate(as, addr, &addr1, &l, false, attrs);
+        if (!memory_access_is_direct(mr, false)) {
+            /* I/O case */
+            release_lock |= prepare_mmio_access(mr);
+            l = memory_access_size(mr, l, addr1);
+            result |= memory_region_dispatch_read(mr, addr1, &val,
+                                                  size_memop(l), attrs);
+            stn_he_p(buf, l, val);
+        } else {
+            /* RAM case */
+            fuzz_dma_read_cb(addr, l, mr, false);
+            ram_ptr = qemu_ram_ptr_length(mr->ram_block, addr1, &l, false);
+            if (attrs.debug && mr->ram_debug_ops) {
+                mr->ram_debug_ops->read(buf, ram_ptr, l, attrs);
+            } else {
+                memcpy(buf, ram_ptr, l);
+            }
+            result = MEMTX_OK;
+        }
+        if (release_lock) {
+            qemu_mutex_unlock_iothread();
+            release_lock = false;
+        }
+
+        len -= l;
+        buf += l;
+        addr += l;
+
+        if (!len) {
+            break;
+        }
+        l = len;
+    }
+    return result;
+}
+
+inline MemTxResult address_space_write_rom_debug(AddressSpace *as,
+                                                 hwaddr addr,
+                                                 MemTxAttrs attrs,
+                                                 const void *ptr,
+                                                 hwaddr len)
+{
+    hwaddr l;
+    uint8_t *ram_ptr;
+    hwaddr addr1;
+    MemoryRegion *mr;
+    const uint8_t *buf = ptr;
+
+    RCU_READ_LOCK_GUARD();
+    while (len > 0) {
+        l = len;
+        mr = address_space_translate(as, addr, &addr1, &l, true, attrs);
+
+        if (!(memory_region_is_ram(mr) ||
+              memory_region_is_romd(mr))) {
+            l = memory_access_size(mr, l, addr1);
+        } else {
+            /* ROM/RAM case */
+            ram_ptr = qemu_map_ram_ptr(mr->ram_block, addr1);
+            if (attrs.debug && mr->ram_debug_ops) {
+                mr->ram_debug_ops->write(ram_ptr, buf, l, attrs);
+            } else {
+                memcpy(ram_ptr, buf, l);
+            }
+            invalidate_and_set_dirty(mr, addr1, l);
+        }
+        len -= l;
+        buf += l;
+        addr += l;
+    }
+    return MEMTX_OK;
+}
+
 int64_t address_space_cache_init(MemoryRegionCache *cache,
                                  AddressSpace *as,
                                  hwaddr addr,
-- 
2.17.1

[PATCH 05/11] exec: add debug version of physical memory read and write API

[Ashish Kalra]

From: Brijesh Singh <brijesh.singh@amd.com>

Adds the following new APIs
- cpu_physical_memory_read_debug
- cpu_physical_memory_write_debug
- cpu_physical_memory_rw_debug
- ldl_phys_debug
- ldq_phys_debug

The subsequent patch will make use of the API introduced, to ensure
that the page table walks are handled correctly when debugging an
SEV guest.

Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>
Signed-off-by: Ashish Kalra <ashish.kalra@amd.com>
---
 include/exec/cpu-common.h | 15 +++++++++++++
 softmmu/physmem.c         | 47 +++++++++++++++++++++++++++++++++++++++
 2 files changed, 62 insertions(+)

diff --git a/include/exec/cpu-common.h b/include/exec/cpu-common.h
index 19805ed6db..d2089e6873 100644
--- a/include/exec/cpu-common.h
+++ b/include/exec/cpu-common.h
@@ -71,11 +71,26 @@ size_t qemu_ram_pagesize_largest(void);
 
 void cpu_physical_memory_rw(hwaddr addr, void *buf,
                             hwaddr len, bool is_write);
+void cpu_physical_memory_rw_debug(hwaddr addr, uint8_t *buf,
+                                  int len, int is_write);
 static inline void cpu_physical_memory_read(hwaddr addr,
                                             void *buf, hwaddr len)
 {
     cpu_physical_memory_rw(addr, buf, len, false);
 }
+static inline void cpu_physical_memory_read_debug(hwaddr addr,
+                                                  void *buf, int len)
+{
+    cpu_physical_memory_rw_debug(addr, buf, len, false);
+}
+static inline void cpu_physical_memory_write_debug(hwaddr addr,
+                                                   const void *buf, int len)
+{
+    cpu_physical_memory_rw_debug(addr, (void *)buf, len, true);
+}
+uint32_t ldl_phys_debug(CPUState *cpu, hwaddr addr);
+uint64_t ldq_phys_debug(CPUState *cpu, hwaddr addr);
+
 static inline void cpu_physical_memory_write(hwaddr addr,
                                              const void *buf, hwaddr len)
 {
diff --git a/softmmu/physmem.c b/softmmu/physmem.c
index 2c08624ca8..6945bd5efe 100644
--- a/softmmu/physmem.c
+++ b/softmmu/physmem.c
@@ -3354,6 +3354,53 @@ inline MemTxResult address_space_write_rom_debug(AddressSpace *as,
     return MEMTX_OK;
 }
 
+uint32_t ldl_phys_debug(CPUState *cpu, hwaddr addr)
+{
+    MemTxAttrs attrs;
+    int asidx = cpu_asidx_from_attrs(cpu, attrs);
+    uint32_t val;
+
+    /* set debug attrs to indicate memory access is from the debugger */
+    attrs.debug = 1;
+
+    debug_ops->read(cpu->cpu_ases[asidx].as, addr, attrs,
+                    (void *) &val, 4);
+
+    return tswap32(val);
+}
+
+uint64_t ldq_phys_debug(CPUState *cpu, hwaddr addr)
+{
+    MemTxAttrs attrs;
+    int asidx = cpu_asidx_from_attrs(cpu, attrs);
+    uint64_t val;
+
+    /* set debug attrs to indicate memory access is from the debugger */
+    attrs.debug = 1;
+
+    debug_ops->read(cpu->cpu_ases[asidx].as, addr, attrs,
+                    (void *) &val, 8);
+    return val;
+}
+
+void cpu_physical_memory_rw_debug(hwaddr addr, uint8_t *buf,
+                                  int len, int is_write)
+{
+    MemTxAttrs attrs;
+
+    /* set debug attrs to indicate memory access is from the debugger */
+    attrs.debug = 1;
+
+    if (is_write) {
+                debug_ops->write(&address_space_memory, addr,
+                                 attrs, buf, len);
+        } else {
+                debug_ops->read(&address_space_memory, addr,
+                                attrs, buf, len);
+        }
+
+}
+
 int64_t address_space_cache_init(MemoryRegionCache *cache,
                                  AddressSpace *as,
                                  hwaddr addr,
-- 
2.17.1

Re: [PATCH 05/11] exec: add debug version of physical memory read and write API

[Dov Murik]

On 16/11/2020 20:51, Ashish Kalra wrote:
> From: Brijesh Singh <brijesh.singh@amd.com>
> 
> Adds the following new APIs
> - cpu_physical_memory_read_debug
> - cpu_physical_memory_write_debug
> - cpu_physical_memory_rw_debug
> - ldl_phys_debug
> - ldq_phys_debug
> 
> The subsequent patch will make use of the API introduced, to ensure
> that the page table walks are handled correctly when debugging an
> SEV guest.
> 
> Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>
> Signed-off-by: Ashish Kalra <ashish.kalra@amd.com>
> ---

[...]


> diff --git a/softmmu/physmem.c b/softmmu/physmem.c
> index 2c08624ca8..6945bd5efe 100644
> --- a/softmmu/physmem.c
> +++ b/softmmu/physmem.c
> @@ -3354,6 +3354,53 @@ inline MemTxResult address_space_write_rom_debug(AddressSpace *as,
>       return MEMTX_OK;
>   }
> 
> +uint32_t ldl_phys_debug(CPUState *cpu, hwaddr addr)
> +{
> +    MemTxAttrs attrs;
> +    int asidx = cpu_asidx_from_attrs(cpu, attrs);
> +    uint32_t val;
> +
> +    /* set debug attrs to indicate memory access is from the debugger */
> +    attrs.debug = 1;
> +
> +    debug_ops->read(cpu->cpu_ases[asidx].as, addr, attrs,
> +                    (void *) &val, 4);
> +
> +    return tswap32(val);
> +}
> +
> +uint64_t ldq_phys_debug(CPUState *cpu, hwaddr addr)
> +{
> +    MemTxAttrs attrs;
> +    int asidx = cpu_asidx_from_attrs(cpu, attrs);
> +    uint64_t val;
> +
> +    /* set debug attrs to indicate memory access is from the debugger */
> +    attrs.debug = 1;
> +
> +    debug_ops->read(cpu->cpu_ases[asidx].as, addr, attrs,
> +                    (void *) &val, 8);
> +    return val;

You probably want tswap64(val) here like in ldl_phys_debug (even though 
I assume it's a noop in the SEV case).

> +}
> +
> +void cpu_physical_memory_rw_debug(hwaddr addr, uint8_t *buf,
> +                                  int len, int is_write)
> +{
> +    MemTxAttrs attrs;
> +
> +    /* set debug attrs to indicate memory access is from the debugger */
> +    attrs.debug = 1;

Maybe:

     MemTxAttrs attrs = { .debug = 1 };

(Also in the functions above.)

> +
> +    if (is_write) {
> +                debug_ops->write(&address_space_memory, addr,
> +                                 attrs, buf, len);
> +        } else {
> +                debug_ops->read(&address_space_memory, addr,
> +                                attrs, buf, len);
> +        }
> +
> +}
> +
>   int64_t address_space_cache_init(MemoryRegionCache *cache,
>                                    AddressSpace *as,
>                                    hwaddr addr,
> 

[PATCH 06/11] monitor/i386: use debug APIs when accessing guest memory

[Ashish Kalra]

From: Brijesh Singh <brijesh.singh@amd.com>

Update the HMP commands to use the debug version of APIs when accessing
guest memory.

Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>
Signed-off-by: Ashish Kalra <ashish.kalra@amd.com>
---
 monitor/misc.c        |  4 ++--
 softmmu/cpus.c        |  2 +-
 target/i386/monitor.c | 54 ++++++++++++++++++++++++-------------------
 3 files changed, 33 insertions(+), 27 deletions(-)

diff --git a/monitor/misc.c b/monitor/misc.c
index 32e6a8c13d..7eba3a6fce 100644
--- a/monitor/misc.c
+++ b/monitor/misc.c
@@ -824,8 +824,8 @@ static void hmp_sum(Monitor *mon, const QDict *qdict)
 
     sum = 0;
     for(addr = start; addr < (start + size); addr++) {
-        uint8_t val = address_space_ldub(&address_space_memory, addr,
-                                         MEMTXATTRS_UNSPECIFIED, NULL);
+        uint8_t val;
+        cpu_physical_memory_read_debug(addr, &val, 1);
         /* BSD sum algorithm ('sum' Unix command) */
         sum = (sum >> 1) | (sum << 15);
         sum += val;
diff --git a/softmmu/cpus.c b/softmmu/cpus.c
index e46ac68ad0..79817330b7 100644
--- a/softmmu/cpus.c
+++ b/softmmu/cpus.c
@@ -779,7 +779,7 @@ void qmp_pmemsave(int64_t addr, int64_t size, const char *filename,
         l = sizeof(buf);
         if (l > size)
             l = size;
-        cpu_physical_memory_read(addr, buf, l);
+        cpu_physical_memory_read_debug(addr, buf, l);
         if (fwrite(buf, 1, l, f) != l) {
             error_setg(errp, QERR_IO_ERROR);
             goto exit;
diff --git a/target/i386/monitor.c b/target/i386/monitor.c
index 7abae3c8df..9ca9c677a5 100644
--- a/target/i386/monitor.c
+++ b/target/i386/monitor.c
@@ -79,7 +79,7 @@ static void tlb_info_32(Monitor *mon, CPUArchState *env)
 
     pgd = env->cr[3] & ~0xfff;
     for(l1 = 0; l1 < 1024; l1++) {
-        cpu_physical_memory_read(pgd + l1 * 4, &pde, 4);
+        cpu_physical_memory_read_debug(pgd + l1 * 4, &pde, 4);
         pde = le32_to_cpu(pde);
         if (pde & PG_PRESENT_MASK) {
             if ((pde & PG_PSE_MASK) && (env->cr[4] & CR4_PSE_MASK)) {
@@ -87,7 +87,8 @@ static void tlb_info_32(Monitor *mon, CPUArchState *env)
                 print_pte(mon, env, (l1 << 22), pde, ~((1 << 21) - 1));
             } else {
                 for(l2 = 0; l2 < 1024; l2++) {
-                    cpu_physical_memory_read((pde & ~0xfff) + l2 * 4, &pte, 4);
+                    cpu_physical_memory_read_debug((pde & ~0xfff) + l2 * 4,
+                                                   &pte, 4);
                     pte = le32_to_cpu(pte);
                     if (pte & PG_PRESENT_MASK) {
                         print_pte(mon, env, (l1 << 22) + (l2 << 12),
@@ -108,12 +109,12 @@ static void tlb_info_pae32(Monitor *mon, CPUArchState *env)
 
     pdp_addr = env->cr[3] & ~0x1f;
     for (l1 = 0; l1 < 4; l1++) {
-        cpu_physical_memory_read(pdp_addr + l1 * 8, &pdpe, 8);
+        cpu_physical_memory_read_debug(pdp_addr + l1 * 8, &pdpe, 8);
         pdpe = le64_to_cpu(pdpe);
         if (pdpe & PG_PRESENT_MASK) {
             pd_addr = pdpe & 0x3fffffffff000ULL;
             for (l2 = 0; l2 < 512; l2++) {
-                cpu_physical_memory_read(pd_addr + l2 * 8, &pde, 8);
+                cpu_physical_memory_read_debug(pd_addr + l2 * 8, &pde, 8);
                 pde = le64_to_cpu(pde);
                 if (pde & PG_PRESENT_MASK) {
                     if (pde & PG_PSE_MASK) {
@@ -123,7 +124,8 @@ static void tlb_info_pae32(Monitor *mon, CPUArchState *env)
                     } else {
                         pt_addr = pde & 0x3fffffffff000ULL;
                         for (l3 = 0; l3 < 512; l3++) {
-                            cpu_physical_memory_read(pt_addr + l3 * 8, &pte, 8);
+                            cpu_physical_memory_read_debug(pt_addr + l3 * 8,
+                                                           &pte, 8);
                             pte = le64_to_cpu(pte);
                             if (pte & PG_PRESENT_MASK) {
                                 print_pte(mon, env, (l1 << 30) + (l2 << 21)
@@ -148,7 +150,7 @@ static void tlb_info_la48(Monitor *mon, CPUArchState *env,
     uint64_t pdp_addr, pd_addr, pt_addr;
 
     for (l1 = 0; l1 < 512; l1++) {
-        cpu_physical_memory_read(pml4_addr + l1 * 8, &pml4e, 8);
+        cpu_physical_memory_read_debug(pml4_addr + l1 * 8, &pml4e, 8);
         pml4e = le64_to_cpu(pml4e);
         if (!(pml4e & PG_PRESENT_MASK)) {
             continue;
@@ -156,7 +158,7 @@ static void tlb_info_la48(Monitor *mon, CPUArchState *env,
 
         pdp_addr = pml4e & 0x3fffffffff000ULL;
         for (l2 = 0; l2 < 512; l2++) {
-            cpu_physical_memory_read(pdp_addr + l2 * 8, &pdpe, 8);
+            cpu_physical_memory_read_debug(pdp_addr + l2 * 8, &pdpe, 8);
             pdpe = le64_to_cpu(pdpe);
             if (!(pdpe & PG_PRESENT_MASK)) {
                 continue;
@@ -171,7 +173,7 @@ static void tlb_info_la48(Monitor *mon, CPUArchState *env,
 
             pd_addr = pdpe & 0x3fffffffff000ULL;
             for (l3 = 0; l3 < 512; l3++) {
-                cpu_physical_memory_read(pd_addr + l3 * 8, &pde, 8);
+                cpu_physical_memory_read_debug(pd_addr + l3 * 8, &pde, 8);
                 pde = le64_to_cpu(pde);
                 if (!(pde & PG_PRESENT_MASK)) {
                     continue;
@@ -186,7 +188,7 @@ static void tlb_info_la48(Monitor *mon, CPUArchState *env,
 
                 pt_addr = pde & 0x3fffffffff000ULL;
                 for (l4 = 0; l4 < 512; l4++) {
-                    cpu_physical_memory_read(pt_addr
+                    cpu_physical_memory_read_debug(pt_addr
                             + l4 * 8,
                             &pte, 8);
                     pte = le64_to_cpu(pte);
@@ -209,7 +211,7 @@ static void tlb_info_la57(Monitor *mon, CPUArchState *env)
 
     pml5_addr = env->cr[3] & 0x3fffffffff000ULL;
     for (l0 = 0; l0 < 512; l0++) {
-        cpu_physical_memory_read(pml5_addr + l0 * 8, &pml5e, 8);
+        cpu_physical_memory_read_debug(pml5_addr + l0 * 8, &pml5e, 8);
         pml5e = le64_to_cpu(pml5e);
         if (pml5e & PG_PRESENT_MASK) {
             tlb_info_la48(mon, env, l0, pml5e & 0x3fffffffff000ULL);
@@ -286,7 +288,7 @@ static void mem_info_32(Monitor *mon, CPUArchState *env)
     last_prot = 0;
     start = -1;
     for(l1 = 0; l1 < 1024; l1++) {
-        cpu_physical_memory_read(pgd + l1 * 4, &pde, 4);
+        cpu_physical_memory_read_debug(pgd + l1 * 4, &pde, 4);
         pde = le32_to_cpu(pde);
         end = l1 << 22;
         if (pde & PG_PRESENT_MASK) {
@@ -295,7 +297,8 @@ static void mem_info_32(Monitor *mon, CPUArchState *env)
                 mem_print(mon, env, &start, &last_prot, end, prot);
             } else {
                 for(l2 = 0; l2 < 1024; l2++) {
-                    cpu_physical_memory_read((pde & ~0xfff) + l2 * 4, &pte, 4);
+                    cpu_physical_memory_read_debug((pde & ~0xfff) + l2 * 4,
+                                                   &pte, 4);
                     pte = le32_to_cpu(pte);
                     end = (l1 << 22) + (l2 << 12);
                     if (pte & PG_PRESENT_MASK) {
@@ -328,13 +331,13 @@ static void mem_info_pae32(Monitor *mon, CPUArchState *env)
     last_prot = 0;
     start = -1;
     for (l1 = 0; l1 < 4; l1++) {
-        cpu_physical_memory_read(pdp_addr + l1 * 8, &pdpe, 8);
+        cpu_physical_memory_read_debug(pdp_addr + l1 * 8, &pdpe, 8);
         pdpe = le64_to_cpu(pdpe);
         end = l1 << 30;
         if (pdpe & PG_PRESENT_MASK) {
             pd_addr = pdpe & 0x3fffffffff000ULL;
             for (l2 = 0; l2 < 512; l2++) {
-                cpu_physical_memory_read(pd_addr + l2 * 8, &pde, 8);
+                cpu_physical_memory_read_debug(pd_addr + l2 * 8, &pde, 8);
                 pde = le64_to_cpu(pde);
                 end = (l1 << 30) + (l2 << 21);
                 if (pde & PG_PRESENT_MASK) {
@@ -345,7 +348,8 @@ static void mem_info_pae32(Monitor *mon, CPUArchState *env)
                     } else {
                         pt_addr = pde & 0x3fffffffff000ULL;
                         for (l3 = 0; l3 < 512; l3++) {
-                            cpu_physical_memory_read(pt_addr + l3 * 8, &pte, 8);
+                            cpu_physical_memory_read_debug(pt_addr + l3 * 8,
+                                                           &pte, 8);
                             pte = le64_to_cpu(pte);
                             end = (l1 << 30) + (l2 << 21) + (l3 << 12);
                             if (pte & PG_PRESENT_MASK) {
@@ -384,13 +388,13 @@ static void mem_info_la48(Monitor *mon, CPUArchState *env)
     last_prot = 0;
     start = -1;
     for (l1 = 0; l1 < 512; l1++) {
-        cpu_physical_memory_read(pml4_addr + l1 * 8, &pml4e, 8);
+        cpu_physical_memory_read_debug(pml4_addr + l1 * 8, &pml4e, 8);
         pml4e = le64_to_cpu(pml4e);
         end = l1 << 39;
         if (pml4e & PG_PRESENT_MASK) {
             pdp_addr = pml4e & 0x3fffffffff000ULL;
             for (l2 = 0; l2 < 512; l2++) {
-                cpu_physical_memory_read(pdp_addr + l2 * 8, &pdpe, 8);
+                cpu_physical_memory_read_debug(pdp_addr + l2 * 8, &pdpe, 8);
                 pdpe = le64_to_cpu(pdpe);
                 end = (l1 << 39) + (l2 << 30);
                 if (pdpe & PG_PRESENT_MASK) {
@@ -402,7 +406,8 @@ static void mem_info_la48(Monitor *mon, CPUArchState *env)
                     } else {
                         pd_addr = pdpe & 0x3fffffffff000ULL;
                         for (l3 = 0; l3 < 512; l3++) {
-                            cpu_physical_memory_read(pd_addr + l3 * 8, &pde, 8);
+                            cpu_physical_memory_read_debug(pd_addr + l3 * 8,
+                                                           &pde, 8);
                             pde = le64_to_cpu(pde);
                             end = (l1 << 39) + (l2 << 30) + (l3 << 21);
                             if (pde & PG_PRESENT_MASK) {
@@ -415,7 +420,7 @@ static void mem_info_la48(Monitor *mon, CPUArchState *env)
                                 } else {
                                     pt_addr = pde & 0x3fffffffff000ULL;
                                     for (l4 = 0; l4 < 512; l4++) {
-                                        cpu_physical_memory_read(pt_addr
+                                        cpu_physical_memory_read_debug(pt_addr
                                                                  + l4 * 8,
                                                                  &pte, 8);
                                         pte = le64_to_cpu(pte);
@@ -464,7 +469,7 @@ static void mem_info_la57(Monitor *mon, CPUArchState *env)
     last_prot = 0;
     start = -1;
     for (l0 = 0; l0 < 512; l0++) {
-        cpu_physical_memory_read(pml5_addr + l0 * 8, &pml5e, 8);
+        cpu_physical_memory_read_debug(pml5_addr + l0 * 8, &pml5e, 8);
         pml5e = le64_to_cpu(pml5e);
         end = l0 << 48;
         if (!(pml5e & PG_PRESENT_MASK)) {
@@ -475,7 +480,7 @@ static void mem_info_la57(Monitor *mon, CPUArchState *env)
 
         pml4_addr = pml5e & 0x3fffffffff000ULL;
         for (l1 = 0; l1 < 512; l1++) {
-            cpu_physical_memory_read(pml4_addr + l1 * 8, &pml4e, 8);
+            cpu_physical_memory_read_debug(pml4_addr + l1 * 8, &pml4e, 8);
             pml4e = le64_to_cpu(pml4e);
             end = (l0 << 48) + (l1 << 39);
             if (!(pml4e & PG_PRESENT_MASK)) {
@@ -486,7 +491,7 @@ static void mem_info_la57(Monitor *mon, CPUArchState *env)
 
             pdp_addr = pml4e & 0x3fffffffff000ULL;
             for (l2 = 0; l2 < 512; l2++) {
-                cpu_physical_memory_read(pdp_addr + l2 * 8, &pdpe, 8);
+                cpu_physical_memory_read_debug(pdp_addr + l2 * 8, &pdpe, 8);
                 pdpe = le64_to_cpu(pdpe);
                 end = (l0 << 48) + (l1 << 39) + (l2 << 30);
                 if (pdpe & PG_PRESENT_MASK) {
@@ -505,7 +510,7 @@ static void mem_info_la57(Monitor *mon, CPUArchState *env)
 
                 pd_addr = pdpe & 0x3fffffffff000ULL;
                 for (l3 = 0; l3 < 512; l3++) {
-                    cpu_physical_memory_read(pd_addr + l3 * 8, &pde, 8);
+                    cpu_physical_memory_read_debug(pd_addr + l3 * 8, &pde, 8);
                     pde = le64_to_cpu(pde);
                     end = (l0 << 48) + (l1 << 39) + (l2 << 30) + (l3 << 21);
                     if (pde & PG_PRESENT_MASK) {
@@ -524,7 +529,8 @@ static void mem_info_la57(Monitor *mon, CPUArchState *env)
 
                     pt_addr = pde & 0x3fffffffff000ULL;
                     for (l4 = 0; l4 < 512; l4++) {
-                        cpu_physical_memory_read(pt_addr + l4 * 8, &pte, 8);
+                        cpu_physical_memory_read_debug(pt_addr + l4 * 8,
+                                                       &pte, 8);
                         pte = le64_to_cpu(pte);
                         end = (l0 << 48) + (l1 << 39) + (l2 << 30) +
                             (l3 << 21) + (l4 << 12);
-- 
2.17.1

Re: [PATCH 06/11] monitor/i386: use debug APIs when accessing guest memory

[Peter Maydell]

On Mon, 16 Nov 2020 at 19:29, Ashish Kalra <Ashish.Kalra@amd.com> wrote:
>
> From: Brijesh Singh <brijesh.singh@amd.com>
>
> Update the HMP commands to use the debug version of APIs when accessing
> guest memory.
>
> Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>
> Signed-off-by: Ashish Kalra <ashish.kalra@amd.com>
> ---
>  monitor/misc.c        |  4 ++--
>  softmmu/cpus.c        |  2 +-
>  target/i386/monitor.c | 54 ++++++++++++++++++++++++-------------------
>  3 files changed, 33 insertions(+), 27 deletions(-)
>
> diff --git a/monitor/misc.c b/monitor/misc.c
> index 32e6a8c13d..7eba3a6fce 100644
> --- a/monitor/misc.c
> +++ b/monitor/misc.c
> @@ -824,8 +824,8 @@ static void hmp_sum(Monitor *mon, const QDict *qdict)
>
>      sum = 0;
>      for(addr = start; addr < (start + size); addr++) {
> -        uint8_t val = address_space_ldub(&address_space_memory, addr,
> -                                         MEMTXATTRS_UNSPECIFIED, NULL);
> +        uint8_t val;
> +        cpu_physical_memory_read_debug(addr, &val, 1);

Don't introduce new uses of cpu_* memory read/write functions, please.
They're an old API that has some flaws, like not being able to report
read/write access errors.

If debug accesses are accesses with a MemTxAttrs that says debug=1,
then you should just provide the right MemTxAttrs here.

thanks
-- PMM

Re: [PATCH 06/11] monitor/i386: use debug APIs when accessing guest memory

[Peter Maydell]

On Mon, 16 Nov 2020 at 19:29, Ashish Kalra <Ashish.Kalra@amd.com> wrote:
>
> From: Brijesh Singh <brijesh.singh@amd.com>
>
> Update the HMP commands to use the debug version of APIs when accessing
> guest memory.
>
> Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>
> Signed-off-by: Ashish Kalra <ashish.kalra@amd.com>
> ---
>  monitor/misc.c        |  4 ++--
>  softmmu/cpus.c        |  2 +-
>  target/i386/monitor.c | 54 ++++++++++++++++++++++++-------------------
>  3 files changed, 33 insertions(+), 27 deletions(-)
>
> diff --git a/monitor/misc.c b/monitor/misc.c
> index 32e6a8c13d..7eba3a6fce 100644
> --- a/monitor/misc.c
> +++ b/monitor/misc.c
> @@ -824,8 +824,8 @@ static void hmp_sum(Monitor *mon, const QDict *qdict)
>
>      sum = 0;
>      for(addr = start; addr < (start + size); addr++) {
> -        uint8_t val = address_space_ldub(&address_space_memory, addr,
> -                                         MEMTXATTRS_UNSPECIFIED, NULL);
> +        uint8_t val;
> +        cpu_physical_memory_read_debug(addr, &val, 1);
>          /* BSD sum algorithm ('sum' Unix command) */
>          sum = (sum >> 1) | (sum << 15);
>          sum += val;

Side note -- are byte-by-byte accesses to the encrypted guest
memory noticeably higher overhead than if we asked for a
larger buffer to be decrypted at once? If so and if anybody
cares about hmp_sum performance we might consider having it
work on a larger buffer at a time rather than byte-by-byte...

thanks
-- PMM

[PATCH 07/11] kvm: introduce debug memory encryption API

[Ashish Kalra]

From: Brijesh Singh <brijesh.singh@amd.com>

In order to support debugging with Secure Encrypted Virtualization (SEV),
add a high-level memory encryption API.

Also add a new API interface to override any CPU class specific callbacks
for supporting debugging with SEV, for example, overriding the guest MMU/
page-table walker callback.

Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>
Signed-off-by: Ashish Kalra <ashish.kalra@amd.com>
---
 accel/kvm/kvm-all.c    | 19 +++++++++++++++++++
 accel/stubs/kvm-stub.c |  8 ++++++++
 include/sysemu/kvm.h   | 15 +++++++++++++++
 3 files changed, 42 insertions(+)

diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
index 9ef5daf4c5..ae85f53e7d 100644
--- a/accel/kvm/kvm-all.c
+++ b/accel/kvm/kvm-all.c
@@ -123,6 +123,8 @@ struct KVMState
     /* memory encryption */
     void *memcrypt_handle;
     int (*memcrypt_encrypt_data)(void *handle, uint8_t *ptr, uint64_t len);
+    void (*memcrypt_debug_ops_memory_region)(void *handle, MemoryRegion *mr);
+    void (*memcrypt_debug_ops_cpu_state)(void *handle, CPUState *cpu);
 
     /* For "info mtree -f" to tell if an MR is registered in KVM */
     int nr_as;
@@ -222,6 +224,23 @@ int kvm_get_max_memslots(void)
     return s->nr_slots;
 }
 
+void kvm_memcrypt_set_debug_ops_memory_region(MemoryRegion *mr)
+{
+    if (kvm_state->memcrypt_handle &&
+        kvm_state->memcrypt_debug_ops_memory_region) {
+        kvm_state->memcrypt_debug_ops_memory_region(kvm_state->memcrypt_handle,
+                                                    mr);
+    }
+}
+
+void kvm_memcrypt_set_debug_ops_cpu_state(CPUState *cs)
+{
+    if (kvm_state->memcrypt_handle &&
+        kvm_state->memcrypt_debug_ops_cpu_state) {
+        kvm_state->memcrypt_debug_ops_cpu_state(kvm_state->memcrypt_handle, cs);
+    }
+}
+
 bool kvm_memcrypt_enabled(void)
 {
     if (kvm_state && kvm_state->memcrypt_handle) {
diff --git a/accel/stubs/kvm-stub.c b/accel/stubs/kvm-stub.c
index 680e099463..bf93431e46 100644
--- a/accel/stubs/kvm-stub.c
+++ b/accel/stubs/kvm-stub.c
@@ -91,6 +91,14 @@ int kvm_memcrypt_encrypt_data(uint8_t *ptr, uint64_t len)
   return 1;
 }
 
+void kvm_memcrypt_set_debug_ops_memory_region(MemoryRegion *mr)
+{
+}
+
+void kvm_memcrypt_set_debug_ops_cpu_state(CPUState *cs)
+{
+}
+
 #ifndef CONFIG_USER_ONLY
 int kvm_irqchip_add_msi_route(KVMState *s, int vector, PCIDevice *dev)
 {
diff --git a/include/sysemu/kvm.h b/include/sysemu/kvm.h
index bb5d5cf497..1bde2e3d71 100644
--- a/include/sysemu/kvm.h
+++ b/include/sysemu/kvm.h
@@ -470,6 +470,21 @@ uint32_t kvm_arch_get_supported_cpuid(KVMState *env, uint32_t function,
                                       uint32_t index, int reg);
 uint64_t kvm_arch_get_supported_msr_feature(KVMState *s, uint32_t index);
 
+/**
+ * kvm_memcrypt_set_debug_ram_ops: set debug_ram_ops callback
+ *
+ * When debug_ram_ops is set, debug access to this memory region will use
+ * memory encryption APIs.
+ */
+void kvm_memcrypt_set_debug_ops_memory_region(MemoryRegion *mr);
+
+/**
+ * kvm_memcrypt_set_debug_ops_cpu_state: override cpu_class callbacks
+ *
+ * This interface allows vendor specific debug ops to override any
+ * cpu_class callbacks.
+ */
+void kvm_memcrypt_set_debug_ops_cpu_state(CPUState *cs);
 
 void kvm_set_sigmask_len(KVMState *s, unsigned int sigmask_len);
 
-- 
2.17.1

[PATCH 08/11] sev/i386: add debug encrypt and decrypt commands

[Ashish Kalra]

From: Brijesh Singh <brijesh.singh@amd.com>

The KVM_SEV_DBG_DECRYPT and KVM_SEV_DBG_ENCRYPT commands are used for
decrypting and encrypting guest memory. The command works only if the
guest policy allows the debugging.

Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>
Signed-off-by: Ashish Kalra <ashish.kalra@amd.com>
---
 accel/kvm/kvm-all.c      |  2 ++
 accel/kvm/sev-stub.c     |  4 +++
 include/sysemu/sev.h     |  1 +
 target/i386/sev.c        | 58 ++++++++++++++++++++++++++++++++++++++++
 target/i386/trace-events |  1 +
 5 files changed, 66 insertions(+)

diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
index ae85f53e7d..042205e3e1 100644
--- a/accel/kvm/kvm-all.c
+++ b/accel/kvm/kvm-all.c
@@ -2232,6 +2232,8 @@ static int kvm_init(MachineState *ms)
         }
 
         kvm_state->memcrypt_encrypt_data = sev_encrypt_data;
+        kvm_state->memcrypt_debug_ops_memory_region =
+            sev_set_debug_ops_memory_region;
     }
 
     ret = kvm_arch_init(ms, s);
diff --git a/accel/kvm/sev-stub.c b/accel/kvm/sev-stub.c
index 4f97452585..3f1f0ef217 100644
--- a/accel/kvm/sev-stub.c
+++ b/accel/kvm/sev-stub.c
@@ -15,6 +15,10 @@
 #include "qemu-common.h"
 #include "sysemu/sev.h"
 
+void sev_set_debug_ops_memory_region(void *handle, MemoryRegion *mr)
+{
+}
+
 int sev_encrypt_data(void *handle, uint8_t *ptr, uint64_t len)
 {
     abort();
diff --git a/include/sysemu/sev.h b/include/sysemu/sev.h
index 98c1ec8d38..6c37247915 100644
--- a/include/sysemu/sev.h
+++ b/include/sysemu/sev.h
@@ -18,4 +18,5 @@
 
 void *sev_guest_init(const char *id);
 int sev_encrypt_data(void *handle, uint8_t *ptr, uint64_t len);
+void sev_set_debug_ops_memory_region(void *handle, MemoryRegion *mr);
 #endif
diff --git a/target/i386/sev.c b/target/i386/sev.c
index 93c4d60b82..3036fb3e43 100644
--- a/target/i386/sev.c
+++ b/target/i386/sev.c
@@ -72,6 +72,8 @@ struct SevGuestState {
 static SevGuestState *sev_guest;
 static Error *sev_mig_blocker;
 
+static MemoryRegionRAMReadWriteOps sev_ops;
+
 static const char *const sev_fw_errlist[] = {
     "",
     "Platform state is invalid",
@@ -679,6 +681,46 @@ sev_vm_state_change(void *opaque, int running, RunState state)
     }
 }
 
+static int
+sev_dbg_enc_dec(uint8_t *dst, const uint8_t *src, uint32_t len, bool write)
+{
+    int ret, error;
+    struct kvm_sev_dbg dbg;
+
+    dbg.src_uaddr = (unsigned long)src;
+    dbg.dst_uaddr = (unsigned long)dst;
+    dbg.len = len;
+
+    trace_kvm_sev_debug(write ? "encrypt" : "decrypt", src, dst, len);
+    ret = sev_ioctl(sev_guest->sev_fd,
+                    write ? KVM_SEV_DBG_ENCRYPT : KVM_SEV_DBG_DECRYPT,
+                    &dbg, &error);
+    if (ret) {
+        error_report("%s (%s) 0x%llx->0x%llx+0x%x ret=%d fw_error=%d '%s'",
+                     __func__, write ? "write" : "read", dbg.src_uaddr,
+                     dbg.dst_uaddr, dbg.len, ret, error,
+                     fw_error_to_str(error));
+    }
+
+    return ret;
+}
+
+static int
+sev_mem_read(uint8_t *dst, const uint8_t *src, uint32_t len, MemTxAttrs attrs)
+{
+    assert(attrs.debug);
+
+    return sev_dbg_enc_dec(dst, src, len, false);
+}
+
+static int
+sev_mem_write(uint8_t *dst, const uint8_t *src, uint32_t len, MemTxAttrs attrs)
+{
+    assert(attrs.debug);
+
+    return sev_dbg_enc_dec(dst, src, len, true);
+}
+
 void *
 sev_guest_init(const char *id)
 {
@@ -785,6 +827,22 @@ sev_encrypt_data(void *handle, uint8_t *ptr, uint64_t len)
     return 0;
 }
 
+void
+sev_set_debug_ops_memory_region(void *handle, MemoryRegion *mr)
+{
+    SevGuestState *s = handle;
+
+    /* If policy does not allow debug then no need to register ops */
+    if (s->policy & SEV_POLICY_NODBG) {
+        return;
+    }
+
+    sev_ops.read = sev_mem_read;
+    sev_ops.write = sev_mem_write;
+
+    memory_region_set_ram_debug_ops(mr, &sev_ops);
+}
+
 static void
 sev_register_types(void)
 {
diff --git a/target/i386/trace-events b/target/i386/trace-events
index 789c700d4a..f91213c5e9 100644
--- a/target/i386/trace-events
+++ b/target/i386/trace-events
@@ -15,3 +15,4 @@ kvm_sev_launch_start(int policy, void *session, void *pdh) "policy 0x%x session
 kvm_sev_launch_update_data(void *addr, uint64_t len) "addr %p len 0x%" PRIu64
 kvm_sev_launch_measurement(const char *value) "data %s"
 kvm_sev_launch_finish(void) ""
+kvm_sev_debug(const char *op, const uint8_t *src, uint8_t *dst, int len) "(%s) src %p dst %p len %d"
-- 
2.17.1

[PATCH 09/11] hw/i386: set ram_debug_ops when memory encryption is enabled

[Ashish Kalra]

From: Brijesh Singh <brijesh.singh@amd.com>

When memory encryption is enabled, the guest RAM and boot flash ROM will
contain the encrypted data. By setting the debug ops allow us to invoke
encryption APIs when accessing the memory for the debug purposes.

Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>
Signed-off-by: Ashish Kalra <ashish.kalra@amd.com>
---
 hw/i386/pc.c       | 9 +++++++++
 hw/i386/pc_sysfw.c | 6 ++++++
 2 files changed, 15 insertions(+)

diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index 5e6c0023e0..dfb63cd686 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -913,6 +913,15 @@ void pc_memory_init(PCMachineState *pcms,
         e820_add_entry(0x100000000ULL, x86ms->above_4g_mem_size, E820_RAM);
     }
 
+    /*
+     * When memory encryption is enabled, the guest RAM will be encrypted with
+     * a guest unique key. Set the debug ops so that any debug access to the
+     * guest RAM will go through the memory encryption APIs.
+     */
+    if (kvm_memcrypt_enabled()) {
+        kvm_memcrypt_set_debug_ops_memory_region(*ram_memory);
+    }
+
     if (!pcmc->has_reserved_memory &&
         (machine->ram_slots ||
          (machine->maxram_size > machine->ram_size))) {
diff --git a/hw/i386/pc_sysfw.c b/hw/i386/pc_sysfw.c
index b6c0822fe3..9f90c9d761 100644
--- a/hw/i386/pc_sysfw.c
+++ b/hw/i386/pc_sysfw.c
@@ -209,6 +209,12 @@ static void pc_system_flash_map(PCMachineState *pcms,
                     error_report("failed to encrypt pflash rom");
                     exit(1);
                 }
+
+                /*
+                 * The pflash ROM is encrypted, set the debug ops so that any
+                 * debug accesses will use memory encryption APIs.
+                 */
+                kvm_memcrypt_set_debug_ops_memory_region(flash_mem);
             }
         }
     }
-- 
2.17.1

[PATCH 10/11] sev/i386: add SEV specific MemoryDebugOps.

[Ashish Kalra]

From: Ashish Kalra <ashish.kalra@amd.com>

Add SEV specific MemoryDebugOps which override the default MemoryDebugOps
when SEV memory encryption is enabled. The SEV specific MemoryDebugOps
invoke the generic address_space_rw_debug helpers which will then invoke
the memory region specific callbacks to handle and access encrypted memory
when guest RAM is accessed.

Also invoke the memory encryption API to override any CPU class specific
callbacks to handle memory encryption.

Specifically for SEV we override CPU class specific guest MMU/page-table walker
to invoke a SEV specific handler which can handle guest encrypted memory and
also clear C-bit when walking SEV guest page table.

Signed-off-by: Ashish Kalra <ashish.kalra@amd.com>
---
 accel/kvm/kvm-all.c  |   1 +
 accel/kvm/sev-stub.c |   4 +
 include/sysemu/sev.h |  11 +++
 target/i386/kvm.c    |   4 +
 target/i386/sev.c    | 185 +++++++++++++++++++++++++++++++++++++++++++
 5 files changed, 205 insertions(+)

diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
index 042205e3e1..6d812d5b09 100644
--- a/accel/kvm/kvm-all.c
+++ b/accel/kvm/kvm-all.c
@@ -2234,6 +2234,7 @@ static int kvm_init(MachineState *ms)
         kvm_state->memcrypt_encrypt_data = sev_encrypt_data;
         kvm_state->memcrypt_debug_ops_memory_region =
             sev_set_debug_ops_memory_region;
+        kvm_state->memcrypt_debug_ops_cpu_state = sev_set_debug_ops_cpu_state;
     }
 
     ret = kvm_arch_init(ms, s);
diff --git a/accel/kvm/sev-stub.c b/accel/kvm/sev-stub.c
index 3f1f0ef217..ad27226058 100644
--- a/accel/kvm/sev-stub.c
+++ b/accel/kvm/sev-stub.c
@@ -19,6 +19,10 @@ void sev_set_debug_ops_memory_region(void *handle, MemoryRegion *mr)
 {
 }
 
+void sev_set_debug_ops_cpu_state(void *handle, CPUState *cpu)
+{
+}
+
 int sev_encrypt_data(void *handle, uint8_t *ptr, uint64_t len)
 {
     abort();
diff --git a/include/sysemu/sev.h b/include/sysemu/sev.h
index 6c37247915..e6f176b85b 100644
--- a/include/sysemu/sev.h
+++ b/include/sysemu/sev.h
@@ -19,4 +19,15 @@
 void *sev_guest_init(const char *id);
 int sev_encrypt_data(void *handle, uint8_t *ptr, uint64_t len);
 void sev_set_debug_ops_memory_region(void *handle, MemoryRegion *mr);
+void sev_set_debug_ops_cpu_state(void *handle, CPUState *cpu);
+hwaddr sev_cpu_get_phys_page_attrs_debug(CPUState *cs, vaddr addr,
+                                         MemTxAttrs *attrs);
+MemTxResult sev_address_space_read_debug(AddressSpace *as, hwaddr addr,
+                                         MemTxAttrs attrs, void *ptr,
+                                         hwaddr len);
+MemTxResult sev_address_space_write_rom_debug(AddressSpace *as,
+                                              hwaddr addr,
+                                              MemTxAttrs attrs,
+                                              const void *ptr,
+                                              hwaddr len);
 #endif
diff --git a/target/i386/kvm.c b/target/i386/kvm.c
index cf46259534..7a2d10b745 100644
--- a/target/i386/kvm.c
+++ b/target/i386/kvm.c
@@ -1838,6 +1838,10 @@ int kvm_arch_init_vcpu(CPUState *cs)
 
     kvm_init_msrs(cpu);
 
+    if (kvm_memcrypt_enabled()) {
+        kvm_memcrypt_set_debug_ops_cpu_state(cs);
+    }
+
     r = hyperv_init_vcpu(cpu);
     if (r) {
         goto fail;
diff --git a/target/i386/sev.c b/target/i386/sev.c
index 3036fb3e43..b942593bc8 100644
--- a/target/i386/sev.c
+++ b/target/i386/sev.c
@@ -843,6 +843,191 @@ sev_set_debug_ops_memory_region(void *handle, MemoryRegion *mr)
     memory_region_set_ram_debug_ops(mr, &sev_ops);
 }
 
+hwaddr sev_cpu_get_phys_page_attrs_debug(CPUState *cs, vaddr addr,
+                                         MemTxAttrs *attrs)
+{
+    X86CPU *cpu = X86_CPU(cs);
+    CPUX86State *env = &cpu->env;
+    target_ulong pde_addr, pte_addr;
+    uint64_t pte;
+    int32_t a20_mask;
+    uint32_t page_offset;
+    int page_size;
+    uint64_t me_mask;
+
+    me_mask = sev_get_me_mask();
+
+    *attrs = cpu_get_mem_attrs(env);
+
+    a20_mask = x86_get_a20_mask(env);
+    if (!(env->cr[0] & CR0_PG_MASK)) {
+        pte = addr & a20_mask;
+        page_size = 4096;
+    } else if (env->cr[4] & CR4_PAE_MASK) {
+        target_ulong pdpe_addr;
+        uint64_t pde, pdpe;
+
+#ifdef TARGET_X86_64
+        if (env->hflags & HF_LMA_MASK) {
+            bool la57 = env->cr[4] & CR4_LA57_MASK;
+            uint64_t pml5e_addr, pml5e;
+            uint64_t pml4e_addr, pml4e;
+            int32_t sext;
+
+            /* test virtual address sign extension */
+            sext = la57 ? (int64_t)addr >> 56 : (int64_t)addr >> 47;
+            if (sext != 0 && sext != -1) {
+                return -1;
+            }
+
+            if (la57) {
+                pml5e_addr = ((env->cr[3] & ~0xfff & me_mask) +
+                        (((addr >> 48) & 0x1ff) << 3)) & a20_mask;
+                pml5e = ldq_phys_debug(cs, pml5e_addr) & me_mask;
+                if (!(pml5e & PG_PRESENT_MASK)) {
+                    return -1;
+                }
+            } else {
+                pml5e = env->cr[3] & me_mask;
+            }
+
+            pml4e_addr = ((pml5e & PG_ADDRESS_MASK) +
+                    (((addr >> 39) & 0x1ff) << 3)) & a20_mask;
+            pml4e = ldq_phys_debug(cs, pml4e_addr) & me_mask;
+            if (!(pml4e & PG_PRESENT_MASK)) {
+                return -1;
+            }
+            pdpe_addr = ((pml4e & PG_ADDRESS_MASK) +
+                         (((addr >> 30) & 0x1ff) << 3)) & a20_mask;
+            pdpe = ldq_phys_debug(cs, pdpe_addr) & me_mask;
+            if (!(pdpe & PG_PRESENT_MASK)) {
+                return -1;
+            }
+            if (pdpe & PG_PSE_MASK) {
+                page_size = 1024 * 1024 * 1024;
+                pte = pdpe;
+                goto out;
+            }
+
+        } else
+#endif
+        {
+            pdpe_addr = ((env->cr[3] & ~0x1f & me_mask) +
+                         ((addr >> 27) & 0x18)) & a20_mask;
+            pdpe = ldq_phys_debug(cs, pdpe_addr) & me_mask;
+            if (!(pdpe & PG_PRESENT_MASK)) {
+                return -1;
+            }
+        }
+
+        pde_addr = ((pdpe & PG_ADDRESS_MASK) +
+                    (((addr >> 21) & 0x1ff) << 3)) & a20_mask;
+        pde = ldq_phys_debug(cs, pde_addr) & me_mask;
+        if (!(pde & PG_PRESENT_MASK)) {
+            return -1;
+        }
+        if (pde & PG_PSE_MASK) {
+            /* 2 MB page */
+            page_size = 2048 * 1024;
+            pte = pde;
+        } else {
+            /* 4 KB page */
+            pte_addr = ((pde & PG_ADDRESS_MASK) +
+                        (((addr >> 12) & 0x1ff) << 3)) & a20_mask;
+            page_size = 4096;
+            pte = ldq_phys_debug(cs, pte_addr) & me_mask;
+        }
+        if (!(pte & PG_PRESENT_MASK)) {
+            return -1;
+        }
+    } else {
+        uint32_t pde;
+
+        /* page directory entry */
+        pde_addr = ((env->cr[3] & ~0xfff & me_mask) +
+                    ((addr >> 20) & 0xffc)) & a20_mask;
+        pde = x86_ldl_phys(cs, pde_addr) & me_mask;
+        if (!(pde & PG_PRESENT_MASK)) {
+            return -1;
+        }
+        if ((pde & PG_PSE_MASK) && (env->cr[4] & CR4_PSE_MASK)) {
+            pte = pde | ((pde & 0x1fe000LL) << (32 - 13));
+            page_size = 4096 * 1024;
+        } else {
+            /* page directory entry */
+            pte_addr = ((pde & ~0xfff) + ((addr >> 10) & 0xffc)) & a20_mask;
+            pte = ldl_phys_debug(cs, pte_addr) & me_mask;
+            if (!(pte & PG_PRESENT_MASK)) {
+                return -1;
+            }
+            page_size = 4096;
+        }
+        pte = pte & a20_mask;
+    }
+
+#ifdef TARGET_X86_64
+out:
+#endif
+    pte &= PG_ADDRESS_MASK & ~(page_size - 1);
+    page_offset = (addr & TARGET_PAGE_MASK) & (page_size - 1);
+    return pte | page_offset;
+}
+
+MemTxResult sev_address_space_write_rom_debug(AddressSpace *as,
+                                              hwaddr addr,
+                                              MemTxAttrs attrs,
+                                              const void *ptr,
+                                              hwaddr len)
+{
+    /* set debug attrs to indicate memory access is from the debugger */
+    attrs.debug = 1;
+
+    /*
+     * Invoke address_space_rw debug helper
+     */
+    return address_space_write_rom_debug(as, addr, attrs, ptr, len);
+}
+
+MemTxResult sev_address_space_read_debug(AddressSpace *as, hwaddr addr,
+                                         MemTxAttrs attrs, void *ptr,
+                                         hwaddr len)
+{
+    /* set debug attrs to indicate memory access is from the debugger */
+    attrs.debug = 1;
+
+    /*
+     * Invoke address_space_rw debug helper
+     */
+    return address_space_read_debug(as, addr, attrs, ptr, len);
+}
+
+static const MemoryDebugOps sev_debug_ops = {
+    .read = sev_address_space_read_debug,
+    .write = sev_address_space_write_rom_debug
+};
+
+void
+sev_set_debug_ops_cpu_state(void *handle, CPUState *cs)
+{
+    SevGuestState *s = handle;
+    CPUClass *cc;
+
+    /* If policy does not allow debug then no need to register ops */
+    if (s->policy & SEV_POLICY_NODBG) {
+        return;
+    }
+
+    cc = CPU_GET_CLASS(cs);
+
+    /*
+     * Override guest MMU lookup/page-table-walker with SEV specific
+     * callback to handle encrypted memory.
+     */
+    cc->get_phys_page_attrs_debug = sev_cpu_get_phys_page_attrs_debug;
+
+    address_space_set_debug_ops(&sev_debug_ops);
+}
+
 static void
 sev_register_types(void)
 {
-- 
2.17.1

[PATCH 11/11] target/i386: clear C-bit when walking SEV guest page table

[Ashish Kalra]

From: Brijesh Singh <brijesh.singh@amd.com>

In SEV-enabled guest the pte entry will have C-bit set, we need to clear
the C-bit when walking the page table.

This ensures that the proper page address translation occurs and, with the
C-bit reset, the true physical address is got.

The pte_mask to be used during guest page table walk is added as a
vendor specific assist/hook as part of the new MemoryDebugOps and
available via the new debug API interface cpu_physical_memory_pte_mask_debug().

Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>
Signed-off-by: Ashish Kalra <ashish.kalra@amd.com>
---
 include/exec/cpu-common.h |  3 ++
 include/exec/memory.h     |  1 +
 softmmu/physmem.c         | 13 +++++++-
 target/i386/monitor.c     | 70 +++++++++++++++++++++++++--------------
 target/i386/sev.c         |  3 +-
 5 files changed, 63 insertions(+), 27 deletions(-)

diff --git a/include/exec/cpu-common.h b/include/exec/cpu-common.h
index d2089e6873..3374573d39 100644
--- a/include/exec/cpu-common.h
+++ b/include/exec/cpu-common.h
@@ -96,6 +96,9 @@ static inline void cpu_physical_memory_write(hwaddr addr,
 {
     cpu_physical_memory_rw(addr, (void *)buf, len, true);
 }
+
+uint64_t cpu_physical_memory_pte_mask_debug(void);
+
 void *cpu_physical_memory_map(hwaddr addr,
                               hwaddr *plen,
                               bool is_write);
diff --git a/include/exec/memory.h b/include/exec/memory.h
index 74f2dcec00..ebe8ffc1eb 100644
--- a/include/exec/memory.h
+++ b/include/exec/memory.h
@@ -2428,6 +2428,7 @@ typedef struct MemoryDebugOps {
     MemTxResult (*write)(AddressSpace *as, hwaddr phys_addr,
                          MemTxAttrs attrs, const void *buf,
                          hwaddr len);
+    uint64_t (*pte_mask)(void);
 } MemoryDebugOps;
 
 void address_space_set_debug_ops(const MemoryDebugOps *ops);
diff --git a/softmmu/physmem.c b/softmmu/physmem.c
index 6945bd5efe..fc6b5588fc 100644
--- a/softmmu/physmem.c
+++ b/softmmu/physmem.c
@@ -166,9 +166,15 @@ struct DirtyBitmapSnapshot {
     unsigned long dirty[];
 };
 
+static uint64_t address_space_pte_mask(void)
+{
+    return ~0;
+}
+
 static const MemoryDebugOps default_debug_ops = {
     .read = address_space_read,
-    .write = address_space_write_rom
+    .write = address_space_write_rom,
+    .pte_mask = address_space_pte_mask
 };
 
 static const MemoryDebugOps *debug_ops = &default_debug_ops;
@@ -3401,6 +3407,11 @@ void cpu_physical_memory_rw_debug(hwaddr addr, uint8_t *buf,
 
 }
 
+uint64_t cpu_physical_memory_pte_mask_debug(void)
+{
+    return debug_ops->pte_mask();
+}
+
 int64_t address_space_cache_init(MemoryRegionCache *cache,
                                  AddressSpace *as,
                                  hwaddr addr,
diff --git a/target/i386/monitor.c b/target/i386/monitor.c
index 9ca9c677a5..c73cac04cb 100644
--- a/target/i386/monitor.c
+++ b/target/i386/monitor.c
@@ -106,16 +106,20 @@ static void tlb_info_pae32(Monitor *mon, CPUArchState *env)
     unsigned int l1, l2, l3;
     uint64_t pdpe, pde, pte;
     uint64_t pdp_addr, pd_addr, pt_addr;
+    uint64_t me_mask;
+
+    me_mask = cpu_physical_memory_pte_mask_debug();
 
     pdp_addr = env->cr[3] & ~0x1f;
+    pdp_addr &= me_mask;
     for (l1 = 0; l1 < 4; l1++) {
         cpu_physical_memory_read_debug(pdp_addr + l1 * 8, &pdpe, 8);
-        pdpe = le64_to_cpu(pdpe);
+        pdpe = le64_to_cpu(pdpe & me_mask);
         if (pdpe & PG_PRESENT_MASK) {
             pd_addr = pdpe & 0x3fffffffff000ULL;
             for (l2 = 0; l2 < 512; l2++) {
                 cpu_physical_memory_read_debug(pd_addr + l2 * 8, &pde, 8);
-                pde = le64_to_cpu(pde);
+                pde = le64_to_cpu(pde & me_mask);
                 if (pde & PG_PRESENT_MASK) {
                     if (pde & PG_PSE_MASK) {
                         /* 2M pages with PAE, CR4.PSE is ignored */
@@ -126,7 +130,7 @@ static void tlb_info_pae32(Monitor *mon, CPUArchState *env)
                         for (l3 = 0; l3 < 512; l3++) {
                             cpu_physical_memory_read_debug(pt_addr + l3 * 8,
                                                            &pte, 8);
-                            pte = le64_to_cpu(pte);
+                            pte = le64_to_cpu(pte & me_mask);
                             if (pte & PG_PRESENT_MASK) {
                                 print_pte(mon, env, (l1 << 30) + (l2 << 21)
                                           + (l3 << 12),
@@ -148,10 +152,13 @@ static void tlb_info_la48(Monitor *mon, CPUArchState *env,
     uint64_t l1, l2, l3, l4;
     uint64_t pml4e, pdpe, pde, pte;
     uint64_t pdp_addr, pd_addr, pt_addr;
+    uint64_t me_mask;
+
+    me_mask = cpu_physical_memory_pte_mask_debug();
 
     for (l1 = 0; l1 < 512; l1++) {
         cpu_physical_memory_read_debug(pml4_addr + l1 * 8, &pml4e, 8);
-        pml4e = le64_to_cpu(pml4e);
+        pml4e = le64_to_cpu(pml4e & me_mask);
         if (!(pml4e & PG_PRESENT_MASK)) {
             continue;
         }
@@ -159,7 +166,7 @@ static void tlb_info_la48(Monitor *mon, CPUArchState *env,
         pdp_addr = pml4e & 0x3fffffffff000ULL;
         for (l2 = 0; l2 < 512; l2++) {
             cpu_physical_memory_read_debug(pdp_addr + l2 * 8, &pdpe, 8);
-            pdpe = le64_to_cpu(pdpe);
+            pdpe = le64_to_cpu(pdpe & me_mask);
             if (!(pdpe & PG_PRESENT_MASK)) {
                 continue;
             }
@@ -174,7 +181,7 @@ static void tlb_info_la48(Monitor *mon, CPUArchState *env,
             pd_addr = pdpe & 0x3fffffffff000ULL;
             for (l3 = 0; l3 < 512; l3++) {
                 cpu_physical_memory_read_debug(pd_addr + l3 * 8, &pde, 8);
-                pde = le64_to_cpu(pde);
+                pde = le64_to_cpu(pde & me_mask);
                 if (!(pde & PG_PRESENT_MASK)) {
                     continue;
                 }
@@ -191,7 +198,7 @@ static void tlb_info_la48(Monitor *mon, CPUArchState *env,
                     cpu_physical_memory_read_debug(pt_addr
                             + l4 * 8,
                             &pte, 8);
-                    pte = le64_to_cpu(pte);
+                    pte = le64_to_cpu(pte & me_mask);
                     if (pte & PG_PRESENT_MASK) {
                         print_pte(mon, env, (l0 << 48) + (l1 << 39) +
                                 (l2 << 30) + (l3 << 21) + (l4 << 12),
@@ -208,13 +215,17 @@ static void tlb_info_la57(Monitor *mon, CPUArchState *env)
     uint64_t l0;
     uint64_t pml5e;
     uint64_t pml5_addr;
+    uint64_t me_mask;
 
-    pml5_addr = env->cr[3] & 0x3fffffffff000ULL;
+    me_mask = cpu_physical_memory_pte_mask_debug();
+
+    pml5_addr = env->cr[3] & 0x3fffffffff000ULL & me_mask;
     for (l0 = 0; l0 < 512; l0++) {
         cpu_physical_memory_read_debug(pml5_addr + l0 * 8, &pml5e, 8);
-        pml5e = le64_to_cpu(pml5e);
+        pml5e = le64_to_cpu(pml5e & me_mask);
         if (pml5e & PG_PRESENT_MASK) {
-            tlb_info_la48(mon, env, l0, pml5e & 0x3fffffffff000ULL);
+            tlb_info_la48(mon, env, l0, pml5e & 0x3fffffffff000ULL &
+                          cpu_physical_memory_pte_mask_debug());
         }
     }
 }
@@ -326,19 +337,22 @@ static void mem_info_pae32(Monitor *mon, CPUArchState *env)
     uint64_t pdpe, pde, pte;
     uint64_t pdp_addr, pd_addr, pt_addr;
     hwaddr start, end;
+    uint64_t me_mask;
 
-    pdp_addr = env->cr[3] & ~0x1f;
+    me_mask = cpu_physical_memory_pte_mask_debug();
+
+    pdp_addr = env->cr[3] & ~0x1f & me_mask;
     last_prot = 0;
     start = -1;
     for (l1 = 0; l1 < 4; l1++) {
         cpu_physical_memory_read_debug(pdp_addr + l1 * 8, &pdpe, 8);
-        pdpe = le64_to_cpu(pdpe);
+        pdpe = le64_to_cpu(pdpe & me_mask);
         end = l1 << 30;
         if (pdpe & PG_PRESENT_MASK) {
             pd_addr = pdpe & 0x3fffffffff000ULL;
             for (l2 = 0; l2 < 512; l2++) {
                 cpu_physical_memory_read_debug(pd_addr + l2 * 8, &pde, 8);
-                pde = le64_to_cpu(pde);
+                pde = le64_to_cpu(pde & me_mask);
                 end = (l1 << 30) + (l2 << 21);
                 if (pde & PG_PRESENT_MASK) {
                     if (pde & PG_PSE_MASK) {
@@ -350,7 +364,7 @@ static void mem_info_pae32(Monitor *mon, CPUArchState *env)
                         for (l3 = 0; l3 < 512; l3++) {
                             cpu_physical_memory_read_debug(pt_addr + l3 * 8,
                                                            &pte, 8);
-                            pte = le64_to_cpu(pte);
+                            pte = le64_to_cpu(pte & me_mask);
                             end = (l1 << 30) + (l2 << 21) + (l3 << 12);
                             if (pte & PG_PRESENT_MASK) {
                                 prot = pte & pde & (PG_USER_MASK | PG_RW_MASK |
@@ -383,19 +397,22 @@ static void mem_info_la48(Monitor *mon, CPUArchState *env)
     uint64_t l1, l2, l3, l4;
     uint64_t pml4e, pdpe, pde, pte;
     uint64_t pml4_addr, pdp_addr, pd_addr, pt_addr, start, end;
+    uint64_t me_mask;
+
+    me_mask = cpu_physical_memory_pte_mask_debug();
 
-    pml4_addr = env->cr[3] & 0x3fffffffff000ULL;
+    pml4_addr = env->cr[3] & 0x3fffffffff000ULL & me_mask;
     last_prot = 0;
     start = -1;
     for (l1 = 0; l1 < 512; l1++) {
         cpu_physical_memory_read_debug(pml4_addr + l1 * 8, &pml4e, 8);
-        pml4e = le64_to_cpu(pml4e);
+        pml4e = le64_to_cpu(pml4e & me_mask);
         end = l1 << 39;
         if (pml4e & PG_PRESENT_MASK) {
             pdp_addr = pml4e & 0x3fffffffff000ULL;
             for (l2 = 0; l2 < 512; l2++) {
                 cpu_physical_memory_read_debug(pdp_addr + l2 * 8, &pdpe, 8);
-                pdpe = le64_to_cpu(pdpe);
+                pdpe = le64_to_cpu(pdpe & me_mask);
                 end = (l1 << 39) + (l2 << 30);
                 if (pdpe & PG_PRESENT_MASK) {
                     if (pdpe & PG_PSE_MASK) {
@@ -408,7 +425,7 @@ static void mem_info_la48(Monitor *mon, CPUArchState *env)
                         for (l3 = 0; l3 < 512; l3++) {
                             cpu_physical_memory_read_debug(pd_addr + l3 * 8,
                                                            &pde, 8);
-                            pde = le64_to_cpu(pde);
+                            pde = le64_to_cpu(pde & me_mask);
                             end = (l1 << 39) + (l2 << 30) + (l3 << 21);
                             if (pde & PG_PRESENT_MASK) {
                                 if (pde & PG_PSE_MASK) {
@@ -423,7 +440,7 @@ static void mem_info_la48(Monitor *mon, CPUArchState *env)
                                         cpu_physical_memory_read_debug(pt_addr
                                                                  + l4 * 8,
                                                                  &pte, 8);
-                                        pte = le64_to_cpu(pte);
+                                        pte = le64_to_cpu(pte & me_mask);
                                         end = (l1 << 39) + (l2 << 30) +
                                             (l3 << 21) + (l4 << 12);
                                         if (pte & PG_PRESENT_MASK) {
@@ -464,13 +481,16 @@ static void mem_info_la57(Monitor *mon, CPUArchState *env)
     uint64_t l0, l1, l2, l3, l4;
     uint64_t pml5e, pml4e, pdpe, pde, pte;
     uint64_t pml5_addr, pml4_addr, pdp_addr, pd_addr, pt_addr, start, end;
+    uint64_t me_mask;
+
+    me_mask = cpu_physical_memory_pte_mask_debug();
 
-    pml5_addr = env->cr[3] & 0x3fffffffff000ULL;
+    pml5_addr = env->cr[3] & 0x3fffffffff000ULL & me_mask;
     last_prot = 0;
     start = -1;
     for (l0 = 0; l0 < 512; l0++) {
         cpu_physical_memory_read_debug(pml5_addr + l0 * 8, &pml5e, 8);
-        pml5e = le64_to_cpu(pml5e);
+        pml5e = le64_to_cpu(pml5e & me_mask);
         end = l0 << 48;
         if (!(pml5e & PG_PRESENT_MASK)) {
             prot = 0;
@@ -481,7 +501,7 @@ static void mem_info_la57(Monitor *mon, CPUArchState *env)
         pml4_addr = pml5e & 0x3fffffffff000ULL;
         for (l1 = 0; l1 < 512; l1++) {
             cpu_physical_memory_read_debug(pml4_addr + l1 * 8, &pml4e, 8);
-            pml4e = le64_to_cpu(pml4e);
+            pml4e = le64_to_cpu(pml4e & me_mask);
             end = (l0 << 48) + (l1 << 39);
             if (!(pml4e & PG_PRESENT_MASK)) {
                 prot = 0;
@@ -492,7 +512,7 @@ static void mem_info_la57(Monitor *mon, CPUArchState *env)
             pdp_addr = pml4e & 0x3fffffffff000ULL;
             for (l2 = 0; l2 < 512; l2++) {
                 cpu_physical_memory_read_debug(pdp_addr + l2 * 8, &pdpe, 8);
-                pdpe = le64_to_cpu(pdpe);
+                pdpe = le64_to_cpu(pdpe & me_mask);
                 end = (l0 << 48) + (l1 << 39) + (l2 << 30);
                 if (pdpe & PG_PRESENT_MASK) {
                     prot = 0;
@@ -511,7 +531,7 @@ static void mem_info_la57(Monitor *mon, CPUArchState *env)
                 pd_addr = pdpe & 0x3fffffffff000ULL;
                 for (l3 = 0; l3 < 512; l3++) {
                     cpu_physical_memory_read_debug(pd_addr + l3 * 8, &pde, 8);
-                    pde = le64_to_cpu(pde);
+                    pde = le64_to_cpu(pde & me_mask);
                     end = (l0 << 48) + (l1 << 39) + (l2 << 30) + (l3 << 21);
                     if (pde & PG_PRESENT_MASK) {
                         prot = 0;
@@ -531,7 +551,7 @@ static void mem_info_la57(Monitor *mon, CPUArchState *env)
                     for (l4 = 0; l4 < 512; l4++) {
                         cpu_physical_memory_read_debug(pt_addr + l4 * 8,
                                                        &pte, 8);
-                        pte = le64_to_cpu(pte);
+                        pte = le64_to_cpu(pte & me_mask);
                         end = (l0 << 48) + (l1 << 39) + (l2 << 30) +
                             (l3 << 21) + (l4 << 12);
                         if (pte & PG_PRESENT_MASK) {
diff --git a/target/i386/sev.c b/target/i386/sev.c
index b942593bc8..97726a639f 100644
--- a/target/i386/sev.c
+++ b/target/i386/sev.c
@@ -1003,7 +1003,8 @@ MemTxResult sev_address_space_read_debug(AddressSpace *as, hwaddr addr,
 
 static const MemoryDebugOps sev_debug_ops = {
     .read = sev_address_space_read_debug,
-    .write = sev_address_space_write_rom_debug
+    .write = sev_address_space_write_rom_debug,
+    .pte_mask = sev_get_me_mask,
 };
 
 void
-- 
2.17.1