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

[Thomas Huth]

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.

Anyway, thanks for splitting this up into a separate patch, it really
gets much more readable this way.

 Thomas