Messages in this thread | | | Subject | Re: [PATCH v5 13/15] KVM: s390: add function process_gib_alert_list() | From | Pierre Morel <> | Date | Wed, 9 Jan 2019 12:39:06 +0100 |
| |
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
| |