[Qemu-devel] [PATCH RFC 0/5] softmmu and s390x memory helper improvements

[Qemu-devel] [PATCH RFC 0/5] softmmu and s390x memory helper improvements

[Aurelien Jarno]

This patch series is an attempt to improve the speed of the s390x TCG
emulation. It's not yet ready (hence the RFC), but I decided to publish
it anyway as it might give an other way to do add the probe_write
feature. I have Cc:ed the persons that were in Cc: of that mail, sorry
if you that it could be spam.

The ideas is to add a new tlb_vaddr_to_host_fill similar to the already
existing tlb_vaddr_to_host function, but which can trigger guest
exception. I guess probe_write could be implemented as a call to this
function, ignoring the returned value. More details are in patch 3.

Aurelien Jarno (5):
  target-s390x: add a cpu_mmu_idx_to_asc function.
  target-390x: support non current ASC in s390_cpu_handle_mmu_fault
  softmmu: add a tlb_vaddr_to_host_common function
  target-s390x: function to adjust the length wrt page boundary
  target-s390x: use softmmu host addr function for mvcp/mvcs

 include/exec/cpu_ldst.h   | 100 +++++++++++++++++++++++++++++++++++-----------
 target-s390x/cpu.h        |  25 ++++++++++--
 target-s390x/helper.c     |   2 +-
 target-s390x/mem_helper.c |  55 ++++++++++++-------------
 target-s390x/translate.c  |   2 -
 5 files changed, 126 insertions(+), 58 deletions(-)

-- 
2.1.4

[Qemu-devel] [PATCH RFC 1/5] target-s390x: add a cpu_mmu_idx_to_asc function.

[Aurelien Jarno]

Use constants to define the MMU indexes, and add a function to do
the reverse conversion of cpu_mmu_index.

Cc: Alexander Graf <agraf@suse.de>
Cc: Richard Henderson <rth@twiddle.net>
Signed-off-by: Aurelien Jarno <aurelien@aurel32.net>
---
 target-s390x/cpu.h | 25 ++++++++++++++++++++++---
 1 file changed, 22 insertions(+), 3 deletions(-)

diff --git a/target-s390x/cpu.h b/target-s390x/cpu.h
index adb9a84..584e74b 100644
--- a/target-s390x/cpu.h
+++ b/target-s390x/cpu.h
@@ -302,15 +302,20 @@ static inline CPU_DoubleU *get_freg(CPUS390XState *cs, int nr)
 #define CR0_LOWPROT             0x0000000010000000ULL
 #define CR0_EDAT                0x0000000000800000ULL
 
+/* MMU */
+#define MMU_PRIMARY_IDX         0
+#define MMU_SECONDARY_IDX       1
+#define MMU_HOME_IDX            2
+
 static inline int cpu_mmu_index (CPUS390XState *env)
 {
     switch (env->psw.mask & PSW_MASK_ASC) {
     case PSW_ASC_PRIMARY:
-        return 0;
+        return MMU_PRIMARY_IDX;
     case PSW_ASC_SECONDARY:
-        return 1;
+        return MMU_SECONDARY_IDX;
     case PSW_ASC_HOME:
-        return 2;
+        return MMU_HOME_IDX;
     case PSW_ASC_ACCREG:
         /* Fallthrough: access register mode is not yet supported */
     default:
@@ -318,6 +323,20 @@ static inline int cpu_mmu_index (CPUS390XState *env)
     }
 }
 
+static inline uint64_t cpu_mmu_idx_to_asc(int mmu_idx)
+{
+    switch (mmu_idx) {
+    case MMU_PRIMARY_IDX:
+        return PSW_ASC_PRIMARY;
+    case MMU_SECONDARY_IDX:
+        return PSW_ASC_SECONDARY;
+    case MMU_HOME_IDX:
+        return PSW_ASC_HOME;
+    default:
+        abort();
+    }
+}
+
 static inline void cpu_get_tb_cpu_state(CPUS390XState* env, target_ulong *pc,
                                         target_ulong *cs_base, int *flags)
 {
-- 
2.1.4

[Qemu-devel] [PATCH RFC 2/5] target-390x: support non current ASC in s390_cpu_handle_mmu_fault

[Aurelien Jarno]

s390_cpu_handle_mmu_fault currently looks at the current ASC mode
defined in PSW mask instead of the MMU index. This prevent emulating
easily instructions using a specific ASC mode. Fix that by using the
MMU index converted back to ASC using the just added cpu_mmu_idx_to_asc
function.

Cc: Alexander Graf <agraf@suse.de>
Cc: Richard Henderson <rth@twiddle.net>
Signed-off-by: Aurelien Jarno <aurelien@aurel32.net>
---
 target-s390x/helper.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/target-s390x/helper.c b/target-s390x/helper.c
index 6b47766..90d273c 100644
--- a/target-s390x/helper.c
+++ b/target-s390x/helper.c
@@ -112,7 +112,7 @@ int s390_cpu_handle_mmu_fault(CPUState *cs, vaddr orig_vaddr,
 {
     S390CPU *cpu = S390_CPU(cs);
     CPUS390XState *env = &cpu->env;
-    uint64_t asc = env->psw.mask & PSW_MASK_ASC;
+    uint64_t asc = cpu_mmu_idx_to_asc(mmu_idx);
     target_ulong vaddr, raddr;
     int prot;
 
-- 
2.1.4

[Qemu-devel] [PATCH RFC 3/5] softmmu: add a tlb_vaddr_to_host_fill function

[Aurelien Jarno]

The softmmu code already provides a tlb_vaddr_to_host function, which
returns the host address corresponding to a guest virtual address,
*if it is already in the QEMU MMU TLB*.

This patch is an attempt to have a function which try to fill the TLB
entry if it is not already in the QEMU MMU TLB, possibly trigger a guest
fault. It can be used directly in helpers. For that it creates a common
function with a boolean to tell if the TLB needs to be filled or not. If
yes, it causes tlb_fill, which might trigger an exception or succeed in
which case the tlbentry pointer need to be reloaded.

I also had to change the MMIO test part. It seems that in write mode
some TLB entries are filled with TLB_NOTDIRTY. They are caught by the
MMIO test and a NULL pointer is returned instead. I am not sure of my
change, but I guess the current softmmu code has the same issue.

At the same time, it defines the same function for the user mode, so
that we can write helpers using the same code for softmmu and user mode,
just like cpu_ldxx_data() functions works for both.

It also replaces hard-coded values for the access-type by the
corresponding constants.

Cc: Richard Henderson <rth@twiddle.net>
Cc: Alexander Graf <agraf@suse.de>
Cc: Paolo Bonzini <pbonzini@redhat.com>
Cc: Yongbok Kim <yongbok.kim@imgtec.com>
Cc: Leon Alrae <leon.alrae@imgtec.com>
Cc: Andreas Färber <afaerber@suse.de>
Cc: Peter Maydell <peter.maydell@linaro.org>
Signed-off-by: Aurelien Jarno <aurelien@aurel32.net>
---
 include/exec/cpu_ldst.h | 100 +++++++++++++++++++++++++++++++++++++-----------
 1 file changed, 77 insertions(+), 23 deletions(-)

diff --git a/include/exec/cpu_ldst.h b/include/exec/cpu_ldst.h
index 1673287..64fe806 100644
--- a/include/exec/cpu_ldst.h
+++ b/include/exec/cpu_ldst.h
@@ -307,37 +307,40 @@ uint64_t helper_ldq_cmmu(CPUArchState *env, target_ulong addr, int mmu_idx);
 #undef MEMSUFFIX
 #undef SOFTMMU_CODE_ACCESS
 
-/**
- * tlb_vaddr_to_host:
- * @env: CPUArchState
- * @addr: guest virtual address to look up
- * @access_type: 0 for read, 1 for write, 2 for execute
- * @mmu_idx: MMU index to use for lookup
- *
- * Look up the specified guest virtual index in the TCG softmmu TLB.
- * If the TLB contains a host virtual address suitable for direct RAM
- * access, then return it. Otherwise (TLB miss, TLB entry is for an
- * I/O access, etc) return NULL.
- *
- * This is the equivalent of the initial fast-path code used by
- * TCG backends for guest load and store accesses.
- */
-static inline void *tlb_vaddr_to_host(CPUArchState *env, target_ulong addr,
-                                      int access_type, int mmu_idx)
+#endif /* defined(CONFIG_USER_ONLY) */
+
+
+
+#if defined(CONFIG_USER_ONLY)
+static inline void *tlb_vaddr_to_host_common(CPUArchState *env,
+                                             target_ulong addr,
+                                             int access_type, int mmu_idx,
+                                             uintptr_t retaddr, bool fill)
+{
+    return g2h(vaddr);
+}
+#else
+static inline void *tlb_vaddr_to_host_common(CPUArchState *env,
+                                             target_ulong addr,
+                                             int access_type, int mmu_idx,
+                                             uintptr_t retaddr, bool fill)
 {
     int index = (addr >> TARGET_PAGE_BITS) & (CPU_TLB_SIZE - 1);
-    CPUTLBEntry *tlbentry = &env->tlb_table[mmu_idx][index];
+    CPUTLBEntry *tlbentry;
     target_ulong tlb_addr;
     uintptr_t haddr;
 
+again:
+    tlbentry = &env->tlb_table[mmu_idx][index];
+
     switch (access_type) {
-    case 0:
+    case MMU_DATA_LOAD:
         tlb_addr = tlbentry->addr_read;
         break;
-    case 1:
+    case MMU_DATA_STORE:
         tlb_addr = tlbentry->addr_write;
         break;
-    case 2:
+    case MMU_INST_FETCH:
         tlb_addr = tlbentry->addr_code;
         break;
     default:
@@ -347,10 +350,14 @@ static inline void *tlb_vaddr_to_host(CPUArchState *env, target_ulong addr,
     if ((addr & TARGET_PAGE_MASK)
         != (tlb_addr & (TARGET_PAGE_MASK | TLB_INVALID_MASK))) {
         /* TLB entry is for a different page */
+        if (fill) {
+            tlb_fill(ENV_GET_CPU(env), addr, access_type, mmu_idx, retaddr);
+            goto again;
+        }
         return NULL;
     }
 
-    if (tlb_addr & ~TARGET_PAGE_MASK) {
+    if (tlb_addr & TLB_MMIO) {
         /* IO access */
         return NULL;
     }
@@ -358,7 +365,54 @@ static inline void *tlb_vaddr_to_host(CPUArchState *env, target_ulong addr,
     haddr = addr + env->tlb_table[mmu_idx][index].addend;
     return (void *)haddr;
 }
+#endif
 
-#endif /* defined(CONFIG_USER_ONLY) */
+/**
+ * tlb_vaddr_to_host:
+ * @env: CPUArchState
+ * @addr: guest virtual address to look up
+ * @access_type: MMU_DATA_LOAD for read, MMU_DATA_STORE for write,
+ *               MMU_INST_FETCH for execute
+ * @mmu_idx: MMU index to use for lookup
+ *
+ * Look up the specified guest virtual index in the TCG softmmu TLB.
+ * If the TLB contains a host virtual address suitable for direct RAM
+ * access, then return it. Otherwise (TLB miss, TLB entry is for an
+ * I/O access, etc) return NULL.
+ *
+ * This is the equivalent of the initial fast-path code used by
+ * TCG backends for guest load and store accesses.
+ */
+static inline void *tlb_vaddr_to_host(CPUArchState *env, target_ulong addr,
+                                      int access_type, int mmu_idx)
+{
+    return tlb_vaddr_to_host_common(env, addr, access_type,
+                                    mmu_idx, 0, false);
+}
+
+/**
+ * tlb_vaddr_to_host_fill:
+ * @env: CPUArchState
+ * @addr: guest virtual address to look up
+ * @access_type: MMU_DATA_LOAD for read, MMU_DATA_STORE for write,
+ *               MMU_INST_FETCH for execute
+ * @mmu_idx: MMU index to use for lookup
+ * @retaddr: address returned by GETPC() when called from a helper or 0
+ *
+ * Look up the specified guest virtual index in the TCG softmmu TLB.
+ * If the TLB contains a host virtual address suitable for direct RAM
+ * access, then return it. In case of a TLB miss, it trigger an exception.
+ * Otherwise (TLB entry is for an I/O access, etc), it returns NULL.
+ *
+ * It is the responsability of the caller to ensure endian conversion, that
+ * page boundaries are not crossed and that access alignement is correct.
+ */
+static inline void *tlb_vaddr_to_host_fill(CPUArchState *env, target_ulong addr,
+                                           int access_type, int mmu_idx,
+                                           uintptr_t retaddr)
+{
+    return tlb_vaddr_to_host_common(env, addr, access_type,
+                                    mmu_idx, retaddr, true);
+}
 
 #endif /* CPU_LDST_H */
-- 
2.1.4

[Qemu-devel] [PATCH RFC 4/5] target-s390x: function to adjust the length wrt page boundary

[Aurelien Jarno]

This patch adds a function to adjust the length of a transfer so that
it doesn't cross a page boundary.

Cc: Alexander Graf <agraf@suse.de>
Cc: Richard Henderson <rth@twiddle.net>
Signed-off-by: Aurelien Jarno <aurelien@aurel32.net>
---
 target-s390x/mem_helper.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/target-s390x/mem_helper.c b/target-s390x/mem_helper.c
index 9fe9ed7..f34f2e1 100644
--- a/target-s390x/mem_helper.c
+++ b/target-s390x/mem_helper.c
@@ -55,6 +55,17 @@ void tlb_fill(CPUState *cs, target_ulong addr, int is_write, int mmu_idx,
 #endif
 
 #ifndef CONFIG_USER_ONLY
+
+/* Reduce the length so that addr + len doesn't cross a page boundary.  */
+static inline uint64_t adj_len_to_page(uint64_t len, uint64_t addr)
+{
+    if ((addr & TARGET_PAGE_MASK) != ((addr + len - 1) & TARGET_PAGE_MASK)) {
+        return -addr & ~TARGET_PAGE_MASK;
+    } else {
+        return len;
+    }
+}
+
 static void mvc_fast_memset(CPUS390XState *env, uint32_t l, uint64_t dest,
                             uint8_t byte)
 {
-- 
2.1.4

[Qemu-devel] [PATCH RFC 5/5] target-s390x: use softmmu host addr function for mvcp/mvcs

[Aurelien Jarno]

mvcp and mvcs helper get access to the physical memory by a call to
mmu_translate for the virtual to real conversion and then using ldb_phys
and stb_phys to physically access the data. In practice this is quite
slow because it bypasses the QEMU softmmu TLB and because stb_phys calls
try to invalidate the corresponding memory for each access.

Instead use the new softmmu guest virtual address to host address
conversion and call memcpy, taking care of not crossing page boundaries.
As we pass the return address, we can skip saving the registers in the
TCG code, a page fault trigger code retranslation instead.

This improves the boot time of a guest by a factor 2.

Cc: Alexander Graf <agraf@suse.de>
Cc: Richard Henderson <rth@twiddle.net>
Signed-off-by: Aurelien Jarno <aurelien@aurel32.net>
---
 target-s390x/mem_helper.c | 44 +++++++++++++++-----------------------------
 target-s390x/translate.c  |  2 --
 2 files changed, 15 insertions(+), 31 deletions(-)

diff --git a/target-s390x/mem_helper.c b/target-s390x/mem_helper.c
index f34f2e1..0efb780 100644
--- a/target-s390x/mem_helper.c
+++ b/target-s390x/mem_helper.c
@@ -1026,40 +1026,26 @@ uint32_t HELPER(csp)(CPUS390XState *env, uint32_t r1, uint64_t r2)
     return cc;
 }
 
-static uint32_t mvc_asc(CPUS390XState *env, int64_t l, uint64_t a1,
-                        uint64_t mode1, uint64_t a2, uint64_t mode2)
+static uint32_t mvc_asc(CPUS390XState *env, uint64_t l, uint64_t a1,
+                        int idx1, uint64_t a2, int idx2, uintptr_t retaddr)
 {
-    CPUState *cs = CPU(s390_env_get_cpu(env));
-    target_ulong src, dest;
-    int flags, cc = 0, i;
+    int cc = 0;
 
-    if (!l) {
-        return 0;
-    } else if (l > 256) {
+    if (l > 256) {
         /* max 256 */
         l = 256;
         cc = 3;
     }
 
-    if (mmu_translate(env, a1, 1, mode1, &dest, &flags, true)) {
-        cpu_loop_exit(CPU(s390_env_get_cpu(env)));
-    }
-    dest |= a1 & ~TARGET_PAGE_MASK;
-
-    if (mmu_translate(env, a2, 0, mode2, &src, &flags, true)) {
-        cpu_loop_exit(CPU(s390_env_get_cpu(env)));
-    }
-    src |= a2 & ~TARGET_PAGE_MASK;
-
-    /* XXX replace w/ memcpy */
-    for (i = 0; i < l; i++) {
-        /* XXX be more clever */
-        if ((((dest + i) & TARGET_PAGE_MASK) != (dest & TARGET_PAGE_MASK)) ||
-            (((src + i) & TARGET_PAGE_MASK) != (src & TARGET_PAGE_MASK))) {
-            mvc_asc(env, l - i, a1 + i, mode1, a2 + i, mode2);
-            break;
-        }
-        stb_phys(cs->as, dest + i, ldub_phys(cs->as, src + i));
+    while (l > 0) {
+        void *src = tlb_vaddr_to_host_fill(env, a2, MMU_DATA_LOAD, idx2, retaddr);
+        void *dest = tlb_vaddr_to_host_fill(env, a1, MMU_DATA_STORE, idx1, retaddr);
+        int len = adj_len_to_page(l, a1);
+        len = adj_len_to_page(len, a2);
+        memcpy(dest, src, len);
+        l -= len;
+        a1 += len;
+        a2 += len;
     }
 
     return cc;
@@ -1070,7 +1056,7 @@ uint32_t HELPER(mvcs)(CPUS390XState *env, uint64_t l, uint64_t a1, uint64_t a2)
     HELPER_LOG("%s: %16" PRIx64 " %16" PRIx64 " %16" PRIx64 "\n",
                __func__, l, a1, a2);
 
-    return mvc_asc(env, l, a1, PSW_ASC_SECONDARY, a2, PSW_ASC_PRIMARY);
+    return mvc_asc(env, l, a1, MMU_SECONDARY_IDX, a2, MMU_PRIMARY_IDX, GETPC());
 }
 
 uint32_t HELPER(mvcp)(CPUS390XState *env, uint64_t l, uint64_t a1, uint64_t a2)
@@ -1078,7 +1064,7 @@ uint32_t HELPER(mvcp)(CPUS390XState *env, uint64_t l, uint64_t a1, uint64_t a2)
     HELPER_LOG("%s: %16" PRIx64 " %16" PRIx64 " %16" PRIx64 "\n",
                __func__, l, a1, a2);
 
-    return mvc_asc(env, l, a1, PSW_ASC_PRIMARY, a2, PSW_ASC_SECONDARY);
+    return mvc_asc(env, l, a1, MMU_PRIMARY_IDX, a2, MMU_SECONDARY_IDX, GETPC());
 }
 
 /* invalidate pte */
diff --git a/target-s390x/translate.c b/target-s390x/translate.c
index 2cfffc4..ac92b47 100644
--- a/target-s390x/translate.c
+++ b/target-s390x/translate.c
@@ -2793,7 +2793,6 @@ static ExitStatus op_mvcp(DisasContext *s, DisasOps *o)
 {
     int r1 = get_field(s->fields, l1);
     check_privileged(s);
-    potential_page_fault(s);
     gen_helper_mvcp(cc_op, cpu_env, regs[r1], o->addr1, o->in2);
     set_cc_static(s);
     return NO_EXIT;
@@ -2803,7 +2802,6 @@ static ExitStatus op_mvcs(DisasContext *s, DisasOps *o)
 {
     int r1 = get_field(s->fields, l1);
     check_privileged(s);
-    potential_page_fault(s);
     gen_helper_mvcs(cc_op, cpu_env, regs[r1], o->addr1, o->in2);
     set_cc_static(s);
     return NO_EXIT;
-- 
2.1.4

Re: [Qemu-devel] [PATCH RFC 3/5] softmmu: add a tlb_vaddr_to_host_fill function

[Aurelien Jarno]

On 2015-06-02 13:26, Aurelien Jarno wrote:
> The softmmu code already provides a tlb_vaddr_to_host function, which
> returns the host address corresponding to a guest virtual address,
> *if it is already in the QEMU MMU TLB*.
> 
> This patch is an attempt to have a function which try to fill the TLB
> entry if it is not already in the QEMU MMU TLB, possibly trigger a guest
> fault. It can be used directly in helpers. For that it creates a common
> function with a boolean to tell if the TLB needs to be filled or not. If
> yes, it causes tlb_fill, which might trigger an exception or succeed in
> which case the tlbentry pointer need to be reloaded.
> 
> I also had to change the MMIO test part. It seems that in write mode
> some TLB entries are filled with TLB_NOTDIRTY. They are caught by the
> MMIO test and a NULL pointer is returned instead. I am not sure of my
> change, but I guess the current softmmu code has the same issue.

It looks like we have to go through the MMIO functions to get the
TLB_NOTDIRTY bit cleaned correctly. This is something we don't want for
probe_write, so we definitely want two different functions.

-- 
Aurelien Jarno                          GPG: 4096R/1DDD8C9B
aurelien@aurel32.net                 http://www.aurel32.net

Re: [Qemu-devel] [PATCH RFC 3/5] softmmu: add a tlb_vaddr_to_host_fill function

[Richard Henderson]

On 06/02/2015 04:26 AM, Aurelien Jarno wrote:
>      int index = (addr >> TARGET_PAGE_BITS) & (CPU_TLB_SIZE - 1);
> -    CPUTLBEntry *tlbentry = &env->tlb_table[mmu_idx][index];
> +    CPUTLBEntry *tlbentry;
>      target_ulong tlb_addr;
>      uintptr_t haddr;
>  
> +again:
> +    tlbentry = &env->tlb_table[mmu_idx][index];
> +
>      switch (access_type) {
> -    case 0:
> +    case MMU_DATA_LOAD:
>          tlb_addr = tlbentry->addr_read;
>          break;
> -    case 1:
> +    case MMU_DATA_STORE:
>          tlb_addr = tlbentry->addr_write;
>          break;
> -    case 2:
> +    case MMU_INST_FETCH:
>          tlb_addr = tlbentry->addr_code;
>          break;
>      default:
> @@ -347,10 +350,14 @@ static inline void *tlb_vaddr_to_host(CPUArchState *env, target_ulong addr,
>      if ((addr & TARGET_PAGE_MASK)
>          != (tlb_addr & (TARGET_PAGE_MASK | TLB_INVALID_MASK))) {
>          /* TLB entry is for a different page */
> +        if (fill) {
> +            tlb_fill(ENV_GET_CPU(env), addr, access_type, mmu_idx, retaddr);
> +            goto again;
> +        }
>          return NULL;
>      }

To properly perform a fill, you also ought to check the victim cache.
There's a macro to do that in softmmu_template.h, which is why I
placed probe_write there.  It's not so convenient to use with a
variable type though.

In addition, the address of tlbentry cannot change, so there's no
point in recomputing that.  Indeed, you'd probably be better off
saving &addr_foo so that you only have to go through the switch once.

  switch (access_type) {
  case N:
    tlb_addr_ptr = &tlbentry->addr_foo;
    break;
  }
  tlb_addr = *tlb_addr_ptr;
  if (...) {
    if (!VICTIM_TLB_HIT(...)) {
      if (!fill) {
        return NULL;
      }
      tlb_fill(...);
    }
    tlb_addr = *tlb_addr_ptr;
  }

and thus there's no loop to be mis-predicted.


r~

Re: [Qemu-devel] [PATCH RFC 3/5] softmmu: add a tlb_vaddr_to_host_fill function

[Richard Henderson]

On 06/02/2015 01:10 PM, Aurelien Jarno wrote:
> It looks like we have to go through the MMIO functions to get the
> TLB_NOTDIRTY bit cleaned correctly. This is something we don't want for
> probe_write, so we definitely want two different functions.

I think that's why target-arm does it's somewhat convoluted loop in which it
stores one byte to the page and then tries again to use tlb_vaddr_to_host.

If the page isn't in the tlb, we perform a complete store and thus both pull
the page into the tlb as well as mark it dirty.  Thus if the page still isn't
present for the second vaddr_to_host, it really is I/O, or is being watched by
the debugger, or something equally unlikely.


r~

Re: [Qemu-devel] [PATCH RFC 3/5] softmmu: add a tlb_vaddr_to_host_fill function

[Peter Maydell]

On 2 June 2015 at 21:58, Richard Henderson <rth@twiddle.net> wrote:
> On 06/02/2015 01:10 PM, Aurelien Jarno wrote:
>> It looks like we have to go through the MMIO functions to get the
>> TLB_NOTDIRTY bit cleaned correctly. This is something we don't want for
>> probe_write, so we definitely want two different functions.
>
> I think that's why target-arm does it's somewhat convoluted loop in which it
> stores one byte to the page and then tries again to use tlb_vaddr_to_host.

Also if we take a fault we must do so with the fault address set
to the exact address passed in by the guest in the register,
even if that isn't the first (QEMU) page in the region being cleared.
So we must test that exact byte first.

-- PMM

Re: [Qemu-devel] [PATCH RFC 3/5] softmmu: add a tlb_vaddr_to_host_fill function

[Aurelien Jarno]

On 2015-06-02 13:54, Richard Henderson wrote:
> On 06/02/2015 04:26 AM, Aurelien Jarno wrote:
> >      int index = (addr >> TARGET_PAGE_BITS) & (CPU_TLB_SIZE - 1);
> > -    CPUTLBEntry *tlbentry = &env->tlb_table[mmu_idx][index];
> > +    CPUTLBEntry *tlbentry;
> >      target_ulong tlb_addr;
> >      uintptr_t haddr;
> >  
> > +again:
> > +    tlbentry = &env->tlb_table[mmu_idx][index];
> > +
> >      switch (access_type) {
> > -    case 0:
> > +    case MMU_DATA_LOAD:
> >          tlb_addr = tlbentry->addr_read;
> >          break;
> > -    case 1:
> > +    case MMU_DATA_STORE:
> >          tlb_addr = tlbentry->addr_write;
> >          break;
> > -    case 2:
> > +    case MMU_INST_FETCH:
> >          tlb_addr = tlbentry->addr_code;
> >          break;
> >      default:
> > @@ -347,10 +350,14 @@ static inline void *tlb_vaddr_to_host(CPUArchState *env, target_ulong addr,
> >      if ((addr & TARGET_PAGE_MASK)
> >          != (tlb_addr & (TARGET_PAGE_MASK | TLB_INVALID_MASK))) {
> >          /* TLB entry is for a different page */
> > +        if (fill) {
> > +            tlb_fill(ENV_GET_CPU(env), addr, access_type, mmu_idx, retaddr);
> > +            goto again;
> > +        }
> >          return NULL;
> >      }
> 
> To properly perform a fill, you also ought to check the victim cache.
> There's a macro to do that in softmmu_template.h, which is why I
> placed probe_write there.  It's not so convenient to use with a
> variable type though.
> 

Unfortunately that means we can't cleanly provide a probe_write function
doing nothing for the user-mode case. That would allow to avoid to many
#ifdef in the helper code. For me the softmmu_template.h is supposed to
only contain the code called by the helpers or by the glue in
cpu_ldst*.h

That also means the current tlb_vaddr_to_host code doesn't look in the
victim cache and that there is no easy way to fix that, though that's
less problematic.

-- 
Aurelien Jarno                          GPG: 4096R/1DDD8C9B
aurelien@aurel32.net                 http://www.aurel32.net

Re: [Qemu-devel] [PATCH RFC 3/5] softmmu: add a tlb_vaddr_to_host_fill function

[Aurelien Jarno]

On 2015-06-02 13:58, Richard Henderson wrote:
> On 06/02/2015 01:10 PM, Aurelien Jarno wrote:
> > It looks like we have to go through the MMIO functions to get the
> > TLB_NOTDIRTY bit cleaned correctly. This is something we don't want for
> > probe_write, so we definitely want two different functions.
> 
> I think that's why target-arm does it's somewhat convoluted loop in which it
> stores one byte to the page and then tries again to use tlb_vaddr_to_host.
> 
> If the page isn't in the tlb, we perform a complete store and thus both pull
> the page into the tlb as well as mark it dirty.  Thus if the page still isn't
> present for the second vaddr_to_host, it really is I/O, or is being watched by
> the debugger, or something equally unlikely.

Unfortunately it seems there is no way to guarantee that the full page
can be marked as dirty at the same time, even on s390x without MMIO.

I will try to rewrite the code to have a fallback code for the initial
TLB filling, that could also be used in the case the whole page can't be
marked as dirty. That's relatively easy when it deals with memset like
functions, but it becomes more tricky for memcpy or string functions.

-- 
Aurelien Jarno                          GPG: 4096R/1DDD8C9B
aurelien@aurel32.net                 http://www.aurel32.net