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

[Thomas Huth]

On 8/19/19 1:58 PM, David Hildenbrand wrote:
> 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).

Ok, I just also saw in the next patch that region 3 already needs some
special handling for EDAT-2, so never mind, it's likely best to keep it
unrolled.

 Thomas