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?
If so, what we actually need might be the following:
1. int s390_init_skey(uint64_t vmstart, uint64_t vmend);
2. kvm_s390_enable_skey()
-> Loop over all KVM memory slots, calling s390_enable_skey()
3. In kvm_arch_commit_memory_region() / gmap_map_segment()
-> call s390_init_skey for all *new* memory
We can of course use some flag to do initialization lazily just as now
(mm->context.uses_skeys).
This avoids initializing storage keys for !guest related stuff (e.g.
your write check above) and we catch new VMAs.
AFAICS we have the same problem already with current s390_enable_skey()
if I am not missing something important here. So this should be fixed
(if applicable) independently and not here.
Any experts?
> +
> + 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;
> }
> }
>
--
Thanks,
David / dhildenb
