On 26/09/2019 09.59, David Hildenbrand wrote: > On 26.09.19 09:52, Thomas Huth wrote: >> 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" >> */ >> > > We also have the PSW/PGM/PER definitions in there without such a note. > What about something generic as: > > diff --git a/target/s390x/cpu.h b/target/s390x/cpu.h > index e74a809257..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. > + *
Fine for me, too. Thomas