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

[Thomas Huth]

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 <address@hidden>
>>>>> ---
>>>>>  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