On 16.02.2018 12:16, Janosch Frank wrote:
> Up to now we always expected to have the storage key facility
> available for our (non-VSIE) KVM guests. For huge page support, we
> need to be able to disable it, so let's introduce that now.
>
> Signed-off-by: Janosch Frank <frankja@xxxxxxxxxxxxxxxxxx>
> Reviewed-by: Farhan Ali <alifm@xxxxxxxxxxxxxxxxxx>
> ---
> arch/s390/include/asm/kvm_host.h | 1 +
> arch/s390/include/asm/mmu.h | 2 +-
> arch/s390/include/asm/mmu_context.h | 2 +-
> arch/s390/include/asm/pgtable.h | 4 ++--
> arch/s390/kvm/kvm-s390.c | 3 ++-
> arch/s390/kvm/priv.c | 22 +++++++++++++---------
> arch/s390/mm/gmap.c | 6 +++---
> arch/s390/mm/pgtable.c | 4 ++--
> 8 files changed, 25 insertions(+), 19 deletions(-)
>
> diff --git a/arch/s390/include/asm/kvm_host.h b/arch/s390/include/asm/kvm_host.h
> index 27918b1..f161ad0 100644
> --- a/arch/s390/include/asm/kvm_host.h
> +++ b/arch/s390/include/asm/kvm_host.h
> @@ -793,6 +793,7 @@ struct kvm_arch{
> int use_irqchip;
> int use_cmma;
> int use_pfmfi;
> + int use_skf;
> int user_cpu_state_ctrl;
> int user_sigp;
> int user_stsi;
> diff --git a/arch/s390/include/asm/mmu.h b/arch/s390/include/asm/mmu.h
> index c639c95..f5ff9db 100644
> --- a/arch/s390/include/asm/mmu.h
> +++ b/arch/s390/include/asm/mmu.h
> @@ -21,7 +21,7 @@ typedef struct {
> /* The mmu context uses extended page tables. */
> unsigned int has_pgste:1;
> /* The mmu context uses storage keys. */
> - unsigned int use_skey:1;
> + unsigned int uses_skeys:1;
> /* The mmu context uses CMM. */
> unsigned int uses_cmm:1;
> } mm_context_t;
> diff --git a/arch/s390/include/asm/mmu_context.h b/arch/s390/include/asm/mmu_context.h
> index d3ebfa8..bc9a2a9 100644
> --- a/arch/s390/include/asm/mmu_context.h
> +++ b/arch/s390/include/asm/mmu_context.h
> @@ -30,7 +30,7 @@ static inline int init_new_context(struct task_struct *tsk,
> test_thread_flag(TIF_PGSTE) ||
> (current->mm && current->mm->context.alloc_pgste);
> mm->context.has_pgste = 0;
> - mm->context.use_skey = 0;
> + mm->context.uses_skeys = 0;
> mm->context.uses_cmm = 0;
> #endif
> switch (mm->context.asce_limit) {
> diff --git a/arch/s390/include/asm/pgtable.h b/arch/s390/include/asm/pgtable.h
> index 9223b4d..4f26425 100644
> --- a/arch/s390/include/asm/pgtable.h
> +++ b/arch/s390/include/asm/pgtable.h
> @@ -509,10 +509,10 @@ static inline int mm_alloc_pgste(struct mm_struct *mm)
> * faults should no longer be backed by zero pages
> */
> #define mm_forbids_zeropage mm_has_pgste
> -static inline int mm_use_skey(struct mm_struct *mm)
> +static inline int mm_uses_skeys(struct mm_struct *mm)
> {
> #ifdef CONFIG_PGSTE
> - if (mm->context.use_skey)
> + if (mm->context.uses_skeys)
> return 1;
> #endif
> return 0;
> diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
> index 8fb6549..90deb7b 100644
> --- a/arch/s390/kvm/kvm-s390.c
> +++ b/arch/s390/kvm/kvm-s390.c
> @@ -1432,7 +1432,7 @@ static long kvm_s390_get_skeys(struct kvm *kvm, struct kvm_s390_skeys *args)
> return -EINVAL;
>
> /* Is this guest using storage keys? */
> - if (!mm_use_skey(current->mm))
> + if (!mm_uses_skeys(current->mm))
> return KVM_S390_GET_SKEYS_NONE;
>
> /* Enforce sane limit on memory allocation */
> @@ -2010,6 +2010,7 @@ int kvm_arch_init_vm(struct kvm *kvm, unsigned long type)
> kvm->arch.css_support = 0;
> kvm->arch.use_irqchip = 0;
> kvm->arch.use_pfmfi = sclp.has_pfmfi;
> + kvm->arch.use_skf = sclp.has_skey;
> kvm->arch.epoch = 0;
>
> spin_lock_init(&kvm->arch.start_stop_lock);
> diff --git a/arch/s390/kvm/priv.c b/arch/s390/kvm/priv.c
> index 76a2380..d9bd147 100644
> --- a/arch/s390/kvm/priv.c
> +++ b/arch/s390/kvm/priv.c
> @@ -208,19 +208,23 @@ int kvm_s390_skey_check_enable(struct kvm_vcpu *vcpu)
> struct kvm_s390_sie_block *sie_block = vcpu->arch.sie_block;
>
> trace_kvm_s390_skey_related_inst(vcpu);
> - if (!(sie_block->ictl & (ICTL_ISKE | ICTL_SSKE | ICTL_RRBE)) &&
> + /* Already enabled? */
> + if (vcpu->kvm->arch.use_skf &&
> + !(sie_block->ictl & (ICTL_ISKE | ICTL_SSKE | ICTL_RRBE)) &&
> !kvm_s390_test_cpuflags(vcpu, CPUSTAT_KSS))
> return rc;
While at it, can you directly "return 0;" here and remove the
initialization of rc to 0? Makes the code easier to read
>
> rc = s390_enable_skey();
> VCPU_EVENT(vcpu, 3, "enabling storage keys for guest: %d", rc);
> - if (!rc) {
> - if (kvm_s390_test_cpuflags(vcpu, CPUSTAT_KSS))
> - kvm_s390_clear_cpuflags(vcpu, CPUSTAT_KSS);
> - else
> - sie_block->ictl &= ~(ICTL_ISKE | ICTL_SSKE |
> - ICTL_RRBE);
> - }
> + if (rc)
> + return rc;
> +
> + if (kvm_s390_test_cpuflags(vcpu, CPUSTAT_KSS))
> + kvm_s390_clear_cpuflags(vcpu, CPUSTAT_KSS);
> + if (!vcpu->kvm->arch.use_skf)
> + sie_block->ictl |= ICTL_ISKE | ICTL_SSKE | ICTL_RRBE;
> + else
> + sie_block->ictl &= ~(ICTL_ISKE | ICTL_SSKE | ICTL_RRBE);
I wonder why
vcpu->arch.sie_block->ictl |= ICTL_ISKE | ICTL_SSKE | ICTL_RRBE;
Is set conditionally (sclp.has_kss) in kvm_arch_vcpu_setup().
Can't we simply always set these bits there and only clear them here
conditionally?
> return rc;
> }
>
> @@ -231,7 +235,7 @@ static int try_handle_skey(struct kvm_vcpu *vcpu)
> rc = kvm_s390_skey_check_enable(vcpu);
> if (rc)
> return rc;
> - if (sclp.has_skey) {
> + if (vcpu->kvm->arch.use_skf) {
> /* with storage-key facility, SIE interprets it for us */
> kvm_s390_retry_instr(vcpu);
> VCPU_EVENT(vcpu, 4, "%s", "retrying storage key operation");
> diff --git a/arch/s390/mm/gmap.c b/arch/s390/mm/gmap.c
> index 2cafcba..dbdcd25 100644
> --- a/arch/s390/mm/gmap.c
> +++ b/arch/s390/mm/gmap.c
> @@ -3094,14 +3094,14 @@ int s390_enable_skey(void)
> int rc = 0;
>
> down_write(&mm->mmap_sem);
> - if (mm_use_skey(mm))
> + if (mm_uses_skeys(mm))
> goto out_up;
>
> - mm->context.use_skey = 1;
> + mm->context.uses_skeys = 1;
> for (vma = mm->mmap; vma; vma = vma->vm_next) {
> if (ksm_madvise(vma, vma->vm_start, vma->vm_end,
> MADV_UNMERGEABLE, &vma->vm_flags)) {
> - mm->context.use_skey = 0;
> + mm->context.uses_skeys = 0;
> rc = -ENOMEM;
> goto out_up;
> }
> diff --git a/arch/s390/mm/pgtable.c b/arch/s390/mm/pgtable.c
> index 871fc65..158c880 100644
> --- a/arch/s390/mm/pgtable.c
> +++ b/arch/s390/mm/pgtable.c
> @@ -158,7 +158,7 @@ static inline pgste_t pgste_update_all(pte_t pte, pgste_t pgste,
> #ifdef CONFIG_PGSTE
> unsigned long address, bits, skey;
>
> - if (!mm_use_skey(mm) || pte_val(pte) & _PAGE_INVALID)
> + if (!mm_uses_skeys(mm) || pte_val(pte) & _PAGE_INVALID)
> return pgste;
> address = pte_val(pte) & PAGE_MASK;
> skey = (unsigned long) page_get_storage_key(address);
> @@ -180,7 +180,7 @@ static inline void pgste_set_key(pte_t *ptep, pgste_t pgste, pte_t entry,
> unsigned long address;
> unsigned long nkey;
>
> - if (!mm_use_skey(mm) || pte_val(entry) & _PAGE_INVALID)
> + if (!mm_uses_skeys(mm) || pte_val(entry) & _PAGE_INVALID)
> return;
> VM_BUG_ON(!(pte_val(*ptep) & _PAGE_INVALID));
> address = pte_val(entry) & PAGE_MASK;
>
--
Thanks,
David / dhildenb
