[02/11] exec: Add new MemoryDebugOps.

Commit Message

Kalra, Ashish Nov. 16, 2020, 6:49 p.m. UTC

From: Ashish Kalra <[e-mail]>

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 <[e-mail]>
---
 include/exec/memory.h | 11 +++++++++++
 softmmu/physmem.c     | 24 ++++++++++++++++++++----
 2 files changed, 31 insertions(+), 4 deletions(-)

Comments

Dr. David Alan Gilbert Dec. 1, 2020, 11:37 a.m. UTC | #1

* Ashish Kalra ([e-mail]) wrote:
> From: Ashish Kalra <[e-mail]>
> 
> 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 <[e-mail]>

Reviewed-by: Dr. David Alan Gilbert <[e-mail]>

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

Peter Maydell Dec. 1, 2020, 11:48 a.m. UTC | #2

On Mon, 16 Nov 2020 at 19:07, Ashish Kalra <[e-mail]> wrote:
>
> From: Ashish Kalra <[e-mail]>
>
> 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

Kalra, Ashish Dec. 1, 2020, 2:27 p.m. UTC | #3

On Tue, Dec 01, 2020 at 11:48:23AM +0000, Peter Maydell wrote:
> On Mon, 16 Nov 2020 at 19:07, Ashish Kalra <[e-mail]> wrote:
> >
> > From: Ashish Kalra <[e-mail]>
> >
> > 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

Peter Maydell Dec. 1, 2020, 2:38 p.m. UTC | #4

On Tue, 1 Dec 2020 at 14:28, Ashish Kalra <[e-mail]> 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

Kalra, Ashish Dec. 1, 2020, 2:49 p.m. UTC | #5

On Tue, Dec 01, 2020 at 02:38:30PM +0000, Peter Maydell wrote:
> On Tue, 1 Dec 2020 at 14:28, Ashish Kalra <[e-mail]> 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

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;