Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Fri, 24 Jun 2022 11:22:25 +0200
From:   Claudio Imbrenda <imbrenda@linux.ibm.com>
To:     Pierre Morel <pmorel@linux.ibm.com>
Cc:     kvm@vger.kernel.org, linux-s390@vger.kernel.org,
        linux-kernel@vger.kernel.org, borntraeger@de.ibm.com,
        frankja@linux.ibm.com, cohuck@redhat.com, david@redhat.com,
        thuth@redhat.com, hca@linux.ibm.com, gor@linux.ibm.com,
        wintera@linux.ibm.com, seiden@linux.ibm.com, nrb@linux.ibm.com
Subject: Re: [PATCH v10 1/3] KVM: s390: ipte lock for SCA access should be
 contained in KVM
Message-ID: <20220624112225.23ee9ddb@p-imbrenda>
In-Reply-To: <20220620125437.37122-2-pmorel@linux.ibm.com>
References: <20220620125437.37122-1-pmorel@linux.ibm.com>
        <20220620125437.37122-2-pmorel@linux.ibm.com>
Organization: IBM
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit
Precedence: bulk

On Mon, 20 Jun 2022 14:54:35 +0200
Pierre Morel <pmorel@linux.ibm.com> wrote:

> We can check if SIIF is enabled by testing the sclp_info struct
> instead of testing the sie control block eca variable.
> sclp.has_ssif is the only requirement to set ECA_SII anyway
> so we can go straight to the source for that.
> 
> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
> Reviewed-by: Janosch Frank <frankja@linux.ibm.com>
> Reviewed-by: David Hildenbrand <david@redhat.com>

Reviewed-by: Claudio Imbrenda <imbrenda@linux.ibm.com>

> ---
>  arch/s390/kvm/gaccess.c | 96 ++++++++++++++++++++---------------------
>  arch/s390/kvm/gaccess.h |  6 +--
>  arch/s390/kvm/priv.c    |  6 +--
>  3 files changed, 54 insertions(+), 54 deletions(-)
> 
> diff --git a/arch/s390/kvm/gaccess.c b/arch/s390/kvm/gaccess.c
> index 227ed0009354..082ec5f2c3a5 100644
> --- a/arch/s390/kvm/gaccess.c
> +++ b/arch/s390/kvm/gaccess.c
> @@ -262,77 +262,77 @@ struct aste {
>  	/* .. more fields there */
>  };
>  
> -int ipte_lock_held(struct kvm_vcpu *vcpu)
> +int ipte_lock_held(struct kvm *kvm)
>  {
> -	if (vcpu->arch.sie_block->eca & ECA_SII) {
> +	if (sclp.has_siif) {
>  		int rc;
>  
> -		read_lock(&vcpu->kvm->arch.sca_lock);
> -		rc = kvm_s390_get_ipte_control(vcpu->kvm)->kh != 0;
> -		read_unlock(&vcpu->kvm->arch.sca_lock);
> +		read_lock(&kvm->arch.sca_lock);
> +		rc = kvm_s390_get_ipte_control(kvm)->kh != 0;
> +		read_unlock(&kvm->arch.sca_lock);
>  		return rc;
>  	}
> -	return vcpu->kvm->arch.ipte_lock_count != 0;
> +	return kvm->arch.ipte_lock_count != 0;
>  }
>  
> -static void ipte_lock_simple(struct kvm_vcpu *vcpu)
> +static void ipte_lock_simple(struct kvm *kvm)
>  {
>  	union ipte_control old, new, *ic;
>  
> -	mutex_lock(&vcpu->kvm->arch.ipte_mutex);
> -	vcpu->kvm->arch.ipte_lock_count++;
> -	if (vcpu->kvm->arch.ipte_lock_count > 1)
> +	mutex_lock(&kvm->arch.ipte_mutex);
> +	kvm->arch.ipte_lock_count++;
> +	if (kvm->arch.ipte_lock_count > 1)
>  		goto out;
>  retry:
> -	read_lock(&vcpu->kvm->arch.sca_lock);
> -	ic = kvm_s390_get_ipte_control(vcpu->kvm);
> +	read_lock(&kvm->arch.sca_lock);
> +	ic = kvm_s390_get_ipte_control(kvm);
>  	do {
>  		old = READ_ONCE(*ic);
>  		if (old.k) {
> -			read_unlock(&vcpu->kvm->arch.sca_lock);
> +			read_unlock(&kvm->arch.sca_lock);
>  			cond_resched();
>  			goto retry;
>  		}
>  		new = old;
>  		new.k = 1;
>  	} while (cmpxchg(&ic->val, old.val, new.val) != old.val);
> -	read_unlock(&vcpu->kvm->arch.sca_lock);
> +	read_unlock(&kvm->arch.sca_lock);
>  out:
> -	mutex_unlock(&vcpu->kvm->arch.ipte_mutex);
> +	mutex_unlock(&kvm->arch.ipte_mutex);
>  }
>  
> -static void ipte_unlock_simple(struct kvm_vcpu *vcpu)
> +static void ipte_unlock_simple(struct kvm *kvm)
>  {
>  	union ipte_control old, new, *ic;
>  
> -	mutex_lock(&vcpu->kvm->arch.ipte_mutex);
> -	vcpu->kvm->arch.ipte_lock_count--;
> -	if (vcpu->kvm->arch.ipte_lock_count)
> +	mutex_lock(&kvm->arch.ipte_mutex);
> +	kvm->arch.ipte_lock_count--;
> +	if (kvm->arch.ipte_lock_count)
>  		goto out;
> -	read_lock(&vcpu->kvm->arch.sca_lock);
> -	ic = kvm_s390_get_ipte_control(vcpu->kvm);
> +	read_lock(&kvm->arch.sca_lock);
> +	ic = kvm_s390_get_ipte_control(kvm);
>  	do {
>  		old = READ_ONCE(*ic);
>  		new = old;
>  		new.k = 0;
>  	} while (cmpxchg(&ic->val, old.val, new.val) != old.val);
> -	read_unlock(&vcpu->kvm->arch.sca_lock);
> -	wake_up(&vcpu->kvm->arch.ipte_wq);
> +	read_unlock(&kvm->arch.sca_lock);
> +	wake_up(&kvm->arch.ipte_wq);
>  out:
> -	mutex_unlock(&vcpu->kvm->arch.ipte_mutex);
> +	mutex_unlock(&kvm->arch.ipte_mutex);
>  }
>  
> -static void ipte_lock_siif(struct kvm_vcpu *vcpu)
> +static void ipte_lock_siif(struct kvm *kvm)
>  {
>  	union ipte_control old, new, *ic;
>  
>  retry:
> -	read_lock(&vcpu->kvm->arch.sca_lock);
> -	ic = kvm_s390_get_ipte_control(vcpu->kvm);
> +	read_lock(&kvm->arch.sca_lock);
> +	ic = kvm_s390_get_ipte_control(kvm);
>  	do {
>  		old = READ_ONCE(*ic);
>  		if (old.kg) {
> -			read_unlock(&vcpu->kvm->arch.sca_lock);
> +			read_unlock(&kvm->arch.sca_lock);
>  			cond_resched();
>  			goto retry;
>  		}
> @@ -340,15 +340,15 @@ static void ipte_lock_siif(struct kvm_vcpu *vcpu)
>  		new.k = 1;
>  		new.kh++;
>  	} while (cmpxchg(&ic->val, old.val, new.val) != old.val);
> -	read_unlock(&vcpu->kvm->arch.sca_lock);
> +	read_unlock(&kvm->arch.sca_lock);
>  }
>  
> -static void ipte_unlock_siif(struct kvm_vcpu *vcpu)
> +static void ipte_unlock_siif(struct kvm *kvm)
>  {
>  	union ipte_control old, new, *ic;
>  
> -	read_lock(&vcpu->kvm->arch.sca_lock);
> -	ic = kvm_s390_get_ipte_control(vcpu->kvm);
> +	read_lock(&kvm->arch.sca_lock);
> +	ic = kvm_s390_get_ipte_control(kvm);
>  	do {
>  		old = READ_ONCE(*ic);
>  		new = old;
> @@ -356,25 +356,25 @@ static void ipte_unlock_siif(struct kvm_vcpu *vcpu)
>  		if (!new.kh)
>  			new.k = 0;
>  	} while (cmpxchg(&ic->val, old.val, new.val) != old.val);
> -	read_unlock(&vcpu->kvm->arch.sca_lock);
> +	read_unlock(&kvm->arch.sca_lock);
>  	if (!new.kh)
> -		wake_up(&vcpu->kvm->arch.ipte_wq);
> +		wake_up(&kvm->arch.ipte_wq);
>  }
>  
> -void ipte_lock(struct kvm_vcpu *vcpu)
> +void ipte_lock(struct kvm *kvm)
>  {
> -	if (vcpu->arch.sie_block->eca & ECA_SII)
> -		ipte_lock_siif(vcpu);
> +	if (sclp.has_siif)
> +		ipte_lock_siif(kvm);
>  	else
> -		ipte_lock_simple(vcpu);
> +		ipte_lock_simple(kvm);
>  }
>  
> -void ipte_unlock(struct kvm_vcpu *vcpu)
> +void ipte_unlock(struct kvm *kvm)
>  {
> -	if (vcpu->arch.sie_block->eca & ECA_SII)
> -		ipte_unlock_siif(vcpu);
> +	if (sclp.has_siif)
> +		ipte_unlock_siif(kvm);
>  	else
> -		ipte_unlock_simple(vcpu);
> +		ipte_unlock_simple(kvm);
>  }
>  
>  static int ar_translation(struct kvm_vcpu *vcpu, union asce *asce, u8 ar,
> @@ -1086,7 +1086,7 @@ int access_guest_with_key(struct kvm_vcpu *vcpu, unsigned long ga, u8 ar,
>  	try_storage_prot_override = storage_prot_override_applicable(vcpu);
>  	need_ipte_lock = psw_bits(*psw).dat && !asce.r;
>  	if (need_ipte_lock)
> -		ipte_lock(vcpu);
> +		ipte_lock(vcpu->kvm);
>  	/*
>  	 * Since we do the access further down ultimately via a move instruction
>  	 * that does key checking and returns an error in case of a protection
> @@ -1127,7 +1127,7 @@ int access_guest_with_key(struct kvm_vcpu *vcpu, unsigned long ga, u8 ar,
>  	}
>  out_unlock:
>  	if (need_ipte_lock)
> -		ipte_unlock(vcpu);
> +		ipte_unlock(vcpu->kvm);
>  	if (nr_pages > ARRAY_SIZE(gpa_array))
>  		vfree(gpas);
>  	return rc;
> @@ -1199,10 +1199,10 @@ int check_gva_range(struct kvm_vcpu *vcpu, unsigned long gva, u8 ar,
>  	rc = get_vcpu_asce(vcpu, &asce, gva, ar, mode);
>  	if (rc)
>  		return rc;
> -	ipte_lock(vcpu);
> +	ipte_lock(vcpu->kvm);
>  	rc = guest_range_to_gpas(vcpu, gva, ar, NULL, length, asce, mode,
>  				 access_key);
> -	ipte_unlock(vcpu);
> +	ipte_unlock(vcpu->kvm);
>  
>  	return rc;
>  }
> @@ -1465,7 +1465,7 @@ int kvm_s390_shadow_fault(struct kvm_vcpu *vcpu, struct gmap *sg,
>  	 * tables/pointers we read stay valid - unshadowing is however
>  	 * always possible - only guest_table_lock protects us.
>  	 */
> -	ipte_lock(vcpu);
> +	ipte_lock(vcpu->kvm);
>  
>  	rc = gmap_shadow_pgt_lookup(sg, saddr, &pgt, &dat_protection, &fake);
>  	if (rc)
> @@ -1499,7 +1499,7 @@ int kvm_s390_shadow_fault(struct kvm_vcpu *vcpu, struct gmap *sg,
>  	pte.p |= dat_protection;
>  	if (!rc)
>  		rc = gmap_shadow_page(sg, saddr, __pte(pte.val));
> -	ipte_unlock(vcpu);
> +	ipte_unlock(vcpu->kvm);
>  	mmap_read_unlock(sg->mm);
>  	return rc;
>  }
> diff --git a/arch/s390/kvm/gaccess.h b/arch/s390/kvm/gaccess.h
> index 1124ff282012..9408d6cc8e2c 100644
> --- a/arch/s390/kvm/gaccess.h
> +++ b/arch/s390/kvm/gaccess.h
> @@ -440,9 +440,9 @@ int read_guest_real(struct kvm_vcpu *vcpu, unsigned long gra, void *data,
>  	return access_guest_real(vcpu, gra, data, len, 0);
>  }
>  
> -void ipte_lock(struct kvm_vcpu *vcpu);
> -void ipte_unlock(struct kvm_vcpu *vcpu);
> -int ipte_lock_held(struct kvm_vcpu *vcpu);
> +void ipte_lock(struct kvm *kvm);
> +void ipte_unlock(struct kvm *kvm);
> +int ipte_lock_held(struct kvm *kvm);
>  int kvm_s390_check_low_addr_prot_real(struct kvm_vcpu *vcpu, unsigned long gra);
>  
>  /* MVPG PEI indication bits */
> diff --git a/arch/s390/kvm/priv.c b/arch/s390/kvm/priv.c
> index 83bb5cf97282..12c464c7cddf 100644
> --- a/arch/s390/kvm/priv.c
> +++ b/arch/s390/kvm/priv.c
> @@ -442,7 +442,7 @@ static int handle_ipte_interlock(struct kvm_vcpu *vcpu)
>  	vcpu->stat.instruction_ipte_interlock++;
>  	if (psw_bits(vcpu->arch.sie_block->gpsw).pstate)
>  		return kvm_s390_inject_program_int(vcpu, PGM_PRIVILEGED_OP);
> -	wait_event(vcpu->kvm->arch.ipte_wq, !ipte_lock_held(vcpu));
> +	wait_event(vcpu->kvm->arch.ipte_wq, !ipte_lock_held(vcpu->kvm));
>  	kvm_s390_retry_instr(vcpu);
>  	VCPU_EVENT(vcpu, 4, "%s", "retrying ipte interlock operation");
>  	return 0;
> @@ -1471,7 +1471,7 @@ static int handle_tprot(struct kvm_vcpu *vcpu)
>  	access_key = (operand2 & 0xf0) >> 4;
>  
>  	if (vcpu->arch.sie_block->gpsw.mask & PSW_MASK_DAT)
> -		ipte_lock(vcpu);
> +		ipte_lock(vcpu->kvm);
>  
>  	ret = guest_translate_address_with_key(vcpu, address, ar, &gpa,
>  					       GACC_STORE, access_key);
> @@ -1508,7 +1508,7 @@ static int handle_tprot(struct kvm_vcpu *vcpu)
>  	}
>  
>  	if (vcpu->arch.sie_block->gpsw.mask & PSW_MASK_DAT)
> -		ipte_unlock(vcpu);
> +		ipte_unlock(vcpu->kvm);
>  	return ret;
>  }
>