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

[Thomas Huth <thuth@redhat.com>]

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.

 Thomas