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

[Brijesh Singh]

On 1/30/18 4:37 PM, Edgar E. Iglesias wrote:
> 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?

If anyone see this coming in near future then having one debug_ops makes
sense and it should be  doable. If we decide to use debug_ops for MMIO
then I could see us needing to add a new argument to debug_ops callback
to let the client knows whether the access came MMIO vs RAM region
because client may do different things for RAM vs MMIO case (e.g in SEV
RAM is encrypted but MMIO is unencrypted).

> 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 <address@hidden>
>>>> Cc: Peter Crosthwaite <address@hidden>
>>>> Cc: Richard Henderson <address@hidden>
>>>> Signed-off-by: Brijesh Singh <address@hidden>
>>>> ---
>>>>  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
>>>>