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

[Pierre Morel]

On 07/01/2019 20:18, Michael Mueller 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
...snip...
> +ÂÂÂ 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. :(

confused, AFAIK IAM is used to allow interrupt for the host
not for the guest.

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

!! sorry to have introduce this confusion.
You did it in the right order.
I should have not send these comments after I gave my R-B

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

!! Sorry, you were right.
We must re-enable interrupt before kicking the vcpu, as you did, or the 
vcpu could go to wait before it gets the interrupt.

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

I still think tis assumption is right: We should not set the IAM during 
the kick.

> > If you agree, with that:
> >
> > Reviewed-by: Pierre Morel<pmorel@xxxxxxxxxxxxx>

Still OK with my R-B, as long as w do not set IAM during the kicking.

Regards,
Pierre


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