On 28.06.2018 10:07, David Hildenbrand wrote:
> On 27.06.2018 15:55, Janosch Frank wrote:
>> From: Dominik Dingel <dingel@xxxxxxxxxxxxxxxxxx>
>>
>> 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@xxxxxxxxxxxxxxxxxx>
>> Signed-off-by: Janosch Frank <frankja@xxxxxxxxxxxxxxxxxx>
>> [replaced page_set_storage_key loop with __storage_key_init_range]
>> Reviewed-by: Martin Schwidefsky <schwidefsky@xxxxxxxxxx>
>> ---
>> 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;
>> }
>> }
>>
>
>
Attachment:
signature.asc
Description: OpenPGP digital signature
