| Message ID | 49BE9EF7.7010005@siemens.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Jan Kiszka wrote: > Align qemu-kvm-x86 with upstream kvm support /wrt software breakpoint > support in ROM code. Hmm, this might not be needed as kvm-userspace is not protecting its ROM. That's why I didn't pushed this so far. However, aligning isn't bad. But dropping this duplication would be better... Jan > > Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com> > --- > > qemu/qemu-kvm-x86.c | 29 ++++++++++++++++++++++++----- > 1 files changed, 24 insertions(+), 5 deletions(-) > > diff --git a/qemu/qemu-kvm-x86.c b/qemu/qemu-kvm-x86.c > index 06ef775..05a9c4c 100644 > --- a/qemu/qemu-kvm-x86.c > +++ b/qemu/qemu-kvm-x86.c > @@ -680,12 +680,32 @@ void kvm_arch_cpu_reset(CPUState *env) > } > } > > -int kvm_arch_insert_sw_breakpoint(CPUState *env, struct kvm_sw_breakpoint *bp) > +#ifdef KVM_CAP_SET_GUEST_DEBUG > +static int kvm_patch_opcode_byte(CPUState *env, target_ulong addr, uint8_t val) > { > - uint8_t int3 = 0xcc; > + target_phys_addr_t phys_page_addr; > + unsigned long pd; > + uint8_t *ptr; > + > + phys_page_addr = cpu_get_phys_page_debug(env, addr & TARGET_PAGE_MASK); > + if (phys_page_addr == -1) > + return -EINVAL; > + > + pd = cpu_get_physical_page_desc(phys_page_addr); > + if ((pd & ~TARGET_PAGE_MASK) != IO_MEM_RAM && > + (pd & ~TARGET_PAGE_MASK) != IO_MEM_ROM && !(pd & IO_MEM_ROMD)) > + return -EINVAL; > > + ptr = phys_ram_base + (pd & TARGET_PAGE_MASK) > + + (addr & ~TARGET_PAGE_MASK); > + *ptr = val; > + return 0; > +} > + > +int kvm_arch_insert_sw_breakpoint(CPUState *env, struct kvm_sw_breakpoint *bp) > +{ > if (cpu_memory_rw_debug(env, bp->pc, (uint8_t *)&bp->saved_insn, 1, 0) || > - cpu_memory_rw_debug(env, bp->pc, &int3, 1, 1)) > + kvm_patch_opcode_byte(env, bp->pc, 0xcc)) > return -EINVAL; > return 0; > } > @@ -695,12 +715,11 @@ int kvm_arch_remove_sw_breakpoint(CPUState *env, struct kvm_sw_breakpoint *bp) > uint8_t int3; > > if (cpu_memory_rw_debug(env, bp->pc, &int3, 1, 0) || int3 != 0xcc || > - cpu_memory_rw_debug(env, bp->pc, (uint8_t *)&bp->saved_insn, 1, 1)) > + kvm_patch_opcode_byte(env, bp->pc, bp->saved_insn)) > return -EINVAL; > return 0; > } > > -#ifdef KVM_CAP_SET_GUEST_DEBUG > static struct { > target_ulong addr; > int len; > -- > To unsubscribe from this list: send the line "unsubscribe kvm" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html >
Jan Kiszka wrote: > Jan Kiszka wrote: > >> Align qemu-kvm-x86 with upstream kvm support /wrt software breakpoint >> support in ROM code. >> > > Hmm, this might not be needed as kvm-userspace is not protecting its > ROM. That's why I didn't pushed this so far. However, aligning isn't > bad. But dropping this duplication would be better... > Aligning is a good thing (and duplication can be fixed in upstream and merged here). But maybe cpu_memory_rw_debug() should use cpu_physical_memory_write_rom() to write, instead of this hack?
Avi Kivity wrote: > Jan Kiszka wrote: >> Jan Kiszka wrote: >> >>> Align qemu-kvm-x86 with upstream kvm support /wrt software breakpoint >>> support in ROM code. >>> >> >> Hmm, this might not be needed as kvm-userspace is not protecting its >> ROM. That's why I didn't pushed this so far. However, aligning isn't >> bad. But dropping this duplication would be better... >> > > Aligning is a good thing (and duplication can be fixed in upstream and > merged here). But maybe cpu_memory_rw_debug() should use > cpu_physical_memory_write_rom() to write, instead of this hack? This is not a hack (it shouldn't have been merged upstream otherwise): cpu_physical_memory_write_rom() takes system-wide physical addresses while kvm_patch_opcode_byte() works with per-CPU linear addresses. And IMHO, the duplication needs to be fixed here by switching to upstream's kvm layer - at some point in the future. As a first step, we may have to look in making them compatible so that you can use the both layers at the same time. Jan
Jan Kiszka wrote: > This is not a hack (it shouldn't have been merged upstream otherwise): > cpu_physical_memory_write_rom() takes system-wide physical addresses > while kvm_patch_opcode_byte() works with per-CPU linear addresses. > From exec.c: > /* virtual memory access for debug */ > int cpu_memory_rw_debug(CPUState *env, target_ulong addr, > uint8_t *buf, int len, int is_write) > { > int l; > target_phys_addr_t phys_addr; > target_ulong page; > > while (len > 0) { > page = addr & TARGET_PAGE_MASK; > phys_addr = cpu_get_phys_page_debug(env, page); > /* if no physical page mapped, return an error */ > if (phys_addr == -1) > return -1; > l = (page + TARGET_PAGE_SIZE) - addr; > if (l > len) > l = len; > cpu_physical_memory_rw(phys_addr + (addr & ~TARGET_PAGE_MASK), > buf, l, is_write); > len -= l; > buf += l; > addr += l; > } > return 0; > } I'm talking about replacing cpu_physical_memory_rw() with cpu_physical_memory_write_rom() (for the write case). Is there a case where the debugger should be prevented from writing into ROM? If so, maybe cpu_memory_rw_debug_rom() for breakpoints? We shouldn't juggle page descriptors in *kvm*.c.
Avi Kivity wrote: > Jan Kiszka wrote: >> This is not a hack (it shouldn't have been merged upstream otherwise): >> cpu_physical_memory_write_rom() takes system-wide physical addresses >> while kvm_patch_opcode_byte() works with per-CPU linear addresses. >> > > From exec.c: > >> /* virtual memory access for debug */ >> int cpu_memory_rw_debug(CPUState *env, target_ulong addr, >> uint8_t *buf, int len, int is_write) >> { >> int l; >> target_phys_addr_t phys_addr; >> target_ulong page; >> >> while (len > 0) { >> page = addr & TARGET_PAGE_MASK; >> phys_addr = cpu_get_phys_page_debug(env, page); >> /* if no physical page mapped, return an error */ >> if (phys_addr == -1) >> return -1; >> l = (page + TARGET_PAGE_SIZE) - addr; >> if (l > len) >> l = len; >> cpu_physical_memory_rw(phys_addr + (addr & ~TARGET_PAGE_MASK), >> buf, l, is_write); >> len -= l; >> buf += l; >> addr += l; >> } >> return 0; >> } > > I'm talking about replacing cpu_physical_memory_rw() with > cpu_physical_memory_write_rom() (for the write case). Ah, miss-read your point. > Is there a case > where the debugger should be prevented from writing into ROM? If so, > maybe cpu_memory_rw_debug_rom() for breakpoints? Good and valid question. So far only the 'M' gdb packet uses cpu_physical_memory_rw in write mode, and I don't know if there is any reason to deny modifying ROM code that way. Hmm, will check with upstream and change there first. So forget about this patch for now. Jan
diff --git a/qemu/qemu-kvm-x86.c b/qemu/qemu-kvm-x86.c index 06ef775..05a9c4c 100644 --- a/qemu/qemu-kvm-x86.c +++ b/qemu/qemu-kvm-x86.c @@ -680,12 +680,32 @@ void kvm_arch_cpu_reset(CPUState *env) } } -int kvm_arch_insert_sw_breakpoint(CPUState *env, struct kvm_sw_breakpoint *bp) +#ifdef KVM_CAP_SET_GUEST_DEBUG +static int kvm_patch_opcode_byte(CPUState *env, target_ulong addr, uint8_t val) { - uint8_t int3 = 0xcc; + target_phys_addr_t phys_page_addr; + unsigned long pd; + uint8_t *ptr; + + phys_page_addr = cpu_get_phys_page_debug(env, addr & TARGET_PAGE_MASK); + if (phys_page_addr == -1) + return -EINVAL; + + pd = cpu_get_physical_page_desc(phys_page_addr); + if ((pd & ~TARGET_PAGE_MASK) != IO_MEM_RAM && + (pd & ~TARGET_PAGE_MASK) != IO_MEM_ROM && !(pd & IO_MEM_ROMD)) + return -EINVAL; + ptr = phys_ram_base + (pd & TARGET_PAGE_MASK) + + (addr & ~TARGET_PAGE_MASK); + *ptr = val; + return 0; +} + +int kvm_arch_insert_sw_breakpoint(CPUState *env, struct kvm_sw_breakpoint *bp) +{ if (cpu_memory_rw_debug(env, bp->pc, (uint8_t *)&bp->saved_insn, 1, 0) || - cpu_memory_rw_debug(env, bp->pc, &int3, 1, 1)) + kvm_patch_opcode_byte(env, bp->pc, 0xcc)) return -EINVAL; return 0; } @@ -695,12 +715,11 @@ int kvm_arch_remove_sw_breakpoint(CPUState *env, struct kvm_sw_breakpoint *bp) uint8_t int3; if (cpu_memory_rw_debug(env, bp->pc, &int3, 1, 0) || int3 != 0xcc || - cpu_memory_rw_debug(env, bp->pc, (uint8_t *)&bp->saved_insn, 1, 1)) + kvm_patch_opcode_byte(env, bp->pc, bp->saved_insn)) return -EINVAL; return 0; } -#ifdef KVM_CAP_SET_GUEST_DEBUG static struct { target_ulong addr; int len;
Align qemu-kvm-x86 with upstream kvm support /wrt software breakpoint support in ROM code. Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com> --- qemu/qemu-kvm-x86.c | 29 ++++++++++++++++++++++++----- 1 files changed, 24 insertions(+), 5 deletions(-) -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html