| Message ID | 20220822235803.1729290-4-richard.henderson@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | target/i386: Use atomic operations for pte updates | expand |
On 23.08.22 01:57, Richard Henderson wrote: > When PAGE_WRITE_INV is set when calling tlb_set_page, > we immediately set TLB_INVALID_MASK in order to force > tlb_fill to be called on the next lookup. Here in > probe_access_internal, we have just called tlb_fill > and eliminated true misses, thus the lookup must be valid. > > This allows us to remove a warning comment from s390x. > There doesn't seem to be a reason to change the code though. > > Cc: David Hildenbrand <david@redhat.com> > Signed-off-by: Richard Henderson <richard.henderson@linaro.org> > --- > accel/tcg/cputlb.c | 10 +++++++++- > target/s390x/tcg/mem_helper.c | 4 ---- > 2 files changed, 9 insertions(+), 5 deletions(-) > > diff --git a/accel/tcg/cputlb.c b/accel/tcg/cputlb.c > index 1509df96b4..5359113e8d 100644 > --- a/accel/tcg/cputlb.c > +++ b/accel/tcg/cputlb.c > @@ -1602,6 +1602,7 @@ static int probe_access_internal(CPUArchState *env, target_ulong addr, > } > tlb_addr = tlb_read_ofs(entry, elt_ofs); > > + flags = TLB_FLAGS_MASK; > page_addr = addr & TARGET_PAGE_MASK; > if (!tlb_hit_page(tlb_addr, page_addr)) { > if (!victim_tlb_hit(env, mmu_idx, index, elt_ofs, page_addr)) { > @@ -1617,10 +1618,17 @@ static int probe_access_internal(CPUArchState *env, target_ulong addr, > > /* TLB resize via tlb_fill may have moved the entry. */ > entry = tlb_entry(env, mmu_idx, addr); > + > + /* > + * With PAGE_WRITE_INV, we set TLB_INVALID_MASK immediately, > + * to force the next access through tlb_fill. We've just > + * called tlb_fill, so we know that this entry *is* valid. > + */ > + flags &= ~TLB_INVALID_MASK; > } > tlb_addr = tlb_read_ofs(entry, elt_ofs); > } > - flags = tlb_addr & TLB_FLAGS_MASK; > + flags &= tlb_addr; > > /* Fold all "mmio-like" bits into TLB_MMIO. This is not RAM. */ > if (unlikely(flags & ~(TLB_WATCHPOINT | TLB_NOTDIRTY))) { > diff --git a/target/s390x/tcg/mem_helper.c b/target/s390x/tcg/mem_helper.c > index fc52aa128b..3758b9e688 100644 > --- a/target/s390x/tcg/mem_helper.c > +++ b/target/s390x/tcg/mem_helper.c > @@ -148,10 +148,6 @@ static int s390_probe_access(CPUArchState *env, target_ulong addr, int size, > #else > int flags; > > - /* > - * For !CONFIG_USER_ONLY, we cannot rely on TLB_INVALID_MASK or haddr==NULL > - * to detect if there was an exception during tlb_fill(). > - */ Yeah, that was primarily only a comment that we rely on tlb_fill_exc to obtain the exact PGM_* value -- and at the same time use it to detect if there was an exception at all. > env->tlb_fill_exc = 0; > flags = probe_access_flags(env, addr, access_type, mmu_idx, nonfault, phost, > ra); Change itself looks good to me. However, it's been a while since I stared at this code, but I wonder how the CONFIG_USER_ONLY path is correct. 1) s390_probe_access() documents to "With nonfault=1, return the PGM_ exception that would have been injected into the guest; return 0 if no exception was detected." But in case of CONFIG_USER_ONLY, we return the flags returned by s390_probe_access(), not a PGM__* value. Maybe it doesn't matter, because we'll simply inject a SIGSEGV in any case ... I'd have assume that we have to check here if there was an exception in a similar way, and detect the actual type. As the old comment indicates, we can either * check for *phost == NULL * flags having TLB_INVALID_MASK set ... and I wonder if we really care about the exception type for CONFIG_USER_ONLY at all. 2) s390_probe_access() documents that for "CONFIG_USER_ONLY, the faulting address is stored to env->__excp_addr.". However, that's only set in s390_cpu_record_sigsegv(). With nonfault=1 that will never actually trigger, right? I assume db9aab5783a2 ("target/s390x: Use probe_access_flags in s390_probe_access") might have introduced both. We had a flag conversion to PGM_ in there and stored env->__excp_addr: flags = page_get_flags(addr); if (!(flags & (access_type == MMU_DATA_LOAD ? PAGE_READ : PAGE_WRITE_ORG))) { env->__excp_addr = addr; flags = (flags & PAGE_VALID) ? PGM_PROTECTION : PGM_ADDRESSING; if (nonfault) { return flags; }
On 8/23/22 02:19, David Hildenbrand wrote: > 1) s390_probe_access() documents to "With nonfault=1, return the PGM_ > exception that would have been injected into the guest; return 0 if no > exception was detected." > > But in case of CONFIG_USER_ONLY, we return the flags returned by > s390_probe_access(), not a PGM__* value. Maybe it doesn't matter, > because we'll simply inject a SIGSEGV in any case ... I would have said it would matter for MVPG, except that is incorrectly *not* marked as a privileged instruction. There should be no CONFIG_USER_ONLY case to answer there. > 2) s390_probe_access() documents that for "CONFIG_USER_ONLY, the > faulting address is stored to env->__excp_addr.". > > However, that's only set in s390_cpu_record_sigsegv(). With nonfault=1 > that will never actually trigger, right? Correct. > I assume db9aab5783a2 ("target/s390x: Use probe_access_flags in > s390_probe_access") might have introduced both. We had a flag conversion > to PGM_ in there and stored env->__excp_addr: Indeed, that commit is faulty in that it breaks the contract of s390_probe_access. It's a shame, though, that we need to carry the extra code for the purpose, and that the generic interfaces are not sufficient. r~
On 8/23/22 08:19, Richard Henderson wrote: > On 8/23/22 02:19, David Hildenbrand wrote: >> 1) s390_probe_access() documents to "With nonfault=1, return the PGM_ >> exception that would have been injected into the guest; return 0 if no >> exception was detected." >> >> But in case of CONFIG_USER_ONLY, we return the flags returned by >> s390_probe_access(), not a PGM__* value. Maybe it doesn't matter, >> because we'll simply inject a SIGSEGV in any case ... > > I would have said it would matter for MVPG, except that is incorrectly *not* marked as a > privileged instruction. There should be no CONFIG_USER_ONLY case to answer there. Ho hum, misread that -- only privileged if access key specified (and we do not check or support those). r~
diff --git a/accel/tcg/cputlb.c b/accel/tcg/cputlb.c index 1509df96b4..5359113e8d 100644 --- a/accel/tcg/cputlb.c +++ b/accel/tcg/cputlb.c @@ -1602,6 +1602,7 @@ static int probe_access_internal(CPUArchState *env, target_ulong addr, } tlb_addr = tlb_read_ofs(entry, elt_ofs); + flags = TLB_FLAGS_MASK; page_addr = addr & TARGET_PAGE_MASK; if (!tlb_hit_page(tlb_addr, page_addr)) { if (!victim_tlb_hit(env, mmu_idx, index, elt_ofs, page_addr)) { @@ -1617,10 +1618,17 @@ static int probe_access_internal(CPUArchState *env, target_ulong addr, /* TLB resize via tlb_fill may have moved the entry. */ entry = tlb_entry(env, mmu_idx, addr); + + /* + * With PAGE_WRITE_INV, we set TLB_INVALID_MASK immediately, + * to force the next access through tlb_fill. We've just + * called tlb_fill, so we know that this entry *is* valid. + */ + flags &= ~TLB_INVALID_MASK; } tlb_addr = tlb_read_ofs(entry, elt_ofs); } - flags = tlb_addr & TLB_FLAGS_MASK; + flags &= tlb_addr; /* Fold all "mmio-like" bits into TLB_MMIO. This is not RAM. */ if (unlikely(flags & ~(TLB_WATCHPOINT | TLB_NOTDIRTY))) { diff --git a/target/s390x/tcg/mem_helper.c b/target/s390x/tcg/mem_helper.c index fc52aa128b..3758b9e688 100644 --- a/target/s390x/tcg/mem_helper.c +++ b/target/s390x/tcg/mem_helper.c @@ -148,10 +148,6 @@ static int s390_probe_access(CPUArchState *env, target_ulong addr, int size, #else int flags; - /* - * For !CONFIG_USER_ONLY, we cannot rely on TLB_INVALID_MASK or haddr==NULL - * to detect if there was an exception during tlb_fill(). - */ env->tlb_fill_exc = 0; flags = probe_access_flags(env, addr, access_type, mmu_idx, nonfault, phost, ra);
When PAGE_WRITE_INV is set when calling tlb_set_page, we immediately set TLB_INVALID_MASK in order to force tlb_fill to be called on the next lookup. Here in probe_access_internal, we have just called tlb_fill and eliminated true misses, thus the lookup must be valid. This allows us to remove a warning comment from s390x. There doesn't seem to be a reason to change the code though. Cc: David Hildenbrand <david@redhat.com> Signed-off-by: Richard Henderson <richard.henderson@linaro.org> --- accel/tcg/cputlb.c | 10 +++++++++- target/s390x/tcg/mem_helper.c | 4 ---- 2 files changed, 9 insertions(+), 5 deletions(-)