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

[David Hildenbrand]

> Am 02.03.2021 um 22:46 schrieb Richard Henderson 
> <richard.hender...@linaro.org>:
> 
> On 3/2/21 11:25 AM, David Hildenbrand wrote:
>>> On 02.03.21 20:12, Thomas Huth wrote:
>>> If the CCO bit is set, MVPG should not generate an exception
>>> but report page translation faults via a CC code, so we have
>>> to check the translation in this case before calling the
>>> access_prepare() function.
>>> 
>>> Signed-off-by: Thomas Huth <th...@redhat.com>
>>> ---
>>>   This patch is required to get Claudio's new kvm-unit-tests patches
>>>   working with TCG: https://www.spinics.net/lists/kvm/msg236784.html
>>> 
>>>   target/s390x/cpu.h         | 14 ++++++++++++++
>>>   target/s390x/excp_helper.c | 14 --------------
>>>   target/s390x/mem_helper.c  | 23 ++++++++++++++++++++++-
>>>   3 files changed, 36 insertions(+), 15 deletions(-)
>>> 
>>> diff --git a/target/s390x/cpu.h b/target/s390x/cpu.h
>>> index 60d434d5ed..731e2c6452 100644
>>> --- a/target/s390x/cpu.h
>>> +++ b/target/s390x/cpu.h
>>> @@ -366,6 +366,20 @@ static inline int cpu_mmu_index(CPUS390XState *env, 
>>> bool ifetch)
>>>   #endif
>>>   }
>>> +static inline uint64_t cpu_mmu_idx_to_asc(int mmu_idx)
>>> +{
>>> +    switch (mmu_idx) {
>>> +    case MMU_PRIMARY_IDX:
>>> +        return PSW_ASC_PRIMARY;
>>> +    case MMU_SECONDARY_IDX:
>>> +        return PSW_ASC_SECONDARY;
>>> +    case MMU_HOME_IDX:
>>> +        return PSW_ASC_HOME;
>>> +    default:
>>> +        abort();
>>> +    }
>>> +}
>>> +
>>>   static inline void cpu_get_tb_cpu_state(CPUS390XState* env, target_ulong 
>>> *pc,
>>>                                           target_ulong *cs_base, uint32_t 
>>> *flags)
>>>   {
>>> diff --git a/target/s390x/excp_helper.c b/target/s390x/excp_helper.c
>>> index ce16af394b..44bff27f8f 100644
>>> --- a/target/s390x/excp_helper.c
>>> +++ b/target/s390x/excp_helper.c
>>> @@ -105,20 +105,6 @@ bool s390_cpu_tlb_fill(CPUState *cs, vaddr address, 
>>> int size,
>>>   #else /* !CONFIG_USER_ONLY */
>>> -static inline uint64_t cpu_mmu_idx_to_asc(int mmu_idx)
>>> -{
>>> -    switch (mmu_idx) {
>>> -    case MMU_PRIMARY_IDX:
>>> -        return PSW_ASC_PRIMARY;
>>> -    case MMU_SECONDARY_IDX:
>>> -        return PSW_ASC_SECONDARY;
>>> -    case MMU_HOME_IDX:
>>> -        return PSW_ASC_HOME;
>>> -    default:
>>> -        abort();
>>> -    }
>>> -}
>>> -
>>>   bool s390_cpu_tlb_fill(CPUState *cs, vaddr address, int size,
>>>                          MMUAccessType access_type, int mmu_idx,
>>>                          bool probe, uintptr_t retaddr)
>>> diff --git a/target/s390x/mem_helper.c b/target/s390x/mem_helper.c
>>> index 25cfede806..c7037adf2c 100644
>>> --- a/target/s390x/mem_helper.c
>>> +++ b/target/s390x/mem_helper.c
>>> @@ -855,10 +855,31 @@ uint32_t HELPER(mvpg)(CPUS390XState *env, uint64_t 
>>> r0, uint64_t r1, uint64_t r2)
>>>       r1 = wrap_address(env, r1 & TARGET_PAGE_MASK);
>>>       r2 = wrap_address(env, r2 & TARGET_PAGE_MASK);
>>> +    /*
>>> +     * If the condition-code-option (CCO) bit is set and DAT is enabled,
>>> +     * we have to check for page table translation faults first:
>>> +     */
>>> +#ifndef CONFIG_USER_ONLY
>>> +    if (extract64(r0, 8, 1) && mmu_idx != MMU_REAL_IDX) {
>>> +        uint64_t asc = cpu_mmu_idx_to_asc(mmu_idx);
>>> +        uint64_t raddr, tec;
>>> +        int flags, exc;
>>> +
>>> +        exc = mmu_translate(env, r2, MMU_DATA_LOAD, asc, &raddr, &flags, 
>>> &tec);
>>> +        if (exc) {
>>> +            return 2;
>>> +        }
>>> +
>>> +        exc = mmu_translate(env, r1, MMU_DATA_STORE, asc, &raddr, &flags, 
>>> &tec);
>>> +        if (exc && exc != PGM_PROTECTION) {
>>> +            return 1;
>>> +        }
>>> +    }
>>> +#endif
>>> +
>> This way you always need two additional translations and don't even check if 
>> we have something in the TLB. While this works, it's quite inefficient.
>> Using probe_access_flags() we can actually lookup the tlb/fill the tlb but 
>> get an error instead of a fault. We could e.g., extent probe_access() to 
>> allow specifying whether we want a fault or not.
> 
> I think probe_access_flags() will do all that you need; no further extension 
> to probe_access() required.  I presume you meant access_prepare() is what you 
> meant to extend?

I was worrying about watchpoint handling etc. as done in probe_access().

But I think what you mean is we can simply do two probe_access_flags() to catch 
these special pgm interrupt, followed by existing access_prepare(). That should 
work I guess.