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
