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

[Thomas Huth <thuth@redhat.com>]

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 <david@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