On 12.08.19 09:20, 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. >> >> Use better names for the region/section/page table entries and for the >> macros to extract relevant parts from virtual address. Introduce defines >> for all defined bits, this will come in handy soon. >> >> All access exceptions now directly go via trigger_access_exception(), >> at a central point. DAT protection checks are performed at a central >> place. >> >> Also, we now catch and indicate invalid addresses of page tables. All >> table entries are accessed via read_table_entry(). >> >> Signed-off-by: David Hildenbrand <da...@redhat.com> >> --- >> target/s390x/cpu.h | 77 +++++--- >> target/s390x/mem_helper.c | 13 +- >> target/s390x/mmu_helper.c | 360 +++++++++++++++++--------------------- >> 3 files changed, 229 insertions(+), 221 deletions(-) >> >> diff --git a/target/s390x/cpu.h b/target/s390x/cpu.h >> index c34992bb2e..1ff14250bd 100644 >> --- a/target/s390x/cpu.h >> +++ b/target/s390x/cpu.h >> @@ -554,26 +554,63 @@ QEMU_BUILD_BUG_ON(sizeof(SysIB) != 4096); >> #define ASCE_TYPE_SEGMENT 0x00 /* segment table type >> */ >> #define ASCE_TABLE_LENGTH 0x03 /* region table length >> */ >> >> -#define REGION_ENTRY_ORIGIN (~0xfffULL) /* region/segment table origin >> */ >> -#define REGION_ENTRY_RO 0x200 /* region/segment protection bit >> */ >> -#define REGION_ENTRY_TF 0xc0 /* region/segment table offset >> */ >> -#define REGION_ENTRY_INV 0x20 /* invalid region table entry >> */ >> -#define REGION_ENTRY_TYPE_MASK 0x0c /* region/segment table type mask >> */ >> -#define REGION_ENTRY_TYPE_R1 0x0c /* region first table type >> */ >> -#define REGION_ENTRY_TYPE_R2 0x08 /* region second table type >> */ >> -#define REGION_ENTRY_TYPE_R3 0x04 /* region third table type >> */ >> -#define REGION_ENTRY_LENGTH 0x03 /* region third length >> */ >> - >> -#define SEGMENT_ENTRY_ORIGIN (~0x7ffULL) /* segment table origin */ >> -#define SEGMENT_ENTRY_FC 0x400 /* format control */ >> -#define SEGMENT_ENTRY_RO 0x200 /* page protection bit */ >> -#define SEGMENT_ENTRY_INV 0x20 /* invalid segment table entry */ >> - >> -#define VADDR_PX 0xff000 /* page index bits */ >> - >> -#define PAGE_RO 0x200 /* HW read-only bit */ >> -#define PAGE_INVALID 0x400 /* HW invalid bit */ >> -#define PAGE_RES0 0x800 /* bit must be zero */ >> +#define REGION_ENTRY_ORIGIN 0xfffffffffffff000ULL >> +#define REGION_ENTRY_P 0x0000000000000200ULL >> +#define REGION_ENTRY_TF 0x00000000000000c0ULL >> +#define REGION_ENTRY_I 0x0000000000000020ULL >> +#define REGION_ENTRY_TT 0x000000000000000cULL >> +#define REGION_ENTRY_TL 0x0000000000000003ULL >> + >> +#define REGION_ENTRY_TT_REGION1 0x000000000000000cULL >> +#define REGION_ENTRY_TT_REGION2 0x0000000000000008ULL >> +#define REGION_ENTRY_TT_REGION3 0x0000000000000004ULL >> + >> +#define REGION3_ENTRY_RFAA 0xffffffff80000000ULL >> +#define REGION3_ENTRY_AV 0x0000000000010000ULL >> +#define REGION3_ENTRY_ACC 0x000000000000f000ULL >> +#define REGION3_ENTRY_F 0x0000000000000800ULL >> +#define REGION3_ENTRY_FC 0x0000000000000400ULL >> +#define REGION3_ENTRY_IEP 0x0000000000000100ULL >> +#define REGION3_ENTRY_CR 0x0000000000000010ULL >> + >> +#define SEGMENT_ENTRY_ORIGIN 0xfffffffffffff800ULL >> +#define SEGMENT_ENTRY_SFAA 0xfffffffffff80000ULL >> +#define SEGMENT_ENTRY_AV 0x0000000000010000ULL >> +#define SEGMENT_ENTRY_ACC 0x000000000000f000ULL >> +#define SEGMENT_ENTRY_F 0x0000000000000800ULL >> +#define SEGMENT_ENTRY_FC 0x0000000000000400ULL >> +#define SEGMENT_ENTRY_P 0x0000000000000200ULL >> +#define SEGMENT_ENTRY_IEP 0x0000000000000100ULL >> +#define SEGMENT_ENTRY_I 0x0000000000000020ULL >> +#define SEGMENT_ENTRY_CS 0x0000000000000010ULL >> +#define SEGMENT_ENTRY_TT 0x000000000000000cULL >> + >> +#define SEGMENT_ENTRY_TT_REGION1 0x000000000000000cULL >> +#define SEGMENT_ENTRY_TT_REGION2 0x0000000000000008ULL >> +#define SEGMENT_ENTRY_TT_REGION3 0x0000000000000004ULL >> +#define SEGMENT_ENTRY_TT_SEGMENT 0x0000000000000000ULL >> + >> +#define PAGE_ENTRY_0 0x0000000000000800ULL >> +#define PAGE_ENTRY_I 0x0000000000000400ULL >> +#define PAGE_ENTRY_P 0x0000000000000200ULL >> +#define PAGE_ENTRY_IEP 0x0000000000000100ULL >> + >> +#define VADDR_REGION1_TX_MASK 0xffe0000000000000ULL >> +#define VADDR_REGION2_TX_MASK 0x001ffc0000000000ULL >> +#define VADDR_REGION3_TX_MASK 0x000003ff80000000ULL >> +#define VADDR_SEGMENT_TX_MASK 0x000000007ff00000ULL >> +#define VADDR_PAGE_TX_MASK 0x00000000000ff000ULL >> + >> +#define VADDR_REGION1_TX(vaddr) (((vaddr) & VADDR_REGION1_TX_MASK) >> >> 53) >> +#define VADDR_REGION2_TX(vaddr) (((vaddr) & VADDR_REGION2_TX_MASK) >> >> 42) >> +#define VADDR_REGION3_TX(vaddr) (((vaddr) & VADDR_REGION3_TX_MASK) >> >> 31) >> +#define VADDR_SEGMENT_TX(vaddr) (((vaddr) & VADDR_SEGMENT_TX_MASK) >> >> 20) >> +#define VADDR_PAGE_TX(vaddr) (((vaddr) & VADDR_PAGE_TX_MASK) >> 12) >> + >> +#define VADDR_REGION1_TL(vaddr) (((vaddr) & 0xc000000000000000ULL) >> >> 62) >> +#define VADDR_REGION2_TL(vaddr) (((vaddr) & 0x0018000000000000ULL) >> >> 51) >> +#define VADDR_REGION3_TL(vaddr) (((vaddr) & 0x0000030000000000ULL) >> >> 40) >> +#define VADDR_SEGMENT_TL(vaddr) (((vaddr) & 0x0000000060000000ULL) >> >> 29) > > Ugh, this patch is quite big, and you're doing multiple things at once > here, e.g. renaming macros from PAGE_INVALID to PAGE_ENTRY_I ... could
I could split out renaming the macros, however, besides a lot of work on my side this won't really make a huge difference here. > you maybe split this up in multiple patches instead? Also, is this > complete rewrite really necessary, just to match your personal taste? I I'm not gonna use lipstick on a pig ;) No honestly, the recursion is just nasty and we can now easily compare the current code with arch/s390/kvm/gaccess.c:guest_translate() > think it would be easier to review if you'd just fix the current code > instead if necessary... No, won't do that. Still thanks for having a look. -- Thanks, David / dhildenb
