On Wed, 5 Dec 2018 10:57:33 +0100
Michael Mueller <mimu@xxxxxxxxxxxxx> wrote:
>
>
> On 05.12.18 10:09, Halil Pasic wrote:
> > On Fri, 30 Nov 2018 15:32:12 +0100
> > Michael Mueller <mimu@xxxxxxxxxxxxx> wrote:
> >
> >> This function processes the gib alert list. It is required to
> >> run when either a gib alert interruption has been received or
> >> a gisa that might be in the alert list is cleared or dropped.
> >>
> >> The GIB alert list contains all GISAs that have pending
> >> adapter interruptions.
> >
> > I'm not sure this last paragraph is 100% correct. If e.g. all vcpus
> > of a guest are in SIE we don't ask for alerting, and we would not get
> > alerted when an IPM bit is set in the GISA.
> >
>
> I will discard the "all" to make it 99.999% correct.
>
:). It ain't important. But discarding "all" helps only marginally. I
think we can have GISAs with IPM all zeros in the list. If I were to try
being constructive, I would propose something like: " The GIB alert list
contains GISAs of guests that may need a some vCPUS kicked
so their GISA interrupts can be delivered". But it really ain't
important. Feel free to keep it as is.
BTW have you noticed my other comment below? It is neither cut of (which
I would interpret as treated separately) nor answered.
Halil
> >>
> >> Signed-off-by: Michael Mueller <mimu@xxxxxxxxxxxxx>
> >> Reviewed-by: Cornelia Huck <cohuck@xxxxxxxxxx>
> >> Reviewed-by: Pierre Morel <pmorel@xxxxxxxxxxxxx>
> >> ---
> >> arch/s390/kvm/interrupt.c | 50 +++++++++++++++++++++++++++++++++++++++++++++++
> >> 1 file changed, 50 insertions(+)
> >>
> >> diff --git a/arch/s390/kvm/interrupt.c b/arch/s390/kvm/interrupt.c
> >> index 251d01f1e9bf..37a5df4e8dac 100644
> >> --- a/arch/s390/kvm/interrupt.c
> >> +++ b/arch/s390/kvm/interrupt.c
> >> @@ -2900,6 +2900,56 @@ static void nullify_gisa(struct kvm_s390_gisa *gisa)
> >> gisa->next_alert = (u32)(u64)gisa;
> >> }
> >>
> >> +#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_vcpu *vcpu;
> >> + 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.
> >> + */
> >> + for (origin = xchg(&gib->alert_list_origin,
> >> + (!final) ? NONE_GISA_ADDR : NULL_GISA_ADDR);
> >> + /* Loop through the just cut-off alert list. */
> >> + origin & GISA_ADDR_MASK;
> >> + origin = next_alert) {
> >> + gisa = (struct kvm_s390_gisa *)(u64)origin;
> >> + next_alert = gisa->next_alert;
> >> + /* Unlink the GISA from the alert list. */
> >> + gisa->next_alert = origin;
> >> + if (!kvm_s390_gisa_get_ipm(gisa))
> >> + continue;
> >> + /*
> >> + * Wake-up an idle vcpu of the kvm this GISA
> >> + * belongs to if available.
> >> + */
> >> + kvm = container_of(gisa, struct sie_page2, gisa)->kvm;
> >> + vcpu = __find_vcpu_for_floating_irq(kvm);
> >> + if (vcpu)
> >> + kvm_s390_vcpu_wakeup(vcpu);
> >
> > I don't quite understand the logic here. You seem to wake up the first
> > idle vcpu, or the next round-robin vcpu (I guess the rr one is actually
> > in SIE). What I would expect is making an attempt at delivering all irqs
> > in the IPM. Since vcpus can mask ISCs a scenario can be constructed where
> > more than one vcpu shuld be woken up.
> >
> > Or am I misunderstanding something?
> >
> > Regards,
> > Halil
> >
> >> + }
> >> + } while (!final);
> >> +}
> >> +
> >> void kvm_s390_gisa_clear(struct kvm *kvm)
> >> {
> >> if (kvm->arch.gisa) {
> >
>
