On 03.12.18 10:43, Cornelia Huck wrote:
On Fri, 30 Nov 2018 15:32:14 +0100
Michael Mueller <mimu@xxxxxxxxxxxxx> wrote:
The patch implements a handler for GIB alert interruptions
on the host. Its task is to alert storage backed guests that
What is "storage backed" in this context? Aren't all our guests storage
backed?
It's an expression used by the AR. But as we don't differentiate between
different
guest types in kvm, this statement is obsolete.
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@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 | 66 ++++++++++++++++++++++++++++++++++++++-------
arch/s390/kvm/kvm-s390.h | 5 ++++
5 files changed, 65 insertions(+), 9 deletions(-)
@@ -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();
I'm a bit confused. Isn't process_gib_alert_list() to continue
processing until millicode stops adding alerts? Why do you need the
outer while loop, then?
Good catch! That's a left over from the old version of
process_gib_alert_list() where I
only had the inner loop and a new list could have build up while
processing the current
list. The loop doesn't hurt but is useless. A single test is sufficient.
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);
Why this change? The only difference here is that the unneeded check
for gisa != NULL is not happening, and the dbf event for clearing is
not logged?
... and the test if the gisa is in the alert list, which can't be the
case during its
initialization.
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();
Same comment as above. I feel like I'm missing something obvious :(
same answer as above
kvm->arch.gisa = NULL;
kvm->arch.iam = 0;
}
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;
Would be easier to read if you had used the 'goto out' pattern from the
beginning :(
Not sure what the comment would have been together with the patch where
kvm_s390_gib_init() was introduced... I will change that, hopefully a
last time. ;)
}
gib = (struct kvm_s390_gib *)get_zeroed_page(GFP_KERNEL | GFP_DMA);
if (!gib) {
KVM_EVENT(3, "gib 0x%pK memory allocation failed", gib);
I'm dropping this dbg message as nobody will be able to see it.
- 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);
I think this should be logged to the syslog as well, so you can find
out what went wrong for modular kvm.
This message here will go into the kernel log instead.
+ rc = -EIO;
+ goto out_free;
}
gib->nisc = nisc;
if (chsc_sgib((u32)(u64)gib)) {
KVM_EVENT(3, "gib 0x%pK AIV association failed", gib);
so will this one...
- 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;
}
