Re: [PATCH v5 10/15] KVM: s390: add functions to (un)register GISC with GISA

[Pierre Morel]

On 19/12/2018 20:17, Michael Mueller wrote:
> Add the IAM (Interruption Alert Mask) to the architecture specific
> kvm struct. This mask in the GISA is used to define for which ISC
> a GIB alert can be issued.
>
> The functions kvm_s390_gisc_register() and kvm_s390_gisc_unregister()
> are used to (un)register a GISC (guest ISC) with a virtual machine and
> its GISA.
>
> Upon successful completion, kvm_s390_gisc_register() returns the
> ISC to be used for GIB alert interruptions. A negative return code
> indicates an error during registration.
>
> Theses functions will be used by other adapter types like AP and PCI to
> request pass-through interruption support.
>
> Signed-off-by: Michael Mueller <mimu@xxxxxxxxxxxxx>
> ---
>   arch/s390/include/asm/kvm_host.h |  9 ++++++
>   arch/s390/kvm/interrupt.c        | 66 ++++++++++++++++++++++++++++++++++++++++
>   2 files changed, 75 insertions(+)
>
> diff --git a/arch/s390/include/asm/kvm_host.h b/arch/s390/include/asm/kvm_host.h
> index 0deba3ae8bc7..2c1259da3636 100644
> --- a/arch/s390/include/asm/kvm_host.h
> +++ b/arch/s390/include/asm/kvm_host.h
> @@ -782,6 +782,9 @@ struct kvm_s390_gisa {
>   			u8  reserved03[11];
>   			u32 airq_count;
>   		} g1;
> +		struct {
> +			u64 word[4];
> +		} u64;
>   	};
>   };
>   
> @@ -849,6 +852,9 @@ struct kvm_arch{
>   	DECLARE_BITMAP(cpu_feat, KVM_S390_VM_CPU_FEAT_NR_BITS);
>   	struct kvm_s390_gisa *gisa;
>   	int gib_in_use;
> +	u8 iam;
> +	u32 iam_ref_count[MAX_ISC + 1];
> +	spinlock_t iam_ref_lock;
>   };
>   
>   #define KVM_HVA_ERR_BAD		(-1UL)
> @@ -882,6 +888,9 @@ void kvm_arch_crypto_set_masks(struct kvm *kvm, unsigned long *apm,
>   extern int sie64a(struct kvm_s390_sie_block *, u64 *);
>   extern char sie_exit;
>   
> +extern int kvm_s390_gisc_register(struct kvm *kvm, u32 gisc);
> +extern int kvm_s390_gisc_unregister(struct kvm *kvm, u32 gisc);
> +
>   static inline void kvm_arch_hardware_disable(void) {}
>   static inline void kvm_arch_check_processor_compat(void *rtn) {}
>   static inline void kvm_arch_sync_events(struct kvm *kvm) {}
> diff --git a/arch/s390/kvm/interrupt.c b/arch/s390/kvm/interrupt.c
> index 5a54360cecfe..1cc3ad2e6c7e 100644
> --- a/arch/s390/kvm/interrupt.c
> +++ b/arch/s390/kvm/interrupt.c
> @@ -229,6 +229,25 @@ static inline u8 int_word_to_isc(u32 int_word)
>    */
>   #define IPM_BIT_OFFSET (offsetof(struct kvm_s390_gisa, ipm) * BITS_PER_BYTE)
>   
> +static inline int set_iam(struct kvm_s390_gisa *gisa, u8 iam)
> +{
> +	u64 word0, _word0;
> +
> +	do {
> +		word0 = READ_ONCE(gisa->u64.word[0]);
> +		/* If the GISA is in the alert list, do nothing. */
> +		if ((u64)gisa != word0 >> 32)
> +			return -EBUSY;

In kvm_s390_gisa_clear() you do:

+        nullify_gisa(kvm->arch.gisa);
+        set_iam(kvm->arch.gisa, kvm->arch.iam);

IIUC this call to set_iam() will always fail.


> +		/*
> +		 * Try to set the IAM or loop, if the IPM has changed
> +		 * or the GISA has been inserted into the alert list.
> +		 */
> +		_word0 = (word0 & ~0xffUL) | iam;
> +	} while (cmpxchg(&gisa->u64.word[0], word0, _word0) != _word0); > +
> +	return 0;
> +}
> +
>   static inline void set_ipm_gisc(struct kvm_s390_gisa *gisa, u32 gisc)
>   {
>   	set_bit_inv(IPM_BIT_OFFSET + gisc, (unsigned long *) gisa);
> @@ -2917,6 +2936,8 @@ void kvm_s390_gisa_init(struct kvm *kvm)
>   	if (!css_general_characteristics.aiv)
>   		return;
>   	kvm->arch.gisa = &kvm->arch.sie_page2->gisa;
> +	kvm->arch.iam = 0;
> +	spin_lock_init(&kvm->arch.iam_ref_lock);
>   	nullify_gisa(kvm->arch.gisa);
>   	kvm->arch.gib_in_use = !!gib;
>   	VM_EVENT(kvm, 3, "gisa 0x%pK initialized", kvm->arch.gisa);
> @@ -2929,6 +2950,51 @@ void kvm_s390_gisa_destroy(struct kvm *kvm)
>   	kvm->arch.gisa = NULL;
>   }
>   
> +int kvm_s390_gisc_register(struct kvm *kvm, u32 gisc)
> +{
> +	if (!kvm->arch.gib_in_use)
> +		return -ENODEV;
> +	if (gisc > MAX_ISC)
> +		return -ERANGE;
> +
> +	spin_lock(&kvm->arch.iam_ref_lock);
> +	if (kvm->arch.iam_ref_count[gisc] == 0)
> +		kvm->arch.iam |= 0x80 >> gisc;
> +	kvm->arch.iam_ref_count[gisc]++;
> +	if (kvm->arch.iam_ref_count[gisc] == 1)
> +		set_iam(kvm->arch.gisa, kvm->arch.iam);

testing the set_iam return value?
Even it should be fine if the caller works correctly, this is done 
before GISA is ever used.

> +	spin_unlock(&kvm->arch.iam_ref_lock);
> +
> +	return gib->nisc;
> +}
> +EXPORT_SYMBOL_GPL(kvm_s390_gisc_register);
> +
> +int kvm_s390_gisc_unregister(struct kvm *kvm, u32 gisc)
> +{
> +	int rc = 0;
> +
> +	if (!kvm->arch.gib_in_use)
> +		return -ENODEV;
> +	if (gisc > MAX_ISC)
> +		return -ERANGE;
> +
> +	spin_lock(&kvm->arch.iam_ref_lock);
> +	if (kvm->arch.iam_ref_count[gisc] == 0) {
> +		rc = -EINVAL;
> +		goto out;
> +	}
> +	kvm->arch.iam_ref_count[gisc]--;
> +	if (kvm->arch.iam_ref_count[gisc] == 0) {
> +		kvm->arch.iam &= ~(0x80 >> gisc);
> +		set_iam(kvm->arch.gisa, kvm->arch.iam);
> +	}
> +out:
> +	spin_unlock(&kvm->arch.iam_ref_lock);
> +
> +	return rc;
> +}
> +EXPORT_SYMBOL_GPL(kvm_s390_gisc_unregister);
> +
>   void kvm_s390_gib_destroy(void)
>   {
>   	if (!gib)


--
Pierre Morel
Linux/KVM/QEMU in BÃblingen - Germany