Re: [PATCH v5 1/2] target/s390x: Implement the MVPG condition-code-option bit

[Richard Henderson]

On 3/11/21 12:16 PM, David Hildenbrand wrote:
> On 11.03.21 18:52, Richard Henderson wrote:
> > On 3/11/21 10:17 AM, David Hildenbrand wrote:
> > > +#if !defined(CONFIG_USER_ONLY)
> > > +    /*
> > > +     * 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;
> > > +#endif
> > > +    flags = probe_access_flags(env, vaddr1, access_type, mmu_idx,
> > > +                               nofault, &haddr1, ra);
> > > +#if !defined(CONFIG_USER_ONLY)
> > > +    if (env->tlb_fill_exc) {
> > > +        return env->tlb_fill_exc;
> > > +    }
> > > +#else
> > > +    if (!haddr1) {
> > > +        env->__excp_addr = vaddr1;
> > > +        return PGM_ADDRESSING;
> > > +    }
> > > +#endif
> >
> > The assumption of PGM_ADDRESSING is incorrect here -- it could still be
> > PGM_PROTECTION, depending on how the page is mapped.
>
> Interesting, I was only looking at the s390x tlb_fill() implementation. But I 
> assume these checks are performed in common code.

Actually, no.  It's a common bug in our linux-user targets, where we don't fill 
in the SIGSEGV si_code correctly.

See e.g. 8db94ab4e5d.

> Thanks, maybe I can factor that out in a nice way. I guess we could do the 
> access via probe_access_flags() and only on error do the page_get_flags()?

Yes, we could do that.  It's certainly better for !nofault, which is the 
common-case user of this function.


r~