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

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

On Tue, Jan 30, 2018 at 04:34:37PM -0600, Brijesh Singh wrote:
> 
> 
> On 1/30/18 3:59 PM, Edgar E. Iglesias wrote:
> > 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.
> 
> Hi Edgar,
> 
> The MMIO regions will contains unencrypted data hence I do not find the
> need to add debug hooks for it. A memcpy() to/from MMIO should be fine.
> If we see the need for MMIO case then it can extended later.

In this use-case yes, but in a general use-case MMIO regions may also
want to differentiate debug from normal accesses.

I'm not saying you need to implement MMIO examples but do we really
need to have ram_debug_ops and mmio_debug_ops in the future?
Can we get away with one somehow?

Cheers,
Edgar

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