[PATCH] kvm-userspace: x86: Support for breakpoints in ROM code

[PATCH] kvm-userspace: x86: Support for breakpoints in ROM code

[Jan Kiszka]

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(-)

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;

Re: [PATCH] kvm-userspace: x86: Support for breakpoints in ROM code

[Jan Kiszka]

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
> 

-- 
Siemens AG, Corporate Technology, CT SE 2
Corporate Competence Center Embedded Linux

Re: [PATCH] kvm-userspace: x86: Support for breakpoints in ROM code

[Avi Kivity]

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?

-- 
error compiling committee.c: too many arguments to function

Re: [PATCH] kvm-userspace: x86: Support for breakpoints in ROM code

[Jan Kiszka]

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

-- 
Siemens AG, Corporate Technology, CT SE 2
Corporate Competence Center Embedded Linux

Re: [PATCH] kvm-userspace: x86: Support for breakpoints in ROM code

[Avi Kivity]

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.

-- 
error compiling committee.c: too many arguments to function

Re: [PATCH] kvm-userspace: x86: Support for breakpoints in ROM code

[Jan Kiszka]

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

-- 
Siemens AG, Corporate Technology, CT SE 2
Corporate Competence Center Embedded Linux