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;
Message-ID: <d986a43b-5828-8145-ff91-1a507434de8b@linux.ibm.com>
Date:   Fri, 4 Feb 2022 12:12:02 +0100
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Thunderbird/91.3.0
Content-Language: en-US
To:     Michael Mueller <mimu@linux.ibm.com>, kvm@vger.kernel.org
Cc:     cohuck@redhat.com, borntraeger@de.ibm.com, thuth@redhat.com,
        pasic@linux.ibm.com, david@redhat.com, linux-s390@vger.kernel.org,
        linux-kernel@vger.kernel.org
References: <20220204085203.3490425-1-mimu@linux.ibm.com>
From:   Janosch Frank <frankja@linux.ibm.com>
Subject: Re: [PATCH] KVM: s390: pv: make use of ultravisor AIV support
In-Reply-To: <20220204085203.3490425-1-mimu@linux.ibm.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk

On 2/4/22 09:52, Michael Mueller wrote:
> This patch enables the ultravisor adapter interruption vitualization
> support indicated by UV feature BIT_UV_FEAT_AIV. This allows ISC
> interruption injection directly into the GISA IPM for PV kvm guests.
> 
> Hardware that does not support this feature will continue to use the
> UV interruption interception method to deliver ISC interruptions to
> PV kvm guests. For this purpose, the ECA_AIV bit for all guest cpus
> will be cleared and the GISA will be disabled during PV guest setup.
> 
> In addition a check in __inject_io() has been removed. That reduces the
> required intructions for interruption handling for PV and traditional
> kvm guests.
> 
> Signed-off-by: Michael Mueller <mimu@linux.ibm.com>
> ---
>   arch/s390/include/asm/uv.h |  1 +
>   arch/s390/kvm/interrupt.c  | 53 +++++++++++++++++++++++++++++++++-----
>   arch/s390/kvm/kvm-s390.c   | 10 ++++---
>   arch/s390/kvm/kvm-s390.h   | 11 ++++++++
>   4 files changed, 66 insertions(+), 9 deletions(-)
> 
> diff --git a/arch/s390/include/asm/uv.h b/arch/s390/include/asm/uv.h
> index 72d3e49c2860..0cb2bbb50ad7 100644
> --- a/arch/s390/include/asm/uv.h
> +++ b/arch/s390/include/asm/uv.h
> @@ -80,6 +80,7 @@ enum uv_cmds_inst {
>   
>   enum uv_feat_ind {
>   	BIT_UV_FEAT_MISC = 0,
> +	BIT_UV_FEAT_AIV = 1,
>   };
>   
>   struct uv_cb_header {
> diff --git a/arch/s390/kvm/interrupt.c b/arch/s390/kvm/interrupt.c
> index c3bd993fdd0c..6ff80069b1fe 100644
> --- a/arch/s390/kvm/interrupt.c
> +++ b/arch/s390/kvm/interrupt.c
> @@ -1900,13 +1900,12 @@ static int __inject_io(struct kvm *kvm, struct kvm_s390_interrupt_info *inti)
>   	isc = int_word_to_isc(inti->io.io_int_word);
>   
>   	/*
> -	 * Do not make use of gisa in protected mode. We do not use the lock
> -	 * checking variant as this is just a performance optimization and we
> -	 * do not hold the lock here. This is ok as the code will pick
> -	 * interrupts from both "lists" for delivery.
> +	 * We do not use the lock checking variant as this is just a
> +	 * performance optimization and we do not hold the lock here.
> +	 * This is ok as the code will pick interrupts from both "lists"
> +	 * for delivery.
>   	 */
> -	if (!kvm_s390_pv_get_handle(kvm) &&
> -	    gi->origin && inti->type & KVM_S390_INT_IO_AI_MASK) {
> +	if (gi->origin && inti->type & KVM_S390_INT_IO_AI_MASK) {

This was previously fenced by the UV by ignoring the gd altogether, right?

>   		VM_EVENT(kvm, 4, "%s isc %1u", "inject: I/O (AI/gisa)", isc);
>   		gisa_set_ipm_gisc(gi->origin, isc);
>   		kfree(inti);
> @@ -3163,9 +3162,32 @@ void kvm_s390_gisa_init(struct kvm *kvm)
>   	VM_EVENT(kvm, 3, "gisa 0x%pK initialized", gi->origin);
>   }
>   
> +void kvm_s390_gisa_enable(struct kvm *kvm)
> +{
> +	struct kvm_s390_gisa_interrupt *gi = &kvm->arch.gisa_int;
> +	struct kvm_vcpu *vcpu;
> +	int i;
> +
> +	if (gi->origin)
> +		return;
> +	kvm_s390_gisa_init(kvm);
> +	kvm_for_each_vcpu(i, vcpu, kvm) {
> +		mutex_lock(&vcpu->mutex);
> +		vcpu->arch.sie_block->gd = kvm_s390_get_gisa_desc(kvm);
> +		if (vcpu->arch.sie_block->gd) {
> +			vcpu->arch.sie_block->eca |= ECA_AIV;
> +			VCPU_EVENT(vcpu, 3, "AIV gisa format-%u enabled for cpu %03u",
> +				   vcpu->arch.sie_block->gd & 0x3, vcpu->vcpu_id);
> +		}
> +		mutex_unlock(&vcpu->mutex);
> +	}
> +}
> +
> +
>   void kvm_s390_gisa_destroy(struct kvm *kvm)
>   {
>   	struct kvm_s390_gisa_interrupt *gi = &kvm->arch.gisa_int;
> +	struct kvm_s390_gisa *gisa = gi->origin;
>   
>   	if (!gi->origin)
>   		return;
> @@ -3176,6 +3198,25 @@ void kvm_s390_gisa_destroy(struct kvm *kvm)
>   		cpu_relax();
>   	hrtimer_cancel(&gi->timer);
>   	gi->origin = NULL;
> +	VM_EVENT(kvm, 3, "gisa 0x%pK destroyed", gisa);
> +}
> +
> +void kvm_s390_gisa_disable(struct kvm *kvm)
> +{
> +	struct kvm_s390_gisa_interrupt *gi = &kvm->arch.gisa_int;
> +	struct kvm_vcpu *vcpu;
> +	int i;
> +
> +	if (!gi->origin)
> +		return;
> +	kvm_for_each_vcpu(i, vcpu, kvm) {
> +		mutex_lock(&vcpu->mutex);
> +		vcpu->arch.sie_block->eca &= ~ECA_AIV;
> +		vcpu->arch.sie_block->gd = 0U;
> +		mutex_unlock(&vcpu->mutex);
> +		VCPU_EVENT(vcpu, 3, "AIV disabled for cpu %03u", vcpu->vcpu_id);
> +	}
> +	kvm_s390_gisa_destroy(kvm);
>   }
>   
>   /**
> diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
> index 14a18ba5ff2c..8839a58e03c7 100644
> --- a/arch/s390/kvm/kvm-s390.c
> +++ b/arch/s390/kvm/kvm-s390.c
> @@ -2273,6 +2273,9 @@ static int kvm_s390_handle_pv(struct kvm *kvm, struct kvm_pv_cmd *cmd)
>   		if (r)
>   			break;
>   
> +		if (!test_bit_inv(BIT_UV_FEAT_AIV, &uv_info.uv_feature_indications))
> +			kvm_s390_gisa_disable(kvm);
> +

If the kvm_s390_cpus_to_pv() fails we'd miss a gisa_enable(), no?

And we might have a problem in kvm_s390_vcpu_setup() as we don't fence 
it for new cpus.


>   		r = kvm_s390_pv_init_vm(kvm, &cmd->rc, &cmd->rrc);
>   		if (r)
>   			break;
> @@ -2300,6 +2303,9 @@ static int kvm_s390_handle_pv(struct kvm *kvm, struct kvm_pv_cmd *cmd)
>   			break;
>   		r = kvm_s390_pv_deinit_vm(kvm, &cmd->rc, &cmd->rrc);
>   
> +		if (use_gisa)
> +			kvm_s390_gisa_enable(kvm);
> +
>   		/* no need to block service interrupts any more */
>   		clear_bit(IRQ_PEND_EXT_SERVICE, &kvm->arch.float_int.masked_irqs);
>   		break;
> @@ -3309,9 +3315,7 @@ int kvm_arch_vcpu_create(struct kvm_vcpu *vcpu)
>   
>   	vcpu->arch.sie_block->icpua = vcpu->vcpu_id;
>   	spin_lock_init(&vcpu->arch.local_int.lock);
> -	vcpu->arch.sie_block->gd = (u32)(u64)vcpu->kvm->arch.gisa_int.origin;
> -	if (vcpu->arch.sie_block->gd && sclp.has_gisaf)
> -		vcpu->arch.sie_block->gd |= GISA_FORMAT1;
> +	vcpu->arch.sie_block->gd = kvm_s390_get_gisa_desc(vcpu->kvm);
>   	seqcount_init(&vcpu->arch.cputm_seqcount);
>   
>   	vcpu->arch.pfault_token = KVM_S390_PFAULT_TOKEN_INVALID;
> diff --git a/arch/s390/kvm/kvm-s390.h b/arch/s390/kvm/kvm-s390.h
> index c07a050d757d..08a622a44f6f 100644
> --- a/arch/s390/kvm/kvm-s390.h
> +++ b/arch/s390/kvm/kvm-s390.h
> @@ -217,6 +217,15 @@ static inline void kvm_s390_set_user_cpu_state_ctrl(struct kvm *kvm)
>   	kvm->arch.user_cpu_state_ctrl = 1;
>   }
>   
> +static inline u32 kvm_s390_get_gisa_desc(struct kvm *kvm)
> +{
> +	u32 gd = (u32)(u64)kvm->arch.gisa_int.origin;
> +
> +	if (gd && sclp.has_gisaf)
> +		gd |= GISA_FORMAT1;
> +	return gd;
> +}
> +
>   /* implemented in pv.c */
>   int kvm_s390_pv_destroy_cpu(struct kvm_vcpu *vcpu, u16 *rc, u16 *rrc);
>   int kvm_s390_pv_create_cpu(struct kvm_vcpu *vcpu, u16 *rc, u16 *rrc);
> @@ -435,6 +444,8 @@ int kvm_s390_get_irq_state(struct kvm_vcpu *vcpu,
>   void kvm_s390_gisa_init(struct kvm *kvm);
>   void kvm_s390_gisa_clear(struct kvm *kvm);
>   void kvm_s390_gisa_destroy(struct kvm *kvm);
> +void kvm_s390_gisa_disable(struct kvm *kvm);
> +void kvm_s390_gisa_enable(struct kvm *kvm);
>   int kvm_s390_gib_init(u8 nisc);
>   void kvm_s390_gib_destroy(void);
>   
>