Re: [PATCH v6 02/23] exec: add ram_debug_ops support

["Edgar E. Iglesias" <edgar.iglesias@xilinx.com>]

On Mon, Jan 29, 2018 at 11:41:11AM -0600, Brijesh Singh wrote:
> Currently, the guest memory access for the debug purpose is performed
> using the memcpy(). Lets extend the 'struct MemoryRegion' to include
> ram_debug_ops callbacks. The ram_debug_ops can be used to override
> memcpy() with something else.
> 
> The feature can be used by encrypted guest -- which can register
> callbacks to override memcpy() with memory encryption/decryption APIs.
> 
> a 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;


Hi,

Do these really need to be RAM specific (ram_debug_ops -> debug_ops) ?
I was hoping a similar infrastructure could be used for MMIO
debug accesses.

Best regards,
Edgar



> 
> memory_region_init_ram(mem, NULL, "memory", size, NULL);
> memory_region_set_ram_debug_ops(mem, ops);
> 
> Cc: Paolo Bonzini <pbonzini@redhat.com>
> Cc: Peter Crosthwaite <crosthwaite.peter@gmail.com>
> Cc: Richard Henderson <rth@twiddle.net>
> Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>
> ---
>  exec.c                | 66 ++++++++++++++++++++++++++++++++++++++-------------
>  include/exec/memory.h | 27 +++++++++++++++++++++
>  2 files changed, 77 insertions(+), 16 deletions(-)
> 
> diff --git a/exec.c b/exec.c
> index 629a5083851d..1919052b7385 100644
> --- a/exec.c
> +++ b/exec.c
> @@ -3050,7 +3050,11 @@ static MemTxResult flatview_write_continue(FlatView *fv, hwaddr addr,
>          } else {
>              /* RAM case */
>              ptr = qemu_ram_ptr_length(mr->ram_block, addr1, &l, false);
> -            memcpy(ptr, buf, l);
> +            if (attrs.debug && mr->ram_debug_ops) {
> +                mr->ram_debug_ops->write(ptr, buf, l, attrs);
> +            } else {
> +                memcpy(ptr, buf, l);
> +            }
>              invalidate_and_set_dirty(mr, addr1, l);
>          }
>  
> @@ -3148,7 +3152,11 @@ MemTxResult flatview_read_continue(FlatView *fv, hwaddr addr,
>          } else {
>              /* RAM case */
>              ptr = qemu_ram_ptr_length(mr->ram_block, addr1, &l, false);
> -            memcpy(buf, ptr, l);
> +            if (attrs.debug && mr->ram_debug_ops) {
> +                mr->ram_debug_ops->read(buf, ptr, l, attrs);
> +            } else {
> +                memcpy(buf, ptr, l);
> +            }
>          }
>  
>          if (release_lock) {
> @@ -3218,11 +3226,13 @@ void cpu_physical_memory_rw(hwaddr addr, uint8_t *buf,
>  
>  enum write_rom_type {
>      WRITE_DATA,
> +    READ_DATA,
>      FLUSH_CACHE,
>  };
>  
> -static inline void cpu_physical_memory_write_rom_internal(AddressSpace *as,
> -    hwaddr addr, const uint8_t *buf, int len, enum write_rom_type type)
> +static inline void cpu_physical_memory_rw_internal(AddressSpace *as,
> +    hwaddr addr, uint8_t *buf, int len, MemTxAttrs attrs,
> +    enum write_rom_type type)
>  {
>      hwaddr l;
>      uint8_t *ptr;
> @@ -3237,12 +3247,33 @@ static inline void cpu_physical_memory_write_rom_internal(AddressSpace *as,
>          if (!(memory_region_is_ram(mr) ||
>                memory_region_is_romd(mr))) {
>              l = memory_access_size(mr, l, addr1);
> +            /* Pass MMIO down to address address_space_rw */
> +            switch (type) {
> +            case READ_DATA:
> +            case WRITE_DATA:
> +                address_space_rw(as, addr1, attrs, buf, l,
> +                                 type == WRITE_DATA);
> +                break;
> +            case FLUSH_CACHE:
> +                break;
> +            }
>          } else {
>              /* ROM/RAM case */
>              ptr = qemu_map_ram_ptr(mr->ram_block, addr1);
>              switch (type) {
> +            case READ_DATA:
> +                if (mr->ram_debug_ops) {
> +                    mr->ram_debug_ops->read(buf, ptr, l, attrs);
> +                } else {
> +                    memcpy(buf, ptr, l);
> +                }
> +                break;
>              case WRITE_DATA:
> -                memcpy(ptr, buf, l);
> +                if (mr->ram_debug_ops) {
> +                    mr->ram_debug_ops->write(ptr, buf, l, attrs);
> +                } else {
> +                    memcpy(ptr, buf, l);
> +                }
>                  invalidate_and_set_dirty(mr, addr1, l);
>                  break;
>              case FLUSH_CACHE:
> @@ -3261,7 +3292,8 @@ static inline void cpu_physical_memory_write_rom_internal(AddressSpace *as,
>  void cpu_physical_memory_write_rom(AddressSpace *as, hwaddr addr,
>                                     const uint8_t *buf, int len)
>  {
> -    cpu_physical_memory_write_rom_internal(as, addr, buf, len, WRITE_DATA);
> +    cpu_physical_memory_rw_internal(as, addr, (uint8_t *)buf, len,
> +            MEMTXATTRS_UNSPECIFIED, WRITE_DATA);
>  }
>  
>  void cpu_flush_icache_range(hwaddr start, int len)
> @@ -3276,8 +3308,10 @@ void cpu_flush_icache_range(hwaddr start, int len)
>          return;
>      }
>  
> -    cpu_physical_memory_write_rom_internal(&address_space_memory,
> -                                           start, NULL, len, FLUSH_CACHE);
> +    cpu_physical_memory_rw_internal(&address_space_memory,
> +                                    start, NULL, len,
> +                                    MEMTXATTRS_UNSPECIFIED,
> +                                    FLUSH_CACHE);
>  }
>  
>  typedef struct {
> @@ -3583,6 +3617,7 @@ int cpu_memory_rw_debug(CPUState *cpu, target_ulong addr,
>      int l;
>      hwaddr phys_addr;
>      target_ulong page;
> +    int type = is_write ? WRITE_DATA : READ_DATA;
>  
>      cpu_synchronize_state(cpu);
>      while (len > 0) {
> @@ -3592,6 +3627,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;
> @@ -3599,14 +3638,9 @@ int cpu_memory_rw_debug(CPUState *cpu, target_ulong addr,
>          if (l > len)
>              l = len;
>          phys_addr += (addr & ~TARGET_PAGE_MASK);
> -        if (is_write) {
> -            cpu_physical_memory_write_rom(cpu->cpu_ases[asidx].as,
> -                                          phys_addr, buf, l);
> -        } else {
> -            address_space_rw(cpu->cpu_ases[asidx].as, phys_addr,
> -                             MEMTXATTRS_UNSPECIFIED,
> -                             buf, l, 0);
> -        }
> +        cpu_physical_memory_rw_internal(cpu->cpu_ases[asidx].as,
> +                                        phys_addr, buf, l, attrs,
> +                                        type);
>          len -= l;
>          buf += l;
>          addr += l;
> diff --git a/include/exec/memory.h b/include/exec/memory.h
> index 07c5d6d59796..4d027fffeebf 100644
> --- a/include/exec/memory.h
> +++ b/include/exec/memory.h
> @@ -215,6 +215,18 @@ typedef 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);
> +};
> +
>  struct MemoryRegion {
>      Object parent_obj;
>  
> @@ -254,6 +266,7 @@ struct MemoryRegion {
>      const char *name;
>      unsigned ioeventfd_nb;
>      MemoryRegionIoeventfd *ioeventfds;
> +    const MemoryRegionRAMReadWriteOps *ram_debug_ops;
>  };
>  
>  struct IOMMUMemoryRegion {
> @@ -624,6 +637,20 @@ void memory_region_init_rom_device_nomigrate(MemoryRegion *mr,
>                                               Error **errp);
>  
>  /**
> + * memory_region_set_ram_debug_ops: Set debug access ops for a given memory region
> + *
> + * @mr: the #MemoryRegion to be initialized
> + * @ops: a function that will be used for 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_reservation: Initialize a memory region that reserves
>   *                                 I/O space.
>   *
> -- 
> 2.9.5
>