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

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

[Mikhail Tyutin]

The fix is to clear TLB_INVALID_MASK bit in tlb_addr, as it happens in other places e.g. load_helper().

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

diff --git a/accel/tcg/cputlb.c b/accel/tcg/cputlb.c
index ba44501a7c..900dfc1079 100644
--- a/accel/tcg/cputlb.c
+++ b/accel/tcg/cputlb.c
@@ -1735,7 +1735,7 @@ bool tlb_plugin_lookup(CPUState *cpu, vaddr addr, int mmu_idx,
      uintptr_t index = tlb_index(env, mmu_idx, addr);
      uint64_t tlb_addr = is_store ? tlb_addr_write(tlbe) : tlbe->addr_read;
  
-    if (likely(tlb_hit(tlb_addr, addr))) {
+    if (likely(tlb_hit(tlb_addr & ~TLB_INVALID_MASK, addr))) {
          /* We must have an iotlb entry for MMIO */
          if (tlb_addr & TLB_MMIO) {
              CPUTLBEntryFull *full;
-- 
2.34.1

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

[Richard Henderson]

On 8/2/23 06:08, Mikhail Tyutin wrote:
> The fix is to clear TLB_INVALID_MASK bit in tlb_addr, as it happens in other places e.g. 
> load_helper().
> 
> Signed-off-by: Dmitriy Solovev <d.solovev@yadro.com>
> Signed-off-by: Mikhail Tyutin <m.tyutin@yadro.com>
> ---
>   accel/tcg/cputlb.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)

The other places in load_helper happen only directly after tlb_fill has succeeded.  Here 
you have no such guarantee.

I think perhaps the save_iotlb_data() call should be applied to loads as well, and then 
tlb_plugin_lookup simplified.


r~

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

[Mikhail Tyutin]

> On 8/2/23 06:08, Mikhail Tyutin wrote:
> > The fix is to clear TLB_INVALID_MASK bit in tlb_addr, as it happens in other places e.g.
> > load_helper().
> >
> > Signed-off-by: Dmitriy Solovev <d.solovev@yadro.com>
> > Signed-off-by: Mikhail Tyutin <m.tyutin@yadro.com>
> > ---
> >   accel/tcg/cputlb.c | 2 +-
> >   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> The other places in load_helper happen only directly after tlb_fill has succeeded.  Here
> you have no such guarantee.
> 
> I think perhaps the save_iotlb_data() call should be applied to loads as well, and then
> tlb_plugin_lookup simplified.
> 

Hello Richard,

We performed testing on more scenarios and noticed that patch when save_iotlb_data() call is added to io_readx
(https://patchew.org/QEMU/20230804110903.19968-1-m.tyutin@yadro.com/). It doesn't work for addresses
in OCRAM region. Those accessed bypass io_writex/io_readx function and therefore don’t invoke save_iotlb_data().
So we observe the wrong value of cpu->saved_iotlb for it.

Would not be better to get back to initial v1 approach when we clean TLB_INVALID_MASK flag in
tlb_plugin_lookup()? It works well for those regions.
(https://patchew.org/QEMU/bf8ae2fd-158a-57b6-6270-2e56b6506421@yadro.com)

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

[Richard Henderson]

On 8/9/23 06:17, Mikhail Tyutin wrote:
> Would not be better to get back to initial v1 approach when we clean TLB_INVALID_MASK flag in
> tlb_plugin_lookup()? It works well for those regions.

You're just as likely to get invalid data.


r~

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

[Mikhail Tyutin]

The fix is to clear TLB_INVALID_MASK bit in tlb_addr, as it happens in 
other places e.g. load_helper().

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

diff --git a/accel/tcg/cputlb.c b/accel/tcg/cputlb.c
index 5b51eff5a4..9045b6330a 100644
--- a/accel/tcg/cputlb.c
+++ b/accel/tcg/cputlb.c
@@ -1708,7 +1708,7 @@ bool tlb_plugin_lookup(CPUState *cpu, vaddr addr, 
int mmu_idx,
      uintptr_t index = tlb_index(env, mmu_idx, addr);
      uint64_t tlb_addr = is_store ? tlb_addr_write(tlbe) : tlbe->addr_read;
  -    if (likely(tlb_hit(tlb_addr, addr))) {
+    if (likely(tlb_hit(tlb_addr & ~TLB_INVALID_MASK, addr))) {
          /* We must have an iotlb entry for MMIO */
          if (tlb_addr & TLB_MMIO) {
              CPUTLBEntryFull *full;
-- 
2.34.1