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

[David Hildenbrand]

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.

> I guess this should be done like
>
> #ifdef CONFIG_USER_ONLY
>       flags = page_get_flags(vaddr1);
>       if (!flags & (access_type == MMU_DATA_LOAD
>                     ? PAGE_READ : PAGE_WRITE)) {
>           env->__excp_addr = vaddr1;
>           if (nofault) {
>               return (flags & PAGE_VALID
>                   ? PGM_PROTECTION : PGM_ADDRESSING);
>           }
>           raise exception.
>       }
>       haddr1 = g2h(vaddr1);
> #else
>       env->tlb_fill_exc = 0;
>       flags = probe_access_flags(...);
>       if (env->tlb_fill_exc) {
>           return env->tlb_fill_exc;
>       }
> #endif
>
> which is pretty ugly, but no worse than what you have above.

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()?


--
Thanks,

David / dhildenb