Re: [PATCH v5 13/15] KVM: s390: add function process_gib_alert_list()
From: Michael Mueller
Date: Mon Jan 07 2019 - 14:18:14 EST
On 03.01.19 15:43, Pierre Morel wrote:
On 19/12/2018 20:17, Michael Mueller 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@xxxxxxxxxxxxx>
---
 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)
static inline ?
+{
+ÂÂÂ 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;
+ÂÂÂÂÂÂÂ }
+ÂÂÂ }
+
+ÂÂÂ 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)
static inline ?
+{
+ÂÂÂ 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 we do not get an IPM here, it must have been stolen by the firmware
for delivery to the guest.
Yes, a running SIE instance took it before we were able to. But is
it still running now? It could have gone to WAIT before we see
that the IPM is clean. Then it was restored already. Otherwise,
it is still running and will go WAIT and then restore the IAM.
I will do some tests on this.
Then why restoring the IAM?
Or do I miss something?
+ÂÂÂ 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;
+
+ÂÂÂ /*
+ÂÂÂÂ * 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;
+
+ÂÂÂ /*
+ÂÂÂÂ * 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);
I do not understand why we need to set IAM here.
The interrupt will be delivered by the firmware as soon as the PSW or
CR6 is changed by any vCPU.
...and if this does not happen we can not deliver the interrupt anyway.
+}
+
+#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;
AFAIU this enable GISA interrupt for the guest...
Only together with the IAM being set what could happen if
__floating_airqs_kick() calls get_ipm and the IPM is clean already. :(
+ÂÂÂÂÂÂÂÂÂÂÂ kvm = container_of(gisa, struct sie_page2, gisa)->kvm;
+ÂÂÂÂÂÂÂÂÂÂÂ /* Kick suitable vcpus */
+ÂÂÂÂÂÂÂÂÂÂÂ __floating_airqs_kick(kvm);
...and here we kick a VCPU for the guest.
Logically I would do it in the otherway, first kicking the vCPU then
enabling the GISA interruption again.
If the IPM bit is cleared by the firmware during delivering the
interrupt to the guest before we enter get_ipm() called by
__floating_airqs_kick() we will set the IAM despite we have a running
CPU handling the IRQ.
I will move the unlink below the kick that will assure get_ipm will
never take the IAM restore path.
In the worst case we can also set the IAM with the GISA in the alert list.
Or we must accept that the firmware can deliver the IPM as soon as we
reset the GISA next field.
See statement above.
+ÂÂÂÂÂÂÂÂÂÂÂ origin = next_alert;
+ÂÂÂÂÂÂÂ }
+ÂÂÂ } while (!final);
+}
+
 static void nullify_gisa(struct kvm_s390_gisa *gisa)
 {
ÂÂÂÂÂ memset(gisa, 0, sizeof(struct kvm_s390_gisa));
I think that avoiding to restore the IAM during the call to get_ipm
inside __floating_airqs_kick() would good.
If you agree, with that:
Reviewed-by: Pierre Morel<pmorel@xxxxxxxxxxxxx>