On 26/09/2019 09.38, David Hildenbrand wrote: > On 26.09.19 09:35, Thomas Huth wrote: >> On 25/09/2019 14.52, David Hildenbrand wrote: >>> Let's use consitent names for the region/section/page table entries and >>> for the macros to extract relevant parts from virtual address. Make them >>> match the definitions in the PoP - e.g., how the televant bits are actually >> >> s/televant/relevant/ >> >>> called. >>> >>> Introduce defines for all bits declared in the PoP. This will come in >>> handy in follow-up patches. >>> >>> Signed-off-by: David Hildenbrand <da...@redhat.com> >>> --- >>> target/s390x/cpu.h | 77 +++++++++++++++++++++++++++++---------- >>> target/s390x/mem_helper.c | 12 +++--- >>> target/s390x/mmu_helper.c | 37 ++++++++++--------- >>> 3 files changed, 83 insertions(+), 43 deletions(-) >>> >>> diff --git a/target/s390x/cpu.h b/target/s390x/cpu.h >>> index 163dae13d7..e74a809257 100644 >>> --- a/target/s390x/cpu.h >>> +++ b/target/s390x/cpu.h >>> @@ -558,26 +558,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 >> >> Any chance that you could keep the comments after the definitions? I >> think they are useful for people who are not 100% familiar with the DAT >> on s390x. > > I thought about that, but do we expect people that don't have a clue > about s390x DAT and don't compare the code against the PoP to understand > our DAT translation just by comments on defines?
I'm not sure that everybody is aware of the PoP ... maybe you could just put a comment in front of the block a la: /* * For details on the following definitions, see the "Dynamic Address * Translation" section in chapter 3 of the "z/Architecture Principles * of Operations - SA22-7832-11" */ ? Thomas
