diff mbox

[v4,6/9] s390/mm: clear huge page storage keys on enable_skey

Message ID 20180627135510.117945-7-frankja@linux.ibm.com (mailing list archive)
State New, archived
Headers show

Commit Message

Janosch Frank June 27, 2018, 1:55 p.m. UTC 
From: Dominik Dingel <dingel@linux.vnet.ibm.com>

When a guest starts using storage keys, we trap and set a default one
for its whole valid address space. With this patch we are now able to
do that for large pages.

To speed up the storage key insertion, we use
__storage_key_init_range, which in-turn will use sske_frame to set
multiple storage keys with one instruction. As it has been previously
used for debuging we have to get rid of the default key check and make
it quiescing.

Signed-off-by: Dominik Dingel <dingel@linux.vnet.ibm.com>
Signed-off-by: Janosch Frank <frankja@linux.vnet.ibm.com>
[replaced page_set_storage_key loop with __storage_key_init_range]
Reviewed-by: Martin Schwidefsky <schwidefsky@de.ibm.com>
---
 arch/s390/mm/gmap.c     | 26 +++++++++++++++++++++++---
 arch/s390/mm/pageattr.c |  6 ++----
 2 files changed, 25 insertions(+), 7 deletions(-)

Comments

David Hildenbrand June 28, 2018, 8:07 a.m. UTC | #1 
On 27.06.2018 15:55, Janosch Frank wrote:
> From: Dominik Dingel <dingel@linux.vnet.ibm.com>
> 
> When a guest starts using storage keys, we trap and set a default one
> for its whole valid address space. With this patch we are now able to
> do that for large pages.
> 
> To speed up the storage key insertion, we use
> __storage_key_init_range, which in-turn will use sske_frame to set
> multiple storage keys with one instruction. As it has been previously
> used for debuging we have to get rid of the default key check and make
> it quiescing.
> 
> Signed-off-by: Dominik Dingel <dingel@linux.vnet.ibm.com>
> Signed-off-by: Janosch Frank <frankja@linux.vnet.ibm.com>
> [replaced page_set_storage_key loop with __storage_key_init_range]
> Reviewed-by: Martin Schwidefsky <schwidefsky@de.ibm.com>
> ---
>  arch/s390/mm/gmap.c     | 26 +++++++++++++++++++++++---
>  arch/s390/mm/pageattr.c |  6 ++----
>  2 files changed, 25 insertions(+), 7 deletions(-)
> 
> diff --git a/arch/s390/mm/gmap.c b/arch/s390/mm/gmap.c
> index 3da4f85ec330..af0a87eeede0 100644
> --- a/arch/s390/mm/gmap.c
> +++ b/arch/s390/mm/gmap.c
> @@ -2761,17 +2761,37 @@ EXPORT_SYMBOL_GPL(s390_enable_sie);
>   * Enable storage key handling from now on and initialize the storage
>   * keys with the default key.
>   */
> -static int __s390_enable_skey(pte_t *pte, unsigned long addr,
> -			      unsigned long next, struct mm_walk *walk)
> +static int __s390_enable_skey_pte(pte_t *pte, unsigned long addr,
> +				  unsigned long next, struct mm_walk *walk)
>  {
>  	/* Clear storage key */
>  	ptep_zap_key(walk->mm, addr, pte);
>  	return 0;
>  }
>  
> +static int __s390_enable_skey_hugetlb(pte_t *pte, unsigned long addr,
> +				      unsigned long hmask, unsigned long next,
> +				      struct mm_walk *walk)
> +{
> +	pmd_t *pmd = (pmd_t *)pte;
> +	unsigned long start, end;
> +
> +	if (pmd_val(*pmd) & _SEGMENT_ENTRY_INVALID
> +	    || !(pmd_val(*pmd) & _SEGMENT_ENTRY_WRITE))

In this file, we usually have the "||"/"&&" at the end of the previous line.

Silly, question: why do we have to handle invalid entries here and why
do we have to skip !write? Can both even happen? Should we simply not
initialize storage keys here?

> +		return 0;
> +
> +	start = pmd_val(*pmd) & HPAGE_MASK;
> +	end = start + HPAGE_SIZE - 1;
> +	__storage_key_init_range(start, end);
> +	return 0;
> +}
> +
>  int s390_enable_skey(void)
>  {
> -	struct mm_walk walk = { .pte_entry = __s390_enable_skey };
> +	struct mm_walk walk = {
> +		.hugetlb_entry = __s390_enable_skey_hugetlb,
> +		.pte_entry = __s390_enable_skey_pte,
> +	};
>  	struct mm_struct *mm = current->mm;
>  	struct vm_area_struct *vma;
>  	int rc = 0;
> diff --git a/arch/s390/mm/pageattr.c b/arch/s390/mm/pageattr.c
> index c44171588d08..f8c6faab41f4 100644
> --- a/arch/s390/mm/pageattr.c
> +++ b/arch/s390/mm/pageattr.c
> @@ -14,7 +14,7 @@
>  
>  static inline unsigned long sske_frame(unsigned long addr, unsigned char skey)
>  {
> -	asm volatile(".insn rrf,0xb22b0000,%[skey],%[addr],9,0"
> +	asm volatile(".insn rrf,0xb22b0000,%[skey],%[addr],1,0"
>  		     : [addr] "+a" (addr) : [skey] "d" (skey));
>  	return addr;
>  }
> @@ -23,8 +23,6 @@ void __storage_key_init_range(unsigned long start, unsigned long end)
>  {
>  	unsigned long boundary, size;
>  
> -	if (!PAGE_DEFAULT_KEY)
> -		return;
>  	while (start < end) {
>  		if (MACHINE_HAS_EDAT1) {
>  			/* set storage keys for a 1MB frame */
> @@ -37,7 +35,7 @@ void __storage_key_init_range(unsigned long start, unsigned long end)
>  				continue;
>  			}
>  		}
> -		page_set_storage_key(start, PAGE_DEFAULT_KEY, 0);
> +		page_set_storage_key(start, PAGE_DEFAULT_KEY, 1);

Why do we have to change quiescing here? And why in sske_frame? (in
other words: why did it work previously?)

>  		start += PAGE_SIZE;
>  	}
>  }
>
Janosch Frank June 28, 2018, 11:13 a.m. UTC | #2 
On 28.06.2018 10:07, David Hildenbrand wrote:
> On 27.06.2018 15:55, Janosch Frank wrote:
>> From: Dominik Dingel <dingel@linux.vnet.ibm.com>
>>
>> When a guest starts using storage keys, we trap and set a default one
>> for its whole valid address space. With this patch we are now able to
>> do that for large pages.
>>
>> To speed up the storage key insertion, we use
>> __storage_key_init_range, which in-turn will use sske_frame to set
>> multiple storage keys with one instruction. As it has been previously
>> used for debuging we have to get rid of the default key check and make
>> it quiescing.
>>
>> Signed-off-by: Dominik Dingel <dingel@linux.vnet.ibm.com>
>> Signed-off-by: Janosch Frank <frankja@linux.vnet.ibm.com>
>> [replaced page_set_storage_key loop with __storage_key_init_range]
>> Reviewed-by: Martin Schwidefsky <schwidefsky@de.ibm.com>
>> ---
>>  arch/s390/mm/gmap.c     | 26 +++++++++++++++++++++++---
>>  arch/s390/mm/pageattr.c |  6 ++----
>>  2 files changed, 25 insertions(+), 7 deletions(-)
>>
>> diff --git a/arch/s390/mm/gmap.c b/arch/s390/mm/gmap.c
>> index 3da4f85ec330..af0a87eeede0 100644
>> --- a/arch/s390/mm/gmap.c
>> +++ b/arch/s390/mm/gmap.c
>> @@ -2761,17 +2761,37 @@ EXPORT_SYMBOL_GPL(s390_enable_sie);
>>   * Enable storage key handling from now on and initialize the storage
>>   * keys with the default key.
>>   */
>> -static int __s390_enable_skey(pte_t *pte, unsigned long addr,
>> -			      unsigned long next, struct mm_walk *walk)
>> +static int __s390_enable_skey_pte(pte_t *pte, unsigned long addr,
>> +				  unsigned long next, struct mm_walk *walk)
>>  {
>>  	/* Clear storage key */
>>  	ptep_zap_key(walk->mm, addr, pte);
>>  	return 0;
>>  }
>>  
>> +static int __s390_enable_skey_hugetlb(pte_t *pte, unsigned long addr,
>> +				      unsigned long hmask, unsigned long next,
>> +				      struct mm_walk *walk)
>> +{
>> +	pmd_t *pmd = (pmd_t *)pte;
>> +	unsigned long start, end;
>> +
>> +	if (pmd_val(*pmd) & _SEGMENT_ENTRY_INVALID
>> +	    || !(pmd_val(*pmd) & _SEGMENT_ENTRY_WRITE))
> 
> In this file, we usually have the "||"/"&&" at the end of the previous line.
> 
> Silly, question: why do we have to handle invalid entries here and why
> do we have to skip !write? Can both even happen? Should we simply not
> initialize storage keys here?

walk->hugetlb_range does not check the I bit before calling the handler,
hence we have to. The write check is indeed weird, especially as it is
the software bit.

> 
>> +		return 0;
>> +
>> +	start = pmd_val(*pmd) & HPAGE_MASK;
>> +	end = start + HPAGE_SIZE - 1;
>> +	__storage_key_init_range(start, end);
>> +	return 0;
>> +}
>> +
>>  int s390_enable_skey(void)
>>  {
>> -	struct mm_walk walk = { .pte_entry = __s390_enable_skey };
>> +	struct mm_walk walk = {
>> +		.hugetlb_entry = __s390_enable_skey_hugetlb,
>> +		.pte_entry = __s390_enable_skey_pte,
>> +	};
>>  	struct mm_struct *mm = current->mm;
>>  	struct vm_area_struct *vma;
>>  	int rc = 0;
>> diff --git a/arch/s390/mm/pageattr.c b/arch/s390/mm/pageattr.c
>> index c44171588d08..f8c6faab41f4 100644
>> --- a/arch/s390/mm/pageattr.c
>> +++ b/arch/s390/mm/pageattr.c
>> @@ -14,7 +14,7 @@
>>  
>>  static inline unsigned long sske_frame(unsigned long addr, unsigned char skey)
>>  {
>> -	asm volatile(".insn rrf,0xb22b0000,%[skey],%[addr],9,0"
>> +	asm volatile(".insn rrf,0xb22b0000,%[skey],%[addr],1,0"
>>  		     : [addr] "+a" (addr) : [skey] "d" (skey));
>>  	return addr;
>>  }
>> @@ -23,8 +23,6 @@ void __storage_key_init_range(unsigned long start, unsigned long end)
>>  {
>>  	unsigned long boundary, size;
>>  
>> -	if (!PAGE_DEFAULT_KEY)
>> -		return;
>>  	while (start < end) {
>>  		if (MACHINE_HAS_EDAT1) {
>>  			/* set storage keys for a 1MB frame */
>> @@ -37,7 +35,7 @@ void __storage_key_init_range(unsigned long start, unsigned long end)
>>  				continue;
>>  			}
>>  		}
>> -		page_set_storage_key(start, PAGE_DEFAULT_KEY, 0);
>> +		page_set_storage_key(start, PAGE_DEFAULT_KEY, 1);
> 
> Why do we have to change quiescing here? And why in sske_frame? (in
> other words: why did it work previously?)

Previously this was only debug code, let me cite Heiko:
The whole code only exists for debugging purposes, where we might set
PAGE_DEFAULT_KEY to a value that is not zero. The last time I did that
however is a couple of years ago. And that was in order to prove that
z/VM modified the storage keys used by Linux.

> 
>>  		start += PAGE_SIZE;
>>  	}
>>  }
>>
> 
>
diff mbox

Patch

diff --git a/arch/s390/mm/gmap.c b/arch/s390/mm/gmap.c
index 3da4f85ec330..af0a87eeede0 100644
--- a/arch/s390/mm/gmap.c
+++ b/arch/s390/mm/gmap.c
@@ -2761,17 +2761,37 @@  EXPORT_SYMBOL_GPL(s390_enable_sie);
  * Enable storage key handling from now on and initialize the storage
  * keys with the default key.
  */
-static int __s390_enable_skey(pte_t *pte, unsigned long addr,
-			      unsigned long next, struct mm_walk *walk)
+static int __s390_enable_skey_pte(pte_t *pte, unsigned long addr,
+				  unsigned long next, struct mm_walk *walk)
 {
 	/* Clear storage key */
 	ptep_zap_key(walk->mm, addr, pte);
 	return 0;
 }
 
+static int __s390_enable_skey_hugetlb(pte_t *pte, unsigned long addr,
+				      unsigned long hmask, unsigned long next,
+				      struct mm_walk *walk)
+{
+	pmd_t *pmd = (pmd_t *)pte;
+	unsigned long start, end;
+
+	if (pmd_val(*pmd) & _SEGMENT_ENTRY_INVALID
+	    || !(pmd_val(*pmd) & _SEGMENT_ENTRY_WRITE))
+		return 0;
+
+	start = pmd_val(*pmd) & HPAGE_MASK;
+	end = start + HPAGE_SIZE - 1;
+	__storage_key_init_range(start, end);
+	return 0;
+}
+
 int s390_enable_skey(void)
 {
-	struct mm_walk walk = { .pte_entry = __s390_enable_skey };
+	struct mm_walk walk = {
+		.hugetlb_entry = __s390_enable_skey_hugetlb,
+		.pte_entry = __s390_enable_skey_pte,
+	};
 	struct mm_struct *mm = current->mm;
 	struct vm_area_struct *vma;
 	int rc = 0;
diff --git a/arch/s390/mm/pageattr.c b/arch/s390/mm/pageattr.c
index c44171588d08..f8c6faab41f4 100644
--- a/arch/s390/mm/pageattr.c
+++ b/arch/s390/mm/pageattr.c
@@ -14,7 +14,7 @@ 
 
 static inline unsigned long sske_frame(unsigned long addr, unsigned char skey)
 {
-	asm volatile(".insn rrf,0xb22b0000,%[skey],%[addr],9,0"
+	asm volatile(".insn rrf,0xb22b0000,%[skey],%[addr],1,0"
 		     : [addr] "+a" (addr) : [skey] "d" (skey));
 	return addr;
 }
@@ -23,8 +23,6 @@  void __storage_key_init_range(unsigned long start, unsigned long end)
 {
 	unsigned long boundary, size;
 
-	if (!PAGE_DEFAULT_KEY)
-		return;
 	while (start < end) {
 		if (MACHINE_HAS_EDAT1) {
 			/* set storage keys for a 1MB frame */
@@ -37,7 +35,7 @@  void __storage_key_init_range(unsigned long start, unsigned long end)
 				continue;
 			}
 		}
-		page_set_storage_key(start, PAGE_DEFAULT_KEY, 0);
+		page_set_storage_key(start, PAGE_DEFAULT_KEY, 1);
 		start += PAGE_SIZE;
 	}
 }