Re: [PATCH 03/14] accel/tcg: Suppress auto-invalidate in probe_access_internal

[David Hildenbrand]

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;
    }



-- 
Thanks,

David / dhildenb