| Message ID | bf8ae2fd-158a-57b6-6270-2e56b6506421@yadro.com |
|---|---|
| State | New |
| Headers | show |
| Series | Fix SEGFAULT on getting physical address of MMIO region. | expand |
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~
> 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)
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~
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;