| Message ID | 20180207160638.98872-2-brijesh.singh@amd.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v7,01/26] memattrs: add debug attribute | expand |
On 02/07/2018 10:06 AM, 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. [meta-comment] Your threading is off. This email was sent with the headers: > In-Reply-To: <20180207160638.98872-1-brijesh.singh@amd.com> > References: <20180207160638.98872-1-brijesh.singh@amd.com> which ties it only to patch 1/26; in turn, that message has no In-Reply-To header at all, making it a top-level thread. Meanwhile, your 0/26 cover letter has: > Message-Id: <20180207160324.98614-1-brijesh.singh@amd.com> which nothing in the rest of the series refers to, making it a separate thread. It would be helpful if you could the threading used in your sending environment, although I don't have any specific suggestions on what that fix would be.
On 2/7/18 10:19 AM, Eric Blake wrote: > On 02/07/2018 10:06 AM, 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. > > [meta-comment] > > Your threading is off. This email was sent with the headers: > > > In-Reply-To: <20180207160638.98872-1-brijesh.singh@amd.com> > > References: <20180207160638.98872-1-brijesh.singh@amd.com> > > which ties it only to patch 1/26; in turn, that message has no > In-Reply-To header at all, making it a top-level thread. Meanwhile, > your 0/26 cover letter has: > >> Message-Id: <20180207160324.98614-1-brijesh.singh@amd.com> > > which nothing in the rest of the series refers to, making it a > separate thread. It would be helpful if you could the threading used > in your sending environment, although I don't have any specific > suggestions on what that fix would be. > I am just looking at the my git send email script log and it seems that after sending the cover-letter patch, exchange server timeout and my script restarting send from patch 02/.. hence threading got messed up. sorry about that. If its an issue then I can resend the whole series. -Brijesh
On 02/07/2018 10:33 AM, Brijesh Singh wrote: >> [meta-comment] >> >> Your threading is off. This email was sent with the headers: >> > I am just looking at the my git send email script log and it seems that > after sending the cover-letter patch, exchange server timeout and my > script restarting send from patch 02/.. hence threading got messed up. > sorry about that. If its an issue then I can resend the whole series. No need to resend for just that issue (we can deal with a one-off, and there may be a v8 anyways depending on what reviewers find). Does your server allow you to pre-specify message ids, or does it always ignore your input and generate its own message ids? If interruptions are a common issue, you can use 'git send-email --in-reply-to=...' to forcefully (try and) thread a second batch of sent mail to an earlier batch that was interrupted from completion. It may also be possible to have 'git format-patch' pre-thread all mails, although that may already match what you were doing (pre-specifying message ids only works if your server preserves those requested message ids from the input instead of blindly replacing it with its own ids)
On 07/02/2018 17:06, Brijesh Singh wrote: > @@ -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) { You also need to tweak flatview_read in include/exec/memory.h (probably by adding an "&& !attrs.debug", which leaves the mr->ram_debug_ops->read to the slow path in exec.c). > @@ -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; I wonder if READ_DATA and WRITE_DATA still need to go down to cpu_physical_memory_rw_internal. Maybe you can just call address_space_rw with &address_space_memory as the address space, and "(MemTxAttrs) { .debug = 1 }" as the attributes. Paolo > @@ -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:
On 02/07/2018 10:51 AM, Paolo Bonzini wrote: > On 07/02/2018 17:06, Brijesh Singh wrote: >> @@ -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) { > > You also need to tweak flatview_read in include/exec/memory.h (probably > by adding an "&& !attrs.debug", which leaves the mr->ram_debug_ops->read > to the slow path in exec.c). > thanks, I will make the changes in flatview_read to take slow path when debug is enabled. >> @@ -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; > > I wonder if READ_DATA and WRITE_DATA still need to go down to > cpu_physical_memory_rw_internal. Maybe you can just call > address_space_rw with &address_space_memory as the address space, and > "(MemTxAttrs) { .debug = 1 }" as the attributes. > I will take a look to see if I can remove passing down the READ_DATA and WRITE_DATA.
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..43445cb9e45d 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 { @@ -623,6 +636,21 @@ void memory_region_init_rom_device_nomigrate(MemoryRegion *mr, uint64_t size, 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.
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; 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 | 28 ++++++++++++++++++++++ 2 files changed, 78 insertions(+), 16 deletions(-)