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

Pierre Morel
2019-01-09T11:39:06Z

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@linux.ibm.com>

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