Apply save_iotlb_data() to io_readx() as well as to io_writex().

Signed-off-by: Dmitriy Solovev <d.solovev@yadro.com>
Signed-off-by: Mikhail Tyutin <m.tyutin@yadro.com>
---
 accel/tcg/cputlb.c | 36 +++++++++++++++++++++---------------
 1 file changed, 21 insertions(+), 15 deletions(-)

diff --git a/accel/tcg/cputlb.c b/accel/tcg/cputlb.c
index ba44501a7c..addce3be38 100644
--- a/accel/tcg/cputlb.c
+++ b/accel/tcg/cputlb.c
@@ -1363,6 +1363,21 @@ static inline void cpu_transaction_failed(CPUState *cpu, hwaddr physaddr,
     }
 }
 
+/*
+ * Save a potentially trashed CPUTLBEntryFull for later lookup by plugin.
+ * This is read by tlb_plugin_lookup if the fulltlb entry doesn't match
+ * because of the side effect of io_writex changing memory layout.
+ */
+static void save_iotlb_data(CPUState *cs, MemoryRegionSection *section,
+                            hwaddr mr_offset)
+{
+#ifdef CONFIG_PLUGIN
+    SavedIOTLB *saved = &cs->saved_iotlb;
+    saved->section = section;
+    saved->mr_offset = mr_offset;
+#endif
+}
+
 static uint64_t io_readx(CPUArchState *env, CPUTLBEntryFull *full,
                          int mmu_idx, vaddr addr, uintptr_t retaddr,
                          MMUAccessType access_type, MemOp op)
@@ -1382,6 +1397,12 @@ static uint64_t io_readx(CPUArchState *env, CPUTLBEntryFull *full,
         cpu_io_recompile(cpu, retaddr);
     }
 
+    /*
+     * The memory_region_dispatch may trigger a flush/resize
+     * so for plugins we save the iotlb_data just in case.
+     */
+    save_iotlb_data(cpu, section, mr_offset);
+
     {
         QEMU_IOTHREAD_LOCK_GUARD();
         r = memory_region_dispatch_read(mr, mr_offset, &val, op, full->attrs);
@@ -1398,21 +1419,6 @@ static uint64_t io_readx(CPUArchState *env, CPUTLBEntryFull *full,
     return val;
 }
 
-/*
- * Save a potentially trashed CPUTLBEntryFull for later lookup by plugin.
- * This is read by tlb_plugin_lookup if the fulltlb entry doesn't match
- * because of the side effect of io_writex changing memory layout.
- */
-static void save_iotlb_data(CPUState *cs, MemoryRegionSection *section,
-                            hwaddr mr_offset)
-{
-#ifdef CONFIG_PLUGIN
-    SavedIOTLB *saved = &cs->saved_iotlb;
-    saved->section = section;
-    saved->mr_offset = mr_offset;
-#endif
-}
-
 static void io_writex(CPUArchState *env, CPUTLBEntryFull *full,
                       int mmu_idx, uint64_t val, vaddr addr,
                       uintptr_t retaddr, MemOp op)
-- 
2.34.1

On Thu, 3 Aug 2023 at 12:00, Mikhail Tyutin <m.tyutin@yadro.com> wrote:
>
> Apply save_iotlb_data() to io_readx() as well as to io_writex().

Could we have a bit more detail in the commit message about
when you can get this segfault?

In particular, does this happen only if you're using plugins?

thanks
-- PMM

> > Apply save_iotlb_data() to io_readx() as well as to io_writex().
> 
> Could we have a bit more detail in the commit message about
> when you can get this segfault?
> 
> In particular, does this happen only if you're using plugins?
> 

I think so. It crashes on specific addresses when plugin calls qemu_plugin_hwaddr_phys_addr(). I haven't seen this crash without a plugin.

On 8/3/23 03:58, Mikhail Tyutin wrote:
> Apply save_iotlb_data() to io_readx() as well as to io_writex().
> 
> Signed-off-by: Dmitriy Solovev <d.solovev@yadro.com>
> Signed-off-by: Mikhail Tyutin <m.tyutin@yadro.com>
> ---
>   accel/tcg/cputlb.c | 36 +++++++++++++++++++++---------------
>   1 file changed, 21 insertions(+), 15 deletions(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>


r~

> 
> diff --git a/accel/tcg/cputlb.c b/accel/tcg/cputlb.c
> index ba44501a7c..addce3be38 100644
> --- a/accel/tcg/cputlb.c
> +++ b/accel/tcg/cputlb.c
> @@ -1363,6 +1363,21 @@ static inline void cpu_transaction_failed(CPUState *cpu, hwaddr physaddr,
>       }
>   }
>   
> +/*
> + * Save a potentially trashed CPUTLBEntryFull for later lookup by plugin.
> + * This is read by tlb_plugin_lookup if the fulltlb entry doesn't match
> + * because of the side effect of io_writex changing memory layout.
> + */
> +static void save_iotlb_data(CPUState *cs, MemoryRegionSection *section,
> +                            hwaddr mr_offset)
> +{
> +#ifdef CONFIG_PLUGIN
> +    SavedIOTLB *saved = &cs->saved_iotlb;
> +    saved->section = section;
> +    saved->mr_offset = mr_offset;
> +#endif
> +}
> +
>   static uint64_t io_readx(CPUArchState *env, CPUTLBEntryFull *full,
>                            int mmu_idx, vaddr addr, uintptr_t retaddr,
>                            MMUAccessType access_type, MemOp op)
> @@ -1382,6 +1397,12 @@ static uint64_t io_readx(CPUArchState *env, CPUTLBEntryFull *full,
>           cpu_io_recompile(cpu, retaddr);
>       }
>   
> +    /*
> +     * The memory_region_dispatch may trigger a flush/resize
> +     * so for plugins we save the iotlb_data just in case.
> +     */
> +    save_iotlb_data(cpu, section, mr_offset);
> +
>       {
>           QEMU_IOTHREAD_LOCK_GUARD();
>           r = memory_region_dispatch_read(mr, mr_offset, &val, op, full->attrs);
> @@ -1398,21 +1419,6 @@ static uint64_t io_readx(CPUArchState *env, CPUTLBEntryFull *full,
>       return val;
>   }
>   
> -/*
> - * Save a potentially trashed CPUTLBEntryFull for later lookup by plugin.
> - * This is read by tlb_plugin_lookup if the fulltlb entry doesn't match
> - * because of the side effect of io_writex changing memory layout.
> - */
> -static void save_iotlb_data(CPUState *cs, MemoryRegionSection *section,
> -                            hwaddr mr_offset)
> -{
> -#ifdef CONFIG_PLUGIN
> -    SavedIOTLB *saved = &cs->saved_iotlb;
> -    saved->section = section;
> -    saved->mr_offset = mr_offset;
> -#endif
> -}
> -
>   static void io_writex(CPUArchState *env, CPUTLBEntryFull *full,
>                         int mmu_idx, uint64_t val, vaddr addr,
>                         uintptr_t retaddr, MemOp op)