Re: [PATCH v3 7/7] s390x/mmu: Convert to non-recursive page table walk

[David Hildenbrand <david@redhat.com>]

On 01.10.19 10:23, Thomas Huth wrote:
> On 01/10/2019 10.17, David Hildenbrand wrote:
>>
>>>>          break;
>>>>      case ASCE_TYPE_SEGMENT:
>>>>          if (VADDR_REGION1_TX(vaddr) || VADDR_REGION2_TX(vaddr) ||
>>>> @@ -253,11 +164,112 @@ static int mmu_translate_asce(CPUS390XState *env, target_ulong vaddr,
>>>>          if (VADDR_SEGMENT_TL(vaddr) > asce_tl) {
>>>>              return PGM_SEGMENT_TRANS;
>>>>          }
>>>> +        gaddr += VADDR_SEGMENT_TX(vaddr) * 8;
>>>> +        break;
>>>> +    default:
>>>> +        g_assert_not_reached();
>>>
>>> As far as I can see, all four cases are handled above, so this default
>>> case should really not be necessary here.
>>
>> Yes, can drop.
>>
>>>
>>>> +    }
>>>> +
>>>> +    switch (asce & ASCE_TYPE_MASK) {
>>>> +    case ASCE_TYPE_REGION1:
>>>> +        if (!read_table_entry(env, gaddr, &entry)) {
>>>> +            return PGM_ADDRESSING;
>>>> +        }
>>>> +        if (entry & REGION_ENTRY_I) {
>>>> +            return PGM_REG_FIRST_TRANS;
>>>> +        }
>>>> +        if ((entry & REGION_ENTRY_TT) != REGION_ENTRY_TT_REGION1) {
>>>> +            return PGM_TRANS_SPEC;
>>>> +        }
>>>> +        if (VADDR_REGION2_TL(vaddr) < (entry & REGION_ENTRY_TF) >> 6 ||
>>>> +            VADDR_REGION2_TL(vaddr) > (entry & REGION_ENTRY_TL)) {
>>>> +            return PGM_REG_SEC_TRANS;
>>>> +        }
>>>> +        if (edat1 && (entry & REGION_ENTRY_P)) {
>>>> +            *flags &= ~PAGE_WRITE;
>>>> +        }
>>>> +        gaddr = (entry & REGION_ENTRY_ORIGIN) + VADDR_REGION2_TX(vaddr) * 8;
>>>> +        /* fall through */
>>>> +    case ASCE_TYPE_REGION2:
>>>> +        if (!read_table_entry(env, gaddr, &entry)) {
>>>> +            return PGM_ADDRESSING;
>>>> +        }
>>>> +        if (entry & REGION_ENTRY_I) {
>>>> +            return PGM_REG_SEC_TRANS;
>>>> +        }
>>>> +        if ((entry & REGION_ENTRY_TT) != REGION_ENTRY_TT_REGION2) {
>>>> +            return PGM_TRANS_SPEC;
>>>> +        }
>>>> +        if (VADDR_REGION3_TL(vaddr) < (entry & REGION_ENTRY_TF) >> 6 ||
>>>> +            VADDR_REGION3_TL(vaddr) > (entry & REGION_ENTRY_TL)) {
>>>> +            return PGM_REG_THIRD_TRANS;
>>>> +        }
>>>> +        if (edat1 && (entry & REGION_ENTRY_P)) {
>>>> +            *flags &= ~PAGE_WRITE;
>>>> +        }
>>>> +        gaddr = (entry & REGION_ENTRY_ORIGIN) + VADDR_REGION3_TX(vaddr) * 8;
>>>> +        /* fall through */
>>>> +    case ASCE_TYPE_REGION3:
>>>> +        if (!read_table_entry(env, gaddr, &entry)) {
>>>> +            return PGM_ADDRESSING;
>>>> +        }
>>>> +        if (entry & REGION_ENTRY_I) {
>>>> +            return PGM_REG_THIRD_TRANS;
>>>> +        }
>>>> +        if ((entry & REGION_ENTRY_TT) != REGION_ENTRY_TT_REGION3) {
>>>> +            return PGM_TRANS_SPEC;
>>>> +        }
>>>> +        if (edat1 && (entry & REGION_ENTRY_P)) {
>>>> +            *flags &= ~PAGE_WRITE;
>>>> +        }
>>>
>>> Shouldn't that check be done below the next if-statement?
>>
>> Does it matter? The flags are irrelevant in case we return an exception,
>> so the order shouldn't matter.
> 
> Hmm, it likely does not matter, but you've got it the other way round in
> all other cases, so I'd vote for doing it here this way, too, for
> consistency.

Oh, in this case, sure! Thanks!

> 
>  Thomas
> 


-- 

Thanks,

David / dhildenb