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

[Halil Pasic]

On Mon, 7 Jan 2019 20:18:03 +0100
Michael Mueller <mimu@xxxxxxxxxxxxx> wrote:

> 
> 
> 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.
> 

I'm very confused by these comments, and especially by your apparent
consensus.

Regards,
Halil