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@xxxxxxxxxx>
> Cc: Peter Crosthwaite <crosthwaite.peter@xxxxxxxxx>
> Cc: Richard Henderson <rth@xxxxxxxxxxx>
> Signed-off-by: Brijesh Singh <brijesh.singh@xxxxxxx>
> ---
> 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
>
