On 12.07.2018 09:50, frankja wrote:
> On Thu, 12 Jul 2018 09:17:45 +0200
> David Hildenbrand <david@xxxxxxxxxx> wrote:
>
>> On 06.07.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 | 32 +++++++++++++++++++++++++++++---
>>> arch/s390/mm/pageattr.c | 6 ++----
>>> 2 files changed, 31 insertions(+), 7 deletions(-)
>>>
>>> diff --git a/arch/s390/mm/gmap.c b/arch/s390/mm/gmap.c
>>> index c898e3a4628b..90f3c9dae983 100644
>>> --- a/arch/s390/mm/gmap.c
>>> +++ b/arch/s390/mm/gmap.c
>>> @@ -2545,17 +2545,43 @@ 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;
>>> +
>>> + /*
>>> + * The write check makes sure we do not set a key on shared
>>> + * memory. This is needed as the walker does not
>>> differentiate
>>> + * between actual guest memory and the process executable
>>> or
>>> + * shared libraries.
>>> + */
>>> + if (pmd_val(*pmd) & _SEGMENT_ENTRY_INVALID ||
>>> + !(pmd_val(*pmd) & _SEGMENT_ENTRY_WRITE))
>>> + return 0;
>>
>> With the given comment, this is interesting. Actually, I am not sure
>> if this interface (s390_enable_skey()) is what we actually want. Think
>> about the following:
>>
>> 1. User space setups a guest.
>> 2. Guest executes a skey operation. Skeys initialized for the whole MM
>> of user space.
>> 3. User space adds more memory to the guest (hotplug via some
>> mechanism)
>> - new kvm memory slot.
>>
>> For 3. I don't think that skeys are initialized for the new memory
>> slot. Am I missing something?
>
> We set the pgste key on xchg with a proper pte, but only when
> mm_use_skey(), so all new mappings have 0 keys after s390_enable_skey().
>
> I asked Martin about pmds and he told me there are other mechanisms
> around for them, but I didn't find them yet.
Ah right, I was missing the pgste_set_key() function. I am also still
looking for the PMD counterpart.
--
Thanks,
David / dhildenb
