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

[Thomas Huth]

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

 Thomas