Re: [kvm-unit-tests PATCH v2 2/5] s390x: lib: fix pgtable.h

From: Janosch Frank <frankja@linux.ibm.com>
To: Claudio Imbrenda <imbrenda@linux.ibm.com>, kvm@vger.kernel.org
Cc: linux-s390@vger.kernel.org, david@redhat.com, thuth@redhat.com,
	cohuck@redhat.com, pmorel@linux.ibm.com
Subject: Re: [kvm-unit-tests PATCH v2 2/5] s390x: lib: fix pgtable.h
Date: Tue, 23 Feb 2021 15:31:06 +0100	[thread overview]
Message-ID: <518e0f86-bbba-bd52-3962-2816b2f8ccf6@linux.ibm.com> (raw)
In-Reply-To: <20210223140759.255670-3-imbrenda@linux.ibm.com>

On 2/23/21 3:07 PM, Claudio Imbrenda wrote:
> Fix pgtable.h:
> 
> * SEGMENT_ENTRY_SFAA had one extra bit set
> * pmd entries don't have a length
> * ipte does not need to clear the lower bits
> * pud entries should use SEGMENT_TABLE_LENGTH, as they point to segment tables
> 
> Signed-off-by: Claudio Imbrenda <imbrenda@linux.ibm.com>
> ---
>  lib/s390x/asm/pgtable.h | 9 ++++-----
>  1 file changed, 4 insertions(+), 5 deletions(-)
> 
> diff --git a/lib/s390x/asm/pgtable.h b/lib/s390x/asm/pgtable.h
> index 277f3480..a2ff2d4e 100644
> --- a/lib/s390x/asm/pgtable.h
> +++ b/lib/s390x/asm/pgtable.h
> @@ -60,7 +60,7 @@
>  #define SEGMENT_SHIFT			20
>  
>  #define SEGMENT_ENTRY_ORIGIN		0xfffffffffffff800UL
> -#define SEGMENT_ENTRY_SFAA		0xfffffffffff80000UL
> +#define SEGMENT_ENTRY_SFAA		0xfffffffffff00000UL
>  #define SEGMENT_ENTRY_AV		0x0000000000010000UL
>  #define SEGMENT_ENTRY_ACC		0x000000000000f000UL
>  #define SEGMENT_ENTRY_F			0x0000000000000800UL
> @@ -183,7 +183,7 @@ static inline pmd_t *pmd_alloc(pud_t *pud, unsigned long addr)
>  	if (pud_none(*pud)) {
>  		pmd_t *pmd = pmd_alloc_one();
>  		pud_val(*pud) = __pa(pmd) | REGION_ENTRY_TT_REGION3 |
> -				REGION_TABLE_LENGTH;
> +				SEGMENT_TABLE_LENGTH;

@David: I'd much rather have REGION_ENTRY_LENGTH instead of
REGION_TABLE_LENGTH and SEGMENT_TABLE_LENGTH.

My argument is that this is not really an attribute of the table and
very much specific to the format of the (region table) entry. We already
have the table order as a length anyway...

Could you tell me what you had in mind when splitting this?

>  	}
>  	return pmd_offset(pud, addr);
>  }
> @@ -202,15 +202,14 @@ static inline pte_t *pte_alloc(pmd_t *pmd, unsigned long addr)
>  {
>  	if (pmd_none(*pmd)) {
>  		pte_t *pte = pte_alloc_one();
> -		pmd_val(*pmd) = __pa(pte) | SEGMENT_ENTRY_TT_SEGMENT |
> -				SEGMENT_TABLE_LENGTH;
> +		pmd_val(*pmd) = __pa(pte) | SEGMENT_ENTRY_TT_SEGMENT;

Uhhhh good catch!

>  	}
>  	return pte_offset(pmd, addr);
>  }
>  
>  static inline void ipte(unsigned long vaddr, pteval_t *p_pte)
>  {
> -	unsigned long table_origin = (unsigned long)p_pte & PAGE_MASK;
> +	unsigned long table_origin = (unsigned long)p_pte;
>  
>  	asm volatile(
>  		"	ipte %0,%1\n"
> 

IPTE ignores that data but having the page mask also doesn't hurt so
generally this is not a fix, right?