[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-Id: <68fae932-b589-f978-3a4a-3bc6643227bb@linux.ibm.com>
Date: Wed, 9 Jan 2019 12:39:06 +0100
From: Pierre Morel <pmorel@...ux.ibm.com>
To: mimu@...ux.ibm.com, KVM Mailing List <kvm@...r.kernel.org>
Cc: Linux-S390 Mailing List <linux-s390@...r.kernel.org>,
linux-kernel@...r.kernel.org,
kvm390-list@...maker.boeblingen.de.ibm.com,
Martin Schwidefsky <schwidefsky@...ibm.com>,
Heiko Carstens <heiko.carstens@...ibm.com>,
Christian Borntraeger <borntraeger@...ibm.com>,
Janosch Frank <frankja@...ux.ibm.com>,
David Hildenbrand <david@...hat.com>,
Cornelia Huck <cohuck@...hat.com>,
Halil Pasic <pasic@...ux.ibm.com>
Subject: Re: [PATCH v5 13/15] KVM: s390: add function process_gib_alert_list()
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@...ux.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
Powered by blists - more mailing lists