On 27/09/2019 11.58, 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 relevant bits are actually > called. > > Introduce defines for all bits declared in the PoP. This will come in > handy in follow-up patches. > > Add a note where additional information about s390x and the used > definitions can be found. > > Signed-off-by: David Hildenbrand <da...@redhat.com> > --- > target/s390x/cpu.h | 81 +++++++++++++++++++++++++++++---------- > target/s390x/mem_helper.c | 12 +++--- > target/s390x/mmu_helper.c | 37 ++++++++++-------- > 3 files changed, 87 insertions(+), 43 deletions(-) > > diff --git a/target/s390x/cpu.h b/target/s390x/cpu.h > index 163dae13d7..690b94c8ea 100644 > --- a/target/s390x/cpu.h > +++ b/target/s390x/cpu.h > @@ -1,6 +1,10 @@ > /* > * S/390 virtual CPU header > * > + * For details on the s390x architecture and used definitions (e.g., > + * PSW, PER and DAT (Dynamic Address Translation)), please refer to > + * the "z/Architecture Principles of Operations" - a.k.a. PoP. > + * > * Copyright (c) 2009 Ulrich Hecht > * Copyright IBM Corp. 2012, 2018 > * > @@ -558,26 +562,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
I think SFAA should be 0xfffffffffff00000ULL instead? > +#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 The above three definitions do not make much sense. TT should always be 00 for segment table entries. > +#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) > > #define SK_C (0x1 << 1) > #define SK_R (0x1 << 2) > diff --git a/target/s390x/mem_helper.c b/target/s390x/mem_helper.c > index 44e535856d..08c5cc6a99 100644 > --- a/target/s390x/mem_helper.c > +++ b/target/s390x/mem_helper.c The rest of the patch looks fine to me. Thomas
