On 31.10.18 13:14, Pierre Morel wrote:
On 25/10/2018 14:37, Michael Mueller wrote:
This function processes a gib alert list. It is required to
run when either a gib alert interruption has been received or
a gisa that might be in the alert list is cleared or dropped.
Signed-off-by: Michael Mueller <mimu@xxxxxxxxxxxxx>
---
arch/s390/kvm/interrupt.c | 38 ++++++++++++++++++++++++++++++++++++++
1 file changed, 38 insertions(+)
diff --git a/arch/s390/kvm/interrupt.c b/arch/s390/kvm/interrupt.c
index 487cad95e2c9..920c065ce1d3 100644
--- a/arch/s390/kvm/interrupt.c
+++ b/arch/s390/kvm/interrupt.c
@@ -2900,6 +2900,44 @@ static void nullify_gisa(struct kvm_s390_gisa
*gisa)
gisa->next_alert = (u32)(u64)gisa;
}
+/*
+ * Before processing, the gib alert list needs to be cut-off from
+ * the gib by means of function unlink_gib_alert_list(). If non NULL,
+ * the list is processed from its latest to oldest entry.
+ *
+ * Processing an gisa entry needs to wake-up a vcpu of the kvm this
gisa
+ * belongs to. Thus, the pending guest interruption will be processed
+ * in SIE context.
+ *
+ * Whenever a gisa is cleared (e.g. on vm reset) or a gisa is dropped
+ * (e.g. on vm termination) it might be part of the gib alert list.
+ * Thus, these operations need to process the alert list as well.
+ */
+static void __maybe_unused process_gib_alert_list(
+ struct kvm_s390_gisa *gisa,
+ struct kvm_s390_gisa *gisa_to_nullify,
+ struct kvm_s390_gisa *gisa_to_drop)
+{
+ struct kvm_s390_gisa *next_alert;
+ struct kvm *kvm;
+
Isn't this function a little bit complex?
I think we can make it clearer.
- The first argument is always AFAIU unlink_gib_alert_list(), then
why not just call this function inside of process_gib_alert_list() ?
That could be done, but my intention was to make that explicit because I
think
that way it is less complex...
I use a clear functional decomposition:
1. get the alert list: unlink_gib_alert_list() -> alert_list
2. process the list: process_gib_alert_list(alert_list, ...)
- The action to do is related to the KVM/GISA, may be some state
inside KVM or GISA to differentiate the processing?
Can you elaborate your thoughts here? I don't understand the question.
All the GISA in this list need to be handled anyway if we are coming
from an IRQ, a CLEAR_IRQ or a RESET but...
Right.
+ for (; gisa; gisa = next_alert) {
+ next_alert = (struct kvm_s390_gisa *)(u64)gisa->next_alert;
+ /* unlink from alert list */
+ gisa->next_alert = (u32)(u64)gisa;
+ /* skip if to clear or drop */
+ if (gisa == gisa_to_nullify ||
+ gisa == gisa_to_drop)
+ continue;
... here:
I am not sure what happens if two guest are being reset at a time,
it seems to me that one is reset and one get kicked.
I will answer that separately...
+ /* wake-up a vcpu of the kvm this gisa belongs to */
+ kvm = container_of(gisa, struct sie_page2, gisa)->kvm;
+ __floating_irq_kick(kvm, KVM_S390_INT_IO(1, 0, 0, 0));
Do we really need to insert a floating interrupt?
Wouldn't a simple kvm_s390_vcpu_wakeup() after choosing the vcpu be
enough?
We need to find a suitable vcpu before calling kvm_s390_vcpu_wakeup().
That is exactly what __floating_irq_kick() does for us, right?
It is *not* inserting a floating interruption.
+ }
+
+ if (gisa_to_nullify)
+ nullify_gisa(gisa_to_nullify);
we already reset the next_alert pointer
if we want to clear the interrupt we just need to clear IPM
Compare with p. 17-172, all remaining fields should be set to zero,
except the next_alert pointer. A can do some calculations to clear
the gisa up to its end without touching the first word. But the
nullify routine is currently also used in the gisa init case that
needs to set the next_alert address initially...
+}
+
void kvm_s390_gisa_clear(struct kvm *kvm)
{
if (kvm->arch.gisa) {
--
Mit freundlichen Grüßen / Kind regards
Michael Müller
IBM Deutschland Research & Development GmbH
Vorsitzender des Aufsichtsrats: Martina Köderitz
Geschäftsführung: Dirk Wittkopp
Sitz der Gesellschaft: Böblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294
