Re: [PATCH v5 13/15] KVM: s390: add function process_gib_alert_list()

[Halil Pasic <pasic@linux.ibm.com>]

On Wed, 19 Dec 2018 20:17:54 +0100
Michael Mueller <mimu@linux.ibm.com> wrote:

> This function processes the Gib Alert List (GAL). It is required
> to run when either a gib alert interruption has been received or
> a gisa that is in the alert list is cleared or dropped.
> 
> The GAL is build up by millicode, when the respective ISC bit is
> set in the Interruption Alert Mask (IAM) and an interruption of
> that class is observed.
> 
> Signed-off-by: Michael Mueller <mimu@linux.ibm.com>
> ---
>  arch/s390/kvm/interrupt.c | 140 ++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 140 insertions(+)
> 
> diff --git a/arch/s390/kvm/interrupt.c b/arch/s390/kvm/interrupt.c
> index 48a93f5e5333..03e7ba4f215a 100644
> --- a/arch/s390/kvm/interrupt.c
> +++ b/arch/s390/kvm/interrupt.c
> @@ -2941,6 +2941,146 @@ int kvm_s390_get_irq_state(struct kvm_vcpu *vcpu, __u8 __user *buf, int len)
>  	return n;
>  }
>  
> +static int __try_airqs_kick(struct kvm *kvm, u8 ipm)
> +{
> +	struct kvm_s390_float_interrupt *fi = &kvm->arch.float_int;
> +	struct kvm_vcpu *vcpu = NULL, *kick_vcpu[MAX_ISC + 1];
> +	int online_vcpus = atomic_read(&kvm->online_vcpus);
> +	u8 ioint_mask, isc_mask, kick_mask = 0x00;
> +	int vcpu_id, kicked = 0;
> +
> +	/* Loop over vcpus in WAIT state. */
> +	for (vcpu_id = find_first_bit(fi->idle_mask, online_vcpus);
> +	     /* Until all pending ISCs have a vcpu open for airqs. */
> +	     (~kick_mask & ipm) && vcpu_id < online_vcpus;
> +	     vcpu_id = find_next_bit(fi->idle_mask, online_vcpus, vcpu_id)) {
> +		vcpu = kvm_get_vcpu(kvm, vcpu_id);
> +		if (psw_ioint_disabled(vcpu))
> +			continue;
> +		ioint_mask = (u8)(vcpu->arch.sie_block->gcr[6] >> 24);
> +		for (isc_mask = 0x80; isc_mask; isc_mask >>= 1) {
> +			/* ISC pending in IPM ? */
> +			if (!(ipm & isc_mask))
> +				continue;
> +			/* vcpu for this ISC already found ? */
> +			if (kick_mask & isc_mask)
> +				continue;
> +			/* vcpu open for airq of this ISC ? */
> +			if (!(ioint_mask & isc_mask))
> +				continue;
> +			/* use this vcpu (for all ISCs in ioint_mask) */
> +			kick_mask |= ioint_mask;
> +			kick_vcpu[kicked++] = vcpu;

Assuming that the vcpu can/will take all ISCs it's currently open for
does not seem right. We kind of rely on this assumption here, or?

> +		}
> +	}
> +
> +	if (vcpu && ~kick_mask & ipm)
> +		VM_EVENT(kvm, 4, "gib alert undeliverable isc mask
> 0x%02x",
> +			 ~kick_mask & ipm);
> +
> +	for (vcpu_id = 0; vcpu_id < kicked; vcpu_id++)
> +		kvm_s390_vcpu_wakeup(kick_vcpu[vcpu_id]);
> +
> +	return (online_vcpus != 0) ? kicked : -ENODEV;
> +}
> +
> +static void __floating_airqs_kick(struct kvm *kvm)
> +{
> +	struct kvm_s390_float_interrupt *fi = &kvm->arch.float_int;
> +	int online_vcpus, kicked;
> +	u8 ipm_t0, ipm;
> +
> +	/* Get IPM and return if clean, IAM has been restored. */
> +	ipm = get_ipm(kvm->arch.gisa, IRQ_FLAG_IAM);
> +	if (!ipm)
> +		return;
> +retry:
> +	ipm_t0 = ipm;
> +
> +	/* Try to kick some vcpus in WAIT state. */
> +	kicked = __try_airqs_kick(kvm, ipm);
> +	if (kicked < 0)
> +		return;
> +
> +	/* Get IPM and return if clean, IAM has been restored. */
> +	ipm = get_ipm(kvm->arch.gisa, IRQ_FLAG_IAM);
> +	if (!ipm)
> +		return;
> +
> +	/* Start over, if new ISC bits are pending in IPM. */
> +	if ((ipm_t0 ^ ipm) & ~ipm_t0)
> +		goto retry;
> +

<MARK A>

> +	/*
> +	 * Return as we just kicked at least one vcpu in WAIT state
> +	 * open for airqs. The IAM will be restored latest when one
> +	 * of them goes into WAIT or STOP state.
> +	 */
> +	if (kicked > 0)
> +		return;

</MARK A>

> +
> +	/*
> +	 * No vcpu was kicked either because no vcpu was in WAIT state
> +	 * or none of the vcpus in WAIT state are open for airqs.
> +	 * Return immediately if no vcpus are in WAIT state.
> +	 * There are vcpus in RUN state. They will process the airqs
> +	 * if not closed for airqs as well. In that case the system will
> +	 * delay airqs until a vcpu decides to take airqs again.
> +	 */
> +	online_vcpus = atomic_read(&kvm->online_vcpus);
> +	if (!bitmap_weight(fi->idle_mask, online_vcpus))
> +		return;
> +
> +	/*
> +	 * None of the vcpus in WAIT state take airqs and we might
> +	 * have no running vcpus as at least one vcpu is in WAIT state
> +	 * and IPM is dirty.
> +	 */
> +	set_iam(kvm->arch.gisa, kvm->arch.iam);
> +}
> +
> +#define NULL_GISA_ADDR 0x00000000UL
> +#define NONE_GISA_ADDR 0x00000001UL
> +#define GISA_ADDR_MASK 0xfffff000UL
> +
> +static void __maybe_unused process_gib_alert_list(void)
> +{
> +	u32 final, next_alert, origin = 0UL;
> +	struct kvm_s390_gisa *gisa;
> +	struct kvm *kvm;
> +
> +	do {
> +		/*
> +		 * If the NONE_GISA_ADDR is still stored in the alert list
> +		 * origin, we will leave the outer loop. No further GISA has
> +		 * been added to the alert list by millicode while processing
> +		 * the current alert list.
> +		 */
> +		final = (origin & NONE_GISA_ADDR);
> +		/*
> +		 * Cut off the alert list and store the NONE_GISA_ADDR in the
> +		 * alert list origin to avoid further GAL interruptions.
> +		 * A new alert list can be build up by millicode in parallel
> +		 * for guests not in the yet cut-off alert list. When in the
> +		 * final loop, store the NULL_GISA_ADDR instead. This will re-
> +		 * enable GAL interruptions on the host again.
> +		 */
> +		origin = xchg(&gib->alert_list_origin,
> +			      (!final) ? NONE_GISA_ADDR : NULL_GISA_ADDR);
> +		/* Loop through the just cut-off alert list. */
> +		while (origin & GISA_ADDR_MASK) {
> +			gisa = (struct kvm_s390_gisa *)(u64)origin;
> +			next_alert = gisa->next_alert;
> +			/* Unlink the GISA from the alert list. */
> +			gisa->next_alert = origin;
> +			kvm = container_of(gisa, struct sie_page2, gisa)->kvm;
> +			/* Kick suitable vcpus */
> +			__floating_airqs_kick(kvm);

We may finish handling the alerted gisa with iam not set e.g.
via some vcpus kicked but ipm still dirty and some vcpus still in wait,
or? 

>From the comments it seems we speculate on being in a safe state, as
these are supposed to return to wait or stop soon-ish, and we will set
iam then (See <MARK A>). I don't quite understand.

According to my current understanding we might end up loosing initiative
in this scenario. Or am I wrong?

Regards,
Halil

> +			origin = next_alert;
> +		}
> +	} while (!final);
> +}
> +
>  static void nullify_gisa(struct kvm_s390_gisa *gisa)
>  {
>  	memset(gisa, 0, sizeof(struct kvm_s390_gisa));