Re: [Qemu-devel] [qemu-s390x] [PATCH-for-4.2 v1 3/9] s390x/mmu: DAT translation rewrite

[David Hildenbrand <david@redhat.com>]

On 19.08.19 13:40, Thomas Huth wrote:
> On 8/5/19 5:29 PM, David Hildenbrand wrote:
>> Let's rewrite the DAT translation in a non-recursive way, similar to
>> arch/s390/kvm/gaccess.c:guest_translate() in KVM. This makes the
>> code much easier to read, compare and maintain.
> 
> Ok, I just had another look at this patch, and even if I don't like the
> complete rewrite just for the sake of it, the new code looks ok to me.
> 
> [...]
>> +    switch (asce & ASCE_TYPE_MASK) {
>> +    case ASCE_TYPE_REGION1:
>> +        if (read_table_entry(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(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(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;
>> +        }
>> +        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 */
> 
> If you don't like recursion, maybe you could at least use a for-loop for
> the region tables? ... the code is really quite repetitive here... just
> my 0.02 €.

I'll split this patch further up once I have time. With the unrolling I
am not yet quite sure - the tables tend to get more different with every
new HW release (especially, see the other patches in this series, like
edat2 and IEP) and we want our branch predictor to produce sane
predictions (especially when processing consecutive pages).

> 
>  Thomas
> 


-- 

Thanks,

David / dhildenb