On 25/10/2018 14:37, Michael Mueller wrote:
The patch implements a handler for GIB alert interruptions
on the host. Its task is to alert storage backed guests that
interrupts are pending for them.
A GIB alert interrupt statistic counter is added as well:
$ cat /proc/interrupts
CPU0 CPU1
...
GAL: 0 0 [I/O] GIB Alert
...
Signed-off-by: Michael Mueller <mimu@xxxxxxxxxxxxx>
---
arch/s390/include/asm/irq.h | 1 +
arch/s390/include/asm/isc.h | 1 +
arch/s390/kernel/irq.c | 1 +
arch/s390/kvm/interrupt.c | 45 +++++++++++++++++++++++++++++++++----
arch/s390/kvm/kvm-s390.c | 5 +++++
5 files changed, 49 insertions(+), 4 deletions(-)
diff --git a/arch/s390/include/asm/irq.h b/arch/s390/include/asm/irq.h
index 2f7f27e5493f..afaf5e3c57fd 100644
--- a/arch/s390/include/asm/irq.h
+++ b/arch/s390/include/asm/irq.h
@@ -62,6 +62,7 @@ enum interruption_class {
IRQIO_MSI,
IRQIO_VIR,
IRQIO_VAI,
+ IRQIO_GAL,
NMI_NMI,
CPU_RST,
NR_ARCH_IRQS
diff --git a/arch/s390/include/asm/isc.h b/arch/s390/include/asm/isc.h
index 6cb9e2ed05b6..b2cc1ec78d06 100644
--- a/arch/s390/include/asm/isc.h
+++ b/arch/s390/include/asm/isc.h
@@ -21,6 +21,7 @@
/* Adapter interrupts. */
#define QDIO_AIRQ_ISC IO_SCH_ISC /* I/O subchannel in qdio mode */
#define PCI_ISC 2 /* PCI I/O subchannels */
+#define GAL_ISC 5 /* GIB alert */
#define AP_ISC 6 /* adjunct processor (crypto) devices */
/* Functions for registration of I/O interruption subclasses */
diff --git a/arch/s390/kernel/irq.c b/arch/s390/kernel/irq.c
index 0e8d68bac82c..0cd5a5f96729 100644
--- a/arch/s390/kernel/irq.c
+++ b/arch/s390/kernel/irq.c
@@ -88,6 +88,7 @@ static const struct irq_class irqclass_sub_desc[] = {
{.irq = IRQIO_MSI, .name = "MSI", .desc = "[I/O] MSI Interrupt" },
{.irq = IRQIO_VIR, .name = "VIR", .desc = "[I/O] Virtual I/O Devices"},
{.irq = IRQIO_VAI, .name = "VAI", .desc = "[I/O] Virtual I/O Devices AI"},
+ {.irq = IRQIO_GAL, .name = "GAL", .desc = "[I/O] GIB Alert"},
{.irq = NMI_NMI, .name = "NMI", .desc = "[NMI] Machine Check"},
{.irq = CPU_RST, .name = "RST", .desc = "[CPU] CPU Restart"},
};
diff --git a/arch/s390/kvm/interrupt.c b/arch/s390/kvm/interrupt.c
index 799e232a0cc5..6d0193173388 100644
--- a/arch/s390/kvm/interrupt.c
+++ b/arch/s390/kvm/interrupt.c
@@ -23,6 +23,7 @@
#include <asm/gmap.h>
#include <asm/switch_to.h>
#include <asm/nmi.h>
+#include <asm/airq.h>
#include "kvm-s390.h"
#include "gaccess.h"
#include "trace-s390.h"
@@ -1000,6 +1001,14 @@ static int __must_check __deliver_io(struct kvm_vcpu *vcpu,
goto out;
}
+ /*
+ * The kvm IAM defines all interruption classes that will be
+ * handled by the GIB alert mechanism if in use.
+ */
+ if (vcpu->kvm->arch.gib_in_use &&
+ 0x80 >> isc & vcpu->kvm->arch.iam)
+ goto out;
I am not sure to understand this goto.
We should not receive this interrupt.
What I understand is that we wrongly think we have to send an interrupt
when testing for pending_interrupts() in deliverable_irqs() because we
report the IPM value.
I think we should modify pending_interrupts() to not report the IPM value.
It may cause problem with using GISA for virtio, so we have to revisit
this part too.
+
if (vcpu->kvm->arch.gisa &&
kvm_s390_gisa_tac_ipm_gisc(vcpu->kvm->arch.gisa, isc)) {
/*
@@ -2886,7 +2895,7 @@ int kvm_s390_get_irq_state(struct kvm_vcpu *vcpu, __u8 __user *buf, int len)
return n;
}
-static struct kvm_s390_gisa __maybe_unused *unlink_gib_alert_list(void)
+static struct kvm_s390_gisa *unlink_gib_alert_list(void)
{
if (!gib)
return NULL;
@@ -2913,7 +2922,7 @@ static void nullify_gisa(struct kvm_s390_gisa *gisa)
* (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(
+static void process_gib_alert_list(
struct kvm_s390_gisa *gisa,
struct kvm_s390_gisa *gisa_to_nullify,
struct kvm_s390_gisa *gisa_to_drop)
@@ -2941,7 +2950,8 @@ static void __maybe_unused process_gib_alert_list(
void kvm_s390_gisa_clear(struct kvm *kvm)
{
if (kvm->arch.gisa) {
- nullify_gisa(kvm->arch.gisa);
+ process_gib_alert_list(unlink_gib_alert_list(),
+ kvm->arch.gisa, NULL);
I do not like this process_gib_alert_list() function.
Here for example, we want to get the GISA from the list and clear it but
we have a side effect to generate interrupts to all VM having a
registered GISA.
VM_EVENT(kvm, 3, "gisa 0x%pK cleared", kvm->arch.gisa);
}
}
@@ -2962,6 +2972,7 @@ void kvm_s390_gisa_destroy(struct kvm *kvm)
{
if (!kvm->arch.gisa)
return;
+ process_gib_alert_list(unlink_gib_alert_list(), NULL, kvm->arch.gisa);
Same remark as above here.
Wouldn't it be possible to have separate functions?
kvm->arch.gisa = NULL;
kvm->arch.iam = 0;
}
@@ -3007,10 +3018,22 @@ int kvm_s390_gisc_unregister(struct kvm *kvm, u32 gisc)
}
EXPORT_SYMBOL_GPL(kvm_s390_gisc_unregister);
+static void gib_alert_irq_handler(struct airq_struct *airq)
+{
+ inc_irq_stat(IRQIO_GAL);
+ process_gib_alert_list(unlink_gib_alert_list(), NULL, NULL);
Is it not possible to reenter this IRQ?
+}
+
+static struct airq_struct gib_alert_irq = {
+ .handler = gib_alert_irq_handler,
+ .lsi_ptr = &gib_alert_irq.lsi_mask,
+};
+
void kvm_s390_gib_destroy(void)
{
if (!gib)
return;
I think we should clear the alert list
before unregistering the interruption.
+ unregister_adapter_interrupt(&gib_alert_irq);
We also should unregister the adapter interrupt only after having
unregister the GIB.
chsc_sgib(0);
free_page((unsigned long)gib);
gib = NULL;
@@ -3034,6 +3057,14 @@ void kvm_s390_gib_init(u8 nisc)
return;
}
+ gib_alert_irq.isc = nisc;
+ rc = register_adapter_interrupt(&gib_alert_irq);
+ if (rc) {
+ KVM_EVENT(3, "gib 0x%pK GAI registration failed rc: %d",
+ gib, rc);
+ goto out_free;
+ }
+
gib->nisc = nisc;
rc = chsc_sgib((u32)(u64)gib);
if (rc) {
@@ -3041,8 +3072,14 @@ void kvm_s390_gib_init(u8 nisc)
gib, rc);
free_page((unsigned long)gib);
gib = NULL;
- return;
+ goto out_unreg;
}
KVM_EVENT(3, "gib 0x%pK (nisc=%d) initialized", gib, gib->nisc);
+ return;
+out_unreg:
+ unregister_adapter_interrupt(&gib_alert_irq);
+out_free:
+ free_page((unsigned long)gib);
+ gib = NULL;
}
diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
index 3b358b62bf55..d1f1550c95b4 100644
--- a/arch/s390/kvm/kvm-s390.c
+++ b/arch/s390/kvm/kvm-s390.c
@@ -3360,6 +3360,8 @@ static int vcpu_pre_run(struct kvm_vcpu *vcpu)
}
atomic_inc(&vcpu->kvm->arch.vcpus_in_sie);
+ if (vcpu->kvm->arch.gib_in_use)
+ vcpu->kvm->arch.gisa->iam = 0;
I am not sure that this is race free....
vcpu->arch.sie_block->icptcode = 0;
cpuflags = atomic_read(&vcpu->arch.sie_block->cpuflags);
@@ -3421,6 +3423,9 @@ static int vcpu_post_run(struct kvm_vcpu *vcpu, int exit_reason)
vcpu->run->s.regs.gprs[15] = vcpu->arch.sie_block->gg15;
atomic_dec(&vcpu->kvm->arch.vcpus_in_sie);
+ if (vcpu->kvm->arch.gib_in_use &&
+ !atomic_fetch_andnot(0, &vcpu->kvm->arch.vcpus_in_sie))
+ vcpu->kvm->arch.gisa->iam = vcpu->kvm->arch.iam;
... with this. Is it?
if (exit_reason == -EINTR) {
VCPU_EVENT(vcpu, 3, "%s", "machine check");
--
Pierre Morel
Linux/KVM/QEMU in Böblingen - Germany
