[Qemu-devel] Xtensa misuse of tb_invalidate_phys_page_range()?

[Qemu-devel] Xtensa misuse of tb_invalidate_phys_page_range()?

[Avi Kivity]

void HELPER(wsr_ibreaka)(uint32_t i, uint32_t v)
{
    if (env->sregs[IBREAKENABLE] & (1 << i) && env->sregs[IBREAKA + i]
!= v) {
        tb_invalidate_phys_page_range(
                env->sregs[IBREAKA + i], env->sregs[IBREAKA + i] + 1, 0);
        tb_invalidate_phys_page_range(v, v + 1, 0);
    }
    env->sregs[IBREAKA + i] = v;
}

tb_invalidate_phys_page_range() expects a virtual address in user mode,
and a ram_addr_t in system mode.  I'm guessing that v is a virtual address?

This needs to be fixed for system mode if so (and in any case - even if
it's a physical address, it needs to be translated to a ram_addr_t).

Similar issue for hw/kvmvapic.c.

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

Re: [Qemu-devel] Xtensa misuse of tb_invalidate_phys_page_range()?

[Max Filippov]

> void HELPER(wsr_ibreaka)(uint32_t i, uint32_t v)
> {
>    if (env->sregs[IBREAKENABLE] & (1 << i) && env->sregs[IBREAKA + i]
> != v) {
>        tb_invalidate_phys_page_range(
>                env->sregs[IBREAKA + i], env->sregs[IBREAKA + i] + 1, 0);
>        tb_invalidate_phys_page_range(v, v + 1, 0);
>    }
>    env->sregs[IBREAKA + i] = v;
> }
>
> tb_invalidate_phys_page_range() expects a virtual address in user mode,
> and a ram_addr_t in system mode.  I'm guessing that v is a virtual address?

Yes, it's a virtual address here, as well as in wsr_lbeg/wsr_lend helpers.
I made a test for it and it actually fails. I wonder how could it stay unnoticed
that long :()

> This needs to be fixed for system mode if so (and in any case - even if
> it's a physical address, it needs to be translated to a ram_addr_t).

Sure. Will try to fix it, though it's completely unclear to me now
how to do it efficiently.

-- 
Thanks.
-- Max

Re: [Qemu-devel] Xtensa misuse of tb_invalidate_phys_page_range()?

[Avi Kivity]

On 03/19/2012 03:51 PM, Max Filippov wrote:
> > void HELPER(wsr_ibreaka)(uint32_t i, uint32_t v)
> > {
> >    if (env->sregs[IBREAKENABLE] & (1 << i) && env->sregs[IBREAKA + i]
> > != v) {
> >        tb_invalidate_phys_page_range(
> >                env->sregs[IBREAKA + i], env->sregs[IBREAKA + i] + 1, 0);
> >        tb_invalidate_phys_page_range(v, v + 1, 0);
> >    }
> >    env->sregs[IBREAKA + i] = v;
> > }
> >
> > tb_invalidate_phys_page_range() expects a virtual address in user mode,
> > and a ram_addr_t in system mode.  I'm guessing that v is a virtual address?
>
> Yes, it's a virtual address here, as well as in wsr_lbeg/wsr_lend helpers.
> I made a test for it and it actually fails. I wonder how could it stay unnoticed
> that long :()

There are many silent breakages like that, don't worry.

> > This needs to be fixed for system mode if so (and in any case - even if
> > it's a physical address, it needs to be translated to a ram_addr_t).
>
> Sure. Will try to fix it, though it's completely unclear to me now
> how to do it efficiently.

Since I'm rewriting this area, don't worry about efficiency.  Let's get
it correct and after the rewrite we can reexamine efficiency.

I imagine you'll need something like breakpoint_invalidate().

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

Re: [Qemu-devel] Xtensa misuse of tb_invalidate_phys_page_range()?

[Max Filippov]

>>> void HELPER(wsr_ibreaka)(uint32_t i, uint32_t v)
>>> {
>>>     if (env->sregs[IBREAKENABLE]&  (1<<  i)&&  env->sregs[IBREAKA + i]
>>> != v) {
>>>         tb_invalidate_phys_page_range(
>>>                 env->sregs[IBREAKA + i], env->sregs[IBREAKA + i] + 1, 0);
>>>         tb_invalidate_phys_page_range(v, v + 1, 0);
>>>     }
>>>     env->sregs[IBREAKA + i] = v;
>>> }
>>>
>>> tb_invalidate_phys_page_range() expects a virtual address in user mode,
>>> and a ram_addr_t in system mode.  I'm guessing that v is a virtual address?
>>
>> Yes, it's a virtual address here, as well as in wsr_lbeg/wsr_lend helpers.
>> I made a test for it and it actually fails. I wonder how could it stay unnoticed
>> that long :()
>
> There are many silent breakages like that, don't worry.
>
>>> This needs to be fixed for system mode if so (and in any case - even if
>>> it's a physical address, it needs to be translated to a ram_addr_t).
>>
>> Sure. Will try to fix it, though it's completely unclear to me now
>> how to do it efficiently.
>
> Since I'm rewriting this area, don't worry about efficiency.  Let's get
> it correct and after the rewrite we can reexamine efficiency.
>
> I imagine you'll need something like breakpoint_invalidate().

The following RFC patch takes the obvious approach of sharing the breakpoint_invalidate
implementation to address this issue. But if we're talking about correctness it's not
100% correct, because guest TLB is independent of both instruction breakpoints and
zero overhead loops, and at the moment of TB invalidation relevant TLB mapping may not
exist.

Even if we managed to record physical addresses of TBs at the current IBREAKA/LEND, it's
not enough because we need to invalidate TBs both at the old and at the new IBREAKA/LEND
virtual addresses.

What do you think, do we need yet another address-to-TB map?

-- 
Thanks.
-- Max

[Qemu-devel] [RFC] target-xtensa: fix tb invalidation for IBREAK and LOOP

[Max Filippov]

Instruction breakpoint/zero overhead loop handling code is built into
TBs pointed to by IBREAKA/LEND SRs. When these or related SRs get
changed TBs at virtual addresses corresponding to their old and their
new values must be invalidated.

Virtual address range is passed to the tb_invalidate_phys_page_range,
which is incorrect in system emulation mode.

To fix it use guest TLB/MMU to translate virtual address to physical
address.

However the guest may not have virtual-to-physical mapping at the moment
of IBREAKA/LEND change. Looks like only host side virtual address-to-tb
mapping similar to one used by the page_find is able to help in this
case.

Signed-off-by: Max Filippov <jcmvbkbc@gmail.com>
---
 exec-all.h                |    1 +
 exec.c                    |    9 ++++++---
 target-xtensa/op_helper.c |   29 ++++++++++++++++++-----------
 3 files changed, 25 insertions(+), 14 deletions(-)

diff --git a/exec-all.h b/exec-all.h
index 93a5b22..3da16ca 100644
--- a/exec-all.h
+++ b/exec-all.h
@@ -96,6 +96,7 @@ void QEMU_NORETURN cpu_loop_exit(CPUArchState *env1);
 int page_unprotect(target_ulong address, unsigned long pc, void *puc);
 void tb_invalidate_phys_page_range(tb_page_addr_t start, tb_page_addr_t end,
                                    int is_cpu_write_access);
+void tb_invalidate_phys_addr(target_phys_addr_t addr);
 void tlb_flush_page(CPUArchState *env, target_ulong addr);
 void tlb_flush(CPUArchState *env, int flush_global);
 #if !defined(CONFIG_USER_ONLY)
diff --git a/exec.c b/exec.c
index 6731ab8..130ecaa 100644
--- a/exec.c
+++ b/exec.c
@@ -1463,13 +1463,11 @@ static void breakpoint_invalidate(CPUArchState *env, target_ulong pc)
     tb_invalidate_phys_page_range(pc, pc + 1, 0);
 }
 #else
-static void breakpoint_invalidate(CPUArchState *env, target_ulong pc)
+void tb_invalidate_phys_addr(target_phys_addr_t addr)
 {
-    target_phys_addr_t addr;
     ram_addr_t ram_addr;
     MemoryRegionSection *section;
 
-    addr = cpu_get_phys_page_debug(env, pc);
     section = phys_page_find(addr >> TARGET_PAGE_BITS);
     if (!(memory_region_is_ram(section->mr)
           || (section->mr->rom_device && section->mr->readable))) {
@@ -1479,6 +1477,11 @@ static void breakpoint_invalidate(CPUArchState *env, target_ulong pc)
         + section_addr(section, addr);
     tb_invalidate_phys_page_range(ram_addr, ram_addr + 1, 0);
 }
+
+static void breakpoint_invalidate(CPUArchState *env, target_ulong pc)
+{
+    tb_invalidate_phys_addr(cpu_get_phys_page_debug(env, pc));
+}
 #endif
 #endif /* TARGET_HAS_ICE */
 
diff --git a/target-xtensa/op_helper.c b/target-xtensa/op_helper.c
index cdef0db..1b7e51f 100644
--- a/target-xtensa/op_helper.c
+++ b/target-xtensa/op_helper.c
@@ -99,6 +99,18 @@ void tlb_fill(CPUXtensaState *env1, target_ulong vaddr, int is_write, int mmu_id
     env = saved_env;
 }
 
+static void tb_invalidate_virtual_addr(CPUXtensaState *env, uint32_t vaddr)
+{
+    uint32_t paddr;
+    uint32_t page_size;
+    unsigned access;
+    int ret = xtensa_get_physical_addr(env, vaddr, 2, 0,
+            &paddr, &page_size, &access);
+    if (ret == 0) {
+        tb_invalidate_phys_addr(paddr);
+    }
+}
+
 void HELPER(exception)(uint32_t excp)
 {
     env->exception_index = excp;
@@ -358,8 +370,7 @@ void HELPER(movsp)(uint32_t pc)
 void HELPER(wsr_lbeg)(uint32_t v)
 {
     if (env->sregs[LBEG] != v) {
-        tb_invalidate_phys_page_range(
-                env->sregs[LEND] - 1, env->sregs[LEND], 0);
+        tb_invalidate_virtual_addr(env, env->sregs[LEND] - 1);
         env->sregs[LBEG] = v;
     }
 }
@@ -367,11 +378,9 @@ void HELPER(wsr_lbeg)(uint32_t v)
 void HELPER(wsr_lend)(uint32_t v)
 {
     if (env->sregs[LEND] != v) {
-        tb_invalidate_phys_page_range(
-                env->sregs[LEND] - 1, env->sregs[LEND], 0);
+        tb_invalidate_virtual_addr(env, env->sregs[LEND] - 1);
         env->sregs[LEND] = v;
-        tb_invalidate_phys_page_range(
-                env->sregs[LEND] - 1, env->sregs[LEND], 0);
+        tb_invalidate_virtual_addr(env, env->sregs[LEND] - 1);
     }
 }
 
@@ -692,8 +701,7 @@ void HELPER(wsr_ibreakenable)(uint32_t v)
 
     for (i = 0; i < env->config->nibreak; ++i) {
         if (change & (1 << i)) {
-            tb_invalidate_phys_page_range(
-                    env->sregs[IBREAKA + i], env->sregs[IBREAKA + i] + 1, 0);
+            tb_invalidate_virtual_addr(env, env->sregs[IBREAKA + i]);
         }
     }
     env->sregs[IBREAKENABLE] = v & ((1 << env->config->nibreak) - 1);
@@ -702,9 +710,8 @@ void HELPER(wsr_ibreakenable)(uint32_t v)
 void HELPER(wsr_ibreaka)(uint32_t i, uint32_t v)
 {
     if (env->sregs[IBREAKENABLE] & (1 << i) && env->sregs[IBREAKA + i] != v) {
-        tb_invalidate_phys_page_range(
-                env->sregs[IBREAKA + i], env->sregs[IBREAKA + i] + 1, 0);
-        tb_invalidate_phys_page_range(v, v + 1, 0);
+        tb_invalidate_virtual_addr(env, env->sregs[IBREAKA + i]);
+        tb_invalidate_virtual_addr(env, v);
     }
     env->sregs[IBREAKA + i] = v;
 }
-- 
1.7.7.6

Re: [Qemu-devel] Xtensa misuse of tb_invalidate_phys_page_range()?

[Avi Kivity]

On 03/25/2012 04:00 AM, Max Filippov wrote:
>>
>> Since I'm rewriting this area, don't worry about efficiency.  Let's get
>> it correct and after the rewrite we can reexamine efficiency.
>>
>> I imagine you'll need something like breakpoint_invalidate().
>
> The following RFC patch takes the obvious approach of sharing the
> breakpoint_invalidate
> implementation to address this issue. 

Looks good.

> But if we're talking about correctness it's not
> 100% correct, because guest TLB is independent of both instruction
> breakpoints and
> zero overhead loops, and at the moment of TB invalidation relevant TLB
> mapping may not
> exist.
>
> Even if we managed to record physical addresses of TBs at the current
> IBREAKA/LEND, it's
> not enough because we need to invalidate TBs both at the old and at
> the new IBREAKA/LEND
> virtual addresses.
>
> What do you think, do we need yet another address-to-TB map?
>

No idea what all that means, sorry.

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