[PATCH v3] Fix SEGFAULT on getting physical address of MMIO region.

[PATCH v3] Fix SEGFAULT on getting physical address of MMIO region.

[Mikhail Tyutin]

Apply save_iotlb_data() to io_readx() as well as to io_writex(). This fixes
SEGFAULT on qemu_plugin_hwaddr_phys_addr() call plugins for addresses inside of
MMIO region.

Signed-off-by: Dmitriy Solovev <d.solovev@yadro.com>
Signed-off-by: Mikhail Tyutin <m.tyutin@yadro.com>
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
---
v3:
- commit wording update


 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

Re: [PATCH v3] Fix SEGFAULT on getting physical address of MMIO region.

[Richard Henderson]

On 8/4/23 04:09, Mikhail Tyutin wrote:
> Apply save_iotlb_data() to io_readx() as well as to io_writex(). This fixes
> SEGFAULT on qemu_plugin_hwaddr_phys_addr() call plugins for addresses inside of
> MMIO region.
> 
> Signed-off-by: Dmitriy Solovev<d.solovev@yadro.com>
> Signed-off-by: Mikhail Tyutin<m.tyutin@yadro.com>
> Reviewed-by: Richard Henderson<richard.henderson@linaro.org>
> ---
> v3:
> - commit wording update
> 
> 
>   accel/tcg/cputlb.c | 36 +++++++++++++++++++++---------------
>   1 file changed, 21 insertions(+), 15 deletions(-)

Queued to tcg-next.


r~

Re: [PATCH v3] Fix SEGFAULT on getting physical address of MMIO region.

[Jonathan Cameron via]

On Fri, 4 Aug 2023 09:15:13 -0700
Richard Henderson <richard.henderson@linaro.org> wrote:

> On 8/4/23 04:09, Mikhail Tyutin wrote:
> > Apply save_iotlb_data() to io_readx() as well as to io_writex(). This fixes
> > SEGFAULT on qemu_plugin_hwaddr_phys_addr() call plugins for addresses inside of
> > MMIO region.
> > 
> > Signed-off-by: Dmitriy Solovev<d.solovev@yadro.com>
> > Signed-off-by: Mikhail Tyutin<m.tyutin@yadro.com>
> > Reviewed-by: Richard Henderson<richard.henderson@linaro.org>
> > ---
> > v3:
> > - commit wording update
> > 
> > 
> >   accel/tcg/cputlb.c | 36 +++++++++++++++++++++---------------
> >   1 file changed, 21 insertions(+), 15 deletions(-)  
> 
> Queued to tcg-next.

I'm not 100% sure it's related but with qemu/master as of a few hours ago
I'm getting a segfault with backtrace (can't copy from relevant machine)
indicating that
io_writex () is calling memory_region_dispatch_write() and getting
a segfault on

if (mr->alias)

mr has very suspect content of 0x13001300130013

The segfault is intermittent (otherwise I get hang later in boot) and
I can only get it to trigger with multiple cpus.

x86 max cpu and q35 machine.

Jonathan


> 
> 
> r~
> 

Re: [PATCH v3] Fix SEGFAULT on getting physical address of MMIO region.

[Jonathan Cameron via]

On Thu, 10 Aug 2023 14:18:50 +0100
Jonathan Cameron via <qemu-devel@nongnu.org> wrote:

> On Fri, 4 Aug 2023 09:15:13 -0700
> Richard Henderson <richard.henderson@linaro.org> wrote:
> 
> > On 8/4/23 04:09, Mikhail Tyutin wrote:  
> > > Apply save_iotlb_data() to io_readx() as well as to io_writex(). This fixes
> > > SEGFAULT on qemu_plugin_hwaddr_phys_addr() call plugins for addresses inside of
> > > MMIO region.
> > > 
> > > Signed-off-by: Dmitriy Solovev<d.solovev@yadro.com>
> > > Signed-off-by: Mikhail Tyutin<m.tyutin@yadro.com>
> > > Reviewed-by: Richard Henderson<richard.henderson@linaro.org>
> > > ---
> > > v3:
> > > - commit wording update
> > > 
> > > 
> > >   accel/tcg/cputlb.c | 36 +++++++++++++++++++++---------------
> > >   1 file changed, 21 insertions(+), 15 deletions(-)    
> > 
> > Queued to tcg-next.  
> 
> I'm not 100% sure it's related but with qemu/master as of a few hours ago
> I'm getting a segfault with backtrace (can't copy from relevant machine)
> indicating that
> io_writex () is calling memory_region_dispatch_write() and getting
> a segfault on
> 
> if (mr->alias)
> 
> mr has very suspect content of 0x13001300130013
> 
> The segfault is intermittent (otherwise I get hang later in boot) and
> I can only get it to trigger with multiple cpus.
> 
> x86 max cpu and q35 machine.
Resolved by
https://gitlab.com/rth7680/qemu/-/commit/78e096b365326dd2b287e914b393d914dbae3b66
softmmu: Use async_run_on_cpu in tcg_commit

Thanks Richard.



> 
> Jonathan
> 
> 
> > 
> > 
> > r~
> >   
> 
> 
>