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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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@xxxxxxxxxxxxx>

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




[Index of Archives]     [KVM ARM]     [KVM ia64]     [KVM ppc]     [Virtualization Tools]     [Spice Development]     [Libvirt]     [Libvirt Users]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite Questions]     [Linux Kernel]     [Linux SCSI]     [XFree86]

  Powered by Linux