Re: [PATCH v2 6/7] s390x/mmu: DAT table definition overhaul

[David Hildenbrand <david@redhat.com>]

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 <david@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.
+ *
  *  Copyright (c) 2009 Ulrich Hecht
  *  Copyright IBM Corp. 2012, 2018
  *

-- 

Thanks,

David / dhildenb