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

[David Hildenbrand <david@redhat.com>]

>>          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.

> 
>> +        if (VADDR_SEGMENT_TL(vaddr) < (entry & REGION_ENTRY_TF) >> 6 ||
>> +            VADDR_SEGMENT_TL(vaddr) > (entry & REGION_ENTRY_TL)) {
>> +            return PGM_SEGMENT_TRANS;
>> +        }
>> +        gaddr = (entry & REGION_ENTRY_ORIGIN) + VADDR_SEGMENT_TX(vaddr) * 8;
>> +        /* fall through */
>> +    case ASCE_TYPE_SEGMENT:
>> +        if (!read_table_entry(env, gaddr, &entry)) {
>> +            return PGM_ADDRESSING;
>> +        }
>> +        if (entry & SEGMENT_ENTRY_I) {
>> +            return PGM_SEGMENT_TRANS;
>> +        }
>> +        if ((entry & SEGMENT_ENTRY_TT) != SEGMENT_ENTRY_TT_SEGMENT) {
>> +            return PGM_TRANS_SPEC;
>> +        }
>> +        if ((entry & SEGMENT_ENTRY_CS) && asce_p) {
>> +            return PGM_TRANS_SPEC;
>> +        }
>> +        if (entry & SEGMENT_ENTRY_P) {
>> +            *flags &= ~PAGE_WRITE;
>> +        }
>> +        if (edat1 && (entry & SEGMENT_ENTRY_FC)) {
>> +            *raddr = (entry & SEGMENT_ENTRY_SFAA) |
>> +                     (vaddr & ~SEGMENT_ENTRY_SFAA);
>> +            return 0;
>> +        }
>> +        gaddr = (entry & SEGMENT_ENTRY_ORIGIN) + VADDR_PAGE_TX(vaddr) * 8;
>>          break;
>> +    default:
>> +        g_assert_not_reached();
> 
> That default case could be dropped, too.

Yes, can do.

Thanks!


-- 

Thanks,

David / dhildenb