Re: [PATCH] KVM: s390: fix gisa destroy operation might lead to cpu stalls

[Michael Mueller <mimu@linux.ibm.com>]

On 23.08.23 18:16, Claudio Imbrenda wrote:
> On Wed, 23 Aug 2023 18:01:59 +0200
> Alexander Gordeev <agordeev@linux.ibm.com> wrote:
> 
>> On Wed, Aug 23, 2023 at 04:09:26PM +0200, Michael Mueller wrote:
>>>
>>>
>>> On 23.08.23 15:23, Alexander Gordeev wrote:
>>>> On Wed, Aug 23, 2023 at 02:41:40PM +0200, Michael Mueller wrote:
>>>> ...
>>>>> diff --git a/arch/s390/kvm/interrupt.c b/arch/s390/kvm/interrupt.c
>>>>> index 9bd0a873f3b1..73153bea6c24 100644
>>>>> --- a/arch/s390/kvm/interrupt.c
>>>>> +++ b/arch/s390/kvm/interrupt.c
>>>>> @@ -3205,8 +3205,10 @@ void kvm_s390_gisa_destroy(struct kvm *kvm)
>>>>>    	if (gi->alert.mask)
>>>>>    		KVM_EVENT(3, "vm 0x%pK has unexpected iam 0x%02x",
>>>>>    			  kvm, gi->alert.mask);
>>>>> -	while (gisa_in_alert_list(gi->origin))
>>>>> -		cpu_relax();
>>>>> +	while (gisa_in_alert_list(gi->origin)) {
>>>>> +		KVM_EVENT(3, "vm 0x%pK gisa in alert list during destroy", kvm);
>>>>> +		process_gib_alert_list();
>>>>
>>>> process_gib_alert_list() has two nested loops and neither of them
>>>> does cpu_relax(). I guess, those are needed instead of one you remove?
>>>
>>> Calling function process_gib_alert_list() guarantees the gisa
>>> is taken out of the alert list immediately and thus the potential
>>> endless loop on gisa_in_alert_list() is solved. The issue surfaced
>>> with the following patch that accidently disabled the GAL interrupt
>>> processing on the host that normaly handles the alert list.
>>> The patch has been reverted from devel and will be re-applied in v2.
>>>
>>> 88a096a7a460 Revert "s390/airq: remove lsi_mask from airq_struct"
>>> a9d17c5d8813 s390/airq: remove lsi_mask from airq_struct
>>>
>>> Does that make sense for you?
>>
>> Not really. If process_gib_alert_list() does guarantee the removal,
>> then it should be a condition, not the loop.
> 
> this is actually a good question. why is it still a loop?

The reason for the loop aproach was that I was not sure if any late
incoming interrupts would bring the gisa back into the alert list
by the firmware.

I verified that this cannot happen if the mask that restores the
interruption alert mask (IAM) is properly set to 0x00 by the last
device driver de-registration before gisa destruction.

In addition I now enforce it to be 0x00 if not already done. (That would
be a bug.) That finally means the *if in_alert_list then 
process_alert_list* is sufficient.

I will send a v2.

> 
>>
>> But I am actually not into this code. Just wanted to point out that
>> cpu_relax() is removed from this loop and the two other loops within
>> process_gib_alert_list() do not have it either.
>>
>> So up to Christian, Janosch and Claudio.
>