[PATCH v4 09/10] KVM: s390: add and wire function gib_alert_irq_handler()

[PATCH v4 09/10] KVM: s390: add and wire function gib_alert_irq_handler()

[Michael Mueller]

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:      23         37   [I/O] GIB Alert
  ...

Signed-off-by: Michael Mueller <mimu@linux.ibm.com>
---
 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   | 66 ++++++++++++++++++++++++++++++++++++++-------
 arch/s390/kvm/kvm-s390.h    |  5 ++++
 5 files changed, 65 insertions(+), 9 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 dd80bdc056a5..d87632996b07 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"
@@ -1099,6 +1100,17 @@ int kvm_s390_handle_wait(struct kvm_vcpu *vcpu)
 	if (kvm_arch_vcpu_runnable(vcpu))
 		return 0;
 
+	/*
+	 * This could be the single vcpu of the guest or the last vcpu
+	 * open for I/O interruptons going into wait state.
+	 */
+	if (vcpu->kvm->arch.gib_in_use &&
+	    !in_alert_list(vcpu->kvm->arch.gisa)) {
+		if (unlikely(kvm_s390_gisa_get_ipm(vcpu->kvm->arch.gisa)))
+			return 0;
+		vcpu->kvm->arch.gisa->iam = vcpu->kvm->arch.iam;
+	}
+
 	if (psw_interrupts_disabled(vcpu)) {
 		VCPU_EVENT(vcpu, 3, "%s", "disabled wait");
 		return -EOPNOTSUPP; /* disabled wait */
@@ -2904,7 +2916,7 @@ static void nullify_gisa(struct kvm_s390_gisa *gisa)
 #define NONE_GISA_ADDR 0x00000001UL
 #define GISA_ADDR_MASK 0xfffff000UL
 
-static void __maybe_unused process_gib_alert_list(void)
+static void process_gib_alert_list(void)
 {
 	u32 final, next_alert, origin = 0UL;
 	struct kvm_s390_gisa *gisa;
@@ -2953,6 +2965,9 @@ static void __maybe_unused process_gib_alert_list(void)
 void kvm_s390_gisa_clear(struct kvm *kvm)
 {
 	if (kvm->arch.gisa) {
+		kvm->arch.gisa->iam = 0;
+		while (in_alert_list(kvm->arch.gisa))
+			process_gib_alert_list();
 		nullify_gisa(kvm->arch.gisa);
 		VM_EVENT(kvm, 3, "gisa 0x%pK cleared", kvm->arch.gisa);
 	}
@@ -2964,9 +2979,9 @@ void kvm_s390_gisa_init(struct kvm *kvm)
 		kvm->arch.gisa = &kvm->arch.sie_page2->gisa;
 		kvm->arch.iam = 0;
 		spin_lock_init(&kvm->arch.iam_ref_lock);
-		VM_EVENT(kvm, 3, "gisa 0x%pK initialized", kvm->arch.gisa);
-		kvm_s390_gisa_clear(kvm);
+		nullify_gisa(kvm->arch.gisa);
 		kvm->arch.gib_in_use = !!gib;
+		VM_EVENT(kvm, 3, "gisa 0x%pK initialized", kvm->arch.gisa);
 	}
 }
 
@@ -2974,6 +2989,10 @@ void kvm_s390_gisa_destroy(struct kvm *kvm)
 {
 	if (!kvm->arch.gisa)
 		return;
+
+	kvm->arch.gisa->iam = 0;
+	while (in_alert_list(kvm->arch.gisa))
+		process_gib_alert_list();
 	kvm->arch.gisa = NULL;
 	kvm->arch.iam = 0;
 }
@@ -3019,36 +3038,65 @@ 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();
+}
+
+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;
 	chsc_sgib(0);
+	unregister_adapter_interrupt(&gib_alert_irq);
 	free_page((unsigned long)gib);
 	gib = NULL;
 }
 
 int kvm_s390_gib_init(u8 nisc)
 {
+	int rc = 0;
+
 	if (!css_general_characteristics.aiv) {
 		KVM_EVENT(3, "%s", "gib not initialized, no AIV facility");
-		return 0;
+		goto out;
 	}
 
 	gib = (struct kvm_s390_gib *)get_zeroed_page(GFP_KERNEL | GFP_DMA);
 	if (!gib) {
 		KVM_EVENT(3, "gib 0x%pK memory allocation failed", gib);
-		return -ENOMEM;
+		rc = -ENOMEM;
+		goto out;
+	}
+
+	gib_alert_irq.isc = nisc;
+	if (register_adapter_interrupt(&gib_alert_irq)) {
+		KVM_EVENT(3, "gib 0x%pK GAI registration failed", gib);
+		rc = -EIO;
+		goto out_free;
 	}
 
 	gib->nisc = nisc;
 	if (chsc_sgib((u32)(u64)gib)) {
 		KVM_EVENT(3, "gib 0x%pK AIV association failed", gib);
-		free_page((unsigned long)gib);
-		gib = NULL;
-		return -EIO;
+		rc = -EIO;
+		goto out_unreg;
 	}
 
 	KVM_EVENT(3, "gib 0x%pK (nisc=%d) initialized", gib, gib->nisc);
-	return 0;
+	return rc;
+
+out_unreg:
+	unregister_adapter_interrupt(&gib_alert_irq);
+out_free:
+	free_page((unsigned long)gib);
+	gib = NULL;
+out:
+	return rc;
 }
diff --git a/arch/s390/kvm/kvm-s390.h b/arch/s390/kvm/kvm-s390.h
index 1a79105b0e9f..1c93ceaeb9e4 100644
--- a/arch/s390/kvm/kvm-s390.h
+++ b/arch/s390/kvm/kvm-s390.h
@@ -196,6 +196,11 @@ static inline int kvm_s390_user_cpu_state_ctrl(struct kvm *kvm)
 	return kvm->arch.user_cpu_state_ctrl != 0;
 }
 
+static inline bool in_alert_list(struct kvm_s390_gisa *gisa)
+{
+	return (u32)(u64)gisa != gisa->next_alert;
+}
+
 /* implemented in interrupt.c */
 int kvm_s390_handle_wait(struct kvm_vcpu *vcpu);
 void kvm_s390_vcpu_wakeup(struct kvm_vcpu *vcpu);
-- 
2.13.4

Re: [PATCH v4 09/10] KVM: s390: add and wire function gib_alert_irq_handler()

[Halil Pasic]

On Wed, 5 Dec 2018 11:26:18 +0100
Michael Mueller <mimu@linux.ibm.com> wrote:

> 
> 
> On 05.12.18 10:35, Halil Pasic wrote:
> > On Fri, 30 Nov 2018 15:32:14 +0100
> > Michael Mueller <mimu@linux.ibm.com> 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:      23         37   [I/O] GIB Alert
> >>    ...
> >>
> >> Signed-off-by: Michael Mueller <mimu@linux.ibm.com>
> >> ---
> > [..]
> >> @@ -1099,6 +1100,17 @@ int kvm_s390_handle_wait(struct kvm_vcpu *vcpu)
> >>   	if (kvm_arch_vcpu_runnable(vcpu))
> >>   		return 0;
> >>   
> >> +	/*
> >> +	 * This could be the single vcpu of the guest or the last vcpu
> >> +	 * open for I/O interruptons going into wait state.
> >> +	 */
> >> +	if (vcpu->kvm->arch.gib_in_use &&
> >> +	    !in_alert_list(vcpu->kvm->arch.gisa)) {
> >> +		if (unlikely(kvm_s390_gisa_get_ipm(vcpu->kvm->arch.gisa)))
> >> +			return 0;
> > 
> > I don't agree with this condition. If I read this correctly, this makes
> > the vcpu go back into SIE if we have GISA interrupts pending. This can
> > basically happen in two ways. First way: we got a new bit in the IPM set
> > between the call to kvm_arch_vcpu_runnable() and here, and there is a
> > hope that the newly set (ISC) is not masked. Second way: there is no new
> > bit in IPM since kvm_arch_vcpu_runnable(). In this case we are certain
> > that the vCPU can not take the GISA interrupts pending. Second way seems
> > more likely to happen to me.
> 
> You know already that I'm introducing a change like:
> 

Yes I knew you are working on some changes based on our
off-line conversations yesterday. Yesterday I was very uncertain, today
I think, I have more clarity. So I decided to bring my point here, so
others are included and can add their insights.

> +
> +static inline u8 __again_runnable(struct kvm_vcpu *vcpu)
> +{
> +       return kvm_s390_gisa_get_ipm(vcpu->kvm->arch.gisa) &
> +               (u8)(vcpu->arch.sie_block->gcr[6] >> 24);
> +}
> +
>   int kvm_s390_handle_wait(struct kvm_vcpu *vcpu)
>   {
>          u64 sltime;
> @@ -1109,7 +1116,7 @@ int kvm_s390_handle_wait(struct kvm_vcpu *vcpu)
>           */
>          if (vcpu->kvm->arch.gib_in_use &&
>              !in_alert_list(vcpu->kvm->arch.gisa)) {
> -               if (unlikely(kvm_s390_gisa_get_ipm(vcpu->kvm->arch.gisa)))
> +               if (unlikely(__again_runnable(vcpu)))
>                          return 0;
>                  vcpu->kvm->arch.gisa->iam = vcpu->kvm->arch.iam;
> 
> 
> That will reduce a *short time window* where an not masked interruption
> might be made pending in addition to a *very short time window*. As
> *shortness* is a very relative expression, the second test will not
> be of relevance and thus be skipped.

I'm confused.

> 
> > 
> >> +		vcpu->kvm->arch.gisa->iam = vcpu->kvm->arch.iam;
> > 
> > Even without the condition above, I'm struggling with understanding your
> > algorithm that controls alerting. For instance, I don't quite understand
> > the !in_alert_list() condition either -- I'm under the impression this
> > is the only place where gisa->iam is set to non-zero. Maybe explaining
> > are the invariants you seek to maintain would help me getting it.
> 
> In opposition to the above, the test !in_alert_list() is relevant. It
> means that the host currently has the control to process the alert list
> as at least this gisa is in the alert list but has not been processed yet.

How does this prevent us form missing an alert. AFAIR the alert list
processing does not set gisa->iam.

> 
> What do you mean with your very last sentence?
>

Hm. I mean the goal of the patches is probably to ensure that the guest
is going to get its GISA interrupt if it can be delivered (not relying
on the timer based wakeup mechanism). I.e. the guest won't lose
initiative. That would an invariant. Another invariant could be no
alerts for guests with all vCPUs in SIE (since alerts are only useful
if at least one vCPU is in a wait state). To put it differently, we
are adding code so the system has certain properties. If I have a good
understanding of what properties are we aiming for, I have a chance to
spot mistakes that prevent us from reaching our goal. I know it's tough.

Regards,
Halil

 
> > 
> > Regards,
> > Halil
> > 
> >> +	}
> >> +
> >>   	if (psw_interrupts_disabled(vcpu)) {
> >>   		VCPU_EVENT(vcpu, 3, "%s", "disabled wait");
> >>   		return -EOPNOTSUPP; /* disabled wait */
>