[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
[Qemu-devel] Re: [PATCH 04/13] Respect length of watchpoints
From: |
Jan Kiszka |
Subject: |
[Qemu-devel] Re: [PATCH 04/13] Respect length of watchpoints |
Date: |
Tue, 14 Oct 2008 20:26:39 +0200 |
User-agent: |
Mozilla/5.0 (X11; U; Linux i686 (x86_64); de; rv:1.8.1.12) Gecko/20080226 SUSE/2.0.0.12-1.1 Thunderbird/2.0.0.12 Mnenhy/0.7.5.666 |
Glauber Costa wrote:
> On Tue, Oct 14, 2008 at 7:12 AM, Jan Kiszka <address@hidden> wrote:
>> This adds length support for watchpoints. To keep things simple, only
>> aligned watchpoints are accepted.
>
> why? It does not seem that much complicated to handle unaligned watchpoints.
> Unless I'm totally wrong, we should just store the value as-is, and
> then check for it.
> As a matter of fact, because we're masking and testing for the mask,
> it seems even more
> complicated to require that. I agree a full aligned world would be a
> happier world, but unfortunately,
> unaligned accesses are quite common in x86.
Unaligned watchpoints also means multi-page watchpoints - and this
introduces some complexity. I think the fact that real x86 hw
watchpoints require alignment as well motivated the simplification. But
if there is a real need for it (e.g. some other arch using the
infrastructure for hw watchpoint emulation), I could rethink this.
However, I would prefer to apply such extension on top of the proposed
implementation.
>
>> Signed-off-by: Jan Kiszka <address@hidden>
>> ---
>> cpu-defs.h | 2 +-
>> exec.c | 30 ++++++++++++++++++++----------
>> 2 files changed, 21 insertions(+), 11 deletions(-)
>>
>> Index: b/exec.c
>> ===================================================================
>> --- a/exec.c
>> +++ b/exec.c
>> @@ -1313,14 +1313,21 @@ static void breakpoint_invalidate(CPUSta
>> int cpu_watchpoint_insert(CPUState *env, target_ulong addr, target_ulong
>> len,
>> int flags, CPUWatchpoint **watchpoint)
>> {
>> + target_ulong len_mask = ~(len - 1);
>> CPUWatchpoint *wp;
>>
>> + /* sanity checks: allow power-of-2 lengths, deny unaligned watchpoints
>> */
>> + if ((len != 1 && len != 2 && len != 4 && len != 8) || (addr &
>> ~len_mask)) {
>> + fprintf(stderr, "qemu: tried to set invalid watchpoint at "
>> + TARGET_FMT_lx ", len=" TARGET_FMT_lu "\n", addr, len);
>> + return -EINVAL;
>> + }
>> wp = qemu_malloc(sizeof(*wp));
>> if (!wp)
>> return -ENOBUFS;
>>
>> wp->vaddr = addr;
>> - wp->len = len;
>> + wp->len_mask = len_mask;
>> wp->flags = flags;
>>
>> wp->next = env->watchpoints;
>> @@ -1344,10 +1351,12 @@ int cpu_watchpoint_insert(CPUState *env,
>> int cpu_watchpoint_remove(CPUState *env, target_ulong addr, target_ulong
>> len,
>> int flags)
>> {
>> + target_ulong len_mask = ~(len - 1);
>> CPUWatchpoint *wp;
>>
>> for (wp = env->watchpoints; wp != NULL; wp = wp->next) {
>> - if (addr == wp->vaddr && len == wp->len && flags == wp->flags) {
>> + if (addr == wp->vaddr && len_mask == wp->len_mask
>> + && flags == wp->flags) {
>> cpu_watchpoint_remove_by_ref(env, wp);
>> return 0;
>> }
>> @@ -2502,7 +2511,7 @@ static CPUWriteMemoryFunc *notdirty_mem_
>> };
>>
>> /* Generate a debug exception if a watchpoint has been hit. */
>> -static void check_watchpoint(int offset, int flags)
>> +static void check_watchpoint(int offset, int len_mask, int flags)
>> {
>> CPUState *env = cpu_single_env;
>> target_ulong vaddr;
>> @@ -2510,7 +2519,8 @@ static void check_watchpoint(int offset,
>>
>> vaddr = (env->mem_io_vaddr & TARGET_PAGE_MASK) + offset;
>> for (wp = env->watchpoints; wp != NULL; wp = wp->next) {
>> - if (vaddr == wp->vaddr && (wp->flags & flags)) {
>> + if ((vaddr == (wp->vaddr & len_mask) ||
>> + (vaddr & wp->len_mask) == wp->vaddr) && (wp->flags & flags)) {
>> env->watchpoint_hit = wp;
>> cpu_interrupt(env, CPU_INTERRUPT_DEBUG);
>> break;
>> @@ -2523,40 +2533,40 @@ static void check_watchpoint(int offset,
>> phys routines. */
>> static uint32_t watch_mem_readb(void *opaque, target_phys_addr_t addr)
>> {
>> - check_watchpoint(addr & ~TARGET_PAGE_MASK, BP_MEM_READ);
>> + check_watchpoint(addr & ~TARGET_PAGE_MASK, ~0x0, BP_MEM_READ);
>> return ldub_phys(addr);
>> }
>>
>> static uint32_t watch_mem_readw(void *opaque, target_phys_addr_t addr)
>> {
>> - check_watchpoint(addr & ~TARGET_PAGE_MASK, BP_MEM_READ);
>> + check_watchpoint(addr & ~TARGET_PAGE_MASK, ~0x1, BP_MEM_READ);
>> return lduw_phys(addr);
>> }
>>
>> static uint32_t watch_mem_readl(void *opaque, target_phys_addr_t addr)
>> {
>> - check_watchpoint(addr & ~TARGET_PAGE_MASK, BP_MEM_READ);
>> + check_watchpoint(addr & ~TARGET_PAGE_MASK, ~0x3, BP_MEM_READ);
>> return ldl_phys(addr);
>> }
>>
>> static void watch_mem_writeb(void *opaque, target_phys_addr_t addr,
>> uint32_t val)
>> {
>> - check_watchpoint(addr & ~TARGET_PAGE_MASK, BP_MEM_WRITE);
>> + check_watchpoint(addr & ~TARGET_PAGE_MASK, ~0x0, BP_MEM_WRITE);
>> stb_phys(addr, val);
>> }
>>
>> static void watch_mem_writew(void *opaque, target_phys_addr_t addr,
>> uint32_t val)
>> {
>> - check_watchpoint(addr & ~TARGET_PAGE_MASK, BP_MEM_WRITE);
>> + check_watchpoint(addr & ~TARGET_PAGE_MASK, ~0x1, BP_MEM_WRITE);
>> stw_phys(addr, val);
>> }
>>
>> static void watch_mem_writel(void *opaque, target_phys_addr_t addr,
>> uint32_t val)
>> {
>> - check_watchpoint(addr & ~TARGET_PAGE_MASK, BP_MEM_WRITE);
>> + check_watchpoint(addr & ~TARGET_PAGE_MASK, ~0x3, BP_MEM_WRITE);
>> stl_phys(addr, val);
>> }
>>
>> Index: b/cpu-defs.h
>> ===================================================================
>> --- a/cpu-defs.h
>> +++ b/cpu-defs.h
>> @@ -148,7 +148,7 @@ typedef struct CPUBreakpoint {
>>
>> typedef struct CPUWatchpoint {
>> target_ulong vaddr;
>> - target_ulong len;
>> + target_ulong len_mask;
>> int flags; /* BP_* */
>> struct CPUWatchpoint *prev, *next;
>> } CPUWatchpoint;
>
> It's less confusing if you call it len_mask from the beginning,
> instead of changing your own patch for that purpose.
OK. If I have to update the involved patches, I will merge this over.
Jan
signature.asc
Description: OpenPGP digital signature
- Re: [Qemu-devel] Re: [PATCH 02/13] Refactor and enhance break/watchpoint API, (continued)
- [Qemu-devel] [PATCH 13/13] x86: Debug register emulation, Jan Kiszka, 2008/10/14
- [Qemu-devel] [PATCH 08/13] Remove premature memop TB terminations, Jan Kiszka, 2008/10/14
- [Qemu-devel] [PATCH 07/13] Restore pc on watchpoint hits, Jan Kiszka, 2008/10/14
- [Qemu-devel] [PATCH 11/13] Add debug exception hook, Jan Kiszka, 2008/10/14
- [Qemu-devel] [PATCH 09/13] qemu: gdbstub: manage CPUs as threads, Jan Kiszka, 2008/10/14
- [Qemu-devel] [PATCH 04/13] Respect length of watchpoints, Jan Kiszka, 2008/10/14