[PATCH v5 00/15] KVM: s390: make use of the GIB

[PATCH v5 00/15] KVM: s390: make use of the GIB

[Michael Mueller]

The Guest Information Area (GIB) and its mechanics is part of
the AIV facility. It provides hardware support to process
Adapter Interruptions (AI) for pagable storage mode guests. 

Whenever a guest cannot process an AI because none of its
vcpus is running in SIE context, a GIB alert interruption is
sent to and handled by the host program to dispatch a vcpu of
the respective guest.

Eventually, the AI is handled by the guest.

v4->v5:
  Between these two versions some significant change was
  introduced:

  a) During IPM look-up, the IAM gets atomically restored
     if the IPM is clean. (The main strategy is: As soon no
     airqs are pending anymore, get prepared for new ones
     arriving).

  b) Kick a set of vcpus in WAIT state, that will be able
     to handle all ISCs in IPM if possible. The main loop
     that processes the GIB alert list is unchanged.

  patches 1/15 to 6/15:
     prepare the existing Interruption and GISA code for
     the introduction of the GIB code.
  patches 7/15 to 9/15:
     kept from v4. 
  patch 10/15:
     restores the IAM in get_ipm() when clean on request
     with additional parameter irq_flags
  patch 12/15:
     modifies kvm_s390_deliver_pending_interrupts() such
     that the IAM is not restored during the deliverable_irqs()
     test as we are about to enter the SIE. Restoring the
     IAM would trigger the alert mechanism unnecessarily
     on SIE entry.
  patch 13/15:
     process_gib_alert_list() now triggers a minimal set
     of idle vcpus capable the handle all pending ISCs.
  patch 14/15:
     the wiring is logical similar to v4 but uses different
     routines
  patch 15/15: 
     kept from v4. 

v3->v4:
  The central change of v4 is that the metric "vcpus_in_sie"
  has been dropped. A least agressive IAM restore strategy
  is being followed now. The consequence is that potentially
  more GAL interruptions are to be handled by the host program.
  The assuption as made that as soon the first vcpu is trying
  to go into wait state, this could be the last vcpu being
  open for I/O interruptions. Thus, to not loose the initiative
  to deliver I/O interruptions as soon as possible, the IAM
  is restored. Adding better heuristics can be done in future.

  I expect this to be the last version of the series.
 
  patch 01/12: Kernel msg now printed when FLIC fails
               to register during arch inititialization.
  patch 03/12: Commit message slightly changed with the
  	       hint that gib initialization is called
	       by last patch of this serias.
  patch 09/12: IAM now restored during kvm_s390_handle_wait()
  patch 10/12: gib destroy case now handled upon unsuccessful
  	       arch inititialization.

All other patches are unchanged.

v2->v3:
  patch 01/12: Fixes a resource dealocation issue upon
  	       unsuccessful exit from kvm_arch_init().
	       It is relevant for patch 11/12 as well. 
  patch 08/12: The function process_gib_alert_list() has
               been rewritten to reduce the number of GAL
	       interruptions on the host for many guest
	       scenarios.
  patch 10/12: The IAM is now never cleared before SIE entry.
               It will be cleared by millicode as soon the
	       first guest interruption is made pending and
	       not directly processed in SIE. The IAM gets
	       restored as soon a vm is idle, i.e. no vcpu
	       of that guest is in SIE context anymore.
  patch 11/12: now integrates with new patch 01/12
  patch 12/12: This patch is just experimental and shall not
               be part of the final series.

The first patch of series v2: "leave AIs in IPM of GISA
during vcpu_pre_run()" has been dropped.

All other patches are unchanged.

v1->v2:
  patch 01/12: New patch. Tt can go also standalone without the
  	       rest of the GIB series presented here but is a
	       requirement 
  patch 03/12: kvm_s390_gib_init() now has a return code
  patch 06/12: unlink_gib_alert_list() now uses cmpxchg() instead
    	       of atomic_xchg()
  patch 08/12: New patch. Foctors out __find_vcpu_for_floating_irq()
  patch 09/12: process_gib_alert_list() has been simplified
               the GISA clear/destroy cases have been removed
  patch 11/12: kvm_s390_gisa_clear/destroy() now clear the IAM
               and process the alert list directly
	       The IAM now gets restored in vcpu_post_run() only if
	       the GISA is not in alert list
  patch 12/12: during kvm_arch_init() now the return code of
    	       kvm_s390_gib_init() is honored.

All other patches are unchanged.

Michael Mueller (15):
  KVM: s390: unregister debug feature on failing arch init
  KVM: s390: coding style issue kvm_s390_gisa_init/clear()
  KVM: s390: factor out nullify_gisa()
  KVM: s390: use pending_irqs_no_gisa() where appropriate
  KVM: s390: unify pending_irqs() and pending_irqs_no_gisa()
  KVM: s390: remove prefix kvm_s390_gisa_ from static inline functions
  s390/cio: add function chsc_sgib()
  KVM: s390: add the GIB and its related life-cyle functions
  KVM: s390: add kvm reference to struct sie_page2
  KVM: s390: add functions to (un)register GISC with GISA
  KVM: s390: restore IAM in get_ipm() when IPM is clean
  KVM: s390: do not restore IAM immediately before SIE entry
  KVM: s390: add function process_gib_alert_list()
  KVM: s390: add and wire function gib_alert_irq_handler()
  KVM: s390: start using the GIB

 Documentation/kmsg/s390/kvm      |  12 ++
 arch/s390/include/asm/cio.h      |   1 +
 arch/s390/include/asm/irq.h      |   1 +
 arch/s390/include/asm/isc.h      |   1 +
 arch/s390/include/asm/kvm_host.h |  22 ++-
 arch/s390/kernel/irq.c           |   1 +
 arch/s390/kvm/interrupt.c        | 396 +++++++++++++++++++++++++++++++++++----
 arch/s390/kvm/kvm-s390.c         |  25 ++-
 arch/s390/kvm/kvm-s390.h         |   2 +
 drivers/s390/cio/chsc.c          |  37 ++++
 drivers/s390/cio/chsc.h          |   1 +
 11 files changed, 457 insertions(+), 42 deletions(-)
 create mode 100644 Documentation/kmsg/s390/kvm

-- 
2.13.4

[PATCH v5 01/15] KVM: s390: unregister debug feature on failing arch init

[Michael Mueller]

Make sure the debug feature and its allocated resources get
released upon unsuccessful architecture initialization.

A related indication of the issue will be reported as kernel
message.

Signed-off-by: Michael Mueller <mimu@linux.ibm.com>
Reviewed-by: Cornelia Huck <cohuck@redhat.com>
Reviewed-by: Pierre Morel <pmorel@linux.ibm.com>
Reviewed-by: David Hildenbrand <david@redhat.com>
Message-Id: <20181130143215.69496-2-mimu@linux.ibm.com>
Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
---
 Documentation/kmsg/s390/kvm | 12 ++++++++++++
 arch/s390/kvm/kvm-s390.c    | 17 ++++++++++++++---
 2 files changed, 26 insertions(+), 3 deletions(-)
 create mode 100644 Documentation/kmsg/s390/kvm

diff --git a/Documentation/kmsg/s390/kvm b/Documentation/kmsg/s390/kvm
new file mode 100644
index 000000000000..76ffe2012254
--- /dev/null
+++ b/Documentation/kmsg/s390/kvm
@@ -0,0 +1,12 @@
+/*?
+ * Text: "Failed to register FLIC rc=%d\n"
+ * Severity: Error
+ * Parameter:
+ *   @1: return code of the FLIC registration call
+ * Description:
+ * The registration of the FLIC (Floating Interrupt Controller Interface)
+ * was not successful.
+ * User action:
+ * If this problem persists after a reload of the kvm kernel module, gather
+ * Linux debug data and contact your support organization.
+ */
diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
index fe24150ff666..d8d8e0788157 100644
--- a/arch/s390/kvm/kvm-s390.c
+++ b/arch/s390/kvm/kvm-s390.c
@@ -417,19 +417,30 @@ static void kvm_s390_cpu_feat_init(void)
 
 int kvm_arch_init(void *opaque)
 {
+	int rc;
+
 	kvm_s390_dbf = debug_register("kvm-trace", 32, 1, 7 * sizeof(long));
 	if (!kvm_s390_dbf)
 		return -ENOMEM;
 
 	if (debug_register_view(kvm_s390_dbf, &debug_sprintf_view)) {
-		debug_unregister(kvm_s390_dbf);
-		return -ENOMEM;
+		rc = -ENOMEM;
+		goto out_debug_unreg;
 	}
 
 	kvm_s390_cpu_feat_init();
 
 	/* Register floating interrupt controller interface. */
-	return kvm_register_device_ops(&kvm_flic_ops, KVM_DEV_TYPE_FLIC);
+	rc = kvm_register_device_ops(&kvm_flic_ops, KVM_DEV_TYPE_FLIC);
+	if (rc) {
+		pr_err("Failed to register FLIC rc=%d\n", rc);
+		goto out_debug_unreg;
+	}
+	return 0;
+
+out_debug_unreg:
+	debug_unregister(kvm_s390_dbf);
+	return rc;
 }
 
 void kvm_arch_exit(void)
-- 
2.13.4

Re: [PATCH v5 01/15] KVM: s390: unregister debug feature on failing arch init

[Cornelia Huck]

On Wed, 19 Dec 2018 20:17:42 +0100
Michael Mueller <mimu@linux.ibm.com> wrote:

> Make sure the debug feature and its allocated resources get
> released upon unsuccessful architecture initialization.
> 
> A related indication of the issue will be reported as kernel
> message.
> 
> Signed-off-by: Michael Mueller <mimu@linux.ibm.com>
> Reviewed-by: Cornelia Huck <cohuck@redhat.com>
> Reviewed-by: Pierre Morel <pmorel@linux.ibm.com>
> Reviewed-by: David Hildenbrand <david@redhat.com>
> Message-Id: <20181130143215.69496-2-mimu@linux.ibm.com>
> Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
> ---
>  Documentation/kmsg/s390/kvm | 12 ++++++++++++
>  arch/s390/kvm/kvm-s390.c    | 17 ++++++++++++++---
>  2 files changed, 26 insertions(+), 3 deletions(-)
>  create mode 100644 Documentation/kmsg/s390/kvm
> 
> diff --git a/Documentation/kmsg/s390/kvm b/Documentation/kmsg/s390/kvm
> new file mode 100644
> index 000000000000..76ffe2012254
> --- /dev/null
> +++ b/Documentation/kmsg/s390/kvm
> @@ -0,0 +1,12 @@
> +/*?
> + * Text: "Failed to register FLIC rc=%d\n"
> + * Severity: Error
> + * Parameter:
> + *   @1: return code of the FLIC registration call
> + * Description:
> + * The registration of the FLIC (Floating Interrupt Controller Interface)
> + * was not successful.
> + * User action:
> + * If this problem persists after a reload of the kvm kernel module, gather
> + * Linux debug data and contact your support organization.
> + */

Hm, it seems that the kmsg stuff crept in there again...

Re: [PATCH v5 01/15] KVM: s390: unregister debug feature on failing arch init

[Michael Mueller]

On 19.12.18 21:10, Cornelia Huck wrote:
> On Wed, 19 Dec 2018 20:17:42 +0100
> Michael Mueller <mimu@linux.ibm.com> wrote:
> 
>> Make sure the debug feature and its allocated resources get
>> released upon unsuccessful architecture initialization.
>>
>> A related indication of the issue will be reported as kernel
>> message.
>>
>> Signed-off-by: Michael Mueller <mimu@linux.ibm.com>
>> Reviewed-by: Cornelia Huck <cohuck@redhat.com>
>> Reviewed-by: Pierre Morel <pmorel@linux.ibm.com>
>> Reviewed-by: David Hildenbrand <david@redhat.com>
>> Message-Id: <20181130143215.69496-2-mimu@linux.ibm.com>
>> Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
>> ---
>>   Documentation/kmsg/s390/kvm | 12 ++++++++++++
>>   arch/s390/kvm/kvm-s390.c    | 17 ++++++++++++++---
>>   2 files changed, 26 insertions(+), 3 deletions(-)
>>   create mode 100644 Documentation/kmsg/s390/kvm
>>
>> diff --git a/Documentation/kmsg/s390/kvm b/Documentation/kmsg/s390/kvm
>> new file mode 100644
>> index 000000000000..76ffe2012254
>> --- /dev/null
>> +++ b/Documentation/kmsg/s390/kvm
>> @@ -0,0 +1,12 @@
>> +/*?
>> + * Text: "Failed to register FLIC rc=%d\n"
>> + * Severity: Error
>> + * Parameter:
>> + *   @1: return code of the FLIC registration call
>> + * Description:
>> + * The registration of the FLIC (Floating Interrupt Controller Interface)
>> + * was not successful.
>> + * User action:
>> + * If this problem persists after a reload of the kvm kernel module, gather
>> + * Linux debug data and contact your support organization.
>> + */
> 
> Hm, it seems that the kmsg stuff crept in there again...
> 

picked from wrong branch, sorry...

Re: [PATCH v5 01/15] KVM: s390: unregister debug feature on failing arch init

[Christian Borntraeger]

On 20.12.2018 08:49, Michael Mueller wrote:
> 
> 
> On 19.12.18 21:10, Cornelia Huck wrote:
>> On Wed, 19 Dec 2018 20:17:42 +0100
>> Michael Mueller <mimu@linux.ibm.com> wrote:
>>
>>> Make sure the debug feature and its allocated resources get
>>> released upon unsuccessful architecture initialization.
>>>
>>> A related indication of the issue will be reported as kernel
>>> message.
>>>
>>> Signed-off-by: Michael Mueller <mimu@linux.ibm.com>
>>> Reviewed-by: Cornelia Huck <cohuck@redhat.com>
>>> Reviewed-by: Pierre Morel <pmorel@linux.ibm.com>
>>> Reviewed-by: David Hildenbrand <david@redhat.com>
>>> Message-Id: <20181130143215.69496-2-mimu@linux.ibm.com>
>>> Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
>>> ---
>>>   Documentation/kmsg/s390/kvm | 12 ++++++++++++
>>>   arch/s390/kvm/kvm-s390.c    | 17 ++++++++++++++---
>>>   2 files changed, 26 insertions(+), 3 deletions(-)
>>>   create mode 100644 Documentation/kmsg/s390/kvm
>>>
>>> diff --git a/Documentation/kmsg/s390/kvm b/Documentation/kmsg/s390/kvm
>>> new file mode 100644
>>> index 000000000000..76ffe2012254
>>> --- /dev/null
>>> +++ b/Documentation/kmsg/s390/kvm
>>> @@ -0,0 +1,12 @@
>>> +/*?
>>> + * Text: "Failed to register FLIC rc=%d\n"
>>> + * Severity: Error
>>> + * Parameter:
>>> + *   @1: return code of the FLIC registration call
>>> + * Description:
>>> + * The registration of the FLIC (Floating Interrupt Controller Interface)
>>> + * was not successful.
>>> + * User action:
>>> + * If this problem persists after a reload of the kvm kernel module, gather
>>> + * Linux debug data and contact your support organization.
>>> + */
>>
>> Hm, it seems that the kmsg stuff crept in there again...
>>
> 
> picked from wrong branch, sorry...
> 

FWIW, this patch without kmsg one is now part of kvm/next.

https://git.kernel.org/pub/scm/virt/kvm/kvm.git/commit/?h=next&id=308c3e6673b012beecb96ef04cc65f4a0e7cdd99

[PATCH v5 02/15] KVM: s390: coding style issue kvm_s390_gisa_init/clear()

[Michael Mueller]

The change hepls to reduce line length and
icreases code readability.

Signed-off-by: Michael Mueller <mimu@linux.ibm.com>
---
 arch/s390/kvm/interrupt.c | 20 ++++++++++----------
 1 file changed, 10 insertions(+), 10 deletions(-)

diff --git a/arch/s390/kvm/interrupt.c b/arch/s390/kvm/interrupt.c
index fcb55b02990e..9a3aed608914 100644
--- a/arch/s390/kvm/interrupt.c
+++ b/arch/s390/kvm/interrupt.c
@@ -2886,20 +2886,20 @@ int kvm_s390_get_irq_state(struct kvm_vcpu *vcpu, __u8 __user *buf, int len)
 
 void kvm_s390_gisa_clear(struct kvm *kvm)
 {
-	if (kvm->arch.gisa) {
-		memset(kvm->arch.gisa, 0, sizeof(struct kvm_s390_gisa));
-		kvm->arch.gisa->next_alert = (u32)(u64)kvm->arch.gisa;
-		VM_EVENT(kvm, 3, "gisa 0x%pK cleared", kvm->arch.gisa);
-	}
+	if (!kvm->arch.gisa)
+		return;
+	memset(kvm->arch.gisa, 0, sizeof(struct kvm_s390_gisa));
+	kvm->arch.gisa->next_alert = (u32)(u64)kvm->arch.gisa;
+	VM_EVENT(kvm, 3, "gisa 0x%pK cleared", kvm->arch.gisa);
 }
 
 void kvm_s390_gisa_init(struct kvm *kvm)
 {
-	if (css_general_characteristics.aiv) {
-		kvm->arch.gisa = &kvm->arch.sie_page2->gisa;
-		VM_EVENT(kvm, 3, "gisa 0x%pK initialized", kvm->arch.gisa);
-		kvm_s390_gisa_clear(kvm);
-	}
+	if (!css_general_characteristics.aiv)
+		return;
+	kvm->arch.gisa = &kvm->arch.sie_page2->gisa;
+	VM_EVENT(kvm, 3, "gisa 0x%pK initialized", kvm->arch.gisa);
+	kvm_s390_gisa_clear(kvm);
 }
 
 void kvm_s390_gisa_destroy(struct kvm *kvm)
-- 
2.13.4

Re: [PATCH v5 02/15] KVM: s390: coding style issue kvm_s390_gisa_init/clear()

[Cornelia Huck]

On Wed, 19 Dec 2018 20:17:43 +0100
Michael Mueller <mimu@linux.ibm.com> wrote:

> The change hepls to reduce line length and

s/hepls/helps/

> icreases code readability.

s/icreases/increases/

> 
> Signed-off-by: Michael Mueller <mimu@linux.ibm.com>
> ---
>  arch/s390/kvm/interrupt.c | 20 ++++++++++----------
>  1 file changed, 10 insertions(+), 10 deletions(-)
> 

Reviewed-by: Cornelia Huck <cohuck@redhat.com>

Re: [PATCH v5 02/15] KVM: s390: coding style issue kvm_s390_gisa_init/clear()

[Pierre Morel]

On 19/12/2018 20:17, Michael Mueller wrote:
> The change hepls to reduce line length and
> icreases code readability.
> 
> Signed-off-by: Michael Mueller <mimu@linux.ibm.com>
> ---
>   arch/s390/kvm/interrupt.c | 20 ++++++++++----------
>   1 file changed, 10 insertions(+), 10 deletions(-)
> 
> diff --git a/arch/s390/kvm/interrupt.c b/arch/s390/kvm/interrupt.c
> index fcb55b02990e..9a3aed608914 100644
> --- a/arch/s390/kvm/interrupt.c
> +++ b/arch/s390/kvm/interrupt.c
> @@ -2886,20 +2886,20 @@ int kvm_s390_get_irq_state(struct kvm_vcpu *vcpu, __u8 __user *buf, int len)
>   
>   void kvm_s390_gisa_clear(struct kvm *kvm)
>   {
> -	if (kvm->arch.gisa) {
> -		memset(kvm->arch.gisa, 0, sizeof(struct kvm_s390_gisa));
> -		kvm->arch.gisa->next_alert = (u32)(u64)kvm->arch.gisa;
> -		VM_EVENT(kvm, 3, "gisa 0x%pK cleared", kvm->arch.gisa);
> -	}
> +	if (!kvm->arch.gisa)
> +		return;
> +	memset(kvm->arch.gisa, 0, sizeof(struct kvm_s390_gisa));
> +	kvm->arch.gisa->next_alert = (u32)(u64)kvm->arch.gisa;
> +	VM_EVENT(kvm, 3, "gisa 0x%pK cleared", kvm->arch.gisa);
>   }
>   
>   void kvm_s390_gisa_init(struct kvm *kvm)
>   {
> -	if (css_general_characteristics.aiv) {
> -		kvm->arch.gisa = &kvm->arch.sie_page2->gisa;
> -		VM_EVENT(kvm, 3, "gisa 0x%pK initialized", kvm->arch.gisa);
> -		kvm_s390_gisa_clear(kvm);
> -	}
> +	if (!css_general_characteristics.aiv)
> +		return;
> +	kvm->arch.gisa = &kvm->arch.sie_page2->gisa;
> +	VM_EVENT(kvm, 3, "gisa 0x%pK initialized", kvm->arch.gisa);

to be annoying, shouldn't the trace be done after the clear?

> +	kvm_s390_gisa_clear(kvm);
>   }
>   
>   void kvm_s390_gisa_destroy(struct kvm *kvm)
> 

Reviewed-by: Pierre Morel<pmorel@linux.ibm.com>


-- 
Pierre Morel
Linux/KVM/QEMU in Böblingen - Germany

Re: [PATCH v5 02/15] KVM: s390: coding style issue kvm_s390_gisa_init/clear()

[Michael Mueller]

On 02.01.19 17:50, Pierre Morel wrote:
> On 19/12/2018 20:17, Michael Mueller wrote:
>> The change hepls to reduce line length and
>> icreases code readability.
>>
>> Signed-off-by: Michael Mueller <mimu@linux.ibm.com>
>> ---
>>   arch/s390/kvm/interrupt.c | 20 ++++++++++----------
>>   1 file changed, 10 insertions(+), 10 deletions(-)
>>
>> diff --git a/arch/s390/kvm/interrupt.c b/arch/s390/kvm/interrupt.c
>> index fcb55b02990e..9a3aed608914 100644
>> --- a/arch/s390/kvm/interrupt.c
>> +++ b/arch/s390/kvm/interrupt.c
>> @@ -2886,20 +2886,20 @@ int kvm_s390_get_irq_state(struct kvm_vcpu 
>> *vcpu, __u8 __user *buf, int len)
>>   void kvm_s390_gisa_clear(struct kvm *kvm)
>>   {
>> -    if (kvm->arch.gisa) {
>> -        memset(kvm->arch.gisa, 0, sizeof(struct kvm_s390_gisa));
>> -        kvm->arch.gisa->next_alert = (u32)(u64)kvm->arch.gisa;
>> -        VM_EVENT(kvm, 3, "gisa 0x%pK cleared", kvm->arch.gisa);
>> -    }
>> +    if (!kvm->arch.gisa)
>> +        return;
>> +    memset(kvm->arch.gisa, 0, sizeof(struct kvm_s390_gisa));
>> +    kvm->arch.gisa->next_alert = (u32)(u64)kvm->arch.gisa;
>> +    VM_EVENT(kvm, 3, "gisa 0x%pK cleared", kvm->arch.gisa);
>>   }
>>   void kvm_s390_gisa_init(struct kvm *kvm)
>>   {
>> -    if (css_general_characteristics.aiv) {
>> -        kvm->arch.gisa = &kvm->arch.sie_page2->gisa;
>> -        VM_EVENT(kvm, 3, "gisa 0x%pK initialized", kvm->arch.gisa);
>> -        kvm_s390_gisa_clear(kvm);
>> -    }
>> +    if (!css_general_characteristics.aiv)
>> +        return;
>> +    kvm->arch.gisa = &kvm->arch.sie_page2->gisa;
>> +    VM_EVENT(kvm, 3, "gisa 0x%pK initialized", kvm->arch.gisa);
> 
> to be annoying, shouldn't the trace be done after the clear?

sure, I will address that

> 
>> +    kvm_s390_gisa_clear(kvm);
>>   }
>>   void kvm_s390_gisa_destroy(struct kvm *kvm)
>>
> 
> Reviewed-by: Pierre Morel<pmorel@linux.ibm.com>
> 
> 

-- 

[PATCH v5 03/15] KVM: s390: factor out nullify_gisa()

[Michael Mueller]

This function will be used by the GISA init and the GISA
clear operation. Thus it gets factored out here.

Signed-off-by: Michael Mueller <mimu@linux.ibm.com>
Reviewed-by: Pierre Morel <pmorel@linux.ibm.com>
Reviewed-by: Janosch Frank <frankja@linux.ibm.com>
Reviewed-by: Cornelia Huck <cohuck@redhat.com>
Reviewed-by: David Hildenbrand <david@redhat.com>
---
 arch/s390/kvm/interrupt.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/arch/s390/kvm/interrupt.c b/arch/s390/kvm/interrupt.c
index 9a3aed608914..c3a8b2fbbcf2 100644
--- a/arch/s390/kvm/interrupt.c
+++ b/arch/s390/kvm/interrupt.c
@@ -2884,12 +2884,17 @@ int kvm_s390_get_irq_state(struct kvm_vcpu *vcpu, __u8 __user *buf, int len)
 	return n;
 }
 
+static void nullify_gisa(struct kvm_s390_gisa *gisa)
+{
+	memset(gisa, 0, sizeof(struct kvm_s390_gisa));
+	gisa->next_alert = (u32)(u64)gisa;
+}
+
 void kvm_s390_gisa_clear(struct kvm *kvm)
 {
 	if (!kvm->arch.gisa)
 		return;
-	memset(kvm->arch.gisa, 0, sizeof(struct kvm_s390_gisa));
-	kvm->arch.gisa->next_alert = (u32)(u64)kvm->arch.gisa;
+	nullify_gisa(kvm->arch.gisa);
 	VM_EVENT(kvm, 3, "gisa 0x%pK cleared", kvm->arch.gisa);
 }
 
@@ -2898,8 +2903,8 @@ void kvm_s390_gisa_init(struct kvm *kvm)
 	if (!css_general_characteristics.aiv)
 		return;
 	kvm->arch.gisa = &kvm->arch.sie_page2->gisa;
+	nullify_gisa(kvm->arch.gisa);
 	VM_EVENT(kvm, 3, "gisa 0x%pK initialized", kvm->arch.gisa);
-	kvm_s390_gisa_clear(kvm);
 }
 
 void kvm_s390_gisa_destroy(struct kvm *kvm)
-- 
2.13.4

[PATCH v5 04/15] KVM: s390: use pending_irqs_no_gisa() where appropriate

[Michael Mueller]

Interruption types that are not represented in GISA shall
use pending_irqs_no_gisa() to test pending interruptions.

Signed-off-by: Michael Mueller <mimu@linux.ibm.com>
---
 arch/s390/kvm/interrupt.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/s390/kvm/interrupt.c b/arch/s390/kvm/interrupt.c
index c3a8b2fbbcf2..093b568b6356 100644
--- a/arch/s390/kvm/interrupt.c
+++ b/arch/s390/kvm/interrupt.c
@@ -353,7 +353,7 @@ static void set_intercept_indicators_io(struct kvm_vcpu *vcpu)
 
 static void set_intercept_indicators_ext(struct kvm_vcpu *vcpu)
 {
-	if (!(pending_irqs(vcpu) & IRQ_PEND_EXT_MASK))
+	if (!(pending_irqs_no_gisa(vcpu) & IRQ_PEND_EXT_MASK))
 		return;
 	if (psw_extint_disabled(vcpu))
 		kvm_s390_set_cpuflags(vcpu, CPUSTAT_EXT_INT);
@@ -363,7 +363,7 @@ static void set_intercept_indicators_ext(struct kvm_vcpu *vcpu)
 
 static void set_intercept_indicators_mchk(struct kvm_vcpu *vcpu)
 {
-	if (!(pending_irqs(vcpu) & IRQ_PEND_MCHK_MASK))
+	if (!(pending_irqs_no_gisa(vcpu) & IRQ_PEND_MCHK_MASK))
 		return;
 	if (psw_mchk_disabled(vcpu))
 		vcpu->arch.sie_block->ictl |= ICTL_LPSW;
-- 
2.13.4

Re: [PATCH v5 04/15] KVM: s390: use pending_irqs_no_gisa() where appropriate

[Cornelia Huck]

On Wed, 19 Dec 2018 20:17:45 +0100
Michael Mueller <mimu@linux.ibm.com> wrote:

> Interruption types that are not represented in GISA shall
> use pending_irqs_no_gisa() to test pending interruptions.
> 
> Signed-off-by: Michael Mueller <mimu@linux.ibm.com>
> ---
>  arch/s390/kvm/interrupt.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 

Reviewed-by: Cornelia Huck <cohuck@redhat.com>

Re: [PATCH v5 04/15] KVM: s390: use pending_irqs_no_gisa() where appropriate

[Pierre Morel]

On 19/12/2018 20:17, Michael Mueller wrote:
> Interruption types that are not represented in GISA shall
> use pending_irqs_no_gisa() to test pending interruptions.
> 
> Signed-off-by: Michael Mueller <mimu@linux.ibm.com>
> ---
>   arch/s390/kvm/interrupt.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/s390/kvm/interrupt.c b/arch/s390/kvm/interrupt.c
> index c3a8b2fbbcf2..093b568b6356 100644
> --- a/arch/s390/kvm/interrupt.c
> +++ b/arch/s390/kvm/interrupt.c
> @@ -353,7 +353,7 @@ static void set_intercept_indicators_io(struct kvm_vcpu *vcpu)
>   
>   static void set_intercept_indicators_ext(struct kvm_vcpu *vcpu)
>   {
> -	if (!(pending_irqs(vcpu) & IRQ_PEND_EXT_MASK))
> +	if (!(pending_irqs_no_gisa(vcpu) & IRQ_PEND_EXT_MASK))
>   		return;
>   	if (psw_extint_disabled(vcpu))
>   		kvm_s390_set_cpuflags(vcpu, CPUSTAT_EXT_INT);
> @@ -363,7 +363,7 @@ static void set_intercept_indicators_ext(struct kvm_vcpu *vcpu)
>   
>   static void set_intercept_indicators_mchk(struct kvm_vcpu *vcpu)
>   {
> -	if (!(pending_irqs(vcpu) & IRQ_PEND_MCHK_MASK))
> +	if (!(pending_irqs_no_gisa(vcpu) & IRQ_PEND_MCHK_MASK))
>   		return;
>   	if (psw_mchk_disabled(vcpu))
>   		vcpu->arch.sie_block->ictl |= ICTL_LPSW;
> 

Reviewed-by: Pierre Morel<pmorel@linux.ibm.com>


-- 
Pierre Morel
Linux/KVM/QEMU in Böblingen - Germany

[PATCH v5 05/15] KVM: s390: unify pending_irqs() and pending_irqs_no_gisa()

[Michael Mueller]

Use a single function with parameter irq_flags to differentiate
between cases.

New irq flag:
   IRQ_FLAG_LOCAL: include vcpu local interruptions pending
   IRQ_FLAG_FLOATING: include vcpu floating interruptions pending
   IRQ_FLAG_GISA: include GISA interruptions pending in IPM

New irq masks:
   IRQ_MASK_ALL: include all types
   IRQ_MASK_NO_GISA: include all types but GISA

Examples:
   pending_irqs(vcpu, IRQ_MASK_ALL)
   pending_irqs(vcpu, IRQ_MASK_NO_GISA)

There will be more irq flags with upcoming patches.

Signed-off-by: Michael Mueller <mimu@linux.ibm.com>
---
 arch/s390/kvm/interrupt.c | 33 +++++++++++++++++++++------------
 1 file changed, 21 insertions(+), 12 deletions(-)

diff --git a/arch/s390/kvm/interrupt.c b/arch/s390/kvm/interrupt.c
index 093b568b6356..4ab20d2eb180 100644
--- a/arch/s390/kvm/interrupt.c
+++ b/arch/s390/kvm/interrupt.c
@@ -31,6 +31,13 @@
 #define PFAULT_DONE 0x0680
 #define VIRTIO_PARAM 0x0d00
 
+#define IRQ_FLAG_LOCAL    0x8000 /* include local interruption pending mask */
+#define IRQ_FLAG_FLOATING 0x4000 /* include float interruption pending mask */
+#define IRQ_FLAG_GISA     0x2000 /* include GISA interruption pending mask */
+
+#define IRQ_MASK_ALL      (IRQ_FLAG_LOCAL | IRQ_FLAG_FLOATING | IRQ_FLAG_GISA)
+#define IRQ_MASK_NO_GISA  (IRQ_MASK_ALL & ~IRQ_FLAG_GISA)
+
 /* handle external calls via sigp interpretation facility */
 static int sca_ext_call_pending(struct kvm_vcpu *vcpu, int *src_id)
 {
@@ -237,16 +244,18 @@ static inline int kvm_s390_gisa_tac_ipm_gisc(struct kvm_s390_gisa *gisa, u32 gis
 	return test_and_clear_bit_inv(IPM_BIT_OFFSET + gisc, (unsigned long *) gisa);
 }
 
-static inline unsigned long pending_irqs_no_gisa(struct kvm_vcpu *vcpu)
+static inline unsigned long pending_irqs(struct kvm_vcpu *vcpu, u16 irq_flags)
 {
-	return vcpu->kvm->arch.float_int.pending_irqs |
-		vcpu->arch.local_int.pending_irqs;
-}
+	unsigned long pending_irqs = 0;
 
-static inline unsigned long pending_irqs(struct kvm_vcpu *vcpu)
-{
-	return pending_irqs_no_gisa(vcpu) |
-		kvm_s390_gisa_get_ipm(vcpu->kvm->arch.gisa) << IRQ_PEND_IO_ISC_7;
+	if (irq_flags & IRQ_FLAG_LOCAL)
+		pending_irqs |= vcpu->arch.local_int.pending_irqs;
+	if (irq_flags & IRQ_FLAG_FLOATING)
+		pending_irqs |= vcpu->kvm->arch.float_int.pending_irqs;
+	if (irq_flags & IRQ_FLAG_GISA)
+		pending_irqs |= kvm_s390_gisa_get_ipm(vcpu->kvm->arch.gisa) <<
+			IRQ_PEND_IO_ISC_7;
+	return pending_irqs;
 }
 
 static inline int isc_to_irq_type(unsigned long isc)
@@ -275,7 +284,7 @@ static unsigned long deliverable_irqs(struct kvm_vcpu *vcpu)
 {
 	unsigned long active_mask;
 
-	active_mask = pending_irqs(vcpu);
+	active_mask = pending_irqs(vcpu, IRQ_MASK_ALL);
 	if (!active_mask)
 		return 0;
 
@@ -343,7 +352,7 @@ static void __reset_intercept_indicators(struct kvm_vcpu *vcpu)
 
 static void set_intercept_indicators_io(struct kvm_vcpu *vcpu)
 {
-	if (!(pending_irqs_no_gisa(vcpu) & IRQ_PEND_IO_MASK))
+	if (!(pending_irqs(vcpu, IRQ_MASK_NO_GISA) & IRQ_PEND_IO_MASK))
 		return;
 	else if (psw_ioint_disabled(vcpu))
 		kvm_s390_set_cpuflags(vcpu, CPUSTAT_IO_INT);
@@ -353,7 +362,7 @@ static void set_intercept_indicators_io(struct kvm_vcpu *vcpu)
 
 static void set_intercept_indicators_ext(struct kvm_vcpu *vcpu)
 {
-	if (!(pending_irqs_no_gisa(vcpu) & IRQ_PEND_EXT_MASK))
+	if (!(pending_irqs(vcpu, IRQ_MASK_NO_GISA) & IRQ_PEND_EXT_MASK))
 		return;
 	if (psw_extint_disabled(vcpu))
 		kvm_s390_set_cpuflags(vcpu, CPUSTAT_EXT_INT);
@@ -363,7 +372,7 @@ static void set_intercept_indicators_ext(struct kvm_vcpu *vcpu)
 
 static void set_intercept_indicators_mchk(struct kvm_vcpu *vcpu)
 {
-	if (!(pending_irqs_no_gisa(vcpu) & IRQ_PEND_MCHK_MASK))
+	if (!(pending_irqs(vcpu, IRQ_MASK_NO_GISA) & IRQ_PEND_MCHK_MASK))
 		return;
 	if (psw_mchk_disabled(vcpu))
 		vcpu->arch.sie_block->ictl |= ICTL_LPSW;
-- 
2.13.4

Re: [PATCH v5 05/15] KVM: s390: unify pending_irqs() and pending_irqs_no_gisa()

[Michael Mueller]

On 19.12.18 20:17, Michael Mueller wrote:
>   
> -static inline unsigned long pending_irqs(struct kvm_vcpu *vcpu)
> -{
> -	return pending_irqs_no_gisa(vcpu) |
> -		kvm_s390_gisa_get_ipm(vcpu->kvm->arch.gisa) << IRQ_PEND_IO_ISC_7;
> +	if (irq_flags & IRQ_FLAG_LOCAL)
> +		pending_irqs |= vcpu->arch.local_int.pending_irqs;
> +	if (irq_flags & IRQ_FLAG_FLOATING)
> +		pending_irqs |= vcpu->kvm->arch.float_int.pending_irqs;
> +	if (irq_flags & IRQ_FLAG_GISA)


Fix crash under vsie:

                 pending_irqs |= vcpu->kvm->arch.float_int.pending_irqs;
-       if (irq_flags & IRQ_FLAG_GISA)
+       if (vcpu->kvm->arch.gisa && irq_flags & IRQ_FLAG_GISA)
                 pending_irqs |= get_ipm(vcpu->kvm->arch.gisa, irq_flags)


> +		pending_irqs |= kvm_s390_gisa_get_ipm(vcpu->kvm->arch.gisa) <<
> +			IRQ_PEND_IO_ISC_7;
> +	return pending_irqs;
>   }

-- 
Mit freundlichen Grüßen / Kind regards
Michael Müller

IBM Deutschland Research & Development GmbH
Vorsitzender des Aufsichtsrats: Matthias Hartmann
Geschäftsführung: Dirk Wittkopp
Sitz der Gesellschaft: Böblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294

Re: [PATCH v5 05/15] KVM: s390: unify pending_irqs() and pending_irqs_no_gisa()

[Cornelia Huck]

On Wed, 19 Dec 2018 20:17:46 +0100
Michael Mueller <mimu@linux.ibm.com> wrote:

> Use a single function with parameter irq_flags to differentiate
> between cases.
> 
> New irq flag:
>    IRQ_FLAG_LOCAL: include vcpu local interruptions pending
>    IRQ_FLAG_FLOATING: include vcpu floating interruptions pending
>    IRQ_FLAG_GISA: include GISA interruptions pending in IPM

I presume that means that irqs may be in more than one set? Or are gisa
irqs not considered floating irqs, because they use a different
mechanism?

> 
> New irq masks:
>    IRQ_MASK_ALL: include all types
>    IRQ_MASK_NO_GISA: include all types but GISA
> 
> Examples:
>    pending_irqs(vcpu, IRQ_MASK_ALL)
>    pending_irqs(vcpu, IRQ_MASK_NO_GISA)
> 
> There will be more irq flags with upcoming patches.
> 
> Signed-off-by: Michael Mueller <mimu@linux.ibm.com>
> ---
>  arch/s390/kvm/interrupt.c | 33 +++++++++++++++++++++------------
>  1 file changed, 21 insertions(+), 12 deletions(-)
> 
> diff --git a/arch/s390/kvm/interrupt.c b/arch/s390/kvm/interrupt.c
> index 093b568b6356..4ab20d2eb180 100644
> --- a/arch/s390/kvm/interrupt.c
> +++ b/arch/s390/kvm/interrupt.c
> @@ -31,6 +31,13 @@
>  #define PFAULT_DONE 0x0680
>  #define VIRTIO_PARAM 0x0d00
>  
> +#define IRQ_FLAG_LOCAL    0x8000 /* include local interruption pending mask */
> +#define IRQ_FLAG_FLOATING 0x4000 /* include float interruption pending mask */
> +#define IRQ_FLAG_GISA     0x2000 /* include GISA interruption pending mask */
> +
> +#define IRQ_MASK_ALL      (IRQ_FLAG_LOCAL | IRQ_FLAG_FLOATING | IRQ_FLAG_GISA)
> +#define IRQ_MASK_NO_GISA  (IRQ_MASK_ALL & ~IRQ_FLAG_GISA)
> +
>  /* handle external calls via sigp interpretation facility */
>  static int sca_ext_call_pending(struct kvm_vcpu *vcpu, int *src_id)
>  {
> @@ -237,16 +244,18 @@ static inline int kvm_s390_gisa_tac_ipm_gisc(struct kvm_s390_gisa *gisa, u32 gis
>  	return test_and_clear_bit_inv(IPM_BIT_OFFSET + gisc, (unsigned long *) gisa);
>  }
>  
> -static inline unsigned long pending_irqs_no_gisa(struct kvm_vcpu *vcpu)
> +static inline unsigned long pending_irqs(struct kvm_vcpu *vcpu, u16 irq_flags)

Any deeper reason why this is a u16? 16 bits should be enough for
everyone? :)

>  {
> -	return vcpu->kvm->arch.float_int.pending_irqs |
> -		vcpu->arch.local_int.pending_irqs;
> -}
> +	unsigned long pending_irqs = 0;
>  
> -static inline unsigned long pending_irqs(struct kvm_vcpu *vcpu)
> -{
> -	return pending_irqs_no_gisa(vcpu) |
> -		kvm_s390_gisa_get_ipm(vcpu->kvm->arch.gisa) << IRQ_PEND_IO_ISC_7;
> +	if (irq_flags & IRQ_FLAG_LOCAL)
> +		pending_irqs |= vcpu->arch.local_int.pending_irqs;
> +	if (irq_flags & IRQ_FLAG_FLOATING)
> +		pending_irqs |= vcpu->kvm->arch.float_int.pending_irqs;
> +	if (irq_flags & IRQ_FLAG_GISA)
> +		pending_irqs |= kvm_s390_gisa_get_ipm(vcpu->kvm->arch.gisa) <<
> +			IRQ_PEND_IO_ISC_7;
> +	return pending_irqs;
>  }
>  
>  static inline int isc_to_irq_type(unsigned long isc)
> @@ -275,7 +284,7 @@ static unsigned long deliverable_irqs(struct kvm_vcpu *vcpu)
>  {
>  	unsigned long active_mask;
>  
> -	active_mask = pending_irqs(vcpu);
> +	active_mask = pending_irqs(vcpu, IRQ_MASK_ALL);
>  	if (!active_mask)
>  		return 0;
>  
> @@ -343,7 +352,7 @@ static void __reset_intercept_indicators(struct kvm_vcpu *vcpu)
>  
>  static void set_intercept_indicators_io(struct kvm_vcpu *vcpu)
>  {
> -	if (!(pending_irqs_no_gisa(vcpu) & IRQ_PEND_IO_MASK))
> +	if (!(pending_irqs(vcpu, IRQ_MASK_NO_GISA) & IRQ_PEND_IO_MASK))

I/O interrupts are always floating, so you probably could check for
only floating (excluding gisa) irqs here.

>  		return;
>  	else if (psw_ioint_disabled(vcpu))
>  		kvm_s390_set_cpuflags(vcpu, CPUSTAT_IO_INT);Store Data
> @@ -353,7 +362,7 @@ static void set_intercept_indicators_io(struct kvm_vcpu *vcpu)
>  
>  static void set_intercept_indicators_ext(struct kvm_vcpu *vcpu)
>  {
> -	if (!(pending_irqs_no_gisa(vcpu) & IRQ_PEND_EXT_MASK))
> +	if (!(pending_irqs(vcpu, IRQ_MASK_NO_GISA) & IRQ_PEND_EXT_MASK))
>  		return;
>  	if (psw_extint_disabled(vcpu))
>  		kvm_s390_set_cpuflags(vcpu, CPUSTAT_EXT_INT);
> @@ -363,7 +372,7 @@ static void set_intercept_indicators_ext(struct kvm_vcpu *vcpu)
>  
>  static void set_intercept_indicators_mchk(struct kvm_vcpu *vcpu)
>  {
> -	if (!(pending_irqs_no_gisa(vcpu) & IRQ_PEND_MCHK_MASK))
> +	if (!(pending_irqs(vcpu, IRQ_MASK_NO_GISA) & IRQ_PEND_MCHK_MASK))
>  		return;
>  	if (psw_mchk_disabled(vcpu))
>  		vcpu->arch.sie_block->ictl |= ICTL_LPSW;

Re: [PATCH v5 05/15] KVM: s390: unify pending_irqs() and pending_irqs_no_gisa()

[Michael Mueller]

On 20.12.18 12:06, Cornelia Huck wrote:
> On Wed, 19 Dec 2018 20:17:46 +0100
> Michael Mueller <mimu@linux.ibm.com> wrote:
> 
>> Use a single function with parameter irq_flags to differentiate
>> between cases.
>>
>> New irq flag:
>>     IRQ_FLAG_LOCAL: include vcpu local interruptions pending
>>     IRQ_FLAG_FLOATING: include vcpu floating interruptions pending
>>     IRQ_FLAG_GISA: include GISA interruptions pending in IPM
> 
> I presume that means that irqs may be in more than one set? Or are gisa
> irqs not considered floating irqs, because they use a different
> mechanism?

Currently, the interruptions managed in GISA are floating only. But
that might change in future. The idea is not to subsume IRQ_FLAG_GISA
in IRQ_FLAG_FLOATING but to be able to address the right set of 
procedures to determine the irq pending set for a given subset of irq
types that have different implementations.

There might be a better name for IRQ_FLAG_FLOATING then?

> 
>>
>> New irq masks:
>>     IRQ_MASK_ALL: include all types
>>     IRQ_MASK_NO_GISA: include all types but GISA
>>
>> Examples:
>>     pending_irqs(vcpu, IRQ_MASK_ALL)
>>     pending_irqs(vcpu, IRQ_MASK_NO_GISA)
>>
>> There will be more irq flags with upcoming patches.
>>
>> Signed-off-by: Michael Mueller <mimu@linux.ibm.com>
>> ---
>>   arch/s390/kvm/interrupt.c | 33 +++++++++++++++++++++------------
>>   1 file changed, 21 insertions(+), 12 deletions(-)
>>
>> diff --git a/arch/s390/kvm/interrupt.c b/arch/s390/kvm/interrupt.c
>> index 093b568b6356..4ab20d2eb180 100644
>> --- a/arch/s390/kvm/interrupt.c
>> +++ b/arch/s390/kvm/interrupt.c
>> @@ -31,6 +31,13 @@
>>   #define PFAULT_DONE 0x0680
>>   #define VIRTIO_PARAM 0x0d00
>>   
>> +#define IRQ_FLAG_LOCAL    0x8000 /* include local interruption pending mask */
>> +#define IRQ_FLAG_FLOATING 0x4000 /* include float interruption pending mask */
>> +#define IRQ_FLAG_GISA     0x2000 /* include GISA interruption pending mask */
>> +
>> +#define IRQ_MASK_ALL      (IRQ_FLAG_LOCAL | IRQ_FLAG_FLOATING | IRQ_FLAG_GISA)
>> +#define IRQ_MASK_NO_GISA  (IRQ_MASK_ALL & ~IRQ_FLAG_GISA)
>> +
>>   /* handle external calls via sigp interpretation facility */
>>   static int sca_ext_call_pending(struct kvm_vcpu *vcpu, int *src_id)
>>   {
>> @@ -237,16 +244,18 @@ static inline int kvm_s390_gisa_tac_ipm_gisc(struct kvm_s390_gisa *gisa, u32 gis
>>   	return test_and_clear_bit_inv(IPM_BIT_OFFSET + gisc, (unsigned long *) gisa);
>>   }
>>   
>> -static inline unsigned long pending_irqs_no_gisa(struct kvm_vcpu *vcpu)
>> +static inline unsigned long pending_irqs(struct kvm_vcpu *vcpu, u16 irq_flags)
> 
> Any deeper reason why this is a u16? 16 bits should be enough for
> everyone? :)

I want to use the 8 bits for the IRQ type and the other 8 for additional 
controls, see: "KVM: s390: restore IAM in get_ipm() when IPM is clean"

> 
>>   {
>> -	return vcpu->kvm->arch.float_int.pending_irqs |
>> -		vcpu->arch.local_int.pending_irqs;
>> -}
>> +	unsigned long pending_irqs = 0;
>>   
>> -static inline unsigned long pending_irqs(struct kvm_vcpu *vcpu)
>> -{
>> -	return pending_irqs_no_gisa(vcpu) |
>> -		kvm_s390_gisa_get_ipm(vcpu->kvm->arch.gisa) << IRQ_PEND_IO_ISC_7;
>> +	if (irq_flags & IRQ_FLAG_LOCAL)
>> +		pending_irqs |= vcpu->arch.local_int.pending_irqs;
>> +	if (irq_flags & IRQ_FLAG_FLOATING)
>> +		pending_irqs |= vcpu->kvm->arch.float_int.pending_irqs;
>> +	if (irq_flags & IRQ_FLAG_GISA)
>> +		pending_irqs |= kvm_s390_gisa_get_ipm(vcpu->kvm->arch.gisa) <<
>> +			IRQ_PEND_IO_ISC_7;
>> +	return pending_irqs;
>>   }
>>   
>>   static inline int isc_to_irq_type(unsigned long isc)
>> @@ -275,7 +284,7 @@ static unsigned long deliverable_irqs(struct kvm_vcpu *vcpu)
>>   {
>>   	unsigned long active_mask;
>>   
>> -	active_mask = pending_irqs(vcpu);
>> +	active_mask = pending_irqs(vcpu, IRQ_MASK_ALL);
>>   	if (!active_mask)
>>   		return 0;
>>   
>> @@ -343,7 +352,7 @@ static void __reset_intercept_indicators(struct kvm_vcpu *vcpu)
>>   
>>   static void set_intercept_indicators_io(struct kvm_vcpu *vcpu)
>>   {
>> -	if (!(pending_irqs_no_gisa(vcpu) & IRQ_PEND_IO_MASK))
>> +	if (!(pending_irqs(vcpu, IRQ_MASK_NO_GISA) & IRQ_PEND_IO_MASK))
> 
> I/O interrupts are always floating, so you probably could check for
> only floating (excluding gisa) irqs here.

That's right.

> 
>>   		return;
>>   	else if (psw_ioint_disabled(vcpu))
>>   		kvm_s390_set_cpuflags(vcpu, CPUSTAT_IO_INT);Store Data
>> @@ -353,7 +362,7 @@ static void set_intercept_indicators_io(struct kvm_vcpu *vcpu)
>>   

Re: [PATCH v5 05/15] KVM: s390: unify pending_irqs() and pending_irqs_no_gisa()

[Halil Pasic]

On Thu, 20 Dec 2018 12:49:56 +0100
Michael Mueller <mimu@linux.ibm.com> wrote:

> 
> 
> On 20.12.18 12:06, Cornelia Huck wrote:
> > On Wed, 19 Dec 2018 20:17:46 +0100
> > Michael Mueller <mimu@linux.ibm.com> wrote:
> > 
> >> Use a single function with parameter irq_flags to differentiate
> >> between cases.
> >>
> >> New irq flag:
> >>     IRQ_FLAG_LOCAL: include vcpu local interruptions pending
> >>     IRQ_FLAG_FLOATING: include vcpu floating interruptions pending
> >>     IRQ_FLAG_GISA: include GISA interruptions pending in IPM
> > 
> > I presume that means that irqs may be in more than one set? Or are gisa
> > irqs not considered floating irqs, because they use a different
> > mechanism?
> 
> Currently, the interruptions managed in GISA are floating only. But
> that might change in future. 

I don't think GISA can be used for non-floating interrupts.

Regards,
Halil

> The idea is not to subsume IRQ_FLAG_GISA
> in IRQ_FLAG_FLOATING but to be able to address the right set of 
> procedures to determine the irq pending set for a given subset of irq
> types that have different implementations.
> 
> There might be a better name for IRQ_FLAG_FLOATING then?
> 

[..]

Re: [PATCH v5 05/15] KVM: s390: unify pending_irqs() and pending_irqs_no_gisa()

[Cornelia Huck]

On Thu, 20 Dec 2018 12:49:56 +0100
Michael Mueller <mimu@linux.ibm.com> wrote:

> On 20.12.18 12:06, Cornelia Huck wrote:
> > On Wed, 19 Dec 2018 20:17:46 +0100
> > Michael Mueller <mimu@linux.ibm.com> wrote:
> >   
> >> Use a single function with parameter irq_flags to differentiate
> >> between cases.
> >>
> >> New irq flag:
> >>     IRQ_FLAG_LOCAL: include vcpu local interruptions pending
> >>     IRQ_FLAG_FLOATING: include vcpu floating interruptions pending
> >>     IRQ_FLAG_GISA: include GISA interruptions pending in IPM  
> > 
> > I presume that means that irqs may be in more than one set? Or are gisa
> > irqs not considered floating irqs, because they use a different
> > mechanism?  
> 
> Currently, the interruptions managed in GISA are floating only. But
> that might change in future. The idea is not to subsume IRQ_FLAG_GISA
> in IRQ_FLAG_FLOATING but to be able to address the right set of 
> procedures to determine the irq pending set for a given subset of irq
> types that have different implementations.
> 
> There might be a better name for IRQ_FLAG_FLOATING then?

So the split is:

- local interrupts that are pending via kvm structures;
- floating interrupts that are pending via kvm structures;
- interrupts that are pending via gisa?

If so, what about
- IRQ_FLAG_KVM_LOCAL
- IRQ_FLAG_KVM_FLOATING
- IRQ_FLAG_GISA (or maybe IRQ_FLAG_GISA_FLOATING, if you need to
  distinguish those later on?)

> 
> >   
> >>
> >> New irq masks:
> >>     IRQ_MASK_ALL: include all types
> >>     IRQ_MASK_NO_GISA: include all types but GISA
> >>
> >> Examples:
> >>     pending_irqs(vcpu, IRQ_MASK_ALL)
> >>     pending_irqs(vcpu, IRQ_MASK_NO_GISA)
> >>
> >> There will be more irq flags with upcoming patches.
> >>
> >> Signed-off-by: Michael Mueller <mimu@linux.ibm.com>
> >> ---
> >>   arch/s390/kvm/interrupt.c | 33 +++++++++++++++++++++------------
> >>   1 file changed, 21 insertions(+), 12 deletions(-)
> >>
> >> diff --git a/arch/s390/kvm/interrupt.c b/arch/s390/kvm/interrupt.c
> >> index 093b568b6356..4ab20d2eb180 100644
> >> --- a/arch/s390/kvm/interrupt.c
> >> +++ b/arch/s390/kvm/interrupt.c
> >> @@ -31,6 +31,13 @@
> >>   #define PFAULT_DONE 0x0680
> >>   #define VIRTIO_PARAM 0x0d00
> >>   
> >> +#define IRQ_FLAG_LOCAL    0x8000 /* include local interruption pending mask */
> >> +#define IRQ_FLAG_FLOATING 0x4000 /* include float interruption pending mask */
> >> +#define IRQ_FLAG_GISA     0x2000 /* include GISA interruption pending mask */
> >> +
> >> +#define IRQ_MASK_ALL      (IRQ_FLAG_LOCAL | IRQ_FLAG_FLOATING | IRQ_FLAG_GISA)
> >> +#define IRQ_MASK_NO_GISA  (IRQ_MASK_ALL & ~IRQ_FLAG_GISA)
> >> +
> >>   /* handle external calls via sigp interpretation facility */
> >>   static int sca_ext_call_pending(struct kvm_vcpu *vcpu, int *src_id)
> >>   {
> >> @@ -237,16 +244,18 @@ static inline int kvm_s390_gisa_tac_ipm_gisc(struct kvm_s390_gisa *gisa, u32 gis
> >>   	return test_and_clear_bit_inv(IPM_BIT_OFFSET + gisc, (unsigned long *) gisa);
> >>   }
> >>   
> >> -static inline unsigned long pending_irqs_no_gisa(struct kvm_vcpu *vcpu)
> >> +static inline unsigned long pending_irqs(struct kvm_vcpu *vcpu, u16 irq_flags)  
> > 
> > Any deeper reason why this is a u16? 16 bits should be enough for
> > everyone? :)  
> 
> I want to use the 8 bits for the IRQ type and the other 8 for additional 
> controls, see: "KVM: s390: restore IAM in get_ipm() when IPM is clean"

Still need to look at that patch, but my question mainly was "why only
16 bits"? I would think making this local variable larger is cheap.

> 
> >   
> >>   {
> >> -	return vcpu->kvm->arch.float_int.pending_irqs |
> >> -		vcpu->arch.local_int.pending_irqs;
> >> -}

Re: [PATCH v5 05/15] KVM: s390: unify pending_irqs() and pending_irqs_no_gisa()

[Michael Mueller]

On 20.12.18 13:21, Cornelia Huck wrote:
> On Thu, 20 Dec 2018 12:49:56 +0100
> Michael Mueller <mimu@linux.ibm.com> wrote:
> 
>> On 20.12.18 12:06, Cornelia Huck wrote:
>>> On Wed, 19 Dec 2018 20:17:46 +0100
>>> Michael Mueller <mimu@linux.ibm.com> wrote:
>>>    
>>>> Use a single function with parameter irq_flags to differentiate
>>>> between cases.
>>>>
>>>> New irq flag:
>>>>      IRQ_FLAG_LOCAL: include vcpu local interruptions pending
>>>>      IRQ_FLAG_FLOATING: include vcpu floating interruptions pending
>>>>      IRQ_FLAG_GISA: include GISA interruptions pending in IPM
>>>
>>> I presume that means that irqs may be in more than one set? Or are gisa
>>> irqs not considered floating irqs, because they use a different
>>> mechanism?
>>
>> Currently, the interruptions managed in GISA are floating only. But
>> that might change in future. The idea is not to subsume IRQ_FLAG_GISA
>> in IRQ_FLAG_FLOATING but to be able to address the right set of
>> procedures to determine the irq pending set for a given subset of irq
>> types that have different implementations.
>>
>> There might be a better name for IRQ_FLAG_FLOATING then?
> 
> So the split is:
> 
> - local interrupts that are pending via kvm structures;
> - floating interrupts that are pending via kvm structures;
> - interrupts that are pending via gisa?
> 
> If so, what about
> - IRQ_FLAG_KVM_LOCAL
> - IRQ_FLAG_KVM_FLOATING
> - IRQ_FLAG_GISA (or maybe IRQ_FLAG_GISA_FLOATING, if you need to
>    distinguish those later on?)


yes, that's the split and I will go with:

IRQ_FLAG_KVM_LOCAL
IRQ_FLAG_KVM_FLOATING
IRQ_FLAG_GISA

initially.

The floating and local selection can be done by related masks

IRQ_MASK_LOCAL
IRQ_MASK_FLOATING

if required.

Thanks!

> 
>>
>>>    
>>>>
>>>> New irq masks:
>>>>      IRQ_MASK_ALL: include all types
>>>>      IRQ_MASK_NO_GISA: include all types but GISA
>>>>
>>>> Examples:
>>>>      pending_irqs(vcpu, IRQ_MASK_ALL)
>>>>      pending_irqs(vcpu, IRQ_MASK_NO_GISA)
>>>>
>>>> There will be more irq flags with upcoming patches.
>>>>
>>>> Signed-off-by: Michael Mueller <mimu@linux.ibm.com>
>>>> ---
>>>>    arch/s390/kvm/interrupt.c | 33 +++++++++++++++++++++------------
>>>>    1 file changed, 21 insertions(+), 12 deletions(-)
>>>>
>>>> diff --git a/arch/s390/kvm/interrupt.c b/arch/s390/kvm/interrupt.c
>>>> index 093b568b6356..4ab20d2eb180 100644
>>>> --- a/arch/s390/kvm/interrupt.c
>>>> +++ b/arch/s390/kvm/interrupt.c
>>>> @@ -31,6 +31,13 @@
>>>>    #define PFAULT_DONE 0x0680
>>>>    #define VIRTIO_PARAM 0x0d00
>>>>    
>>>> +#define IRQ_FLAG_LOCAL    0x8000 /* include local interruption pending mask */
>>>> +#define IRQ_FLAG_FLOATING 0x4000 /* include float interruption pending mask */
>>>> +#define IRQ_FLAG_GISA     0x2000 /* include GISA interruption pending mask */
>>>> +
>>>> +#define IRQ_MASK_ALL      (IRQ_FLAG_LOCAL | IRQ_FLAG_FLOATING | IRQ_FLAG_GISA)
>>>> +#define IRQ_MASK_NO_GISA  (IRQ_MASK_ALL & ~IRQ_FLAG_GISA)
>>>> +
>>>>    /* handle external calls via sigp interpretation facility */
>>>>    static int sca_ext_call_pending(struct kvm_vcpu *vcpu, int *src_id)
>>>>    {
>>>> @@ -237,16 +244,18 @@ static inline int kvm_s390_gisa_tac_ipm_gisc(struct kvm_s390_gisa *gisa, u32 gis
>>>>    	return test_and_clear_bit_inv(IPM_BIT_OFFSET + gisc, (unsigned long *) gisa);
>>>>    }
>>>>    
>>>> -static inline unsigned long pending_irqs_no_gisa(struct kvm_vcpu *vcpu)
>>>> +static inline unsigned long pending_irqs(struct kvm_vcpu *vcpu, u16 irq_flags)
>>>
>>> Any deeper reason why this is a u16? 16 bits should be enough for
>>> everyone? :)
>>
>> I want to use the 8 bits for the IRQ type and the other 8 for additional
>> controls, see: "KVM: s390: restore IAM in get_ipm() when IPM is clean"
> 
> Still need to look at that patch, but my question mainly was "why only
> 16 bits"? I would think making this local variable larger is cheap.
> 

I will enlarge the flag mask to u32 with 16 bits for the IRQ types then.

>>
>>>    
>>>>    {
>>>> -	return vcpu->kvm->arch.float_int.pending_irqs |
>>>> -		vcpu->arch.local_int.pending_irqs;
>>>> -}
> 

-- 
Mit freundlichen Grüßen / Kind regards
Michael Müller

IBM Deutschland Research & Development GmbH
Vorsitzender des Aufsichtsrats: Matthias Hartmann
Geschäftsführung: Dirk Wittkopp
Sitz der Gesellschaft: Böblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294

Re: [PATCH v5 05/15] KVM: s390: unify pending_irqs() and pending_irqs_no_gisa()

[pierre morel]

Le 12/20/18 à 13:33, Michael Mueller a écrit :
> 
> 
> On 20.12.18 13:21, Cornelia Huck wrote:
>> On Thu, 20 Dec 2018 12:49:56 +0100
>> Michael Mueller <mimu@linux.ibm.com> wrote:
>>
>>> On 20.12.18 12:06, Cornelia Huck wrote:
>>>> On Wed, 19 Dec 2018 20:17:46 +0100
>>>> Michael Mueller <mimu@linux.ibm.com> wrote:
>>>>> Use a single function with parameter irq_flags to differentiate
>>>>> between cases.
>>>>>
...snip
>>>>>    }
>>>>> -static inline unsigned long pending_irqs_no_gisa(struct kvm_vcpu 
>>>>> *vcpu)
>>>>> +static inline unsigned long pending_irqs(struct kvm_vcpu *vcpu, 
>>>>> u16 irq_flags)
>>>>
>>>> Any deeper reason why this is a u16? 16 bits should be enough for
>>>> everyone? :)
>>>
>>> I want to use the 8 bits for the IRQ type and the other 8 for additional
>>> controls, see: "KVM: s390: restore IAM in get_ipm() when IPM is clean"
>>
>> Still need to look at that patch, but my question mainly was "why only
>> 16 bits"? I would think making this local variable larger is cheap.
>>

+1

> 
> I will enlarge the flag mask to u32 with 16 bits for the IRQ types then.

AFAIK CPU generally work better with int (or long)
Is there any hardware reason to restrict the size?

> 
>>>
>>>>>    {
>>>>> -    return vcpu->kvm->arch.float_int.pending_irqs |
>>>>> -        vcpu->arch.local_int.pending_irqs;
>>>>> -}
>>
> 

Re: [PATCH v5 05/15] KVM: s390: unify pending_irqs() and pending_irqs_no_gisa()

[Michael Mueller]

On 20.12.18 16:43, pierre morel wrote:
> 
> 
> Le 12/20/18 à 13:33, Michael Mueller a écrit :
>>
>>
>> On 20.12.18 13:21, Cornelia Huck wrote:
>>> On Thu, 20 Dec 2018 12:49:56 +0100
>>> Michael Mueller <mimu@linux.ibm.com> wrote:
>>>
>>>> On 20.12.18 12:06, Cornelia Huck wrote:
>>>>> On Wed, 19 Dec 2018 20:17:46 +0100
>>>>> Michael Mueller <mimu@linux.ibm.com> wrote:
>>>>>> Use a single function with parameter irq_flags to differentiate
>>>>>> between cases.
>>>>>>
> ...snip
>>>>>>    }
>>>>>> -static inline unsigned long pending_irqs_no_gisa(struct kvm_vcpu 
>>>>>> *vcpu)
>>>>>> +static inline unsigned long pending_irqs(struct kvm_vcpu *vcpu, 
>>>>>> u16 irq_flags)
>>>>>
>>>>> Any deeper reason why this is a u16? 16 bits should be enough for
>>>>> everyone? :)
>>>>
>>>> I want to use the 8 bits for the IRQ type and the other 8 for 
>>>> additional
>>>> controls, see: "KVM: s390: restore IAM in get_ipm() when IPM is clean"
>>>
>>> Still need to look at that patch, but my question mainly was "why only
>>> 16 bits"? I would think making this local variable larger is cheap.
>>>
> 
> +1
> 
>>
>> I will enlarge the flag mask to u32 with 16 bits for the IRQ types then.
> 
> AFAIK CPU generally work better with int (or long)
> Is there any hardware reason to restrict the size?

It's already changed to 4 bytes

> 
>>
>>>>
>>>>>>    {
>>>>>> -    return vcpu->kvm->arch.float_int.pending_irqs |
>>>>>> -        vcpu->arch.local_int.pending_irqs;
>>>>>> -}
>>>
>>
> 

[PATCH v5 06/15] KVM: s390: remove prefix kvm_s390_gisa_ from static inline functions

[Michael Mueller]

This will shorten the length of code lines.
All GISA related static inline functions are local to interrupt.c

Signed-off-by: Michael Mueller <mimu@linux.ibm.com>
---
 arch/s390/kvm/interrupt.c | 26 +++++++++++++-------------
 1 file changed, 13 insertions(+), 13 deletions(-)

diff --git a/arch/s390/kvm/interrupt.c b/arch/s390/kvm/interrupt.c
index 4ab20d2eb180..9b1fa39b6f90 100644
--- a/arch/s390/kvm/interrupt.c
+++ b/arch/s390/kvm/interrupt.c
@@ -224,22 +224,22 @@ static inline u8 int_word_to_isc(u32 int_word)
  */
 #define IPM_BIT_OFFSET (offsetof(struct kvm_s390_gisa, ipm) * BITS_PER_BYTE)
 
-static inline void kvm_s390_gisa_set_ipm_gisc(struct kvm_s390_gisa *gisa, u32 gisc)
+static inline void set_ipm_gisc(struct kvm_s390_gisa *gisa, u32 gisc)
 {
 	set_bit_inv(IPM_BIT_OFFSET + gisc, (unsigned long *) gisa);
 }
 
-static inline u8 kvm_s390_gisa_get_ipm(struct kvm_s390_gisa *gisa)
+static inline u8 get_ipm(struct kvm_s390_gisa *gisa)
 {
 	return READ_ONCE(gisa->ipm);
 }
 
-static inline void kvm_s390_gisa_clear_ipm_gisc(struct kvm_s390_gisa *gisa, u32 gisc)
+static inline void clear_ipm_gisc(struct kvm_s390_gisa *gisa, u32 gisc)
 {
 	clear_bit_inv(IPM_BIT_OFFSET + gisc, (unsigned long *) gisa);
 }
 
-static inline int kvm_s390_gisa_tac_ipm_gisc(struct kvm_s390_gisa *gisa, u32 gisc)
+static inline int tac_ipm_gisc(struct kvm_s390_gisa *gisa, u32 gisc)
 {
 	return test_and_clear_bit_inv(IPM_BIT_OFFSET + gisc, (unsigned long *) gisa);
 }
@@ -253,7 +253,7 @@ static inline unsigned long pending_irqs(struct kvm_vcpu *vcpu, u16 irq_flags)
 	if (irq_flags & IRQ_FLAG_FLOATING)
 		pending_irqs |= vcpu->kvm->arch.float_int.pending_irqs;
 	if (irq_flags & IRQ_FLAG_GISA)
-		pending_irqs |= kvm_s390_gisa_get_ipm(vcpu->kvm->arch.gisa) <<
+		pending_irqs |= get_ipm(vcpu->kvm->arch.gisa) <<
 			IRQ_PEND_IO_ISC_7;
 	return pending_irqs;
 }
@@ -1008,7 +1008,7 @@ static int __must_check __deliver_io(struct kvm_vcpu *vcpu,
 	}
 
 	if (vcpu->kvm->arch.gisa &&
-	    kvm_s390_gisa_tac_ipm_gisc(vcpu->kvm->arch.gisa, isc)) {
+	    tac_ipm_gisc(vcpu->kvm->arch.gisa, isc)) {
 		/*
 		 * in case an adapter interrupt was not delivered
 		 * in SIE context KVM will handle the delivery
@@ -1550,10 +1550,10 @@ static int get_top_gisa_isc(struct kvm *kvm, u64 isc_mask, u32 schid)
 	if (!kvm->arch.gisa)
 		goto out;
 
-	active_mask = (isc_mask & kvm_s390_gisa_get_ipm(kvm->arch.gisa) << 24) << 32;
+	active_mask = (isc_mask & get_ipm(kvm->arch.gisa) << 24) << 32;
 	while (active_mask) {
 		isc = __fls(active_mask) ^ (BITS_PER_LONG - 1);
-		if (kvm_s390_gisa_tac_ipm_gisc(kvm->arch.gisa, isc))
+		if (tac_ipm_gisc(kvm->arch.gisa, isc))
 			return isc;
 		clear_bit_inv(isc, &active_mask);
 	}
@@ -1593,7 +1593,7 @@ struct kvm_s390_interrupt_info *kvm_s390_get_io_int(struct kvm *kvm,
 	/* both types of interrupts present */
 	if (int_word_to_isc(inti->io.io_int_word) <= isc) {
 		/* classical IO int with higher priority */
-		kvm_s390_gisa_set_ipm_gisc(kvm->arch.gisa, isc);
+		set_ipm_gisc(kvm->arch.gisa, isc);
 		goto out;
 	}
 gisa_out:
@@ -1605,7 +1605,7 @@ struct kvm_s390_interrupt_info *kvm_s390_get_io_int(struct kvm *kvm,
 			kvm_s390_reinject_io_int(kvm, inti);
 		inti = tmp_inti;
 	} else
-		kvm_s390_gisa_set_ipm_gisc(kvm->arch.gisa, isc);
+		set_ipm_gisc(kvm->arch.gisa, isc);
 out:
 	return inti;
 }
@@ -1703,7 +1703,7 @@ static int __inject_io(struct kvm *kvm, struct kvm_s390_interrupt_info *inti)
 
 	if (kvm->arch.gisa && inti->type & KVM_S390_INT_IO_AI_MASK) {
 		VM_EVENT(kvm, 4, "%s isc %1u", "inject: I/O (AI/gisa)", isc);
-		kvm_s390_gisa_set_ipm_gisc(kvm->arch.gisa, isc);
+		set_ipm_gisc(kvm->arch.gisa, isc);
 		kfree(inti);
 		return 0;
 	}
@@ -2036,14 +2036,14 @@ static int get_all_floating_irqs(struct kvm *kvm, u8 __user *usrbuf, u64 len)
 	max_irqs = len / sizeof(struct kvm_s390_irq);
 
 	if (kvm->arch.gisa &&
-	    kvm_s390_gisa_get_ipm(kvm->arch.gisa)) {
+	    get_ipm(kvm->arch.gisa)) {
 		for (i = 0; i <= MAX_ISC; i++) {
 			if (n == max_irqs) {
 				/* signal userspace to try again */
 				ret = -ENOMEM;
 				goto out_nolock;
 			}
-			if (kvm_s390_gisa_tac_ipm_gisc(kvm->arch.gisa, i)) {
+			if (tac_ipm_gisc(kvm->arch.gisa, i)) {
 				irq = (struct kvm_s390_irq *) &buf[n];
 				irq->type = KVM_S390_INT_IO(1, 0, 0, 0);
 				irq->u.io.io_int_word = isc_to_int_word(i);
-- 
2.13.4

Re: [PATCH v5 06/15] KVM: s390: remove prefix kvm_s390_gisa_ from static inline functions

[Cornelia Huck]

On Wed, 19 Dec 2018 20:17:47 +0100
Michael Mueller <mimu@linux.ibm.com> wrote:

> This will shorten the length of code lines.
> All GISA related static inline functions are local to interrupt.c
> 
> Signed-off-by: Michael Mueller <mimu@linux.ibm.com>
> ---
>  arch/s390/kvm/interrupt.c | 26 +++++++++++++-------------
>  1 file changed, 13 insertions(+), 13 deletions(-)
> 
> diff --git a/arch/s390/kvm/interrupt.c b/arch/s390/kvm/interrupt.c
> index 4ab20d2eb180..9b1fa39b6f90 100644
> --- a/arch/s390/kvm/interrupt.c
> +++ b/arch/s390/kvm/interrupt.c
> @@ -224,22 +224,22 @@ static inline u8 int_word_to_isc(u32 int_word)
>   */
>  #define IPM_BIT_OFFSET (offsetof(struct kvm_s390_gisa, ipm) * BITS_PER_BYTE)
>  
> -static inline void kvm_s390_gisa_set_ipm_gisc(struct kvm_s390_gisa *gisa, u32 gisc)
> +static inline void set_ipm_gisc(struct kvm_s390_gisa *gisa, u32 gisc)
>  {
>  	set_bit_inv(IPM_BIT_OFFSET + gisc, (unsigned long *) gisa);
>  }
>  
> -static inline u8 kvm_s390_gisa_get_ipm(struct kvm_s390_gisa *gisa)
> +static inline u8 get_ipm(struct kvm_s390_gisa *gisa)
>  {
>  	return READ_ONCE(gisa->ipm);
>  }
>  
> -static inline void kvm_s390_gisa_clear_ipm_gisc(struct kvm_s390_gisa *gisa, u32 gisc)
> +static inline void clear_ipm_gisc(struct kvm_s390_gisa *gisa, u32 gisc)
>  {
>  	clear_bit_inv(IPM_BIT_OFFSET + gisc, (unsigned long *) gisa);
>  }
>  
> -static inline int kvm_s390_gisa_tac_ipm_gisc(struct kvm_s390_gisa *gisa, u32 gisc)
> +static inline int tac_ipm_gisc(struct kvm_s390_gisa *gisa, u32 gisc)
>  {
>  	return test_and_clear_bit_inv(IPM_BIT_OFFSET + gisc, (unsigned long *) gisa);
>  }

I don't disagree with making them shorter, but I think the code would
be more readable if you only dropped the kvm_s390_ prefix and kept
annotating the functions as gisa_.

Re: [PATCH v5 06/15] KVM: s390: remove prefix kvm_s390_gisa_ from static inline functions

[Michael Mueller]

On 20.12.18 13:24, Cornelia Huck wrote:
> On Wed, 19 Dec 2018 20:17:47 +0100
> Michael Mueller <mimu@linux.ibm.com> wrote:
> 
>> This will shorten the length of code lines.
>> All GISA related static inline functions are local to interrupt.c
>>
>> Signed-off-by: Michael Mueller <mimu@linux.ibm.com>
>> ---
>>   arch/s390/kvm/interrupt.c | 26 +++++++++++++-------------
>>   1 file changed, 13 insertions(+), 13 deletions(-)
>>
>> diff --git a/arch/s390/kvm/interrupt.c b/arch/s390/kvm/interrupt.c
>> index 4ab20d2eb180..9b1fa39b6f90 100644
>> --- a/arch/s390/kvm/interrupt.c
>> +++ b/arch/s390/kvm/interrupt.c
>> @@ -224,22 +224,22 @@ static inline u8 int_word_to_isc(u32 int_word)
>>    */
>>   #define IPM_BIT_OFFSET (offsetof(struct kvm_s390_gisa, ipm) * BITS_PER_BYTE)
>>   
>> -static inline void kvm_s390_gisa_set_ipm_gisc(struct kvm_s390_gisa *gisa, u32 gisc)
>> +static inline void set_ipm_gisc(struct kvm_s390_gisa *gisa, u32 gisc)
>>   {
>>   	set_bit_inv(IPM_BIT_OFFSET + gisc, (unsigned long *) gisa);
>>   }
>>   
>> -static inline u8 kvm_s390_gisa_get_ipm(struct kvm_s390_gisa *gisa)
>> +static inline u8 get_ipm(struct kvm_s390_gisa *gisa)
>>   {
>>   	return READ_ONCE(gisa->ipm);
>>   }
>>   
>> -static inline void kvm_s390_gisa_clear_ipm_gisc(struct kvm_s390_gisa *gisa, u32 gisc)
>> +static inline void clear_ipm_gisc(struct kvm_s390_gisa *gisa, u32 gisc)
>>   {
>>   	clear_bit_inv(IPM_BIT_OFFSET + gisc, (unsigned long *) gisa);
>>   }
>>   
>> -static inline int kvm_s390_gisa_tac_ipm_gisc(struct kvm_s390_gisa *gisa, u32 gisc)
>> +static inline int tac_ipm_gisc(struct kvm_s390_gisa *gisa, u32 gisc)
>>   {
>>   	return test_and_clear_bit_inv(IPM_BIT_OFFSET + gisc, (unsigned long *) gisa);
>>   }
> 
> I don't disagree with making them shorter, but I think the code would
> be more readable if you only dropped the kvm_s390_ prefix and kept
> annotating the functions as gisa_.

applied

> 

[PATCH v5 07/15] s390/cio: add function chsc_sgib()

[Michael Mueller]

This patch implements the Set Guest Information Block operation
to request association or disassociation of a Guest Information
Block (GIB) with the Adapter Interruption Facility. The operation
is required to receive GIB alert interrupts for guest adapters
in conjunction with AIV and GISA.

Signed-off-by: Michael Mueller <mimu@linux.ibm.com>
Reviewed-by: Sebastian Ott <sebott@linux.ibm.com>
Reviewed-by: Pierre Morel <pmorel@linux.ibm.com>
Reviewed-by: Christian Borntraeger <borntraeger@de.ibm.com>
Acked-by: Halil Pasic <pasic@linux.ibm.com>
Acked-by: Janosch Frank <frankja@linux.ibm.com>
Acked-by: Cornelia Huck <cohuck@redhat.com>
---
 arch/s390/include/asm/cio.h |  1 +
 drivers/s390/cio/chsc.c     | 37 +++++++++++++++++++++++++++++++++++++
 drivers/s390/cio/chsc.h     |  1 +
 3 files changed, 39 insertions(+)

diff --git a/arch/s390/include/asm/cio.h b/arch/s390/include/asm/cio.h
index 225667652069..1727180e8ca1 100644
--- a/arch/s390/include/asm/cio.h
+++ b/arch/s390/include/asm/cio.h
@@ -331,5 +331,6 @@ extern void css_schedule_reprobe(void);
 /* Function from drivers/s390/cio/chsc.c */
 int chsc_sstpc(void *page, unsigned int op, u16 ctrl, u64 *clock_delta);
 int chsc_sstpi(void *page, void *result, size_t size);
+int chsc_sgib(u32 origin);
 
 #endif
diff --git a/drivers/s390/cio/chsc.c b/drivers/s390/cio/chsc.c
index a0baee25134c..4159c63a5fd2 100644
--- a/drivers/s390/cio/chsc.c
+++ b/drivers/s390/cio/chsc.c
@@ -1382,3 +1382,40 @@ int chsc_pnso_brinfo(struct subchannel_id schid,
 	return chsc_error_from_response(brinfo_area->response.code);
 }
 EXPORT_SYMBOL_GPL(chsc_pnso_brinfo);
+
+int chsc_sgib(u32 origin)
+{
+	struct {
+		struct chsc_header request;
+		u16 op;
+		u8  reserved01[2];
+		u8  reserved02:4;
+		u8  fmt:4;
+		u8  reserved03[7];
+		/* operation data area begin */
+		u8  reserved04[4];
+		u32 gib_origin;
+		u8  reserved05[10];
+		u8  aix;
+		u8  reserved06[4029];
+		struct chsc_header response;
+		u8  reserved07[4];
+	} *sgib_area;
+	int ret;
+
+	spin_lock_irq(&chsc_page_lock);
+	memset(chsc_page, 0, PAGE_SIZE);
+	sgib_area = chsc_page;
+	sgib_area->request.length = 0x0fe0;
+	sgib_area->request.code = 0x0021;
+	sgib_area->op = 0x1;
+	sgib_area->gib_origin = origin;
+
+	ret = chsc(sgib_area);
+	if (ret == 0)
+		ret = chsc_error_from_response(sgib_area->response.code);
+	spin_unlock_irq(&chsc_page_lock);
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(chsc_sgib);
diff --git a/drivers/s390/cio/chsc.h b/drivers/s390/cio/chsc.h
index 78aba8d94eec..e57d68e325a3 100644
--- a/drivers/s390/cio/chsc.h
+++ b/drivers/s390/cio/chsc.h
@@ -164,6 +164,7 @@ int chsc_get_channel_measurement_chars(struct channel_path *chp);
 int chsc_ssqd(struct subchannel_id schid, struct chsc_ssqd_area *ssqd);
 int chsc_sadc(struct subchannel_id schid, struct chsc_scssc_area *scssc,
 	      u64 summary_indicator_addr, u64 subchannel_indicator_addr);
+int chsc_sgib(u32 origin);
 int chsc_error_from_response(int response);
 
 int chsc_siosl(struct subchannel_id schid);
-- 
2.13.4

[PATCH v5 08/15] KVM: s390: add the GIB and its related life-cyle functions

[Michael Mueller]

The Guest Information Block (GIB) links the GISA of all guests
that have adapter interrupts pending. These interrupts cannot be
delivered because no vcpu of these guests is currently running in
SIE context. Instead, a GIB alert is issued on the host to schedule
these guests to run.

This mechanism allows to process adapter interrupts for currently
not running guests.

The GIB is created during host initialization and associated with
the Adapter Interruption Facility in case an Adapter Interruption
Virtualization Facility is available.

The GIB initialization and thus the activation of the related code
will be done in an upcoming patch of this series.

Signed-off-by: Michael Mueller <mimu@linux.ibm.com>
Reviewed-by: Janosch Frank <frankja@linux.ibm.com>
Reviewed-by: Christian Borntraeger <borntraeger@de.ibm.com>
---
 arch/s390/include/asm/kvm_host.h | 10 +++++++++
 arch/s390/kvm/interrupt.c        | 44 ++++++++++++++++++++++++++++++++++++++++
 arch/s390/kvm/kvm-s390.c         |  1 +
 arch/s390/kvm/kvm-s390.h         |  2 ++
 4 files changed, 57 insertions(+)

diff --git a/arch/s390/include/asm/kvm_host.h b/arch/s390/include/asm/kvm_host.h
index d5d24889c3bc..537e5e59f27e 100644
--- a/arch/s390/include/asm/kvm_host.h
+++ b/arch/s390/include/asm/kvm_host.h
@@ -785,6 +785,15 @@ struct kvm_s390_gisa {
 	};
 };
 
+struct kvm_s390_gib {
+	u32 alert_list_origin;
+	u32 reserved01;
+	u8:5;
+	u8  nisc:3;
+	u8  reserved03[3];
+	u32 reserved04[5];
+};
+
 /*
  * sie_page2 has to be allocated as DMA because fac_list, crycb and
  * gisa need 31bit addresses in the sie control block.
@@ -838,6 +847,7 @@ struct kvm_arch{
 	/* subset of available cpu features enabled by user space */
 	DECLARE_BITMAP(cpu_feat, KVM_S390_VM_CPU_FEAT_NR_BITS);
 	struct kvm_s390_gisa *gisa;
+	int gib_in_use;
 };
 
 #define KVM_HVA_ERR_BAD		(-1UL)
diff --git a/arch/s390/kvm/interrupt.c b/arch/s390/kvm/interrupt.c
index 9b1fa39b6f90..5a54360cecfe 100644
--- a/arch/s390/kvm/interrupt.c
+++ b/arch/s390/kvm/interrupt.c
@@ -7,6 +7,9 @@
  *    Author(s): Carsten Otte <cotte@de.ibm.com>
  */
 
+#define KMSG_COMPONENT "kvm-s390"
+#define pr_fmt(fmt) KMSG_COMPONENT ": " fmt
+
 #include <linux/interrupt.h>
 #include <linux/kvm_host.h>
 #include <linux/hrtimer.h>
@@ -38,6 +41,8 @@
 #define IRQ_MASK_ALL      (IRQ_FLAG_LOCAL | IRQ_FLAG_FLOATING | IRQ_FLAG_GISA)
 #define IRQ_MASK_NO_GISA  (IRQ_MASK_ALL & ~IRQ_FLAG_GISA)
 
+static struct kvm_s390_gib *gib;
+
 /* handle external calls via sigp interpretation facility */
 static int sca_ext_call_pending(struct kvm_vcpu *vcpu, int *src_id)
 {
@@ -2913,6 +2918,7 @@ void kvm_s390_gisa_init(struct kvm *kvm)
 		return;
 	kvm->arch.gisa = &kvm->arch.sie_page2->gisa;
 	nullify_gisa(kvm->arch.gisa);
+	kvm->arch.gib_in_use = !!gib;
 	VM_EVENT(kvm, 3, "gisa 0x%pK initialized", kvm->arch.gisa);
 }
 
@@ -2922,3 +2928,41 @@ void kvm_s390_gisa_destroy(struct kvm *kvm)
 		return;
 	kvm->arch.gisa = NULL;
 }
+
+void kvm_s390_gib_destroy(void)
+{
+	if (!gib)
+		return;
+	chsc_sgib(0);
+	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");
+		goto out;
+	}
+
+	gib = (struct kvm_s390_gib *)get_zeroed_page(GFP_KERNEL | GFP_DMA);
+	if (!gib) {
+		rc = -ENOMEM;
+		goto out;
+	}
+
+	gib->nisc = nisc;
+	if (chsc_sgib((u32)(u64)gib)) {
+		pr_err("Associating the GIB with the AIV facility failed\n");
+		free_page((unsigned long)gib);
+		gib = NULL;
+		rc = -EIO;
+		goto out;
+	}
+
+	KVM_EVENT(3, "gib 0x%pK (nisc=%d) initialized", gib, gib->nisc);
+out:
+	return rc;
+}
diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
index d8d8e0788157..7bc24e16a31d 100644
--- a/arch/s390/kvm/kvm-s390.c
+++ b/arch/s390/kvm/kvm-s390.c
@@ -445,6 +445,7 @@ int kvm_arch_init(void *opaque)
 
 void kvm_arch_exit(void)
 {
+	kvm_s390_gib_destroy();
 	debug_unregister(kvm_s390_dbf);
 }
 
diff --git a/arch/s390/kvm/kvm-s390.h b/arch/s390/kvm/kvm-s390.h
index 1f6e36cdce0d..1a79105b0e9f 100644
--- a/arch/s390/kvm/kvm-s390.h
+++ b/arch/s390/kvm/kvm-s390.h
@@ -381,6 +381,8 @@ int kvm_s390_get_irq_state(struct kvm_vcpu *vcpu,
 void kvm_s390_gisa_init(struct kvm *kvm);
 void kvm_s390_gisa_clear(struct kvm *kvm);
 void kvm_s390_gisa_destroy(struct kvm *kvm);
+int kvm_s390_gib_init(u8 nisc);
+void kvm_s390_gib_destroy(void);
 
 /* implemented in guestdbg.c */
 void kvm_s390_backup_guest_per_regs(struct kvm_vcpu *vcpu);
-- 
2.13.4

Re: [PATCH v5 08/15] KVM: s390: add the GIB and its related life-cyle functions

[Cornelia Huck]

On Wed, 19 Dec 2018 20:17:49 +0100
Michael Mueller <mimu@linux.ibm.com> wrote:

> The Guest Information Block (GIB) links the GISA of all guests
> that have adapter interrupts pending. These interrupts cannot be
> delivered because no vcpu of these guests is currently running in
> SIE context. Instead, a GIB alert is issued on the host to schedule
> these guests to run.
> 
> This mechanism allows to process adapter interrupts for currently
> not running guests.
> 
> The GIB is created during host initialization and associated with
> the Adapter Interruption Facility in case an Adapter Interruption
> Virtualization Facility is available.
> 
> The GIB initialization and thus the activation of the related code
> will be done in an upcoming patch of this series.
> 
> Signed-off-by: Michael Mueller <mimu@linux.ibm.com>
> Reviewed-by: Janosch Frank <frankja@linux.ibm.com>
> Reviewed-by: Christian Borntraeger <borntraeger@de.ibm.com>
> ---
>  arch/s390/include/asm/kvm_host.h | 10 +++++++++
>  arch/s390/kvm/interrupt.c        | 44 ++++++++++++++++++++++++++++++++++++++++
>  arch/s390/kvm/kvm-s390.c         |  1 +
>  arch/s390/kvm/kvm-s390.h         |  2 ++
>  4 files changed, 57 insertions(+)

> +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");
> +		goto out;
> +	}
> +
> +	gib = (struct kvm_s390_gib *)get_zeroed_page(GFP_KERNEL | GFP_DMA);
> +	if (!gib) {
> +		rc = -ENOMEM;
> +		goto out;
> +	}
> +
> +	gib->nisc = nisc;
> +	if (chsc_sgib((u32)(u64)gib)) {
> +		pr_err("Associating the GIB with the AIV facility failed\n");

I presume that logging the chsc response code (or returning it instead
of -EIO) would not give any interesting information?

> +		free_page((unsigned long)gib);
> +		gib = NULL;
> +		rc = -EIO;
> +		goto out;
> +	}
> +
> +	KVM_EVENT(3, "gib 0x%pK (nisc=%d) initialized", gib, gib->nisc);
> +out:
> +	return rc;
> +}

Anyhow,
Reviewed-by: Cornelia Huck <cohuck@redhat.com>

Re: [PATCH v5 08/15] KVM: s390: add the GIB and its related life-cyle functions

[Pierre Morel]

On 19/12/2018 20:17, Michael Mueller wrote:
> The Guest Information Block (GIB) links the GISA of all guests
> that have adapter interrupts pending. These interrupts cannot be
> delivered because no vcpu of these guests is currently running in
> SIE context. Instead, a GIB alert is issued on the host to schedule
> these guests to run.

IMHO "Instead" is not exact.
The GIB alert is always issued on adapter interrupt if IAM is set even 
if the guest is running in SIE context.

I would prefer to rephrase to something like:
"If enabled, a GIB alert is issued on the host..." ?


Reviewed-by: Pierre Morel<pmorel@linux.ibm.com>


> 
> This mechanism allows to process adapter interrupts for currently
> not running guests.
> 
> The GIB is created during host initialization and associated with
> the Adapter Interruption Facility in case an Adapter Interruption
> Virtualization Facility is available.
> 
> The GIB initialization and thus the activation of the related code
> will be done in an upcoming patch of this series.
> 
> Signed-off-by: Michael Mueller <mimu@linux.ibm.com>
> Reviewed-by: Janosch Frank <frankja@linux.ibm.com>
> Reviewed-by: Christian Borntraeger <borntraeger@de.ibm.com>
> ---
>   arch/s390/include/asm/kvm_host.h | 10 +++++++++
>   arch/s390/kvm/interrupt.c        | 44 ++++++++++++++++++++++++++++++++++++++++
>   arch/s390/kvm/kvm-s390.c         |  1 +
>   arch/s390/kvm/kvm-s390.h         |  2 ++
>   4 files changed, 57 insertions(+)
> 
> diff --git a/arch/s390/include/asm/kvm_host.h b/arch/s390/include/asm/kvm_host.h
> index d5d24889c3bc..537e5e59f27e 100644
> --- a/arch/s390/include/asm/kvm_host.h
> +++ b/arch/s390/include/asm/kvm_host.h
> @@ -785,6 +785,15 @@ struct kvm_s390_gisa {
>   	};
>   };
>   
> +struct kvm_s390_gib {
> +	u32 alert_list_origin;
> +	u32 reserved01;
> +	u8:5;
> +	u8  nisc:3;
> +	u8  reserved03[3];
> +	u32 reserved04[5];
> +};
> +
>   /*
>    * sie_page2 has to be allocated as DMA because fac_list, crycb and
>    * gisa need 31bit addresses in the sie control block.
> @@ -838,6 +847,7 @@ struct kvm_arch{
>   	/* subset of available cpu features enabled by user space */
>   	DECLARE_BITMAP(cpu_feat, KVM_S390_VM_CPU_FEAT_NR_BITS);
>   	struct kvm_s390_gisa *gisa;
> +	int gib_in_use;
>   };
>   
>   #define KVM_HVA_ERR_BAD		(-1UL)
> diff --git a/arch/s390/kvm/interrupt.c b/arch/s390/kvm/interrupt.c
> index 9b1fa39b6f90..5a54360cecfe 100644
> --- a/arch/s390/kvm/interrupt.c
> +++ b/arch/s390/kvm/interrupt.c
> @@ -7,6 +7,9 @@
>    *    Author(s): Carsten Otte <cotte@de.ibm.com>
>    */
>   
> +#define KMSG_COMPONENT "kvm-s390"
> +#define pr_fmt(fmt) KMSG_COMPONENT ": " fmt
> +
>   #include <linux/interrupt.h>
>   #include <linux/kvm_host.h>
>   #include <linux/hrtimer.h>
> @@ -38,6 +41,8 @@
>   #define IRQ_MASK_ALL      (IRQ_FLAG_LOCAL | IRQ_FLAG_FLOATING | IRQ_FLAG_GISA)
>   #define IRQ_MASK_NO_GISA  (IRQ_MASK_ALL & ~IRQ_FLAG_GISA)
>   
> +static struct kvm_s390_gib *gib;
> +
>   /* handle external calls via sigp interpretation facility */
>   static int sca_ext_call_pending(struct kvm_vcpu *vcpu, int *src_id)
>   {
> @@ -2913,6 +2918,7 @@ void kvm_s390_gisa_init(struct kvm *kvm)
>   		return;
>   	kvm->arch.gisa = &kvm->arch.sie_page2->gisa;
>   	nullify_gisa(kvm->arch.gisa);
> +	kvm->arch.gib_in_use = !!gib;
>   	VM_EVENT(kvm, 3, "gisa 0x%pK initialized", kvm->arch.gisa);
>   }
>   
> @@ -2922,3 +2928,41 @@ void kvm_s390_gisa_destroy(struct kvm *kvm)
>   		return;
>   	kvm->arch.gisa = NULL;
>   }
> +
> +void kvm_s390_gib_destroy(void)
> +{
> +	if (!gib)
> +		return;
> +	chsc_sgib(0);
> +	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");
> +		goto out;
> +	}
> +
> +	gib = (struct kvm_s390_gib *)get_zeroed_page(GFP_KERNEL | GFP_DMA);
> +	if (!gib) {
> +		rc = -ENOMEM;
> +		goto out;
> +	}
> +
> +	gib->nisc = nisc;
> +	if (chsc_sgib((u32)(u64)gib)) {
> +		pr_err("Associating the GIB with the AIV facility failed\n");
> +		free_page((unsigned long)gib);
> +		gib = NULL;
> +		rc = -EIO;
> +		goto out;
> +	}
> +
> +	KVM_EVENT(3, "gib 0x%pK (nisc=%d) initialized", gib, gib->nisc);
> +out:
> +	return rc;
> +}
> diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
> index d8d8e0788157..7bc24e16a31d 100644
> --- a/arch/s390/kvm/kvm-s390.c
> +++ b/arch/s390/kvm/kvm-s390.c
> @@ -445,6 +445,7 @@ int kvm_arch_init(void *opaque)
>   
>   void kvm_arch_exit(void)
>   {
> +	kvm_s390_gib_destroy();
>   	debug_unregister(kvm_s390_dbf);
>   }
>   
> diff --git a/arch/s390/kvm/kvm-s390.h b/arch/s390/kvm/kvm-s390.h
> index 1f6e36cdce0d..1a79105b0e9f 100644
> --- a/arch/s390/kvm/kvm-s390.h
> +++ b/arch/s390/kvm/kvm-s390.h
> @@ -381,6 +381,8 @@ int kvm_s390_get_irq_state(struct kvm_vcpu *vcpu,
>   void kvm_s390_gisa_init(struct kvm *kvm);
>   void kvm_s390_gisa_clear(struct kvm *kvm);
>   void kvm_s390_gisa_destroy(struct kvm *kvm);
> +int kvm_s390_gib_init(u8 nisc);
> +void kvm_s390_gib_destroy(void);
>   
>   /* implemented in guestdbg.c */
>   void kvm_s390_backup_guest_per_regs(struct kvm_vcpu *vcpu);
> 


-- 
Pierre Morel
Linux/KVM/QEMU in Böblingen - Germany

Re: [PATCH v5 08/15] KVM: s390: add the GIB and its related life-cyle functions

[Michael Mueller]

On 03.01.19 10:49, Pierre Morel wrote:
> On 19/12/2018 20:17, Michael Mueller wrote:
>> The Guest Information Block (GIB) links the GISA of all guests
>> that have adapter interrupts pending. These interrupts cannot be
>> delivered because no vcpu of these guests is currently running in
>> SIE context. Instead, a GIB alert is issued on the host to schedule
>> these guests to run.
> 
> IMHO "Instead" is not exact.
> The GIB alert is always issued on adapter interrupt if IAM is set even 
> if the guest is running in SIE context.
> 
> I would prefer to rephrase to something like:
> "If enabled, a GIB alert is issued on the host..." ?

I agree.

> 
> 
> Reviewed-by: Pierre Morel<pmorel@linux.ibm.com>
> 
> 
>>
>> This mechanism allows to process adapter interrupts for currently
>> not running guests.
>>
>> The GIB is created during host initialization and associated with
>> the Adapter Interruption Facility in case an Adapter Interruption
>> Virtualization Facility is available.
>>
>> The GIB initialization and thus the activation of the related code
>> will be done in an upcoming patch of this series.
>>
>> Signed-off-by: Michael Mueller <mimu@linux.ibm.com>
>> Reviewed-by: Janosch Frank <frankja@linux.ibm.com>
>> Reviewed-by: Christian Borntraeger <borntraeger@de.ibm.com>
>> ---
>>   arch/s390/include/asm/kvm_host.h | 10 +++++++++

[PATCH v5 09/15] KVM: s390: add kvm reference to struct sie_page2

[Michael Mueller]

Adding the kvm reference to struct sie_page2 will allow to
determine the kvm a given gisa belongs to:

  container_of(gisa, struct sie_page2, gisa)->kvm

This functionality will be required to process a gisa in
gib alert interruption context.

Signed-off-by: Michael Mueller <mimu@linux.ibm.com>
Reviewed-by: Pierre Morel <pmorel@linux.ibm.com>
Reviewed-by: Cornelia Huck <cohuck@redhat.com>
Reviewed-by: David Hildenbrand <david@redhat.com>
---
 arch/s390/include/asm/kvm_host.h | 3 ++-
 arch/s390/kvm/kvm-s390.c         | 1 +
 2 files changed, 3 insertions(+), 1 deletion(-)

diff --git a/arch/s390/include/asm/kvm_host.h b/arch/s390/include/asm/kvm_host.h
index 537e5e59f27e..0deba3ae8bc7 100644
--- a/arch/s390/include/asm/kvm_host.h
+++ b/arch/s390/include/asm/kvm_host.h
@@ -802,7 +802,8 @@ struct sie_page2 {
 	__u64 fac_list[S390_ARCH_FAC_LIST_SIZE_U64];	/* 0x0000 */
 	struct kvm_s390_crypto_cb crycb;		/* 0x0800 */
 	struct kvm_s390_gisa gisa;			/* 0x0900 */
-	u8 reserved920[0x1000 - 0x920];			/* 0x0920 */
+	struct kvm *kvm;				/* 0x0920 */
+	u8 reserved928[0x1000 - 0x928];			/* 0x0928 */
 };
 
 struct kvm_s390_vsie {
diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
index 7bc24e16a31d..2d10e175862c 100644
--- a/arch/s390/kvm/kvm-s390.c
+++ b/arch/s390/kvm/kvm-s390.c
@@ -2220,6 +2220,7 @@ int kvm_arch_init_vm(struct kvm *kvm, unsigned long type)
 	if (!kvm->arch.sie_page2)
 		goto out_err;
 
+	kvm->arch.sie_page2->kvm = kvm;
 	kvm->arch.model.fac_list = kvm->arch.sie_page2->fac_list;
 
 	for (i = 0; i < kvm_s390_fac_size(); i++) {
-- 
2.13.4

[PATCH v5 10/15] KVM: s390: add functions to (un)register GISC with GISA

[Michael Mueller]

Add the IAM (Interruption Alert Mask) to the architecture specific
kvm struct. This mask in the GISA is used to define for which ISC
a GIB alert can be issued.

The functions kvm_s390_gisc_register() and kvm_s390_gisc_unregister()
are used to (un)register a GISC (guest ISC) with a virtual machine and
its GISA.

Upon successful completion, kvm_s390_gisc_register() returns the
ISC to be used for GIB alert interruptions. A negative return code
indicates an error during registration.

Theses functions will be used by other adapter types like AP and PCI to
request pass-through interruption support.

Signed-off-by: Michael Mueller <mimu@linux.ibm.com>
---
 arch/s390/include/asm/kvm_host.h |  9 ++++++
 arch/s390/kvm/interrupt.c        | 66 ++++++++++++++++++++++++++++++++++++++++
 2 files changed, 75 insertions(+)

diff --git a/arch/s390/include/asm/kvm_host.h b/arch/s390/include/asm/kvm_host.h
index 0deba3ae8bc7..2c1259da3636 100644
--- a/arch/s390/include/asm/kvm_host.h
+++ b/arch/s390/include/asm/kvm_host.h
@@ -782,6 +782,9 @@ struct kvm_s390_gisa {
 			u8  reserved03[11];
 			u32 airq_count;
 		} g1;
+		struct {
+			u64 word[4];
+		} u64;
 	};
 };
 
@@ -849,6 +852,9 @@ struct kvm_arch{
 	DECLARE_BITMAP(cpu_feat, KVM_S390_VM_CPU_FEAT_NR_BITS);
 	struct kvm_s390_gisa *gisa;
 	int gib_in_use;
+	u8 iam;
+	u32 iam_ref_count[MAX_ISC + 1];
+	spinlock_t iam_ref_lock;
 };
 
 #define KVM_HVA_ERR_BAD		(-1UL)
@@ -882,6 +888,9 @@ void kvm_arch_crypto_set_masks(struct kvm *kvm, unsigned long *apm,
 extern int sie64a(struct kvm_s390_sie_block *, u64 *);
 extern char sie_exit;
 
+extern int kvm_s390_gisc_register(struct kvm *kvm, u32 gisc);
+extern int kvm_s390_gisc_unregister(struct kvm *kvm, u32 gisc);
+
 static inline void kvm_arch_hardware_disable(void) {}
 static inline void kvm_arch_check_processor_compat(void *rtn) {}
 static inline void kvm_arch_sync_events(struct kvm *kvm) {}
diff --git a/arch/s390/kvm/interrupt.c b/arch/s390/kvm/interrupt.c
index 5a54360cecfe..1cc3ad2e6c7e 100644
--- a/arch/s390/kvm/interrupt.c
+++ b/arch/s390/kvm/interrupt.c
@@ -229,6 +229,25 @@ static inline u8 int_word_to_isc(u32 int_word)
  */
 #define IPM_BIT_OFFSET (offsetof(struct kvm_s390_gisa, ipm) * BITS_PER_BYTE)
 
+static inline int set_iam(struct kvm_s390_gisa *gisa, u8 iam)
+{
+	u64 word0, _word0;
+
+	do {
+		word0 = READ_ONCE(gisa->u64.word[0]);
+		/* If the GISA is in the alert list, do nothing. */
+		if ((u64)gisa != word0 >> 32)
+			return -EBUSY;
+		/*
+		 * Try to set the IAM or loop, if the IPM has changed
+		 * or the GISA has been inserted into the alert list.
+		 */
+		_word0 = (word0 & ~0xffUL) | iam;
+	} while (cmpxchg(&gisa->u64.word[0], word0, _word0) != _word0);
+
+	return 0;
+}
+
 static inline void set_ipm_gisc(struct kvm_s390_gisa *gisa, u32 gisc)
 {
 	set_bit_inv(IPM_BIT_OFFSET + gisc, (unsigned long *) gisa);
@@ -2917,6 +2936,8 @@ void kvm_s390_gisa_init(struct kvm *kvm)
 	if (!css_general_characteristics.aiv)
 		return;
 	kvm->arch.gisa = &kvm->arch.sie_page2->gisa;
+	kvm->arch.iam = 0;
+	spin_lock_init(&kvm->arch.iam_ref_lock);
 	nullify_gisa(kvm->arch.gisa);
 	kvm->arch.gib_in_use = !!gib;
 	VM_EVENT(kvm, 3, "gisa 0x%pK initialized", kvm->arch.gisa);
@@ -2929,6 +2950,51 @@ void kvm_s390_gisa_destroy(struct kvm *kvm)
 	kvm->arch.gisa = NULL;
 }
 
+int kvm_s390_gisc_register(struct kvm *kvm, u32 gisc)
+{
+	if (!kvm->arch.gib_in_use)
+		return -ENODEV;
+	if (gisc > MAX_ISC)
+		return -ERANGE;
+
+	spin_lock(&kvm->arch.iam_ref_lock);
+	if (kvm->arch.iam_ref_count[gisc] == 0)
+		kvm->arch.iam |= 0x80 >> gisc;
+	kvm->arch.iam_ref_count[gisc]++;
+	if (kvm->arch.iam_ref_count[gisc] == 1)
+		set_iam(kvm->arch.gisa, kvm->arch.iam);
+	spin_unlock(&kvm->arch.iam_ref_lock);
+
+	return gib->nisc;
+}
+EXPORT_SYMBOL_GPL(kvm_s390_gisc_register);
+
+int kvm_s390_gisc_unregister(struct kvm *kvm, u32 gisc)
+{
+	int rc = 0;
+
+	if (!kvm->arch.gib_in_use)
+		return -ENODEV;
+	if (gisc > MAX_ISC)
+		return -ERANGE;
+
+	spin_lock(&kvm->arch.iam_ref_lock);
+	if (kvm->arch.iam_ref_count[gisc] == 0) {
+		rc = -EINVAL;
+		goto out;
+	}
+	kvm->arch.iam_ref_count[gisc]--;
+	if (kvm->arch.iam_ref_count[gisc] == 0) {
+		kvm->arch.iam &= ~(0x80 >> gisc);
+		set_iam(kvm->arch.gisa, kvm->arch.iam);
+	}
+out:
+	spin_unlock(&kvm->arch.iam_ref_lock);
+
+	return rc;
+}
+EXPORT_SYMBOL_GPL(kvm_s390_gisc_unregister);
+
 void kvm_s390_gib_destroy(void)
 {
 	if (!gib)
-- 
2.13.4

Re: [PATCH v5 10/15] KVM: s390: add functions to (un)register GISC with GISA

[Michael Mueller]

On 19.12.18 20:17, Michael Mueller wrote:
> Add the IAM (Interruption Alert Mask) to the architecture specific
> kvm struct. This mask in the GISA is used to define for which ISC
> a GIB alert can be issued.
> 
> The functions kvm_s390_gisc_register() and kvm_s390_gisc_unregister()
> are used to (un)register a GISC (guest ISC) with a virtual machine and
> its GISA.
> 
> Upon successful completion, kvm_s390_gisc_register() returns the
> ISC to be used for GIB alert interruptions. A negative return code
> indicates an error during registration.
> 
> Theses functions will be used by other adapter types like AP and PCI to
> request pass-through interruption support.
> 
> Signed-off-by: Michael Mueller <mimu@linux.ibm.com>
> ---
>   arch/s390/include/asm/kvm_host.h |  9 ++++++
>   arch/s390/kvm/interrupt.c        | 66 ++++++++++++++++++++++++++++++++++++++++
>   2 files changed, 75 insertions(+)
> 

...

> --- a/arch/s390/kvm/interrupt.c
> +++ b/arch/s390/kvm/interrupt.c
> @@ -229,6 +229,25 @@ static inline u8 int_word_to_isc(u32 int_word)
>    */
>   #define IPM_BIT_OFFSET (offsetof(struct kvm_s390_gisa, ipm) * BITS_PER_BYTE)
>   
> +static inline int set_iam(struct kvm_s390_gisa *gisa, u8 iam)

I will rename this one here as well to gisa_set_iam()

> +{
> +	u64 word0, _word0;
> +
> +	do {
> +		word0 = READ_ONCE(gisa->u64.word[0]);
> +		/* If the GISA is in the alert list, do nothing. */
> +		if ((u64)gisa != word0 >> 32)
> +			return -EBUSY;
> +		/*
> +		 * Try to set the IAM or loop, if the IPM has changed
> +		 * or the GISA has been inserted into the alert list.
> +		 */
> +		_word0 = (word0 & ~0xffUL) | iam;
> +	} while (cmpxchg(&gisa->u64.word[0], word0, _word0) != _word0);
> +
> +	return 0;
> +}
> +

Re: [PATCH v5 10/15] KVM: s390: add functions to (un)register GISC with GISA

[Pierre Morel]

On 19/12/2018 20:17, Michael Mueller wrote:
> Add the IAM (Interruption Alert Mask) to the architecture specific
> kvm struct. This mask in the GISA is used to define for which ISC
> a GIB alert can be issued.
> 
> The functions kvm_s390_gisc_register() and kvm_s390_gisc_unregister()
> are used to (un)register a GISC (guest ISC) with a virtual machine and
> its GISA.
> 
> Upon successful completion, kvm_s390_gisc_register() returns the
> ISC to be used for GIB alert interruptions. A negative return code
> indicates an error during registration.
> 
> Theses functions will be used by other adapter types like AP and PCI to
> request pass-through interruption support.
> 
> Signed-off-by: Michael Mueller <mimu@linux.ibm.com>
> ---
>   arch/s390/include/asm/kvm_host.h |  9 ++++++
>   arch/s390/kvm/interrupt.c        | 66 ++++++++++++++++++++++++++++++++++++++++
>   2 files changed, 75 insertions(+)
> 
> diff --git a/arch/s390/include/asm/kvm_host.h b/arch/s390/include/asm/kvm_host.h
> index 0deba3ae8bc7..2c1259da3636 100644
> --- a/arch/s390/include/asm/kvm_host.h
> +++ b/arch/s390/include/asm/kvm_host.h
> @@ -782,6 +782,9 @@ struct kvm_s390_gisa {
>   			u8  reserved03[11];
>   			u32 airq_count;
>   		} g1;
> +		struct {
> +			u64 word[4];
> +		} u64;
>   	};
>   };
>   
> @@ -849,6 +852,9 @@ struct kvm_arch{
>   	DECLARE_BITMAP(cpu_feat, KVM_S390_VM_CPU_FEAT_NR_BITS);
>   	struct kvm_s390_gisa *gisa;
>   	int gib_in_use;
> +	u8 iam;
> +	u32 iam_ref_count[MAX_ISC + 1];
> +	spinlock_t iam_ref_lock;
>   };
>   
>   #define KVM_HVA_ERR_BAD		(-1UL)
> @@ -882,6 +888,9 @@ void kvm_arch_crypto_set_masks(struct kvm *kvm, unsigned long *apm,
>   extern int sie64a(struct kvm_s390_sie_block *, u64 *);
>   extern char sie_exit;
>   
> +extern int kvm_s390_gisc_register(struct kvm *kvm, u32 gisc);
> +extern int kvm_s390_gisc_unregister(struct kvm *kvm, u32 gisc);
> +
>   static inline void kvm_arch_hardware_disable(void) {}
>   static inline void kvm_arch_check_processor_compat(void *rtn) {}
>   static inline void kvm_arch_sync_events(struct kvm *kvm) {}
> diff --git a/arch/s390/kvm/interrupt.c b/arch/s390/kvm/interrupt.c
> index 5a54360cecfe..1cc3ad2e6c7e 100644
> --- a/arch/s390/kvm/interrupt.c
> +++ b/arch/s390/kvm/interrupt.c
> @@ -229,6 +229,25 @@ static inline u8 int_word_to_isc(u32 int_word)
>    */
>   #define IPM_BIT_OFFSET (offsetof(struct kvm_s390_gisa, ipm) * BITS_PER_BYTE)
>   
> +static inline int set_iam(struct kvm_s390_gisa *gisa, u8 iam)
> +{
> +	u64 word0, _word0;
> +
> +	do {
> +		word0 = READ_ONCE(gisa->u64.word[0]);
> +		/* If the GISA is in the alert list, do nothing. */
> +		if ((u64)gisa != word0 >> 32)
> +			return -EBUSY;

In kvm_s390_gisa_clear() you do:

+        nullify_gisa(kvm->arch.gisa);
+        set_iam(kvm->arch.gisa, kvm->arch.iam);

IIUC this call to set_iam() will always fail.


> +		/*
> +		 * Try to set the IAM or loop, if the IPM has changed
> +		 * or the GISA has been inserted into the alert list.
> +		 */
> +		_word0 = (word0 & ~0xffUL) | iam;
> +	} while (cmpxchg(&gisa->u64.word[0], word0, _word0) != _word0); > +
> +	return 0;
> +}
> +
>   static inline void set_ipm_gisc(struct kvm_s390_gisa *gisa, u32 gisc)
>   {
>   	set_bit_inv(IPM_BIT_OFFSET + gisc, (unsigned long *) gisa);
> @@ -2917,6 +2936,8 @@ void kvm_s390_gisa_init(struct kvm *kvm)
>   	if (!css_general_characteristics.aiv)
>   		return;
>   	kvm->arch.gisa = &kvm->arch.sie_page2->gisa;
> +	kvm->arch.iam = 0;
> +	spin_lock_init(&kvm->arch.iam_ref_lock);
>   	nullify_gisa(kvm->arch.gisa);
>   	kvm->arch.gib_in_use = !!gib;
>   	VM_EVENT(kvm, 3, "gisa 0x%pK initialized", kvm->arch.gisa);
> @@ -2929,6 +2950,51 @@ void kvm_s390_gisa_destroy(struct kvm *kvm)
>   	kvm->arch.gisa = NULL;
>   }
>   
> +int kvm_s390_gisc_register(struct kvm *kvm, u32 gisc)
> +{
> +	if (!kvm->arch.gib_in_use)
> +		return -ENODEV;
> +	if (gisc > MAX_ISC)
> +		return -ERANGE;
> +
> +	spin_lock(&kvm->arch.iam_ref_lock);
> +	if (kvm->arch.iam_ref_count[gisc] == 0)
> +		kvm->arch.iam |= 0x80 >> gisc;
> +	kvm->arch.iam_ref_count[gisc]++;
> +	if (kvm->arch.iam_ref_count[gisc] == 1)
> +		set_iam(kvm->arch.gisa, kvm->arch.iam);

testing the set_iam return value?
Even it should be fine if the caller works correctly, this is done 
before GISA is ever used.

> +	spin_unlock(&kvm->arch.iam_ref_lock);
> +
> +	return gib->nisc;
> +}
> +EXPORT_SYMBOL_GPL(kvm_s390_gisc_register);
> +
> +int kvm_s390_gisc_unregister(struct kvm *kvm, u32 gisc)
> +{
> +	int rc = 0;
> +
> +	if (!kvm->arch.gib_in_use)
> +		return -ENODEV;
> +	if (gisc > MAX_ISC)
> +		return -ERANGE;
> +
> +	spin_lock(&kvm->arch.iam_ref_lock);
> +	if (kvm->arch.iam_ref_count[gisc] == 0) {
> +		rc = -EINVAL;
> +		goto out;
> +	}
> +	kvm->arch.iam_ref_count[gisc]--;
> +	if (kvm->arch.iam_ref_count[gisc] == 0) {
> +		kvm->arch.iam &= ~(0x80 >> gisc);
> +		set_iam(kvm->arch.gisa, kvm->arch.iam);
> +	}
> +out:
> +	spin_unlock(&kvm->arch.iam_ref_lock);
> +
> +	return rc;
> +}
> +EXPORT_SYMBOL_GPL(kvm_s390_gisc_unregister);
> +
>   void kvm_s390_gib_destroy(void)
>   {
>   	if (!gib)
> 


-- 
Pierre Morel
Linux/KVM/QEMU in Böblingen - Germany

Re: [PATCH v5 10/15] KVM: s390: add functions to (un)register GISC with GISA

[Pierre Morel]

On 02/01/2019 18:29, Pierre Morel wrote:
> On 19/12/2018 20:17, Michael Mueller wrote:
>> Add the IAM (Interruption Alert Mask) to the architecture specific
>> kvm struct. This mask in the GISA is used to define for which ISC
>> a GIB alert can be issued.
>>
>> The functions kvm_s390_gisc_register() and kvm_s390_gisc_unregister()
>> are used to (un)register a GISC (guest ISC) with a virtual machine and
>> its GISA.
>>
>> Upon successful completion, kvm_s390_gisc_register() returns the
>> ISC to be used for GIB alert interruptions. A negative return code
>> indicates an error during registration.
>>
>> Theses functions will be used by other adapter types like AP and PCI to
>> request pass-through interruption support.
>>
>> Signed-off-by: Michael Mueller <mimu@linux.ibm.com>
>> ---
>>   arch/s390/include/asm/kvm_host.h |  9 ++++++
>>   arch/s390/kvm/interrupt.c        | 66 
>> ++++++++++++++++++++++++++++++++++++++++
>>   2 files changed, 75 insertions(+)
>>
>> diff --git a/arch/s390/include/asm/kvm_host.h 
>> b/arch/s390/include/asm/kvm_host.h
>> index 0deba3ae8bc7..2c1259da3636 100644
>> --- a/arch/s390/include/asm/kvm_host.h
>> +++ b/arch/s390/include/asm/kvm_host.h
>> @@ -782,6 +782,9 @@ struct kvm_s390_gisa {
>>               u8  reserved03[11];
>>               u32 airq_count;
>>           } g1;
>> +        struct {
>> +            u64 word[4];
>> +        } u64;
>>       };
>>   };
>> @@ -849,6 +852,9 @@ struct kvm_arch{
>>       DECLARE_BITMAP(cpu_feat, KVM_S390_VM_CPU_FEAT_NR_BITS);
>>       struct kvm_s390_gisa *gisa;
>>       int gib_in_use;
>> +    u8 iam;
>> +    u32 iam_ref_count[MAX_ISC + 1];
>> +    spinlock_t iam_ref_lock;
>>   };
>>   #define KVM_HVA_ERR_BAD        (-1UL)
>> @@ -882,6 +888,9 @@ void kvm_arch_crypto_set_masks(struct kvm *kvm, 
>> unsigned long *apm,
>>   extern int sie64a(struct kvm_s390_sie_block *, u64 *);
>>   extern char sie_exit;
>> +extern int kvm_s390_gisc_register(struct kvm *kvm, u32 gisc);
>> +extern int kvm_s390_gisc_unregister(struct kvm *kvm, u32 gisc);
>> +
>>   static inline void kvm_arch_hardware_disable(void) {}
>>   static inline void kvm_arch_check_processor_compat(void *rtn) {}
>>   static inline void kvm_arch_sync_events(struct kvm *kvm) {}
>> diff --git a/arch/s390/kvm/interrupt.c b/arch/s390/kvm/interrupt.c
>> index 5a54360cecfe..1cc3ad2e6c7e 100644
>> --- a/arch/s390/kvm/interrupt.c
>> +++ b/arch/s390/kvm/interrupt.c
>> @@ -229,6 +229,25 @@ static inline u8 int_word_to_isc(u32 int_word)
>>    */
>>   #define IPM_BIT_OFFSET (offsetof(struct kvm_s390_gisa, ipm) * 
>> BITS_PER_BYTE)
>> +static inline int set_iam(struct kvm_s390_gisa *gisa, u8 iam)
>> +{
>> +    u64 word0, _word0;
>> +
>> +    do {
>> +        word0 = READ_ONCE(gisa->u64.word[0]);
>> +        /* If the GISA is in the alert list, do nothing. */
>> +        if ((u64)gisa != word0 >> 32)
>> +            return -EBUSY;
> 
> In kvm_s390_gisa_clear() you do:
> 
> +        nullify_gisa(kvm->arch.gisa);
> +        set_iam(kvm->arch.gisa, kvm->arch.iam);
> 
> IIUC this call to set_iam() will always fail.

Hum, sorry, I oversaw a patch which corrected this problem.
Sorry for the noise.
(I should have seen it since I copied the right lines!)

Regards,
Pierre

-- 
Pierre Morel
Linux/KVM/QEMU in Böblingen - Germany

Re: [PATCH v5 10/15] KVM: s390: add functions to (un)register GISC with GISA

[Cornelia Huck]

On Wed, 2 Jan 2019 18:29:00 +0100
Pierre Morel <pmorel@linux.ibm.com> wrote:

> On 19/12/2018 20:17, Michael Mueller wrote:
> > Add the IAM (Interruption Alert Mask) to the architecture specific
> > kvm struct. This mask in the GISA is used to define for which ISC
> > a GIB alert can be issued.
> > 
> > The functions kvm_s390_gisc_register() and kvm_s390_gisc_unregister()
> > are used to (un)register a GISC (guest ISC) with a virtual machine and
> > its GISA.
> > 
> > Upon successful completion, kvm_s390_gisc_register() returns the
> > ISC to be used for GIB alert interruptions. A negative return code
> > indicates an error during registration.
> > 
> > Theses functions will be used by other adapter types like AP and PCI to
> > request pass-through interruption support.
> > 
> > Signed-off-by: Michael Mueller <mimu@linux.ibm.com>
> > ---
> >   arch/s390/include/asm/kvm_host.h |  9 ++++++
> >   arch/s390/kvm/interrupt.c        | 66 ++++++++++++++++++++++++++++++++++++++++
> >   2 files changed, 75 insertions(+)
> > 

> > +int kvm_s390_gisc_register(struct kvm *kvm, u32 gisc)
> > +{
> > +	if (!kvm->arch.gib_in_use)
> > +		return -ENODEV;
> > +	if (gisc > MAX_ISC)
> > +		return -ERANGE;
> > +
> > +	spin_lock(&kvm->arch.iam_ref_lock);
> > +	if (kvm->arch.iam_ref_count[gisc] == 0)
> > +		kvm->arch.iam |= 0x80 >> gisc;
> > +	kvm->arch.iam_ref_count[gisc]++;
> > +	if (kvm->arch.iam_ref_count[gisc] == 1)
> > +		set_iam(kvm->arch.gisa, kvm->arch.iam);  
> 
> testing the set_iam return value?
> Even it should be fine if the caller works correctly, this is done 
> before GISA is ever used.

My feeling is that checking the return code is a good idea, even if it
Should Never Fail(tm).

> 
> > +	spin_unlock(&kvm->arch.iam_ref_lock);
> > +
> > +	return gib->nisc;
> > +}
> > +EXPORT_SYMBOL_GPL(kvm_s390_gisc_register);
> > +
> > +int kvm_s390_gisc_unregister(struct kvm *kvm, u32 gisc)
> > +{
> > +	int rc = 0;
> > +
> > +	if (!kvm->arch.gib_in_use)
> > +		return -ENODEV;
> > +	if (gisc > MAX_ISC)
> > +		return -ERANGE;
> > +
> > +	spin_lock(&kvm->arch.iam_ref_lock);
> > +	if (kvm->arch.iam_ref_count[gisc] == 0) {
> > +		rc = -EINVAL;
> > +		goto out;
> > +	}
> > +	kvm->arch.iam_ref_count[gisc]--;
> > +	if (kvm->arch.iam_ref_count[gisc] == 0) {
> > +		kvm->arch.iam &= ~(0x80 >> gisc);
> > +		set_iam(kvm->arch.gisa, kvm->arch.iam);

Any chance of this function failing here? If yes, would there be any
implications?

> > +	}
> > +out:
> > +	spin_unlock(&kvm->arch.iam_ref_lock);
> > +
> > +	return rc;
> > +}
> > +EXPORT_SYMBOL_GPL(kvm_s390_gisc_unregister);
> > +
> >   void kvm_s390_gib_destroy(void)
> >   {
> >   	if (!gib)
> >   
> 
> 

Re: [PATCH v5 10/15] KVM: s390: add functions to (un)register GISC with GISA

[Michael Mueller]

On 04.01.19 14:19, Cornelia Huck wrote:
> On Wed, 2 Jan 2019 18:29:00 +0100
> Pierre Morel <pmorel@linux.ibm.com> wrote:
> 
>> On 19/12/2018 20:17, Michael Mueller wrote:
>>> Add the IAM (Interruption Alert Mask) to the architecture specific
>>> kvm struct. This mask in the GISA is used to define for which ISC
>>> a GIB alert can be issued.
>>>
>>> The functions kvm_s390_gisc_register() and kvm_s390_gisc_unregister()
>>> are used to (un)register a GISC (guest ISC) with a virtual machine and
>>> its GISA.
>>>
>>> Upon successful completion, kvm_s390_gisc_register() returns the
>>> ISC to be used for GIB alert interruptions. A negative return code
>>> indicates an error during registration.
>>>
>>> Theses functions will be used by other adapter types like AP and PCI to
>>> request pass-through interruption support.
>>>
>>> Signed-off-by: Michael Mueller <mimu@linux.ibm.com>
>>> ---
>>>    arch/s390/include/asm/kvm_host.h |  9 ++++++
>>>    arch/s390/kvm/interrupt.c        | 66 ++++++++++++++++++++++++++++++++++++++++
>>>    2 files changed, 75 insertions(+)
>>>
> 
>>> +int kvm_s390_gisc_register(struct kvm *kvm, u32 gisc)
>>> +{
>>> +	if (!kvm->arch.gib_in_use)
>>> +		return -ENODEV;
>>> +	if (gisc > MAX_ISC)
>>> +		return -ERANGE;
>>> +
>>> +	spin_lock(&kvm->arch.iam_ref_lock);
>>> +	if (kvm->arch.iam_ref_count[gisc] == 0)
>>> +		kvm->arch.iam |= 0x80 >> gisc;
>>> +	kvm->arch.iam_ref_count[gisc]++;
>>> +	if (kvm->arch.iam_ref_count[gisc] == 1)
>>> +		set_iam(kvm->arch.gisa, kvm->arch.iam);
>>
>> testing the set_iam return value?
>> Even it should be fine if the caller works correctly, this is done
>> before GISA is ever used.

There is a rc but a check here is not required.

There are three cases:

a) This is the first ISC that gets registered, then the GISA is
    not in use and IAM is set in the GISA.

b) A second ISC gets registered and the GISA is *not* in the
    alert list. Then the IAM is set here as well.

c) A second ISC gets registered and the GISA is in the
    alert list. Then the IAM is intentionally not set here
    by set_iam(). It will be restored by get_ipm() with
    the new IAM value by the gib alert processing code.


> 
> My feeling is that checking the return code is a good idea, even if it
> Should Never Fail(tm).
> 
>>
>>> +	spin_unlock(&kvm->arch.iam_ref_lock);
>>> +
>>> +	return gib->nisc;
>>> +}
>>> +EXPORT_SYMBOL_GPL(kvm_s390_gisc_register);
>>> +
>>> +int kvm_s390_gisc_unregister(struct kvm *kvm, u32 gisc)
>>> +{
>>> +	int rc = 0;
>>> +
>>> +	if (!kvm->arch.gib_in_use)
>>> +		return -ENODEV;
>>> +	if (gisc > MAX_ISC)
>>> +		return -ERANGE;
>>> +
>>> +	spin_lock(&kvm->arch.iam_ref_lock);
>>> +	if (kvm->arch.iam_ref_count[gisc] == 0) {
>>> +		rc = -EINVAL;
>>> +		goto out;
>>> +	}
>>> +	kvm->arch.iam_ref_count[gisc]--;
>>> +	if (kvm->arch.iam_ref_count[gisc] == 0) {
>>> +		kvm->arch.iam &= ~(0x80 >> gisc);
>>> +		set_iam(kvm->arch.gisa, kvm->arch.iam);
> 
> Any chance of this function failing here? If yes, would there be any
> implications?

It is the same here.

> 
>>> +	}
>>> +out:
>>> +	spin_unlock(&kvm->arch.iam_ref_lock);
>>> +
>>> +	return rc;
>>> +}
>>> +EXPORT_SYMBOL_GPL(kvm_s390_gisc_unregister);
>>> +
>>>    void kvm_s390_gib_destroy(void)
>>>    {
>>>    	if (!gib)
>>>    
>>
>>
> 

Re: [PATCH v5 10/15] KVM: s390: add functions to (un)register GISC with GISA

[Cornelia Huck]

On Mon, 7 Jan 2019 18:38:02 +0100
Michael Mueller <mimu@linux.ibm.com> wrote:

> On 04.01.19 14:19, Cornelia Huck wrote:
> > On Wed, 2 Jan 2019 18:29:00 +0100
> > Pierre Morel <pmorel@linux.ibm.com> wrote:
> >   
> >> On 19/12/2018 20:17, Michael Mueller wrote:  
> >>> Add the IAM (Interruption Alert Mask) to the architecture specific
> >>> kvm struct. This mask in the GISA is used to define for which ISC
> >>> a GIB alert can be issued.
> >>>
> >>> The functions kvm_s390_gisc_register() and kvm_s390_gisc_unregister()
> >>> are used to (un)register a GISC (guest ISC) with a virtual machine and
> >>> its GISA.
> >>>
> >>> Upon successful completion, kvm_s390_gisc_register() returns the
> >>> ISC to be used for GIB alert interruptions. A negative return code
> >>> indicates an error during registration.
> >>>
> >>> Theses functions will be used by other adapter types like AP and PCI to
> >>> request pass-through interruption support.
> >>>
> >>> Signed-off-by: Michael Mueller <mimu@linux.ibm.com>
> >>> ---
> >>>    arch/s390/include/asm/kvm_host.h |  9 ++++++
> >>>    arch/s390/kvm/interrupt.c        | 66 ++++++++++++++++++++++++++++++++++++++++
> >>>    2 files changed, 75 insertions(+)
> >>>  
> >   
> >>> +int kvm_s390_gisc_register(struct kvm *kvm, u32 gisc)
> >>> +{
> >>> +	if (!kvm->arch.gib_in_use)
> >>> +		return -ENODEV;
> >>> +	if (gisc > MAX_ISC)
> >>> +		return -ERANGE;
> >>> +
> >>> +	spin_lock(&kvm->arch.iam_ref_lock);
> >>> +	if (kvm->arch.iam_ref_count[gisc] == 0)
> >>> +		kvm->arch.iam |= 0x80 >> gisc;
> >>> +	kvm->arch.iam_ref_count[gisc]++;
> >>> +	if (kvm->arch.iam_ref_count[gisc] == 1)
> >>> +		set_iam(kvm->arch.gisa, kvm->arch.iam);  
> >>
> >> testing the set_iam return value?
> >> Even it should be fine if the caller works correctly, this is done
> >> before GISA is ever used.  
> 
> There is a rc but a check here is not required.
> 
> There are three cases:
> 
> a) This is the first ISC that gets registered, then the GISA is
>     not in use and IAM is set in the GISA.
> 
> b) A second ISC gets registered and the GISA is *not* in the
>     alert list. Then the IAM is set here as well.
> 
> c) A second ISC gets registered and the GISA is in the
>     alert list. Then the IAM is intentionally not set here
>     by set_iam(). It will be restored by get_ipm() with
>     the new IAM value by the gib alert processing code.
> 
> 
> > 
> > My feeling is that checking the return code is a good idea, even if it
> > Should Never Fail(tm).
> >   
> >>  
> >>> +	spin_unlock(&kvm->arch.iam_ref_lock);
> >>> +
> >>> +	return gib->nisc;
> >>> +}
> >>> +EXPORT_SYMBOL_GPL(kvm_s390_gisc_register);
> >>> +
> >>> +int kvm_s390_gisc_unregister(struct kvm *kvm, u32 gisc)
> >>> +{
> >>> +	int rc = 0;
> >>> +
> >>> +	if (!kvm->arch.gib_in_use)
> >>> +		return -ENODEV;
> >>> +	if (gisc > MAX_ISC)
> >>> +		return -ERANGE;
> >>> +
> >>> +	spin_lock(&kvm->arch.iam_ref_lock);
> >>> +	if (kvm->arch.iam_ref_count[gisc] == 0) {
> >>> +		rc = -EINVAL;
> >>> +		goto out;
> >>> +	}
> >>> +	kvm->arch.iam_ref_count[gisc]--;
> >>> +	if (kvm->arch.iam_ref_count[gisc] == 0) {
> >>> +		kvm->arch.iam &= ~(0x80 >> gisc);
> >>> +		set_iam(kvm->arch.gisa, kvm->arch.iam);  
> > 
> > Any chance of this function failing here? If yes, would there be any
> > implications?  
> 
> It is the same here.

I'm not sure that I follow: This is the reverse operation
(unregistering the gisc). Can we rely on get_ipm() to do any fixup
later? Is that a problem for the caller?

Apologies if I sound confused (well, that's because I probably am);
this is hard to review without access to the hardware specification.

> 
> >   
> >>> +	}
> >>> +out:
> >>> +	spin_unlock(&kvm->arch.iam_ref_lock);
> >>> +
> >>> +	return rc;
> >>> +}
> >>> +EXPORT_SYMBOL_GPL(kvm_s390_gisc_unregister);
> >>> +
> >>>    void kvm_s390_gib_destroy(void)
> >>>    {
> >>>    	if (!gib)
> >>>      
> >>
> >>  
> >   
> 

Re: [PATCH v5 10/15] KVM: s390: add functions to (un)register GISC with GISA

[Michael Mueller]

On 08.01.19 11:34, Cornelia Huck wrote:
> On Mon, 7 Jan 2019 18:38:02 +0100
> Michael Mueller <mimu@linux.ibm.com> wrote:
> 
>> On 04.01.19 14:19, Cornelia Huck wrote:
>>> On Wed, 2 Jan 2019 18:29:00 +0100
>>> Pierre Morel <pmorel@linux.ibm.com> wrote:
>>>    
>>>> On 19/12/2018 20:17, Michael Mueller wrote:
>>>>> Add the IAM (Interruption Alert Mask) to the architecture specific
>>>>> kvm struct. This mask in the GISA is used to define for which ISC
>>>>> a GIB alert can be issued.
>>>>>
>>>>> The functions kvm_s390_gisc_register() and kvm_s390_gisc_unregister()
>>>>> are used to (un)register a GISC (guest ISC) with a virtual machine and
>>>>> its GISA.
>>>>>
>>>>> Upon successful completion, kvm_s390_gisc_register() returns the
>>>>> ISC to be used for GIB alert interruptions. A negative return code
>>>>> indicates an error during registration.
>>>>>
>>>>> Theses functions will be used by other adapter types like AP and PCI to
>>>>> request pass-through interruption support.
>>>>>
>>>>> Signed-off-by: Michael Mueller <mimu@linux.ibm.com>
>>>>> ---
>>>>>     arch/s390/include/asm/kvm_host.h |  9 ++++++
>>>>>     arch/s390/kvm/interrupt.c        | 66 ++++++++++++++++++++++++++++++++++++++++
>>>>>     2 files changed, 75 insertions(+)
>>>>>   
>>>    
>>>>> +int kvm_s390_gisc_register(struct kvm *kvm, u32 gisc)
>>>>> +{
>>>>> +	if (!kvm->arch.gib_in_use)
>>>>> +		return -ENODEV;
>>>>> +	if (gisc > MAX_ISC)
>>>>> +		return -ERANGE;
>>>>> +
>>>>> +	spin_lock(&kvm->arch.iam_ref_lock);
>>>>> +	if (kvm->arch.iam_ref_count[gisc] == 0)
>>>>> +		kvm->arch.iam |= 0x80 >> gisc;
>>>>> +	kvm->arch.iam_ref_count[gisc]++;
>>>>> +	if (kvm->arch.iam_ref_count[gisc] == 1)
>>>>> +		set_iam(kvm->arch.gisa, kvm->arch.iam);
>>>>
>>>> testing the set_iam return value?
>>>> Even it should be fine if the caller works correctly, this is done
>>>> before GISA is ever used.
>>
>> There is a rc but a check here is not required.
>>
>> There are three cases:
>>
>> a) This is the first ISC that gets registered, then the GISA is
>>      not in use and IAM is set in the GISA.
>>
>> b) A second ISC gets registered and the GISA is *not* in the
>>      alert list. Then the IAM is set here as well.
>>
>> c) A second ISC gets registered and the GISA is in the
>>      alert list. Then the IAM is intentionally not set here
>>      by set_iam(). It will be restored by get_ipm() with
>>      the new IAM value by the gib alert processing code.
>>
>>
>>>
>>> My feeling is that checking the return code is a good idea, even if it
>>> Should Never Fail(tm).
>>>    
>>>>   
>>>>> +	spin_unlock(&kvm->arch.iam_ref_lock);
>>>>> +
>>>>> +	return gib->nisc;
>>>>> +}
>>>>> +EXPORT_SYMBOL_GPL(kvm_s390_gisc_register);
>>>>> +
>>>>> +int kvm_s390_gisc_unregister(struct kvm *kvm, u32 gisc)
>>>>> +{
>>>>> +	int rc = 0;
>>>>> +
>>>>> +	if (!kvm->arch.gib_in_use)
>>>>> +		return -ENODEV;
>>>>> +	if (gisc > MAX_ISC)
>>>>> +		return -ERANGE;
>>>>> +
>>>>> +	spin_lock(&kvm->arch.iam_ref_lock);
>>>>> +	if (kvm->arch.iam_ref_count[gisc] == 0) {
>>>>> +		rc = -EINVAL;
>>>>> +		goto out;
>>>>> +	}
>>>>> +	kvm->arch.iam_ref_count[gisc]--;
>>>>> +	if (kvm->arch.iam_ref_count[gisc] == 0) {
>>>>> +		kvm->arch.iam &= ~(0x80 >> gisc);
>>>>> +		set_iam(kvm->arch.gisa, kvm->arch.iam);
>>>
>>> Any chance of this function failing here? If yes, would there be any
>>> implications?
>>
>> It is the same here.
> 
> I'm not sure that I follow: This is the reverse operation
> (unregistering the gisc). Can we rely on get_ipm() to do any fixup
> later? Is that a problem for the caller?
> 
> Apologies if I sound confused (well, that's because I probably am);
> this is hard to review without access to the hardware specification.

I think nothing will happen because the AP CLR IRQ call (Pierre?)
has already taken offline the last AP device.


> 
>>
>>>    
>>>>> +	}
>>>>> +out:
>>>>> +	spin_unlock(&kvm->arch.iam_ref_lock);
>>>>> +
>>>>> +	return rc;
>>>>> +}
>>>>> +EXPORT_SYMBOL_GPL(kvm_s390_gisc_unregister);
>>>>> +
>>>>>     void kvm_s390_gib_destroy(void)
>>>>>     {
>>>>>     	if (!gib)
>>>>>       
>>>>
>>>>   
>>>    
>>
> 

Re: [PATCH v5 10/15] KVM: s390: add functions to (un)register GISC with GISA

[Cornelia Huck]

On Tue, 8 Jan 2019 14:07:06 +0100
Michael Mueller <mimu@linux.ibm.com> wrote:

> On 08.01.19 11:34, Cornelia Huck wrote:
> > On Mon, 7 Jan 2019 18:38:02 +0100
> > Michael Mueller <mimu@linux.ibm.com> wrote:
> >   
> >> On 04.01.19 14:19, Cornelia Huck wrote:  
> >>> On Wed, 2 Jan 2019 18:29:00 +0100
> >>> Pierre Morel <pmorel@linux.ibm.com> wrote:
> >>>      
> >>>> On 19/12/2018 20:17, Michael Mueller wrote:  
> >>>>> Add the IAM (Interruption Alert Mask) to the architecture specific
> >>>>> kvm struct. This mask in the GISA is used to define for which ISC
> >>>>> a GIB alert can be issued.
> >>>>>
> >>>>> The functions kvm_s390_gisc_register() and kvm_s390_gisc_unregister()
> >>>>> are used to (un)register a GISC (guest ISC) with a virtual machine and
> >>>>> its GISA.
> >>>>>
> >>>>> Upon successful completion, kvm_s390_gisc_register() returns the
> >>>>> ISC to be used for GIB alert interruptions. A negative return code
> >>>>> indicates an error during registration.
> >>>>>
> >>>>> Theses functions will be used by other adapter types like AP and PCI to
> >>>>> request pass-through interruption support.
> >>>>>
> >>>>> Signed-off-by: Michael Mueller <mimu@linux.ibm.com>
> >>>>> ---
> >>>>>     arch/s390/include/asm/kvm_host.h |  9 ++++++
> >>>>>     arch/s390/kvm/interrupt.c        | 66 ++++++++++++++++++++++++++++++++++++++++
> >>>>>     2 files changed, 75 insertions(+)
> >>>>>     
> >>>      
> >>>>> +int kvm_s390_gisc_register(struct kvm *kvm, u32 gisc)
> >>>>> +{
> >>>>> +	if (!kvm->arch.gib_in_use)
> >>>>> +		return -ENODEV;
> >>>>> +	if (gisc > MAX_ISC)
> >>>>> +		return -ERANGE;
> >>>>> +
> >>>>> +	spin_lock(&kvm->arch.iam_ref_lock);
> >>>>> +	if (kvm->arch.iam_ref_count[gisc] == 0)
> >>>>> +		kvm->arch.iam |= 0x80 >> gisc;
> >>>>> +	kvm->arch.iam_ref_count[gisc]++;
> >>>>> +	if (kvm->arch.iam_ref_count[gisc] == 1)
> >>>>> +		set_iam(kvm->arch.gisa, kvm->arch.iam);  
> >>>>
> >>>> testing the set_iam return value?
> >>>> Even it should be fine if the caller works correctly, this is done
> >>>> before GISA is ever used.  
> >>
> >> There is a rc but a check here is not required.
> >>
> >> There are three cases:
> >>
> >> a) This is the first ISC that gets registered, then the GISA is
> >>      not in use and IAM is set in the GISA.
> >>
> >> b) A second ISC gets registered and the GISA is *not* in the
> >>      alert list. Then the IAM is set here as well.
> >>
> >> c) A second ISC gets registered and the GISA is in the
> >>      alert list. Then the IAM is intentionally not set here
> >>      by set_iam(). It will be restored by get_ipm() with
> >>      the new IAM value by the gib alert processing code.
> >>
> >>  
> >>>
> >>> My feeling is that checking the return code is a good idea, even if it
> >>> Should Never Fail(tm).
> >>>      
> >>>>     
> >>>>> +	spin_unlock(&kvm->arch.iam_ref_lock);
> >>>>> +
> >>>>> +	return gib->nisc;
> >>>>> +}
> >>>>> +EXPORT_SYMBOL_GPL(kvm_s390_gisc_register);
> >>>>> +
> >>>>> +int kvm_s390_gisc_unregister(struct kvm *kvm, u32 gisc)
> >>>>> +{
> >>>>> +	int rc = 0;
> >>>>> +
> >>>>> +	if (!kvm->arch.gib_in_use)
> >>>>> +		return -ENODEV;
> >>>>> +	if (gisc > MAX_ISC)
> >>>>> +		return -ERANGE;
> >>>>> +
> >>>>> +	spin_lock(&kvm->arch.iam_ref_lock);
> >>>>> +	if (kvm->arch.iam_ref_count[gisc] == 0) {
> >>>>> +		rc = -EINVAL;
> >>>>> +		goto out;
> >>>>> +	}
> >>>>> +	kvm->arch.iam_ref_count[gisc]--;
> >>>>> +	if (kvm->arch.iam_ref_count[gisc] == 0) {
> >>>>> +		kvm->arch.iam &= ~(0x80 >> gisc);
> >>>>> +		set_iam(kvm->arch.gisa, kvm->arch.iam);  
> >>>
> >>> Any chance of this function failing here? If yes, would there be any
> >>> implications?  
> >>
> >> It is the same here.  
> > 
> > I'm not sure that I follow: This is the reverse operation
> > (unregistering the gisc). Can we rely on get_ipm() to do any fixup
> > later? Is that a problem for the caller?
> > 
> > Apologies if I sound confused (well, that's because I probably am);
> > this is hard to review without access to the hardware specification.  
> 
> I think nothing will happen because the AP CLR IRQ call (Pierre?)
> has already taken offline the last AP device.

But doesn't that rely on behaviour by the caller?

If the caller does weird and broken stuff, this function should return
an error, shouldn't it?

> 
> 
> >   
> >>  
> >>>      
> >>>>> +	}
> >>>>> +out:
> >>>>> +	spin_unlock(&kvm->arch.iam_ref_lock);
> >>>>> +
> >>>>> +	return rc;
> >>>>> +}
> >>>>> +EXPORT_SYMBOL_GPL(kvm_s390_gisc_unregister);
> >>>>> +
> >>>>>     void kvm_s390_gib_destroy(void)
> >>>>>     {
> >>>>>     	if (!gib)
> >>>>>         
> >>>>
> >>>>     
> >>>      
> >>  
> >   
> 

Re: [PATCH v5 10/15] KVM: s390: add functions to (un)register GISC with GISA

[Halil Pasic]

On Tue, 8 Jan 2019 11:34:44 +0100
Cornelia Huck <cohuck@redhat.com> wrote:

> > >>    
> > >>> +	spin_unlock(&kvm->arch.iam_ref_lock);
> > >>> +
> > >>> +	return gib->nisc;
> > >>> +}
> > >>> +EXPORT_SYMBOL_GPL(kvm_s390_gisc_register);
> > >>> +
> > >>> +int kvm_s390_gisc_unregister(struct kvm *kvm, u32 gisc)
> > >>> +{
> > >>> +	int rc = 0;
> > >>> +
> > >>> +	if (!kvm->arch.gib_in_use)
> > >>> +		return -ENODEV;
> > >>> +	if (gisc > MAX_ISC)
> > >>> +		return -ERANGE;
> > >>> +
> > >>> +	spin_lock(&kvm->arch.iam_ref_lock);
> > >>> +	if (kvm->arch.iam_ref_count[gisc] == 0) {
> > >>> +		rc = -EINVAL;
> > >>> +		goto out;
> > >>> +	}
> > >>> +	kvm->arch.iam_ref_count[gisc]--;
> > >>> +	if (kvm->arch.iam_ref_count[gisc] == 0) {
> > >>> +		kvm->arch.iam &= ~(0x80 >> gisc);
> > >>> +		set_iam(kvm->arch.gisa, kvm->arch.iam);    
> > > 
> > > Any chance of this function failing here? If yes, would there be any
> > > implications?    
> > 
> > It is the same here.  
> 
> I'm not sure that I follow: This is the reverse operation
> (unregistering the gisc). Can we rely on get_ipm() to do any fixup
> later? Is that a problem for the caller?

IMHO gib alerts are all about not loosing initiative. I.e. avoiding
substantially delayed delivery of interrupts due to vCPUs in wait
state.

Thus we must avoid letting go before setting up stuff (gisa.iam under
consideration of gisa ipm, gisa.next_alert, and gib.alert_list_origin)
so that we can react on the next interrupt (if necessary).

On the other hand, getting more gisa alerts than necessary is not
fatal -- better avoided if not too much hassle of course.

Bottom line is, while it may be critical that the IAM changes implied
by register take place immediately, unregister is a different story.

Regards,
Halil

> 
> Apologies if I sound confused (well, that's because I probably am);
> this is hard to review without access to the hardware specification.

Re: [PATCH v5 10/15] KVM: s390: add functions to (un)register GISC with GISA

[Cornelia Huck]

On Tue, 8 Jan 2019 14:36:13 +0100
Halil Pasic <pasic@linux.ibm.com> wrote:

> On Tue, 8 Jan 2019 11:34:44 +0100
> Cornelia Huck <cohuck@redhat.com> wrote:
> 
> > > >>      
> > > >>> +	spin_unlock(&kvm->arch.iam_ref_lock);
> > > >>> +
> > > >>> +	return gib->nisc;
> > > >>> +}
> > > >>> +EXPORT_SYMBOL_GPL(kvm_s390_gisc_register);
> > > >>> +
> > > >>> +int kvm_s390_gisc_unregister(struct kvm *kvm, u32 gisc)
> > > >>> +{
> > > >>> +	int rc = 0;
> > > >>> +
> > > >>> +	if (!kvm->arch.gib_in_use)
> > > >>> +		return -ENODEV;
> > > >>> +	if (gisc > MAX_ISC)
> > > >>> +		return -ERANGE;
> > > >>> +
> > > >>> +	spin_lock(&kvm->arch.iam_ref_lock);
> > > >>> +	if (kvm->arch.iam_ref_count[gisc] == 0) {
> > > >>> +		rc = -EINVAL;
> > > >>> +		goto out;
> > > >>> +	}
> > > >>> +	kvm->arch.iam_ref_count[gisc]--;
> > > >>> +	if (kvm->arch.iam_ref_count[gisc] == 0) {
> > > >>> +		kvm->arch.iam &= ~(0x80 >> gisc);
> > > >>> +		set_iam(kvm->arch.gisa, kvm->arch.iam);      
> > > > 
> > > > Any chance of this function failing here? If yes, would there be any
> > > > implications?      
> > > 
> > > It is the same here.    
> > 
> > I'm not sure that I follow: This is the reverse operation
> > (unregistering the gisc). Can we rely on get_ipm() to do any fixup
> > later? Is that a problem for the caller?  
> 
> IMHO gib alerts are all about not loosing initiative. I.e. avoiding
> substantially delayed delivery of interrupts due to vCPUs in wait
> state.
> 
> Thus we must avoid letting go before setting up stuff (gisa.iam under
> consideration of gisa ipm, gisa.next_alert, and gib.alert_list_origin)
> so that we can react on the next interrupt (if necessary).
> 
> On the other hand, getting more gisa alerts than necessary is not
> fatal -- better avoided if not too much hassle of course.

Yes, unless the caller does not expect any more alerts after
unregistering (I guess that's what you're saying?)

> 
> Bottom line is, while it may be critical that the IAM changes implied
> by register take place immediately, unregister is a different story.

It does feel a bit weird, though. But maybe I just have trouble
grasping the design :/

Re: [PATCH v5 10/15] KVM: s390: add functions to (un)register GISC with GISA

[Halil Pasic]

On Tue, 8 Jan 2019 14:41:35 +0100
Cornelia Huck <cohuck@redhat.com> wrote:

> On Tue, 8 Jan 2019 14:36:13 +0100
> Halil Pasic <pasic@linux.ibm.com> wrote:
> 
> > On Tue, 8 Jan 2019 11:34:44 +0100
> > Cornelia Huck <cohuck@redhat.com> wrote:
> > 
> > > > >>      
> > > > >>> +	spin_unlock(&kvm->arch.iam_ref_lock);
> > > > >>> +
> > > > >>> +	return gib->nisc;
> > > > >>> +}
> > > > >>> +EXPORT_SYMBOL_GPL(kvm_s390_gisc_register);
> > > > >>> +
> > > > >>> +int kvm_s390_gisc_unregister(struct kvm *kvm, u32 gisc)
> > > > >>> +{
> > > > >>> +	int rc = 0;
> > > > >>> +
> > > > >>> +	if (!kvm->arch.gib_in_use)
> > > > >>> +		return -ENODEV;
> > > > >>> +	if (gisc > MAX_ISC)
> > > > >>> +		return -ERANGE;
> > > > >>> +
> > > > >>> +	spin_lock(&kvm->arch.iam_ref_lock);
> > > > >>> +	if (kvm->arch.iam_ref_count[gisc] == 0) {
> > > > >>> +		rc = -EINVAL;
> > > > >>> +		goto out;
> > > > >>> +	}
> > > > >>> +	kvm->arch.iam_ref_count[gisc]--;
> > > > >>> +	if (kvm->arch.iam_ref_count[gisc] == 0) {
> > > > >>> +		kvm->arch.iam &= ~(0x80 >> gisc);
> > > > >>> +		set_iam(kvm->arch.gisa, kvm->arch.iam);      
> > > > > 
> > > > > Any chance of this function failing here? If yes, would there be any
> > > > > implications?      
> > > > 
> > > > It is the same here.    
> > > 
> > > I'm not sure that I follow: This is the reverse operation
> > > (unregistering the gisc). Can we rely on get_ipm() to do any fixup
> > > later? Is that a problem for the caller?  
> > 
> > IMHO gib alerts are all about not loosing initiative. I.e. avoiding
> > substantially delayed delivery of interrupts due to vCPUs in wait
> > state.
> > 
> > Thus we must avoid letting go before setting up stuff (gisa.iam under
> > consideration of gisa ipm, gisa.next_alert, and gib.alert_list_origin)
> > so that we can react on the next interrupt (if necessary).
> > 
> > On the other hand, getting more gisa alerts than necessary is not
> > fatal -- better avoided if not too much hassle of course.
> 
> Yes, unless the caller does not expect any more alerts after
> unregistering (I guess that's what you're saying?)

Just to make sure, the caller, which I guess would probably be some
sort of a driver, does not process the alerts. It is kvm that does.

But you are right, the properties of unregister need do be considered in
the cleanup code (e.g. resets). Sure, we must avoid the
process_gib_alert_list() (which is common, i.e. ain't tied to any
VM) poking dead GISAs. Frankly, I've my hands full with the normal
operation scenario, and I did not pay much attention to the cleanup yet. 

> 
> > 
> > Bottom line is, while it may be critical that the IAM changes implied
> > by register take place immediately, unregister is a different story.
> 
> It does feel a bit weird, though. But maybe I just have trouble
> grasping the design :/
> 

You are on the only one. I have extreme difficulties following some of
the discussion here -- despite of the fact that I have quite some
confidence in my understanding of the GISA/GIB architecture.

Regards,
Halil

[PATCH v5 11/15] KVM: s390: restore IAM in get_ipm() when IPM is clean

[Michael Mueller]

The patch adds the parameter irq_flags and allows to
restore the Interruption Alert Mask (IAM) in the GISA
atomically while guaranteeing the IPM is clean.

The function returns the IPM of the GISA. If the returned
value is 0x00 and the IRQ_FLAG_IAM was set, the IAM has
been restored.

New irq flag:
  IRQ_FLAG_IAM: When set, the IAM is restored if no ISC bit
  		is set in the IPM, i.e. no new airqs are
		pending. The test and restore operations
		are done atomically.

Signed-off-by: Michael Mueller <mimu@linux.ibm.com>
---
 arch/s390/kvm/interrupt.c | 34 +++++++++++++++++++++++++++++-----
 1 file changed, 29 insertions(+), 5 deletions(-)

diff --git a/arch/s390/kvm/interrupt.c b/arch/s390/kvm/interrupt.c
index 1cc3ad2e6c7e..8307717e3caf 100644
--- a/arch/s390/kvm/interrupt.c
+++ b/arch/s390/kvm/interrupt.c
@@ -37,6 +37,7 @@
 #define IRQ_FLAG_LOCAL    0x8000 /* include local interruption pending mask */
 #define IRQ_FLAG_FLOATING 0x4000 /* include float interruption pending mask */
 #define IRQ_FLAG_GISA     0x2000 /* include GISA interruption pending mask */
+#define IRQ_FLAG_IAM      0x0080 /* when set try to restore IAM */
 
 #define IRQ_MASK_ALL      (IRQ_FLAG_LOCAL | IRQ_FLAG_FLOATING | IRQ_FLAG_GISA)
 #define IRQ_MASK_NO_GISA  (IRQ_MASK_ALL & ~IRQ_FLAG_GISA)
@@ -253,9 +254,32 @@ static inline void set_ipm_gisc(struct kvm_s390_gisa *gisa, u32 gisc)
 	set_bit_inv(IPM_BIT_OFFSET + gisc, (unsigned long *) gisa);
 }
 
-static inline u8 get_ipm(struct kvm_s390_gisa *gisa)
+static inline u8 get_ipm(struct kvm_s390_gisa *gisa, u16 irq_flags)
 {
-	return READ_ONCE(gisa->ipm);
+	u64 word, _word;
+	u8 ipm;
+
+	if (!(irq_flags & IRQ_FLAG_IAM))
+		return READ_ONCE(gisa->ipm);
+
+	do {
+		word = READ_ONCE(gisa->u64.word[0]);
+		ipm = word >> 24;
+		/* If the GISA is in the alert list, return the IPM. */
+		if ((u64)gisa != word >> 32)
+			return ipm;
+		/* If the IPM is dirty, return the IPM. */
+		if (ipm)
+			return ipm;
+		/*
+		 * Try to restore the IAM or loop, if the IPM is dirty
+		 * again or the GISA has been inserted into the alert list.
+		 */
+		_word = (word & ~0xffUL) |
+			container_of(gisa, struct sie_page2, gisa)->kvm->arch.iam;
+	} while (cmpxchg(&gisa->u64.word[0], word, _word) != _word);
+
+	return 0;
 }
 
 static inline void clear_ipm_gisc(struct kvm_s390_gisa *gisa, u32 gisc)
@@ -277,7 +301,7 @@ static inline unsigned long pending_irqs(struct kvm_vcpu *vcpu, u16 irq_flags)
 	if (irq_flags & IRQ_FLAG_FLOATING)
 		pending_irqs |= vcpu->kvm->arch.float_int.pending_irqs;
 	if (irq_flags & IRQ_FLAG_GISA)
-		pending_irqs |= get_ipm(vcpu->kvm->arch.gisa) <<
+		pending_irqs |= get_ipm(vcpu->kvm->arch.gisa, irq_flags) <<
 			IRQ_PEND_IO_ISC_7;
 	return pending_irqs;
 }
@@ -1574,7 +1598,7 @@ static int get_top_gisa_isc(struct kvm *kvm, u64 isc_mask, u32 schid)
 	if (!kvm->arch.gisa)
 		goto out;
 
-	active_mask = (isc_mask & get_ipm(kvm->arch.gisa) << 24) << 32;
+	active_mask = (isc_mask & get_ipm(kvm->arch.gisa, IRQ_FLAG_IAM) << 24) << 32;
 	while (active_mask) {
 		isc = __fls(active_mask) ^ (BITS_PER_LONG - 1);
 		if (tac_ipm_gisc(kvm->arch.gisa, isc))
@@ -2060,7 +2084,7 @@ static int get_all_floating_irqs(struct kvm *kvm, u8 __user *usrbuf, u64 len)
 	max_irqs = len / sizeof(struct kvm_s390_irq);
 
 	if (kvm->arch.gisa &&
-	    get_ipm(kvm->arch.gisa)) {
+	    get_ipm(kvm->arch.gisa, IRQ_FLAG_IAM)) {
 		for (i = 0; i <= MAX_ISC; i++) {
 			if (n == max_irqs) {
 				/* signal userspace to try again */
-- 
2.13.4

Re: [PATCH v5 11/15] KVM: s390: restore IAM in get_ipm() when IPM is clean

[Pierre Morel]

On 19/12/2018 20:17, Michael Mueller wrote:
> The patch adds the parameter irq_flags and allows to
> restore the Interruption Alert Mask (IAM) in the GISA
> atomically while guaranteeing the IPM is clean.
> 
> The function returns the IPM of the GISA. If the returned
> value is 0x00 and the IRQ_FLAG_IAM was set, the IAM has
> been restored.
> 
> New irq flag:
>    IRQ_FLAG_IAM: When set, the IAM is restored if no ISC bit
>    		is set in the IPM, i.e. no new airqs are
> 		pending. The test and restore operations
> 		are done atomically.
> 
> Signed-off-by: Michael Mueller <mimu@linux.ibm.com>
> ---
>   arch/s390/kvm/interrupt.c | 34 +++++++++++++++++++++++++++++-----
>   1 file changed, 29 insertions(+), 5 deletions(-)
> 
> diff --git a/arch/s390/kvm/interrupt.c b/arch/s390/kvm/interrupt.c
> index 1cc3ad2e6c7e..8307717e3caf 100644
> --- a/arch/s390/kvm/interrupt.c
> +++ b/arch/s390/kvm/interrupt.c
> @@ -37,6 +37,7 @@
>   #define IRQ_FLAG_LOCAL    0x8000 /* include local interruption pending mask */
>   #define IRQ_FLAG_FLOATING 0x4000 /* include float interruption pending mask */
>   #define IRQ_FLAG_GISA     0x2000 /* include GISA interruption pending mask */
> +#define IRQ_FLAG_IAM      0x0080 /* when set try to restore IAM */
>   
>   #define IRQ_MASK_ALL      (IRQ_FLAG_LOCAL | IRQ_FLAG_FLOATING | IRQ_FLAG_GISA)
>   #define IRQ_MASK_NO_GISA  (IRQ_MASK_ALL & ~IRQ_FLAG_GISA)
> @@ -253,9 +254,32 @@ static inline void set_ipm_gisc(struct kvm_s390_gisa *gisa, u32 gisc)
>   	set_bit_inv(IPM_BIT_OFFSET + gisc, (unsigned long *) gisa);
>   }
>   
> -static inline u8 get_ipm(struct kvm_s390_gisa *gisa)
> +static inline u8 get_ipm(struct kvm_s390_gisa *gisa, u16 irq_flags)
>   {
> -	return READ_ONCE(gisa->ipm);
> +	u64 word, _word;
> +	u8 ipm;
> +
> +	if (!(irq_flags & IRQ_FLAG_IAM))
> +		return READ_ONCE(gisa->ipm);
> +
> +	do {
> +		word = READ_ONCE(gisa->u64.word[0]);
> +		ipm = word >> 24;
> +		/* If the GISA is in the alert list, return the IPM. */
> +		if ((u64)gisa != word >> 32)
> +			return ipm;
> +		/* If the IPM is dirty, return the IPM. */
> +		if (ipm)
> +			return ipm;
> +		/*
> +		 * Try to restore the IAM or loop, if the IPM is dirty
> +		 * again or the GISA has been inserted into the alert list.
> +		 */
> +		_word = (word & ~0xffUL) |
> +			container_of(gisa, struct sie_page2, gisa)->kvm->arch.iam;
> +	} while (cmpxchg(&gisa->u64.word[0], word, _word) != _word);
> +
> +	return 0;
>   }

Personal opinion, but this function do more than just getting the IPM, 
shouldn't it be reflected in the function name?

>   
>   static inline void clear_ipm_gisc(struct kvm_s390_gisa *gisa, u32 gisc)
> @@ -277,7 +301,7 @@ static inline unsigned long pending_irqs(struct kvm_vcpu *vcpu, u16 irq_flags)
>   	if (irq_flags & IRQ_FLAG_FLOATING)
>   		pending_irqs |= vcpu->kvm->arch.float_int.pending_irqs;
>   	if (irq_flags & IRQ_FLAG_GISA)
> -		pending_irqs |= get_ipm(vcpu->kvm->arch.gisa) <<
> +		pending_irqs |= get_ipm(vcpu->kvm->arch.gisa, irq_flags) <<
>   			IRQ_PEND_IO_ISC_7;
>   	return pending_irqs;
>   }
> @@ -1574,7 +1598,7 @@ static int get_top_gisa_isc(struct kvm *kvm, u64 isc_mask, u32 schid)
>   	if (!kvm->arch.gisa)
>   		goto out;
>   
> -	active_mask = (isc_mask & get_ipm(kvm->arch.gisa) << 24) << 32;
> +	active_mask = (isc_mask & get_ipm(kvm->arch.gisa, IRQ_FLAG_IAM) << 24) << 32;

why do we need to set the IAM here?

>   	while (active_mask) {
>   		isc = __fls(active_mask) ^ (BITS_PER_LONG - 1);
>   		if (tac_ipm_gisc(kvm->arch.gisa, isc))
> @@ -2060,7 +2084,7 @@ static int get_all_floating_irqs(struct kvm *kvm, u8 __user *usrbuf, u64 len)
>   	max_irqs = len / sizeof(struct kvm_s390_irq);
>   
>   	if (kvm->arch.gisa &&
> -	    get_ipm(kvm->arch.gisa)) {
> +	    get_ipm(kvm->arch.gisa, IRQ_FLAG_IAM)) {

and here?

>   		for (i = 0; i <= MAX_ISC; i++) {
>   			if (n == max_irqs) {
>   				/* signal userspace to try again */
> 


-- 
Pierre Morel
Linux/KVM/QEMU in Böblingen - Germany

Re: [PATCH v5 11/15] KVM: s390: restore IAM in get_ipm() when IPM is clean

[Michael Mueller]

On 03.01.19 16:06, Pierre Morel wrote:
> On 19/12/2018 20:17, Michael Mueller wrote:
>> The patch adds the parameter irq_flags and allows to
>> restore the Interruption Alert Mask (IAM) in the GISA
>> atomically while guaranteeing the IPM is clean.
>>
>> The function returns the IPM of the GISA. If the returned
>> value is 0x00 and the IRQ_FLAG_IAM was set, the IAM has
>> been restored.
>>
>> New irq flag:
>>    IRQ_FLAG_IAM: When set, the IAM is restored if no ISC bit
>>            is set in the IPM, i.e. no new airqs are
>>         pending. The test and restore operations
>>         are done atomically.
>>
>> Signed-off-by: Michael Mueller <mimu@linux.ibm.com>
>> ---
>>   arch/s390/kvm/interrupt.c | 34 +++++++++++++++++++++++++++++-----
>>   1 file changed, 29 insertions(+), 5 deletions(-)
>>
>> diff --git a/arch/s390/kvm/interrupt.c b/arch/s390/kvm/interrupt.c
>> index 1cc3ad2e6c7e..8307717e3caf 100644
>> --- a/arch/s390/kvm/interrupt.c
>> +++ b/arch/s390/kvm/interrupt.c
>> @@ -37,6 +37,7 @@
>>   #define IRQ_FLAG_LOCAL    0x8000 /* include local interruption 
>> pending mask */
>>   #define IRQ_FLAG_FLOATING 0x4000 /* include float interruption 
>> pending mask */
>>   #define IRQ_FLAG_GISA     0x2000 /* include GISA interruption 
>> pending mask */
>> +#define IRQ_FLAG_IAM      0x0080 /* when set try to restore IAM */
>>   #define IRQ_MASK_ALL      (IRQ_FLAG_LOCAL | IRQ_FLAG_FLOATING | 
>> IRQ_FLAG_GISA)
>>   #define IRQ_MASK_NO_GISA  (IRQ_MASK_ALL & ~IRQ_FLAG_GISA)
>> @@ -253,9 +254,32 @@ static inline void set_ipm_gisc(struct 
>> kvm_s390_gisa *gisa, u32 gisc)
>>       set_bit_inv(IPM_BIT_OFFSET + gisc, (unsigned long *) gisa);
>>   }
>> -static inline u8 get_ipm(struct kvm_s390_gisa *gisa)
>> +static inline u8 get_ipm(struct kvm_s390_gisa *gisa, u16 irq_flags)
>>   {
>> -    return READ_ONCE(gisa->ipm);
>> +    u64 word, _word;
>> +    u8 ipm;
>> +
>> +    if (!(irq_flags & IRQ_FLAG_IAM))
>> +        return READ_ONCE(gisa->ipm);
>> +
>> +    do {
>> +        word = READ_ONCE(gisa->u64.word[0]);
>> +        ipm = word >> 24;
>> +        /* If the GISA is in the alert list, return the IPM. */
>> +        if ((u64)gisa != word >> 32)
>> +            return ipm;
>> +        /* If the IPM is dirty, return the IPM. */
>> +        if (ipm)
>> +            return ipm;
>> +        /*
>> +         * Try to restore the IAM or loop, if the IPM is dirty
>> +         * again or the GISA has been inserted into the alert list.
>> +         */
>> +        _word = (word & ~0xffUL) |
>> +            container_of(gisa, struct sie_page2, gisa)->kvm->arch.iam;
>> +    } while (cmpxchg(&gisa->u64.word[0], word, _word) != _word);
>> +
>> +    return 0;
>>   }
> 
> Personal opinion, but this function do more than just getting the IPM, 
> shouldn't it be reflected in the function name?

The main task of this function is to fetch the IPM. The additional
functionality is expressed in the flags.

A call like get_ipm_try_iam_restore(kvm, 0) would be more confusing.

> 
>>   static inline void clear_ipm_gisc(struct kvm_s390_gisa *gisa, u32 gisc)
>> @@ -277,7 +301,7 @@ static inline unsigned long pending_irqs(struct 
>> kvm_vcpu *vcpu, u16 irq_flags)
>>       if (irq_flags & IRQ_FLAG_FLOATING)
>>           pending_irqs |= vcpu->kvm->arch.float_int.pending_irqs;
>>       if (irq_flags & IRQ_FLAG_GISA)
>> -        pending_irqs |= get_ipm(vcpu->kvm->arch.gisa) <<
>> +        pending_irqs |= get_ipm(vcpu->kvm->arch.gisa, irq_flags) <<
>>               IRQ_PEND_IO_ISC_7;
>>       return pending_irqs;
>>   }
>> @@ -1574,7 +1598,7 @@ static int get_top_gisa_isc(struct kvm *kvm, u64 
>> isc_mask, u32 schid)
>>       if (!kvm->arch.gisa)
>>           goto out;
>> -    active_mask = (isc_mask & get_ipm(kvm->arch.gisa) << 24) << 32;
>> +    active_mask = (isc_mask & get_ipm(kvm->arch.gisa, IRQ_FLAG_IAM) 
>> << 24) << 32;
> 
> why do we need to set the IAM here?

Yes, it should not be required, only when a vcpu goes into wait state.

> 
>>       while (active_mask) {
>>           isc = __fls(active_mask) ^ (BITS_PER_LONG - 1);
>>           if (tac_ipm_gisc(kvm->arch.gisa, isc))
>> @@ -2060,7 +2084,7 @@ static int get_all_floating_irqs(struct kvm 
>> *kvm, u8 __user *usrbuf, u64 len)
>>       max_irqs = len / sizeof(struct kvm_s390_irq);
>>       if (kvm->arch.gisa &&
>> -        get_ipm(kvm->arch.gisa)) {
>> +        get_ipm(kvm->arch.gisa, IRQ_FLAG_IAM)) {
> 
> and here?

Yes, it should not be required, only when a vcpu goes into wait state.

> 
>>           for (i = 0; i <= MAX_ISC; i++) {
>>               if (n == max_irqs) {
>>                   /* signal userspace to try again */
>>
> 
> 

Re: [PATCH v5 11/15] KVM: s390: restore IAM in get_ipm() when IPM is clean

[Halil Pasic]

On Wed, 19 Dec 2018 20:17:52 +0100
Michael Mueller <mimu@linux.ibm.com> wrote:

> The patch adds the parameter irq_flags and allows to
> restore the Interruption Alert Mask (IAM) in the GISA
> atomically while guaranteeing the IPM is clean.
> 
> The function returns the IPM of the GISA. If the returned
> value is 0x00 and the IRQ_FLAG_IAM was set, the IAM has
> been restored.
> 
> New irq flag:
>   IRQ_FLAG_IAM: When set, the IAM is restored if no ISC bit
>   		is set in the IPM, i.e. no new airqs are
> 		pending. The test and restore operations
> 		are done atomically.
> 
> Signed-off-by: Michael Mueller <mimu@linux.ibm.com>
> ---
>  arch/s390/kvm/interrupt.c | 34 +++++++++++++++++++++++++++++-----
>  1 file changed, 29 insertions(+), 5 deletions(-)
> 
> diff --git a/arch/s390/kvm/interrupt.c b/arch/s390/kvm/interrupt.c
> index 1cc3ad2e6c7e..8307717e3caf 100644
> --- a/arch/s390/kvm/interrupt.c
> +++ b/arch/s390/kvm/interrupt.c
> @@ -37,6 +37,7 @@
>  #define IRQ_FLAG_LOCAL    0x8000 /* include local interruption pending mask */
>  #define IRQ_FLAG_FLOATING 0x4000 /* include float interruption pending mask */
>  #define IRQ_FLAG_GISA     0x2000 /* include GISA interruption pending mask */
> +#define IRQ_FLAG_IAM      0x0080 /* when set try to restore IAM */
>  
>  #define IRQ_MASK_ALL      (IRQ_FLAG_LOCAL | IRQ_FLAG_FLOATING | IRQ_FLAG_GISA)
>  #define IRQ_MASK_NO_GISA  (IRQ_MASK_ALL & ~IRQ_FLAG_GISA)
> @@ -253,9 +254,32 @@ static inline void set_ipm_gisc(struct kvm_s390_gisa *gisa, u32 gisc)
>  	set_bit_inv(IPM_BIT_OFFSET + gisc, (unsigned long *) gisa);
>  }
>  
> -static inline u8 get_ipm(struct kvm_s390_gisa *gisa)
> +static inline u8 get_ipm(struct kvm_s390_gisa *gisa, u16 irq_flags)
>  {
> -	return READ_ONCE(gisa->ipm);
> +	u64 word, _word;
> +	u8 ipm;
> +
> +	if (!(irq_flags & IRQ_FLAG_IAM))
> +		return READ_ONCE(gisa->ipm);
> +
> +	do {
> +		word = READ_ONCE(gisa->u64.word[0]);
> +		ipm = word >> 24;
> +		/* If the GISA is in the alert list, return the IPM. */
> +		if ((u64)gisa != word >> 32)
> +			return ipm;
> +		/* If the IPM is dirty, return the IPM. */
> +		if (ipm)
> +			return ipm;
> +		/*
> +		 * Try to restore the IAM or loop, if the IPM is dirty
> +		 * again or the GISA has been inserted into the alert list.
> +		 */
> +		_word = (word & ~0xffUL) |
> +			container_of(gisa, struct sie_page2, gisa)->kvm->arch.iam;
> +	} while (cmpxchg(&gisa->u64.word[0], word, _word) != _word);
                                                             ^^

Shouldn't this rather be:
	} while (cmpxchg(&gisa->u64.word[0], word, _word) != word);

Regards,
Halil
> +
> +	return 0;
>  }

Re: [PATCH v5 11/15] KVM: s390: restore IAM in get_ipm() when IPM is clean

[Michael Mueller]

On 07.01.19 00:32, Halil Pasic wrote:
> On Wed, 19 Dec 2018 20:17:52 +0100
> Michael Mueller <mimu@linux.ibm.com> wrote:
> 
>> The patch adds the parameter irq_flags and allows to
>> restore the Interruption Alert Mask (IAM) in the GISA
>> atomically while guaranteeing the IPM is clean.
>>
>> The function returns the IPM of the GISA. If the returned
>> value is 0x00 and the IRQ_FLAG_IAM was set, the IAM has
>> been restored.
>>
>> New irq flag:
>>    IRQ_FLAG_IAM: When set, the IAM is restored if no ISC bit
>>    		is set in the IPM, i.e. no new airqs are
>> 		pending. The test and restore operations
>> 		are done atomically.
>>
>> Signed-off-by: Michael Mueller <mimu@linux.ibm.com>
>> ---
>>   arch/s390/kvm/interrupt.c | 34 +++++++++++++++++++++++++++++-----
>>   1 file changed, 29 insertions(+), 5 deletions(-)
>>
>> diff --git a/arch/s390/kvm/interrupt.c b/arch/s390/kvm/interrupt.c
>> index 1cc3ad2e6c7e..8307717e3caf 100644
>> --- a/arch/s390/kvm/interrupt.c
>> +++ b/arch/s390/kvm/interrupt.c
>> @@ -37,6 +37,7 @@
>>   #define IRQ_FLAG_LOCAL    0x8000 /* include local interruption pending mask */
>>   #define IRQ_FLAG_FLOATING 0x4000 /* include float interruption pending mask */
>>   #define IRQ_FLAG_GISA     0x2000 /* include GISA interruption pending mask */
>> +#define IRQ_FLAG_IAM      0x0080 /* when set try to restore IAM */
>>   
>>   #define IRQ_MASK_ALL      (IRQ_FLAG_LOCAL | IRQ_FLAG_FLOATING | IRQ_FLAG_GISA)
>>   #define IRQ_MASK_NO_GISA  (IRQ_MASK_ALL & ~IRQ_FLAG_GISA)
>> @@ -253,9 +254,32 @@ static inline void set_ipm_gisc(struct kvm_s390_gisa *gisa, u32 gisc)
>>   	set_bit_inv(IPM_BIT_OFFSET + gisc, (unsigned long *) gisa);
>>   }
>>   
>> -static inline u8 get_ipm(struct kvm_s390_gisa *gisa)
>> +static inline u8 get_ipm(struct kvm_s390_gisa *gisa, u16 irq_flags)
>>   {
>> -	return READ_ONCE(gisa->ipm);
>> +	u64 word, _word;
>> +	u8 ipm;
>> +
>> +	if (!(irq_flags & IRQ_FLAG_IAM))
>> +		return READ_ONCE(gisa->ipm);
>> +
>> +	do {
>> +		word = READ_ONCE(gisa->u64.word[0]);
>> +		ipm = word >> 24;
>> +		/* If the GISA is in the alert list, return the IPM. */
>> +		if ((u64)gisa != word >> 32)
>> +			return ipm;
>> +		/* If the IPM is dirty, return the IPM. */
>> +		if (ipm)
>> +			return ipm;
>> +		/*
>> +		 * Try to restore the IAM or loop, if the IPM is dirty
>> +		 * again or the GISA has been inserted into the alert list.
>> +		 */
>> +		_word = (word & ~0xffUL) |
>> +			container_of(gisa, struct sie_page2, gisa)->kvm->arch.iam;
>> +	} while (cmpxchg(&gisa->u64.word[0], word, _word) != _word);
>                                                               ^^
> 
> Shouldn't this rather be:
> 	} while (cmpxchg(&gisa->u64.word[0], word, _word) != word);

Yes, the comparison needs to be done against the old value of
the memory region.

> 
> Regards,
> Halil
>> +
>> +	return 0;
>>   }

[PATCH v5 12/15] KVM: s390: do not restore IAM immediately before SIE entry

[Michael Mueller]

The IAM shall no be restored when deliverable interruptions are
delivered to vcpus by means of the PSW swap mechanism. That would
trigger the GIB alert millicode although we know that SIE will be
able to handle the pending interruption on entry.

Signed-off-by: Michael Mueller <mimu@linux.ibm.com>
---
 arch/s390/kvm/interrupt.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/arch/s390/kvm/interrupt.c b/arch/s390/kvm/interrupt.c
index 8307717e3caf..48a93f5e5333 100644
--- a/arch/s390/kvm/interrupt.c
+++ b/arch/s390/kvm/interrupt.c
@@ -328,11 +328,11 @@ static unsigned long disable_iscs(struct kvm_vcpu *vcpu,
 	return active_mask;
 }
 
-static unsigned long deliverable_irqs(struct kvm_vcpu *vcpu)
+static unsigned long deliverable_irqs(struct kvm_vcpu *vcpu, u16 irq_flags)
 {
 	unsigned long active_mask;
 
-	active_mask = pending_irqs(vcpu, IRQ_MASK_ALL);
+	active_mask = pending_irqs(vcpu, irq_flags);
 	if (!active_mask)
 		return 0;
 
@@ -1090,7 +1090,7 @@ int kvm_s390_ext_call_pending(struct kvm_vcpu *vcpu)
 
 int kvm_s390_vcpu_has_irq(struct kvm_vcpu *vcpu, int exclude_stop)
 {
-	if (deliverable_irqs(vcpu))
+	if (deliverable_irqs(vcpu, IRQ_MASK_ALL | IRQ_FLAG_IAM))
 		return 1;
 
 	if (kvm_cpu_has_pending_timer(vcpu))
@@ -1262,7 +1262,7 @@ int __must_check kvm_s390_deliver_pending_interrupts(struct kvm_vcpu *vcpu)
 	if (cpu_timer_irq_pending(vcpu))
 		set_bit(IRQ_PEND_EXT_CPU_TIMER, &li->pending_irqs);
 
-	while ((irqs = deliverable_irqs(vcpu)) && !rc) {
+	while ((irqs = deliverable_irqs(vcpu, IRQ_MASK_ALL)) && !rc) {
 		/* bits are in the reverse order of interrupt priority */
 		irq_type = find_last_bit(&irqs, IRQ_PEND_COUNT);
 		switch (irq_type) {
-- 
2.13.4

Re: [PATCH v5 12/15] KVM: s390: do not restore IAM immediately before SIE entry

[Pierre Morel]

On 19/12/2018 20:17, Michael Mueller wrote:
> The IAM shall no be restored when deliverable interruptions are
> delivered to vcpus by means of the PSW swap mechanism. That would
> trigger the GIB alert millicode although we know that SIE will be
> able to handle the pending interruption on entry.
> 
> Signed-off-by: Michael Mueller <mimu@linux.ibm.com>
> ---
>   arch/s390/kvm/interrupt.c | 8 ++++----
>   1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/arch/s390/kvm/interrupt.c b/arch/s390/kvm/interrupt.c
> index 8307717e3caf..48a93f5e5333 100644
> --- a/arch/s390/kvm/interrupt.c
> +++ b/arch/s390/kvm/interrupt.c
> @@ -328,11 +328,11 @@ static unsigned long disable_iscs(struct kvm_vcpu *vcpu,
>   	return active_mask;
>   }
>   
> -static unsigned long deliverable_irqs(struct kvm_vcpu *vcpu)
> +static unsigned long deliverable_irqs(struct kvm_vcpu *vcpu, u16 irq_flags)
>   {
>   	unsigned long active_mask;
>   
> -	active_mask = pending_irqs(vcpu, IRQ_MASK_ALL);
> +	active_mask = pending_irqs(vcpu, irq_flags);
>   	if (!active_mask)
>   		return 0;
>   
> @@ -1090,7 +1090,7 @@ int kvm_s390_ext_call_pending(struct kvm_vcpu *vcpu)
>   
>   int kvm_s390_vcpu_has_irq(struct kvm_vcpu *vcpu, int exclude_stop)
>   {
> -	if (deliverable_irqs(vcpu))
> +	if (deliverable_irqs(vcpu, IRQ_MASK_ALL | IRQ_FLAG_IAM))

Why do we need to restore IAM here?



-- 
Pierre Morel
Linux/KVM/QEMU in Böblingen - Germany

Re: [PATCH v5 12/15] KVM: s390: do not restore IAM immediately before SIE entry

[Michael Mueller]

On 03.01.19 16:00, Pierre Morel wrote:
> On 19/12/2018 20:17, Michael Mueller wrote:
>> The IAM shall no be restored when deliverable interruptions are
>> delivered to vcpus by means of the PSW swap mechanism. That would
>> trigger the GIB alert millicode although we know that SIE will be
>> able to handle the pending interruption on entry.
>>
>> Signed-off-by: Michael Mueller <mimu@linux.ibm.com>
>> ---
>>   arch/s390/kvm/interrupt.c | 8 ++++----
>>   1 file changed, 4 insertions(+), 4 deletions(-)
>>
>> diff --git a/arch/s390/kvm/interrupt.c b/arch/s390/kvm/interrupt.c
>> index 8307717e3caf..48a93f5e5333 100644
>> --- a/arch/s390/kvm/interrupt.c
>> +++ b/arch/s390/kvm/interrupt.c
>> @@ -328,11 +328,11 @@ static unsigned long disable_iscs(struct 
>> kvm_vcpu *vcpu,
>>       return active_mask;
>>   }
>> -static unsigned long deliverable_irqs(struct kvm_vcpu *vcpu)
>> +static unsigned long deliverable_irqs(struct kvm_vcpu *vcpu, u16 
>> irq_flags)
>>   {
>>       unsigned long active_mask;
>> -    active_mask = pending_irqs(vcpu, IRQ_MASK_ALL);
>> +    active_mask = pending_irqs(vcpu, irq_flags);
>>       if (!active_mask)
>>           return 0;
>> @@ -1090,7 +1090,7 @@ int kvm_s390_ext_call_pending(struct kvm_vcpu 
>> *vcpu)
>>   int kvm_s390_vcpu_has_irq(struct kvm_vcpu *vcpu, int exclude_stop)
>>   {
>> -    if (deliverable_irqs(vcpu))
>> +    if (deliverable_irqs(vcpu, IRQ_MASK_ALL | IRQ_FLAG_IAM))
> 
> Why do we need to restore IAM here?


please see kvm_s390_handle_wait()

It calls kvm_arch_vcpu_runnable() / kvm_s390_vcpu_has_irq()

That's the place where we want the IAM to be restored when
no ISC is pending in the IPM anymore.

> 
> 
> 

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

[Michael Mueller]

This function processes the Gib Alert List (GAL). It is required
to run when either a gib alert interruption has been received or
a gisa that is in the alert list is cleared or dropped.

The GAL is build up by millicode, when the respective ISC bit is
set in the Interruption Alert Mask (IAM) and an interruption of
that class is observed.

Signed-off-by: Michael Mueller <mimu@linux.ibm.com>
---
 arch/s390/kvm/interrupt.c | 140 ++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 140 insertions(+)

diff --git a/arch/s390/kvm/interrupt.c b/arch/s390/kvm/interrupt.c
index 48a93f5e5333..03e7ba4f215a 100644
--- a/arch/s390/kvm/interrupt.c
+++ b/arch/s390/kvm/interrupt.c
@@ -2941,6 +2941,146 @@ int kvm_s390_get_irq_state(struct kvm_vcpu *vcpu, __u8 __user *buf, int len)
 	return n;
 }
 
+static int __try_airqs_kick(struct kvm *kvm, u8 ipm)
+{
+	struct kvm_s390_float_interrupt *fi = &kvm->arch.float_int;
+	struct kvm_vcpu *vcpu = NULL, *kick_vcpu[MAX_ISC + 1];
+	int online_vcpus = atomic_read(&kvm->online_vcpus);
+	u8 ioint_mask, isc_mask, kick_mask = 0x00;
+	int vcpu_id, kicked = 0;
+
+	/* Loop over vcpus in WAIT state. */
+	for (vcpu_id = find_first_bit(fi->idle_mask, online_vcpus);
+	     /* Until all pending ISCs have a vcpu open for airqs. */
+	     (~kick_mask & ipm) && vcpu_id < online_vcpus;
+	     vcpu_id = find_next_bit(fi->idle_mask, online_vcpus, vcpu_id)) {
+		vcpu = kvm_get_vcpu(kvm, vcpu_id);
+		if (psw_ioint_disabled(vcpu))
+			continue;
+		ioint_mask = (u8)(vcpu->arch.sie_block->gcr[6] >> 24);
+		for (isc_mask = 0x80; isc_mask; isc_mask >>= 1) {
+			/* ISC pending in IPM ? */
+			if (!(ipm & isc_mask))
+				continue;
+			/* vcpu for this ISC already found ? */
+			if (kick_mask & isc_mask)
+				continue;
+			/* vcpu open for airq of this ISC ? */
+			if (!(ioint_mask & isc_mask))
+				continue;
+			/* use this vcpu (for all ISCs in ioint_mask) */
+			kick_mask |= ioint_mask;
+			kick_vcpu[kicked++] = vcpu;
+		}
+	}
+
+	if (vcpu && ~kick_mask & ipm)
+		VM_EVENT(kvm, 4, "gib alert undeliverable isc mask 0x%02x",
+			 ~kick_mask & ipm);
+
+	for (vcpu_id = 0; vcpu_id < kicked; vcpu_id++)
+		kvm_s390_vcpu_wakeup(kick_vcpu[vcpu_id]);
+
+	return (online_vcpus != 0) ? kicked : -ENODEV;
+}
+
+static void __floating_airqs_kick(struct kvm *kvm)
+{
+	struct kvm_s390_float_interrupt *fi = &kvm->arch.float_int;
+	int online_vcpus, kicked;
+	u8 ipm_t0, ipm;
+
+	/* Get IPM and return if clean, IAM has been restored. */
+	ipm = get_ipm(kvm->arch.gisa, IRQ_FLAG_IAM);
+	if (!ipm)
+		return;
+retry:
+	ipm_t0 = ipm;
+
+	/* Try to kick some vcpus in WAIT state. */
+	kicked = __try_airqs_kick(kvm, ipm);
+	if (kicked < 0)
+		return;
+
+	/* Get IPM and return if clean, IAM has been restored. */
+	ipm = get_ipm(kvm->arch.gisa, IRQ_FLAG_IAM);
+	if (!ipm)
+		return;
+
+	/* Start over, if new ISC bits are pending in IPM. */
+	if ((ipm_t0 ^ ipm) & ~ipm_t0)
+		goto retry;
+
+	/*
+	 * Return as we just kicked at least one vcpu in WAIT state
+	 * open for airqs. The IAM will be restored latest when one
+	 * of them goes into WAIT or STOP state.
+	 */
+	if (kicked > 0)
+		return;
+
+	/*
+	 * No vcpu was kicked either because no vcpu was in WAIT state
+	 * or none of the vcpus in WAIT state are open for airqs.
+	 * Return immediately if no vcpus are in WAIT state.
+	 * There are vcpus in RUN state. They will process the airqs
+	 * if not closed for airqs as well. In that case the system will
+	 * delay airqs until a vcpu decides to take airqs again.
+	 */
+	online_vcpus = atomic_read(&kvm->online_vcpus);
+	if (!bitmap_weight(fi->idle_mask, online_vcpus))
+		return;
+
+	/*
+	 * None of the vcpus in WAIT state take airqs and we might
+	 * have no running vcpus as at least one vcpu is in WAIT state
+	 * and IPM is dirty.
+	 */
+	set_iam(kvm->arch.gisa, kvm->arch.iam);
+}
+
+#define NULL_GISA_ADDR 0x00000000UL
+#define NONE_GISA_ADDR 0x00000001UL
+#define GISA_ADDR_MASK 0xfffff000UL
+
+static void __maybe_unused process_gib_alert_list(void)
+{
+	u32 final, next_alert, origin = 0UL;
+	struct kvm_s390_gisa *gisa;
+	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;
+			kvm = container_of(gisa, struct sie_page2, gisa)->kvm;
+			/* Kick suitable vcpus */
+			__floating_airqs_kick(kvm);
+			origin = next_alert;
+		}
+	} while (!final);
+}
+
 static void nullify_gisa(struct kvm_s390_gisa *gisa)
 {
 	memset(gisa, 0, sizeof(struct kvm_s390_gisa));
-- 
2.13.4

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

[Pierre Morel]

On 19/12/2018 20:17, Michael Mueller wrote:
> This function processes the Gib Alert List (GAL). It is required
> to run when either a gib alert interruption has been received or
> a gisa that is in the alert list is cleared or dropped.
> 
> The GAL is build up by millicode, when the respective ISC bit is
> set in the Interruption Alert Mask (IAM) and an interruption of
> that class is observed.
> 
> Signed-off-by: Michael Mueller <mimu@linux.ibm.com>
> ---
>   arch/s390/kvm/interrupt.c | 140 ++++++++++++++++++++++++++++++++++++++++++++++
>   1 file changed, 140 insertions(+)
> 
> diff --git a/arch/s390/kvm/interrupt.c b/arch/s390/kvm/interrupt.c
> index 48a93f5e5333..03e7ba4f215a 100644
> --- a/arch/s390/kvm/interrupt.c
> +++ b/arch/s390/kvm/interrupt.c
> @@ -2941,6 +2941,146 @@ int kvm_s390_get_irq_state(struct kvm_vcpu *vcpu, __u8 __user *buf, int len)
>   	return n;
>   }
>   
> +static int __try_airqs_kick(struct kvm *kvm, u8 ipm)

static inline ?

> +{
> +	struct kvm_s390_float_interrupt *fi = &kvm->arch.float_int;
> +	struct kvm_vcpu *vcpu = NULL, *kick_vcpu[MAX_ISC + 1];
> +	int online_vcpus = atomic_read(&kvm->online_vcpus);
> +	u8 ioint_mask, isc_mask, kick_mask = 0x00;
> +	int vcpu_id, kicked = 0;
> +
> +	/* Loop over vcpus in WAIT state. */
> +	for (vcpu_id = find_first_bit(fi->idle_mask, online_vcpus);
> +	     /* Until all pending ISCs have a vcpu open for airqs. */
> +	     (~kick_mask & ipm) && vcpu_id < online_vcpus;
> +	     vcpu_id = find_next_bit(fi->idle_mask, online_vcpus, vcpu_id)) {
> +		vcpu = kvm_get_vcpu(kvm, vcpu_id);
> +		if (psw_ioint_disabled(vcpu))
> +			continue;
> +		ioint_mask = (u8)(vcpu->arch.sie_block->gcr[6] >> 24);
> +		for (isc_mask = 0x80; isc_mask; isc_mask >>= 1) {
> +			/* ISC pending in IPM ? */
> +			if (!(ipm & isc_mask))
> +				continue;
> +			/* vcpu for this ISC already found ? */
> +			if (kick_mask & isc_mask)
> +				continue;
> +			/* vcpu open for airq of this ISC ? */
> +			if (!(ioint_mask & isc_mask))
> +				continue;
> +			/* use this vcpu (for all ISCs in ioint_mask) */
> +			kick_mask |= ioint_mask; > +			kick_vcpu[kicked++] = vcpu;
> +		}
> +	}
> +
> +	if (vcpu && ~kick_mask & ipm)
> +		VM_EVENT(kvm, 4, "gib alert undeliverable isc mask 0x%02x",
> +			 ~kick_mask & ipm);
> +
> +	for (vcpu_id = 0; vcpu_id < kicked; vcpu_id++)
> +		kvm_s390_vcpu_wakeup(kick_vcpu[vcpu_id]);
> +
> +	return (online_vcpus != 0) ? kicked : -ENODEV;
> +}
> +
> +static void __floating_airqs_kick(struct kvm *kvm)
static inline ?

> +{
> +	struct kvm_s390_float_interrupt *fi = &kvm->arch.float_int;
> +	int online_vcpus, kicked;
> +	u8 ipm_t0, ipm;
> +
> +	/* Get IPM and return if clean, IAM has been restored. */
> +	ipm = get_ipm(kvm->arch.gisa, IRQ_FLAG_IAM);

If we do not get an IPM here, it must have been stolen by the firmware 
for delivery to the guest.
Then why restoring the IAM?

Or do I miss something?

> +	if (!ipm)
> +		return;
> +retry:
> +	ipm_t0 = ipm;
> +
> +	/* Try to kick some vcpus in WAIT state. */
> +	kicked = __try_airqs_kick(kvm, ipm);
> +	if (kicked < 0)
> +		return;
> +
> +	/* Get IPM and return if clean, IAM has been restored. */
> +	ipm = get_ipm(kvm->arch.gisa, IRQ_FLAG_IAM);
> +	if (!ipm)
> +		return;
> +
> +	/* Start over, if new ISC bits are pending in IPM. */
> +	if ((ipm_t0 ^ ipm) & ~ipm_t0)
> +		goto retry;
> +
> +	/*
> +	 * Return as we just kicked at least one vcpu in WAIT state
> +	 * open for airqs. The IAM will be restored latest when one
> +	 * of them goes into WAIT or STOP state.
> +	 */
> +	if (kicked > 0)
> +		return;
> +
> +	/*
> +	 * No vcpu was kicked either because no vcpu was in WAIT state
> +	 * or none of the vcpus in WAIT state are open for airqs.
> +	 * Return immediately if no vcpus are in WAIT state.
> +	 * There are vcpus in RUN state. They will process the airqs
> +	 * if not closed for airqs as well. In that case the system will
> +	 * delay airqs until a vcpu decides to take airqs again.
> +	 */
> +	online_vcpus = atomic_read(&kvm->online_vcpus);
> +	if (!bitmap_weight(fi->idle_mask, online_vcpus))
> +		return;
> +
> +	/*
> +	 * None of the vcpus in WAIT state take airqs and we might
> +	 * have no running vcpus as at least one vcpu is in WAIT state
> +	 * and IPM is dirty.
> +	 */
> +	set_iam(kvm->arch.gisa, kvm->arch.iam);

I do not understand why we need to set IAM here.
The interrupt will be delivered by the firmware as soon as the PSW or 
CR6 is changed by any vCPU.
...and if this does not happen we can not deliver the interrupt anyway.

> +}
> +
> +#define NULL_GISA_ADDR 0x00000000UL
> +#define NONE_GISA_ADDR 0x00000001UL
> +#define GISA_ADDR_MASK 0xfffff000UL
> +
> +static void __maybe_unused process_gib_alert_list(void)
> +{
> +	u32 final, next_alert, origin = 0UL;
> +	struct kvm_s390_gisa *gisa;
> +	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...

> +			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.

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.
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.

> +			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.

If you agree, with that:

Reviewed-by: Pierre Morel<pmorel@linux.ibm.com>


-- 
Pierre Morel
Linux/KVM/QEMU in Böblingen - Germany

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

[Michael Mueller]

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
>> to run when either a gib alert interruption has been received or
>> a gisa that is in the alert list is cleared or dropped.
>>
>> The GAL is build up by millicode, when the respective ISC bit is
>> set in the Interruption Alert Mask (IAM) and an interruption of
>> that class is observed.
>>
>> Signed-off-by: Michael Mueller <mimu@linux.ibm.com>
>> ---
>>   arch/s390/kvm/interrupt.c | 140 
>> ++++++++++++++++++++++++++++++++++++++++++++++
>>   1 file changed, 140 insertions(+)
>>
>> diff --git a/arch/s390/kvm/interrupt.c b/arch/s390/kvm/interrupt.c
>> index 48a93f5e5333..03e7ba4f215a 100644
>> --- a/arch/s390/kvm/interrupt.c
>> +++ b/arch/s390/kvm/interrupt.c
>> @@ -2941,6 +2941,146 @@ int kvm_s390_get_irq_state(struct kvm_vcpu 
>> *vcpu, __u8 __user *buf, int len)
>>       return n;
>>   }
>> +static int __try_airqs_kick(struct kvm *kvm, u8 ipm)
> 
> static inline ?
> 
>> +{
>> +    struct kvm_s390_float_interrupt *fi = &kvm->arch.float_int;
>> +    struct kvm_vcpu *vcpu = NULL, *kick_vcpu[MAX_ISC + 1];
>> +    int online_vcpus = atomic_read(&kvm->online_vcpus);
>> +    u8 ioint_mask, isc_mask, kick_mask = 0x00;
>> +    int vcpu_id, kicked = 0;
>> +
>> +    /* Loop over vcpus in WAIT state. */
>> +    for (vcpu_id = find_first_bit(fi->idle_mask, online_vcpus);
>> +         /* Until all pending ISCs have a vcpu open for airqs. */
>> +         (~kick_mask & ipm) && vcpu_id < online_vcpus;
>> +         vcpu_id = find_next_bit(fi->idle_mask, online_vcpus, 
>> vcpu_id)) {
>> +        vcpu = kvm_get_vcpu(kvm, vcpu_id);
>> +        if (psw_ioint_disabled(vcpu))
>> +            continue;
>> +        ioint_mask = (u8)(vcpu->arch.sie_block->gcr[6] >> 24);
>> +        for (isc_mask = 0x80; isc_mask; isc_mask >>= 1) {
>> +            /* ISC pending in IPM ? */
>> +            if (!(ipm & isc_mask))
>> +                continue;
>> +            /* vcpu for this ISC already found ? */
>> +            if (kick_mask & isc_mask)
>> +                continue;
>> +            /* vcpu open for airq of this ISC ? */
>> +            if (!(ioint_mask & isc_mask))
>> +                continue;
>> +            /* use this vcpu (for all ISCs in ioint_mask) */
>> +            kick_mask |= ioint_mask; > +            
>> kick_vcpu[kicked++] = vcpu;
>> +        }
>> +    }
>> +
>> +    if (vcpu && ~kick_mask & ipm)
>> +        VM_EVENT(kvm, 4, "gib alert undeliverable isc mask 0x%02x",
>> +             ~kick_mask & ipm);
>> +
>> +    for (vcpu_id = 0; vcpu_id < kicked; vcpu_id++)
>> +        kvm_s390_vcpu_wakeup(kick_vcpu[vcpu_id]);
>> +
>> +    return (online_vcpus != 0) ? kicked : -ENODEV;
>> +}
>> +
>> +static void __floating_airqs_kick(struct kvm *kvm)
> static inline ?
> 
>> +{
>> +    struct kvm_s390_float_interrupt *fi = &kvm->arch.float_int;
>> +    int online_vcpus, kicked;
>> +    u8 ipm_t0, ipm;
>> +
>> +    /* Get IPM and return if clean, IAM has been restored. */
>> +    ipm = get_ipm(kvm->arch.gisa, IRQ_FLAG_IAM);
> 
> If we do not get an IPM here, it must have been stolen by the firmware 
> for delivery to the guest.

Yes, a running SIE instance took it before we were able to. But is
it still running now? It could have gone to WAIT before we see
that the IPM is clean. Then it was restored already. Otherwise,
it is still running and will go WAIT and then restore the IAM.

I will do some tests on this.

> Then why restoring the IAM?
> 
> Or do I miss something?
> 
>> +    if (!ipm)
>> +        return;
>> +retry:
>> +    ipm_t0 = ipm;
>> +
>> +    /* Try to kick some vcpus in WAIT state. */
>> +    kicked = __try_airqs_kick(kvm, ipm);
>> +    if (kicked < 0)
>> +        return;
>> +
>> +    /* Get IPM and return if clean, IAM has been restored. */
>> +    ipm = get_ipm(kvm->arch.gisa, IRQ_FLAG_IAM);
>> +    if (!ipm)
>> +        return;
>> +
>> +    /* Start over, if new ISC bits are pending in IPM. */
>> +    if ((ipm_t0 ^ ipm) & ~ipm_t0)
>> +        goto retry;
>> +
>> +    /*
>> +     * Return as we just kicked at least one vcpu in WAIT state
>> +     * open for airqs. The IAM will be restored latest when one
>> +     * of them goes into WAIT or STOP state.
>> +     */
>> +    if (kicked > 0)
>> +        return;
>> +
>> +    /*
>> +     * No vcpu was kicked either because no vcpu was in WAIT state
>> +     * or none of the vcpus in WAIT state are open for airqs.
>> +     * Return immediately if no vcpus are in WAIT state.
>> +     * There are vcpus in RUN state. They will process the airqs
>> +     * if not closed for airqs as well. In that case the system will
>> +     * delay airqs until a vcpu decides to take airqs again.
>> +     */
>> +    online_vcpus = atomic_read(&kvm->online_vcpus);
>> +    if (!bitmap_weight(fi->idle_mask, online_vcpus))
>> +        return;
>> +
>> +    /*
>> +     * None of the vcpus in WAIT state take airqs and we might
>> +     * have no running vcpus as at least one vcpu is in WAIT state
>> +     * and IPM is dirty.
>> +     */
>> +    set_iam(kvm->arch.gisa, kvm->arch.iam);
> 
> I do not understand why we need to set IAM here.
> The interrupt will be delivered by the firmware as soon as the PSW or 
> CR6 is changed by any vCPU.
> ...and if this does not happen we can not deliver the interrupt anyway.
> 
>> +}
>> +
>> +#define NULL_GISA_ADDR 0x00000000UL
>> +#define NONE_GISA_ADDR 0x00000001UL
>> +#define GISA_ADDR_MASK 0xfffff000UL
>> +
>> +static void __maybe_unused process_gib_alert_list(void)
>> +{
>> +    u32 final, next_alert, origin = 0UL;
>> +    struct kvm_s390_gisa *gisa;
>> +    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. :(

> 
>> +            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.
> 
> 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.

> 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.
> 
> If you agree, with that:
> 
> Reviewed-by: Pierre Morel<pmorel@linux.ibm.com>
> 
> 

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

[Halil Pasic]

On Mon, 7 Jan 2019 20:18:03 +0100
Michael Mueller <mimu@linux.ibm.com> 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
> >> to run when either a gib alert interruption has been received or
> >> a gisa that is in the alert list is cleared or dropped.
> >>
> >> The GAL is build up by millicode, when the respective ISC bit is
> >> set in the Interruption Alert Mask (IAM) and an interruption of
> >> that class is observed.
> >>
> >> Signed-off-by: Michael Mueller <mimu@linux.ibm.com>
> >> ---
> >>   arch/s390/kvm/interrupt.c | 140 
> >> ++++++++++++++++++++++++++++++++++++++++++++++
> >>   1 file changed, 140 insertions(+)
> >>
> >> diff --git a/arch/s390/kvm/interrupt.c b/arch/s390/kvm/interrupt.c
> >> index 48a93f5e5333..03e7ba4f215a 100644
> >> --- a/arch/s390/kvm/interrupt.c
> >> +++ b/arch/s390/kvm/interrupt.c
> >> @@ -2941,6 +2941,146 @@ int kvm_s390_get_irq_state(struct kvm_vcpu 
> >> *vcpu, __u8 __user *buf, int len)
> >>       return n;
> >>   }
> >> +static int __try_airqs_kick(struct kvm *kvm, u8 ipm)
> > 
> > static inline ?
> > 
> >> +{
> >> +    struct kvm_s390_float_interrupt *fi = &kvm->arch.float_int;
> >> +    struct kvm_vcpu *vcpu = NULL, *kick_vcpu[MAX_ISC + 1];
> >> +    int online_vcpus = atomic_read(&kvm->online_vcpus);
> >> +    u8 ioint_mask, isc_mask, kick_mask = 0x00;
> >> +    int vcpu_id, kicked = 0;
> >> +
> >> +    /* Loop over vcpus in WAIT state. */
> >> +    for (vcpu_id = find_first_bit(fi->idle_mask, online_vcpus);
> >> +         /* Until all pending ISCs have a vcpu open for airqs. */
> >> +         (~kick_mask & ipm) && vcpu_id < online_vcpus;
> >> +         vcpu_id = find_next_bit(fi->idle_mask, online_vcpus, 
> >> vcpu_id)) {
> >> +        vcpu = kvm_get_vcpu(kvm, vcpu_id);
> >> +        if (psw_ioint_disabled(vcpu))
> >> +            continue;
> >> +        ioint_mask = (u8)(vcpu->arch.sie_block->gcr[6] >> 24);
> >> +        for (isc_mask = 0x80; isc_mask; isc_mask >>= 1) {
> >> +            /* ISC pending in IPM ? */
> >> +            if (!(ipm & isc_mask))
> >> +                continue;
> >> +            /* vcpu for this ISC already found ? */
> >> +            if (kick_mask & isc_mask)
> >> +                continue;
> >> +            /* vcpu open for airq of this ISC ? */
> >> +            if (!(ioint_mask & isc_mask))
> >> +                continue;
> >> +            /* use this vcpu (for all ISCs in ioint_mask) */
> >> +            kick_mask |= ioint_mask; > +            
> >> kick_vcpu[kicked++] = vcpu;
> >> +        }
> >> +    }
> >> +
> >> +    if (vcpu && ~kick_mask & ipm)
> >> +        VM_EVENT(kvm, 4, "gib alert undeliverable isc mask 0x%02x",
> >> +             ~kick_mask & ipm);
> >> +
> >> +    for (vcpu_id = 0; vcpu_id < kicked; vcpu_id++)
> >> +        kvm_s390_vcpu_wakeup(kick_vcpu[vcpu_id]);
> >> +
> >> +    return (online_vcpus != 0) ? kicked : -ENODEV;
> >> +}
> >> +
> >> +static void __floating_airqs_kick(struct kvm *kvm)
> > static inline ?
> > 
> >> +{
> >> +    struct kvm_s390_float_interrupt *fi = &kvm->arch.float_int;
> >> +    int online_vcpus, kicked;
> >> +    u8 ipm_t0, ipm;
> >> +
> >> +    /* Get IPM and return if clean, IAM has been restored. */
> >> +    ipm = get_ipm(kvm->arch.gisa, IRQ_FLAG_IAM);
> > 
> > If we do not get an IPM here, it must have been stolen by the firmware 
> > for delivery to the guest.
> 
> Yes, a running SIE instance took it before we were able to. But is
> it still running now? It could have gone to WAIT before we see
> that the IPM is clean. Then it was restored already. Otherwise,
> it is still running and will go WAIT and then restore the IAM.
> 
> I will do some tests on this.
> 
> > Then why restoring the IAM?
> > 
> > Or do I miss something?
> > 
> >> +    if (!ipm)
> >> +        return;
> >> +retry:
> >> +    ipm_t0 = ipm;
> >> +
> >> +    /* Try to kick some vcpus in WAIT state. */
> >> +    kicked = __try_airqs_kick(kvm, ipm);
> >> +    if (kicked < 0)
> >> +        return;
> >> +
> >> +    /* Get IPM and return if clean, IAM has been restored. */
> >> +    ipm = get_ipm(kvm->arch.gisa, IRQ_FLAG_IAM);
> >> +    if (!ipm)
> >> +        return;
> >> +
> >> +    /* Start over, if new ISC bits are pending in IPM. */
> >> +    if ((ipm_t0 ^ ipm) & ~ipm_t0)
> >> +        goto retry;
> >> +
> >> +    /*
> >> +     * Return as we just kicked at least one vcpu in WAIT state
> >> +     * open for airqs. The IAM will be restored latest when one
> >> +     * of them goes into WAIT or STOP state.
> >> +     */
> >> +    if (kicked > 0)
> >> +        return;
> >> +
> >> +    /*
> >> +     * No vcpu was kicked either because no vcpu was in WAIT state
> >> +     * or none of the vcpus in WAIT state are open for airqs.
> >> +     * Return immediately if no vcpus are in WAIT state.
> >> +     * There are vcpus in RUN state. They will process the airqs
> >> +     * if not closed for airqs as well. In that case the system will
> >> +     * delay airqs until a vcpu decides to take airqs again.
> >> +     */
> >> +    online_vcpus = atomic_read(&kvm->online_vcpus);
> >> +    if (!bitmap_weight(fi->idle_mask, online_vcpus))
> >> +        return;
> >> +
> >> +    /*
> >> +     * None of the vcpus in WAIT state take airqs and we might
> >> +     * have no running vcpus as at least one vcpu is in WAIT state
> >> +     * and IPM is dirty.
> >> +     */
> >> +    set_iam(kvm->arch.gisa, kvm->arch.iam);
> > 
> > I do not understand why we need to set IAM here.
> > The interrupt will be delivered by the firmware as soon as the PSW or 
> > CR6 is changed by any vCPU.
> > ...and if this does not happen we can not deliver the interrupt anyway.
> > 
> >> +}
> >> +
> >> +#define NULL_GISA_ADDR 0x00000000UL
> >> +#define NONE_GISA_ADDR 0x00000001UL
> >> +#define GISA_ADDR_MASK 0xfffff000UL
> >> +
> >> +static void __maybe_unused process_gib_alert_list(void)
> >> +{
> >> +    u32 final, next_alert, origin = 0UL;
> >> +    struct kvm_s390_gisa *gisa;
> >> +    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. :(
> 
> > 
> >> +            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.
> > 
> > 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.
> 
> > 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.
> 

I'm very confused by these comments, and especially by your apparent
consensus.

Regards,
Halil

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

[Pierre Morel]

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@linux.ibm.com>

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

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

[Michael Mueller]

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
>> to run when either a gib alert interruption has been received or
>> a gisa that is in the alert list is cleared or dropped.
>>
>> The GAL is build up by millicode, when the respective ISC bit is
>> set in the Interruption Alert Mask (IAM) and an interruption of
>> that class is observed.
>>
>> Signed-off-by: Michael Mueller <mimu@linux.ibm.com>
>> ---
>>   arch/s390/kvm/interrupt.c | 140 
>> ++++++++++++++++++++++++++++++++++++++++++++++
>>   1 file changed, 140 insertions(+)
>>
>> diff --git a/arch/s390/kvm/interrupt.c b/arch/s390/kvm/interrupt.c
>> index 48a93f5e5333..03e7ba4f215a 100644
>> --- a/arch/s390/kvm/interrupt.c
>> +++ b/arch/s390/kvm/interrupt.c
>> @@ -2941,6 +2941,146 @@ int kvm_s390_get_irq_state(struct kvm_vcpu 
>> *vcpu, __u8 __user *buf, int len)
>>       return n;
>>   }
>>   +static int __try_airqs_kick(struct kvm *kvm, u8 ipm)
>
> static inline ?


will add


>
>> +{
>> +    struct kvm_s390_float_interrupt *fi = &kvm->arch.float_int;
>> +    struct kvm_vcpu *vcpu = NULL, *kick_vcpu[MAX_ISC + 1];
>> +    int online_vcpus = atomic_read(&kvm->online_vcpus);
>> +    u8 ioint_mask, isc_mask, kick_mask = 0x00;
>> +    int vcpu_id, kicked = 0;
>> +
>> +    /* Loop over vcpus in WAIT state. */
>> +    for (vcpu_id = find_first_bit(fi->idle_mask, online_vcpus);
>> +         /* Until all pending ISCs have a vcpu open for airqs. */
>> +         (~kick_mask & ipm) && vcpu_id < online_vcpus;
>> +         vcpu_id = find_next_bit(fi->idle_mask, online_vcpus, 
>> vcpu_id)) {
>> +        vcpu = kvm_get_vcpu(kvm, vcpu_id);
>> +        if (psw_ioint_disabled(vcpu))
>> +            continue;
>> +        ioint_mask = (u8)(vcpu->arch.sie_block->gcr[6] >> 24);
>> +        for (isc_mask = 0x80; isc_mask; isc_mask >>= 1) {
>> +            /* ISC pending in IPM ? */
>> +            if (!(ipm & isc_mask))
>> +                continue;
>> +            /* vcpu for this ISC already found ? */
>> +            if (kick_mask & isc_mask)
>> +                continue;
>> +            /* vcpu open for airq of this ISC ? */
>> +            if (!(ioint_mask & isc_mask))
>> +                continue;
>> +            /* use this vcpu (for all ISCs in ioint_mask) */
>> +            kick_mask |= ioint_mask; > + kick_vcpu[kicked++] = vcpu;
>> +        }
>> +    }
>> +
>> +    if (vcpu && ~kick_mask & ipm)
>> +        VM_EVENT(kvm, 4, "gib alert undeliverable isc mask 0x%02x",
>> +             ~kick_mask & ipm);
>> +
>> +    for (vcpu_id = 0; vcpu_id < kicked; vcpu_id++)
>> +        kvm_s390_vcpu_wakeup(kick_vcpu[vcpu_id]);
>> +
>> +    return (online_vcpus != 0) ? kicked : -ENODEV;
>> +}
>> +
>> +static void __floating_airqs_kick(struct kvm *kvm)
> static inline ?


and here as well


>
>> +{
>> +    struct kvm_s390_float_interrupt *fi = &kvm->arch.float_int;
>> +    int online_vcpus, kicked;
>> +    u8 ipm_t0, ipm;
>> +
>> +    /* Get IPM and return if clean, IAM has been restored. */
>> +    ipm = get_ipm(kvm->arch.gisa, IRQ_FLAG_IAM);
>
> If we do not get an IPM here, it must have been stolen by the firmware 
> for delivery to the guest.
> Then why restoring the IAM?
>
> Or do I miss something?
>
>> +    if (!ipm)
>> +        return;
>> +retry:
>> +    ipm_t0 = ipm;
>> +
>> +    /* Try to kick some vcpus in WAIT state. */
>> +    kicked = __try_airqs_kick(kvm, ipm);
>> +    if (kicked < 0)
>> +        return;
>> +
>> +    /* Get IPM and return if clean, IAM has been restored. */
>> +    ipm = get_ipm(kvm->arch.gisa, IRQ_FLAG_IAM);
>> +    if (!ipm)
>> +        return;
>> +
>> +    /* Start over, if new ISC bits are pending in IPM. */
>> +    if ((ipm_t0 ^ ipm) & ~ipm_t0)
>> +        goto retry;
>> +
>> +    /*
>> +     * Return as we just kicked at least one vcpu in WAIT state
>> +     * open for airqs. The IAM will be restored latest when one
>> +     * of them goes into WAIT or STOP state.
>> +     */
>> +    if (kicked > 0)
>> +        return;
>> +
>> +    /*
>> +     * No vcpu was kicked either because no vcpu was in WAIT state
>> +     * or none of the vcpus in WAIT state are open for airqs.
>> +     * Return immediately if no vcpus are in WAIT state.
>> +     * There are vcpus in RUN state. They will process the airqs
>> +     * if not closed for airqs as well. In that case the system will
>> +     * delay airqs until a vcpu decides to take airqs again.
>> +     */
>> +    online_vcpus = atomic_read(&kvm->online_vcpus);
>> +    if (!bitmap_weight(fi->idle_mask, online_vcpus))
>> +        return;
>> +
>> +    /*
>> +     * None of the vcpus in WAIT state take airqs and we might
>> +     * have no running vcpus as at least one vcpu is in WAIT state
>> +     * and IPM is dirty.
>> +     */
>> +    set_iam(kvm->arch.gisa, kvm->arch.iam);
>
> I do not understand why we need to set IAM here.
> The interrupt will be delivered by the firmware as soon as the PSW or 
> CR6 is changed by any vCPU.
> ...and if this does not happen we can not deliver the interrupt anyway.
>
>> +}
>> +
>> +#define NULL_GISA_ADDR 0x00000000UL
>> +#define NONE_GISA_ADDR 0x00000001UL
>> +#define GISA_ADDR_MASK 0xfffff000UL
>> +
>> +static void __maybe_unused process_gib_alert_list(void)
>> +{
>> +    u32 final, next_alert, origin = 0UL;
>> +    struct kvm_s390_gisa *gisa;
>> +    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...
>
>> +            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.
>
> 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.
> 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.
>
>> +            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.
>
> If you agree, with that:
>
> Reviewed-by: Pierre Morel<pmorel@linux.ibm.com>
>
>
-- 
Mit freundlichen Grüßen / Kind regards
Michael Müller

IBM Deutschland Research & Development GmbH
Vorsitzender des Aufsichtsrats: Matthias Hartmann
Geschäftsführung: Dirk Wittkopp
Sitz der Gesellschaft: Böblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294

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

[Heiko Carstens]

On Mon, Jan 07, 2019 at 08:19:26PM +0100, 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
> >>to run when either a gib alert interruption has been received or
> >>a gisa that is in the alert list is cleared or dropped.
> >>
> >>The GAL is build up by millicode, when the respective ISC bit is
> >>set in the Interruption Alert Mask (IAM) and an interruption of
> >>that class is observed.
> >>
> >>Signed-off-by: Michael Mueller <mimu@linux.ibm.com>
> >>---
> >>  arch/s390/kvm/interrupt.c | 140
> >>++++++++++++++++++++++++++++++++++++++++++++++
> >>  1 file changed, 140 insertions(+)
> >>
> >>diff --git a/arch/s390/kvm/interrupt.c b/arch/s390/kvm/interrupt.c
> >>index 48a93f5e5333..03e7ba4f215a 100644
> >>--- a/arch/s390/kvm/interrupt.c
> >>+++ b/arch/s390/kvm/interrupt.c
> >>@@ -2941,6 +2941,146 @@ int kvm_s390_get_irq_state(struct kvm_vcpu
> >>*vcpu, __u8 __user *buf, int len)
> >>      return n;
> >>  }
> >>  +static int __try_airqs_kick(struct kvm *kvm, u8 ipm)
> >
> >static inline ?

Why?

In general it is a good idea to leave it up to the compiler to decide
if it is worth to inline a function or not, unless you have a good
reason to force inlining.

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

[Halil Pasic]

On Wed, 19 Dec 2018 20:17:54 +0100
Michael Mueller <mimu@linux.ibm.com> wrote:

> This function processes the Gib Alert List (GAL). It is required
> to run when either a gib alert interruption has been received or
> a gisa that is in the alert list is cleared or dropped.
> 
> The GAL is build up by millicode, when the respective ISC bit is
> set in the Interruption Alert Mask (IAM) and an interruption of
> that class is observed.
> 
> Signed-off-by: Michael Mueller <mimu@linux.ibm.com>
> ---
>  arch/s390/kvm/interrupt.c | 140 ++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 140 insertions(+)
> 
> diff --git a/arch/s390/kvm/interrupt.c b/arch/s390/kvm/interrupt.c
> index 48a93f5e5333..03e7ba4f215a 100644
> --- a/arch/s390/kvm/interrupt.c
> +++ b/arch/s390/kvm/interrupt.c
> @@ -2941,6 +2941,146 @@ int kvm_s390_get_irq_state(struct kvm_vcpu *vcpu, __u8 __user *buf, int len)
>  	return n;
>  }
>  
> +static int __try_airqs_kick(struct kvm *kvm, u8 ipm)
> +{
> +	struct kvm_s390_float_interrupt *fi = &kvm->arch.float_int;
> +	struct kvm_vcpu *vcpu = NULL, *kick_vcpu[MAX_ISC + 1];
> +	int online_vcpus = atomic_read(&kvm->online_vcpus);
> +	u8 ioint_mask, isc_mask, kick_mask = 0x00;
> +	int vcpu_id, kicked = 0;
> +
> +	/* Loop over vcpus in WAIT state. */
> +	for (vcpu_id = find_first_bit(fi->idle_mask, online_vcpus);
> +	     /* Until all pending ISCs have a vcpu open for airqs. */
> +	     (~kick_mask & ipm) && vcpu_id < online_vcpus;
> +	     vcpu_id = find_next_bit(fi->idle_mask, online_vcpus, vcpu_id)) {
> +		vcpu = kvm_get_vcpu(kvm, vcpu_id);
> +		if (psw_ioint_disabled(vcpu))
> +			continue;
> +		ioint_mask = (u8)(vcpu->arch.sie_block->gcr[6] >> 24);
> +		for (isc_mask = 0x80; isc_mask; isc_mask >>= 1) {
> +			/* ISC pending in IPM ? */
> +			if (!(ipm & isc_mask))
> +				continue;
> +			/* vcpu for this ISC already found ? */
> +			if (kick_mask & isc_mask)
> +				continue;
> +			/* vcpu open for airq of this ISC ? */
> +			if (!(ioint_mask & isc_mask))
> +				continue;
> +			/* use this vcpu (for all ISCs in ioint_mask) */
> +			kick_mask |= ioint_mask;
> +			kick_vcpu[kicked++] = vcpu;

Assuming that the vcpu can/will take all ISCs it's currently open for
does not seem right. We kind of rely on this assumption here, or?

> +		}
> +	}
> +
> +	if (vcpu && ~kick_mask & ipm)
> +		VM_EVENT(kvm, 4, "gib alert undeliverable isc mask
> 0x%02x",
> +			 ~kick_mask & ipm);
> +
> +	for (vcpu_id = 0; vcpu_id < kicked; vcpu_id++)
> +		kvm_s390_vcpu_wakeup(kick_vcpu[vcpu_id]);
> +
> +	return (online_vcpus != 0) ? kicked : -ENODEV;
> +}
> +
> +static void __floating_airqs_kick(struct kvm *kvm)
> +{
> +	struct kvm_s390_float_interrupt *fi = &kvm->arch.float_int;
> +	int online_vcpus, kicked;
> +	u8 ipm_t0, ipm;
> +
> +	/* Get IPM and return if clean, IAM has been restored. */
> +	ipm = get_ipm(kvm->arch.gisa, IRQ_FLAG_IAM);
> +	if (!ipm)
> +		return;
> +retry:
> +	ipm_t0 = ipm;
> +
> +	/* Try to kick some vcpus in WAIT state. */
> +	kicked = __try_airqs_kick(kvm, ipm);
> +	if (kicked < 0)
> +		return;
> +
> +	/* Get IPM and return if clean, IAM has been restored. */
> +	ipm = get_ipm(kvm->arch.gisa, IRQ_FLAG_IAM);
> +	if (!ipm)
> +		return;
> +
> +	/* Start over, if new ISC bits are pending in IPM. */
> +	if ((ipm_t0 ^ ipm) & ~ipm_t0)
> +		goto retry;
> +

<MARK A>

> +	/*
> +	 * Return as we just kicked at least one vcpu in WAIT state
> +	 * open for airqs. The IAM will be restored latest when one
> +	 * of them goes into WAIT or STOP state.
> +	 */
> +	if (kicked > 0)
> +		return;

</MARK A>

> +
> +	/*
> +	 * No vcpu was kicked either because no vcpu was in WAIT state
> +	 * or none of the vcpus in WAIT state are open for airqs.
> +	 * Return immediately if no vcpus are in WAIT state.
> +	 * There are vcpus in RUN state. They will process the airqs
> +	 * if not closed for airqs as well. In that case the system will
> +	 * delay airqs until a vcpu decides to take airqs again.
> +	 */
> +	online_vcpus = atomic_read(&kvm->online_vcpus);
> +	if (!bitmap_weight(fi->idle_mask, online_vcpus))
> +		return;
> +
> +	/*
> +	 * None of the vcpus in WAIT state take airqs and we might
> +	 * have no running vcpus as at least one vcpu is in WAIT state
> +	 * and IPM is dirty.
> +	 */
> +	set_iam(kvm->arch.gisa, kvm->arch.iam);
> +}
> +
> +#define NULL_GISA_ADDR 0x00000000UL
> +#define NONE_GISA_ADDR 0x00000001UL
> +#define GISA_ADDR_MASK 0xfffff000UL
> +
> +static void __maybe_unused process_gib_alert_list(void)
> +{
> +	u32 final, next_alert, origin = 0UL;
> +	struct kvm_s390_gisa *gisa;
> +	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;
> +			kvm = container_of(gisa, struct sie_page2, gisa)->kvm;
> +			/* Kick suitable vcpus */
> +			__floating_airqs_kick(kvm);

We may finish handling the alerted gisa with iam not set e.g.
via some vcpus kicked but ipm still dirty and some vcpus still in wait,
or? 

From the comments it seems we speculate on being in a safe state, as
these are supposed to return to wait or stop soon-ish, and we will set
iam then (See <MARK A>). I don't quite understand.

According to my current understanding we might end up loosing initiative
in this scenario. Or am I wrong?

Regards,
Halil

> +			origin = next_alert;
> +		}
> +	} while (!final);
> +}
> +
>  static void nullify_gisa(struct kvm_s390_gisa *gisa)
>  {
>  	memset(gisa, 0, sizeof(struct kvm_s390_gisa));

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

[Michael Mueller]

On 08.01.19 13:59, Halil Pasic wrote:
> On Wed, 19 Dec 2018 20:17:54 +0100
> Michael Mueller <mimu@linux.ibm.com> wrote:
> 
>> This function processes the Gib Alert List (GAL). It is required
>> to run when either a gib alert interruption has been received or
>> a gisa that is in the alert list is cleared or dropped.
>>
>> The GAL is build up by millicode, when the respective ISC bit is
>> set in the Interruption Alert Mask (IAM) and an interruption of
>> that class is observed.
>>
>> Signed-off-by: Michael Mueller <mimu@linux.ibm.com>
>> ---
>>   arch/s390/kvm/interrupt.c | 140 ++++++++++++++++++++++++++++++++++++++++++++++
>>   1 file changed, 140 insertions(+)
>>
>> diff --git a/arch/s390/kvm/interrupt.c b/arch/s390/kvm/interrupt.c
>> index 48a93f5e5333..03e7ba4f215a 100644
>> --- a/arch/s390/kvm/interrupt.c
>> +++ b/arch/s390/kvm/interrupt.c
>> @@ -2941,6 +2941,146 @@ int kvm_s390_get_irq_state(struct kvm_vcpu *vcpu, __u8 __user *buf, int len)
>>   	return n;
>>   }
>>   
>> +static int __try_airqs_kick(struct kvm *kvm, u8 ipm)
>> +{
>> +	struct kvm_s390_float_interrupt *fi = &kvm->arch.float_int;
>> +	struct kvm_vcpu *vcpu = NULL, *kick_vcpu[MAX_ISC + 1];
>> +	int online_vcpus = atomic_read(&kvm->online_vcpus);
>> +	u8 ioint_mask, isc_mask, kick_mask = 0x00;
>> +	int vcpu_id, kicked = 0;
>> +
>> +	/* Loop over vcpus in WAIT state. */
>> +	for (vcpu_id = find_first_bit(fi->idle_mask, online_vcpus);
>> +	     /* Until all pending ISCs have a vcpu open for airqs. */
>> +	     (~kick_mask & ipm) && vcpu_id < online_vcpus;
>> +	     vcpu_id = find_next_bit(fi->idle_mask, online_vcpus, vcpu_id)) {
>> +		vcpu = kvm_get_vcpu(kvm, vcpu_id);
>> +		if (psw_ioint_disabled(vcpu))
>> +			continue;
>> +		ioint_mask = (u8)(vcpu->arch.sie_block->gcr[6] >> 24);
>> +		for (isc_mask = 0x80; isc_mask; isc_mask >>= 1) {
>> +			/* ISC pending in IPM ? */
>> +			if (!(ipm & isc_mask))
>> +				continue;
>> +			/* vcpu for this ISC already found ? */
>> +			if (kick_mask & isc_mask)
>> +				continue;
>> +			/* vcpu open for airq of this ISC ? */
>> +			if (!(ioint_mask & isc_mask))
>> +				continue;
>> +			/* use this vcpu (for all ISCs in ioint_mask) */
>> +			kick_mask |= ioint_mask;
>> +			kick_vcpu[kicked++] = vcpu;
> 
> Assuming that the vcpu can/will take all ISCs it's currently open for
> does not seem right. We kind of rely on this assumption here, or?

My latest version of this routine already follows a different strategy.
It looks for a horizontal distribution of pending ISCs among idle vcpus.

> 
>> +		}
>> +	}
>> +
>> +	if (vcpu && ~kick_mask & ipm)
>> +		VM_EVENT(kvm, 4, "gib alert undeliverable isc mask
>> 0x%02x",
>> +			 ~kick_mask & ipm);
>> +
>> +	for (vcpu_id = 0; vcpu_id < kicked; vcpu_id++)
>> +		kvm_s390_vcpu_wakeup(kick_vcpu[vcpu_id]);
>> +
>> +	return (online_vcpus != 0) ? kicked : -ENODEV;
>> +}
>> +
>> +static void __floating_airqs_kick(struct kvm *kvm)
>> +{
>> +	struct kvm_s390_float_interrupt *fi = &kvm->arch.float_int;
>> +	int online_vcpus, kicked;
>> +	u8 ipm_t0, ipm;
>> +
>> +	/* Get IPM and return if clean, IAM has been restored. */
>> +	ipm = get_ipm(kvm->arch.gisa, IRQ_FLAG_IAM);
>> +	if (!ipm)
>> +		return;
>> +retry:
>> +	ipm_t0 = ipm;
>> +
>> +	/* Try to kick some vcpus in WAIT state. */
>> +	kicked = __try_airqs_kick(kvm, ipm);
>> +	if (kicked < 0)
>> +		return;
>> +
>> +	/* Get IPM and return if clean, IAM has been restored. */
>> +	ipm = get_ipm(kvm->arch.gisa, IRQ_FLAG_IAM);
>> +	if (!ipm)
>> +		return;
>> +
>> +	/* Start over, if new ISC bits are pending in IPM. */
>> +	if ((ipm_t0 ^ ipm) & ~ipm_t0)
>> +		goto retry;
>> +
> 
> <MARK A>
> 
>> +	/*
>> +	 * Return as we just kicked at least one vcpu in WAIT state
>> +	 * open for airqs. The IAM will be restored latest when one
>> +	 * of them goes into WAIT or STOP state.
>> +	 */
>> +	if (kicked > 0)
>> +		return;
> 
> </MARK A>
> 
>> +
>> +	/*
>> +	 * No vcpu was kicked either because no vcpu was in WAIT state
>> +	 * or none of the vcpus in WAIT state are open for airqs.
>> +	 * Return immediately if no vcpus are in WAIT state.
>> +	 * There are vcpus in RUN state. They will process the airqs
>> +	 * if not closed for airqs as well. In that case the system will
>> +	 * delay airqs until a vcpu decides to take airqs again.
>> +	 */
>> +	online_vcpus = atomic_read(&kvm->online_vcpus);
>> +	if (!bitmap_weight(fi->idle_mask, online_vcpus))
>> +		return;
>> +
>> +	/*
>> +	 * None of the vcpus in WAIT state take airqs and we might
>> +	 * have no running vcpus as at least one vcpu is in WAIT state
>> +	 * and IPM is dirty.
>> +	 */
>> +	set_iam(kvm->arch.gisa, kvm->arch.iam);
>> +}
>> +
>> +#define NULL_GISA_ADDR 0x00000000UL
>> +#define NONE_GISA_ADDR 0x00000001UL
>> +#define GISA_ADDR_MASK 0xfffff000UL
>> +
>> +static void __maybe_unused process_gib_alert_list(void)
>> +{
>> +	u32 final, next_alert, origin = 0UL;
>> +	struct kvm_s390_gisa *gisa;
>> +	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;
>> +			kvm = container_of(gisa, struct sie_page2, gisa)->kvm;
>> +			/* Kick suitable vcpus */
>> +			__floating_airqs_kick(kvm);
> 
> We may finish handling the alerted gisa with iam not set e.g.
> via some vcpus kicked but ipm still dirty and some vcpus still in wait,
> or?

My above mentioned change to the routine identifying the vcpus to kick
will select one vcpu for each ISC pending if possible (depends on the
number of idle vcpus and their respective interruption masks and the
pending ISCs).

That does not exclude the principle scenario that maybe only one vcpu
is kicked and multiple ISCs are pending (ipm still dirty) although
have never observed this with a Linux guest.

What I was trying to avoid was a kind of busy loop running in addition
to the kicked vcpus monitoring the IPM state for resource utilization
reasons.

> 
>  From the comments it seems we speculate on being in a safe state, as
> these are supposed to return to wait or stop soon-ish, and we will set
> iam then (See <MARK A>). I don't quite understand.


Yes, the next vcpu going idle shall restore the IAM or process the
top ISC pending if the iomask (GCR) allows. vcpus are not allowed to go 
in disabled wait (IO int disabled by PSW). If all vcpus always (for
some time) mask a specific ISC the guest does not want to get 
interrupted for that ISC but will as soon a running vcpu will open
the mask again.

> 
> According to my current understanding we might end up loosing initiative
> in this scenario. Or am I wrong?

I currently don't have proof for you being wrong but have not observed
the situation yet.

> 
> Regards,
> Halil
> 
>> +			origin = next_alert;
>> +		}
>> +	} while (!final);
>> +}
>> +
>>   static void nullify_gisa(struct kvm_s390_gisa *gisa)
>>   {
>>   	memset(gisa, 0, sizeof(struct kvm_s390_gisa));
> 

-

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

[Halil Pasic]

On Tue, 8 Jan 2019 16:21:17 +0100
Michael Mueller <mimu@linux.ibm.com> wrote:

> >> +			gisa->next_alert = origin;
> >> +			kvm = container_of(gisa, struct sie_page2, gisa)->kvm;
> >> +			/* Kick suitable vcpus */
> >> +			__floating_airqs_kick(kvm);  
> > 
> > We may finish handling the alerted gisa with iam not set e.g.
> > via some vcpus kicked but ipm still dirty and some vcpus still in wait,
> > or?  
> 
> My above mentioned change to the routine identifying the vcpus to kick
> will select one vcpu for each ISC pending if possible (depends on the
> number of idle vcpus and their respective interruption masks and the
> pending ISCs).
> 
> That does not exclude the principle scenario that maybe only one vcpu
> is kicked and multiple ISCs are pending (ipm still dirty) although
> have never observed this with a Linux guest.
> 

IMHO we have to differentiate between the general case and between what
can happen with current or historical Linux guests.

Regarding Linux guests, I'm under the impression each version was quite
there. I says so, also because I have a reasonable amount of confidence
in your testing.

> What I was trying to avoid was a kind of busy loop running in addition
> to the kicked vcpus monitoring the IPM state for resource utilization
> reasons.
> 

Got it. I think we need a clean switch-over (between our
code makes sure the no pending interrupts are going to get stalled in
spite of waiting vcpus that could take them, and between we are good now
and if we are not good any more we will get an alert) nevertheless. 

> > 
> >  From the comments it seems we speculate on being in a safe state, as
> > these are supposed to return to wait or stop soon-ish, and we will set
> > iam then (See <MARK A>). I don't quite understand.  
> 
> 
> Yes, the next vcpu going idle shall restore the IAM or process the
> top ISC pending if the iomask (GCR) allows. vcpus are not allowed to go 
> in disabled wait (IO int disabled by PSW). If all vcpus always (for
> some time) mask a specific ISC the guest does not want to get 
> interrupted for that ISC but will as soon a running vcpu will open
> the mask again.
> 

My understanding on the guarantees we can provide based on the fact that
we kicked some vcpus is still lacking. Maybe let us discuss this offline.

> > 
> > According to my current understanding we might end up loosing initiative
> > in this scenario. Or am I wrong?  
> 
> I currently don't have proof for you being wrong but have not observed
> the situation yet.


See above. Proving that no irq's can get substantially delayed
needlessly (where needlessly means, there is a vcpu in wait state that
could take the irq) would be a proof enough that I'm wrong. Let's
make this discussion more efficient by utilizing co-location to cut
down the RTT.

Regards,
Halil

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

[Pierre Morel]

On 08/01/2019 16:21, Michael Mueller wrote:
> 
> 
> On 08.01.19 13:59, Halil Pasic wrote:
>> On Wed, 19 Dec 2018 20:17:54 +0100
>> Michael Mueller <mimu@linux.ibm.com> wrote:
>>
>>> This function processes the Gib Alert List (GAL). It is required
>>> to run when either a gib alert interruption has been received or
>>> a gisa that is in the alert list is cleared or dropped.
>>>
>>> The GAL is build up by millicode, when the respective ISC bit is
>>> set in the Interruption Alert Mask (IAM) and an interruption of
>>> that class is observed.
>>>
>>> Signed-off-by: Michael Mueller <mimu@linux.ibm.com>
>>> ---
>>>   arch/s390/kvm/interrupt.c | 140 
>>> ++++++++++++++++++++++++++++++++++++++++++++++
>>>   1 file changed, 140 insertions(+)
>>>
>>> diff --git a/arch/s390/kvm/interrupt.c b/arch/s390/kvm/interrupt.c
>>> index 48a93f5e5333..03e7ba4f215a 100644
>>> --- a/arch/s390/kvm/interrupt.c
>>> +++ b/arch/s390/kvm/interrupt.c
>>> @@ -2941,6 +2941,146 @@ int kvm_s390_get_irq_state(struct kvm_vcpu 
>>> *vcpu, __u8 __user *buf, int len)
>>>       return n;
>>>   }
>>> +static int __try_airqs_kick(struct kvm *kvm, u8 ipm)
>>> +{
>>> +    struct kvm_s390_float_interrupt *fi = &kvm->arch.float_int;
>>> +    struct kvm_vcpu *vcpu = NULL, *kick_vcpu[MAX_ISC + 1];
>>> +    int online_vcpus = atomic_read(&kvm->online_vcpus);
>>> +    u8 ioint_mask, isc_mask, kick_mask = 0x00;
>>> +    int vcpu_id, kicked = 0;
>>> +
>>> +    /* Loop over vcpus in WAIT state. */
>>> +    for (vcpu_id = find_first_bit(fi->idle_mask, online_vcpus);
>>> +         /* Until all pending ISCs have a vcpu open for airqs. */
>>> +         (~kick_mask & ipm) && vcpu_id < online_vcpus;
>>> +         vcpu_id = find_next_bit(fi->idle_mask, online_vcpus, 
>>> vcpu_id)) {
>>> +        vcpu = kvm_get_vcpu(kvm, vcpu_id);
>>> +        if (psw_ioint_disabled(vcpu))
>>> +            continue;
>>> +        ioint_mask = (u8)(vcpu->arch.sie_block->gcr[6] >> 24);
>>> +        for (isc_mask = 0x80; isc_mask; isc_mask >>= 1) {
>>> +            /* ISC pending in IPM ? */
>>> +            if (!(ipm & isc_mask))
>>> +                continue;
>>> +            /* vcpu for this ISC already found ? */
>>> +            if (kick_mask & isc_mask)
>>> +                continue;
>>> +            /* vcpu open for airq of this ISC ? */
>>> +            if (!(ioint_mask & isc_mask))
>>> +                continue;
>>> +            /* use this vcpu (for all ISCs in ioint_mask) */
>>> +            kick_mask |= ioint_mask;
>>> +            kick_vcpu[kicked++] = vcpu;
>>
>> Assuming that the vcpu can/will take all ISCs it's currently open for
>> does not seem right. We kind of rely on this assumption here, or?

why does it not seem right?

> 
> My latest version of this routine already follows a different strategy.
> It looks for a horizontal distribution of pending ISCs among idle vcpus.
> 

May be you should separate the GAL IRQ handling and the algorithm of the 
vCPU to kick in different patches to ease the review.


Regards,
Pierre

-- 
Pierre Morel
Linux/KVM/QEMU in Böblingen - Germany

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

[Halil Pasic]

On Wed, 9 Jan 2019 13:14:17 +0100
Pierre Morel <pmorel@linux.ibm.com> wrote:

> On 08/01/2019 16:21, Michael Mueller wrote:
> > 
> > 
> > On 08.01.19 13:59, Halil Pasic wrote:
> >> On Wed, 19 Dec 2018 20:17:54 +0100
> >> Michael Mueller <mimu@linux.ibm.com> wrote:
> >>
> >>> This function processes the Gib Alert List (GAL). It is required
> >>> to run when either a gib alert interruption has been received or
> >>> a gisa that is in the alert list is cleared or dropped.
> >>>
> >>> The GAL is build up by millicode, when the respective ISC bit is
> >>> set in the Interruption Alert Mask (IAM) and an interruption of
> >>> that class is observed.
> >>>
> >>> Signed-off-by: Michael Mueller <mimu@linux.ibm.com>
> >>> ---
> >>>   arch/s390/kvm/interrupt.c | 140 
> >>> ++++++++++++++++++++++++++++++++++++++++++++++
> >>>   1 file changed, 140 insertions(+)
> >>>
> >>> diff --git a/arch/s390/kvm/interrupt.c b/arch/s390/kvm/interrupt.c
> >>> index 48a93f5e5333..03e7ba4f215a 100644
> >>> --- a/arch/s390/kvm/interrupt.c
> >>> +++ b/arch/s390/kvm/interrupt.c
> >>> @@ -2941,6 +2941,146 @@ int kvm_s390_get_irq_state(struct kvm_vcpu 
> >>> *vcpu, __u8 __user *buf, int len)
> >>>       return n;
> >>>   }
> >>> +static int __try_airqs_kick(struct kvm *kvm, u8 ipm)
> >>> +{
> >>> +    struct kvm_s390_float_interrupt *fi = &kvm->arch.float_int;
> >>> +    struct kvm_vcpu *vcpu = NULL, *kick_vcpu[MAX_ISC + 1];
> >>> +    int online_vcpus = atomic_read(&kvm->online_vcpus);
> >>> +    u8 ioint_mask, isc_mask, kick_mask = 0x00;
> >>> +    int vcpu_id, kicked = 0;
> >>> +
> >>> +    /* Loop over vcpus in WAIT state. */
> >>> +    for (vcpu_id = find_first_bit(fi->idle_mask, online_vcpus);
> >>> +         /* Until all pending ISCs have a vcpu open for airqs. */
> >>> +         (~kick_mask & ipm) && vcpu_id < online_vcpus;
> >>> +         vcpu_id = find_next_bit(fi->idle_mask, online_vcpus, 
> >>> vcpu_id)) {
> >>> +        vcpu = kvm_get_vcpu(kvm, vcpu_id);
> >>> +        if (psw_ioint_disabled(vcpu))
> >>> +            continue;
> >>> +        ioint_mask = (u8)(vcpu->arch.sie_block->gcr[6] >> 24);
> >>> +        for (isc_mask = 0x80; isc_mask; isc_mask >>= 1) {
> >>> +            /* ISC pending in IPM ? */
> >>> +            if (!(ipm & isc_mask))
> >>> +                continue;
> >>> +            /* vcpu for this ISC already found ? */
> >>> +            if (kick_mask & isc_mask)
> >>> +                continue;
> >>> +            /* vcpu open for airq of this ISC ? */
> >>> +            if (!(ioint_mask & isc_mask))
> >>> +                continue;
> >>> +            /* use this vcpu (for all ISCs in ioint_mask) */
> >>> +            kick_mask |= ioint_mask;
> >>> +            kick_vcpu[kicked++] = vcpu;
> >>
> >> Assuming that the vcpu can/will take all ISCs it's currently open for
> >> does not seem right. We kind of rely on this assumption here, or?
> 
> why does it not seem right?
> 

When an interrupt is delivered a psw-swap takes place. The new-psw
may fence IO interrupts. Thus for example if we have the vcpu open for
all ISCs and 0, 1 and 2 pending, we may end up only delivering 0, if the
psw-swap corresponding to delivering 0 closes the vcpu for IO
interrupts. After guest has control, we don't have control over the rest
of the story. 

> > 
> > My latest version of this routine already follows a different strategy.
> > It looks for a horizontal distribution of pending ISCs among idle vcpus.
> > 
> 
> May be you should separate the GAL IRQ handling and the algorithm of the 
> vCPU to kick in different patches to ease the review.
> 
> 

No strong opinion here. I found it convenient to have the most of the
logic in one patch/email.

Regards,
Halil

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

[Pierre Morel]

On 09/01/2019 14:10, Halil Pasic wrote:
> On Wed, 9 Jan 2019 13:14:17 +0100
> Pierre Morel <pmorel@linux.ibm.com> wrote:
> 
>> On 08/01/2019 16:21, Michael Mueller wrote:
>>>
>>>
>>> On 08.01.19 13:59, Halil Pasic wrote:
>>>> On Wed, 19 Dec 2018 20:17:54 +0100
>>>> Michael Mueller <mimu@linux.ibm.com> wrote:
>>>>
>>>>> This function processes the Gib Alert List (GAL). It is required
>>>>> to run when either a gib alert interruption has been received or
>>>>> a gisa that is in the alert list is cleared or dropped.
>>>>>
>>>>> The GAL is build up by millicode, when the respective ISC bit is
>>>>> set in the Interruption Alert Mask (IAM) and an interruption of
>>>>> that class is observed.
>>>>>
>>>>> Signed-off-by: Michael Mueller <mimu@linux.ibm.com>
>>>>> ---
>>>>>    arch/s390/kvm/interrupt.c | 140
>>>>> ++++++++++++++++++++++++++++++++++++++++++++++
>>>>>    1 file changed, 140 insertions(+)
>>>>>
>>>>> diff --git a/arch/s390/kvm/interrupt.c b/arch/s390/kvm/interrupt.c
>>>>> index 48a93f5e5333..03e7ba4f215a 100644
>>>>> --- a/arch/s390/kvm/interrupt.c
>>>>> +++ b/arch/s390/kvm/interrupt.c
>>>>> @@ -2941,6 +2941,146 @@ int kvm_s390_get_irq_state(struct kvm_vcpu
>>>>> *vcpu, __u8 __user *buf, int len)
>>>>>        return n;
>>>>>    }
>>>>> +static int __try_airqs_kick(struct kvm *kvm, u8 ipm)
>>>>> +{
>>>>> +    struct kvm_s390_float_interrupt *fi = &kvm->arch.float_int;
>>>>> +    struct kvm_vcpu *vcpu = NULL, *kick_vcpu[MAX_ISC + 1];
>>>>> +    int online_vcpus = atomic_read(&kvm->online_vcpus);
>>>>> +    u8 ioint_mask, isc_mask, kick_mask = 0x00;
>>>>> +    int vcpu_id, kicked = 0;
>>>>> +
>>>>> +    /* Loop over vcpus in WAIT state. */
>>>>> +    for (vcpu_id = find_first_bit(fi->idle_mask, online_vcpus);
>>>>> +         /* Until all pending ISCs have a vcpu open for airqs. */
>>>>> +         (~kick_mask & ipm) && vcpu_id < online_vcpus;
>>>>> +         vcpu_id = find_next_bit(fi->idle_mask, online_vcpus,
>>>>> vcpu_id)) {
>>>>> +        vcpu = kvm_get_vcpu(kvm, vcpu_id);
>>>>> +        if (psw_ioint_disabled(vcpu))
>>>>> +            continue;
>>>>> +        ioint_mask = (u8)(vcpu->arch.sie_block->gcr[6] >> 24);
>>>>> +        for (isc_mask = 0x80; isc_mask; isc_mask >>= 1) {
>>>>> +            /* ISC pending in IPM ? */
>>>>> +            if (!(ipm & isc_mask))
>>>>> +                continue;
>>>>> +            /* vcpu for this ISC already found ? */
>>>>> +            if (kick_mask & isc_mask)
>>>>> +                continue;
>>>>> +            /* vcpu open for airq of this ISC ? */
>>>>> +            if (!(ioint_mask & isc_mask))
>>>>> +                continue;
>>>>> +            /* use this vcpu (for all ISCs in ioint_mask) */
>>>>> +            kick_mask |= ioint_mask;
>>>>> +            kick_vcpu[kicked++] = vcpu;
>>>>
>>>> Assuming that the vcpu can/will take all ISCs it's currently open for
>>>> does not seem right. We kind of rely on this assumption here, or?
>>
>> why does it not seem right?
>>
> 
> When an interrupt is delivered a psw-swap takes place. The new-psw
> may fence IO interrupts. Thus for example if we have the vcpu open for
> all ISCs and 0, 1 and 2 pending, we may end up only delivering 0, if the
> psw-swap corresponding to delivering 0 closes the vcpu for IO
> interrupts. After guest has control, we don't have control over the rest
> of the story.

OK I think I understand your concern, waking up a single waiting vCPU 
per ISC is not enough.
We must wake all vCPU in wait state having at least one matching ISC bit.

Regards,
Pierre


-- 
Pierre Morel
Linux/KVM/QEMU in Böblingen - Germany

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

[Halil Pasic]

On Wed, 9 Jan 2019 15:49:56 +0100
Pierre Morel <pmorel@linux.ibm.com> wrote:

> On 09/01/2019 14:10, Halil Pasic wrote:
> > On Wed, 9 Jan 2019 13:14:17 +0100
> > Pierre Morel <pmorel@linux.ibm.com> wrote:
> > 
> >> On 08/01/2019 16:21, Michael Mueller wrote:
> >>>
> >>>
> >>> On 08.01.19 13:59, Halil Pasic wrote:
> >>>> On Wed, 19 Dec 2018 20:17:54 +0100
> >>>> Michael Mueller <mimu@linux.ibm.com> wrote:
> >>>>
> >>>>> This function processes the Gib Alert List (GAL). It is required
> >>>>> to run when either a gib alert interruption has been received or
> >>>>> a gisa that is in the alert list is cleared or dropped.
> >>>>>
> >>>>> The GAL is build up by millicode, when the respective ISC bit is
> >>>>> set in the Interruption Alert Mask (IAM) and an interruption of
> >>>>> that class is observed.
> >>>>>
> >>>>> Signed-off-by: Michael Mueller <mimu@linux.ibm.com>
> >>>>> ---
> >>>>>    arch/s390/kvm/interrupt.c | 140
> >>>>> ++++++++++++++++++++++++++++++++++++++++++++++
> >>>>>    1 file changed, 140 insertions(+)
> >>>>>
> >>>>> diff --git a/arch/s390/kvm/interrupt.c b/arch/s390/kvm/interrupt.c
> >>>>> index 48a93f5e5333..03e7ba4f215a 100644
> >>>>> --- a/arch/s390/kvm/interrupt.c
> >>>>> +++ b/arch/s390/kvm/interrupt.c
> >>>>> @@ -2941,6 +2941,146 @@ int kvm_s390_get_irq_state(struct kvm_vcpu
> >>>>> *vcpu, __u8 __user *buf, int len)
> >>>>>        return n;
> >>>>>    }
> >>>>> +static int __try_airqs_kick(struct kvm *kvm, u8 ipm)
> >>>>> +{
> >>>>> +    struct kvm_s390_float_interrupt *fi = &kvm->arch.float_int;
> >>>>> +    struct kvm_vcpu *vcpu = NULL, *kick_vcpu[MAX_ISC + 1];
> >>>>> +    int online_vcpus = atomic_read(&kvm->online_vcpus);
> >>>>> +    u8 ioint_mask, isc_mask, kick_mask = 0x00;
> >>>>> +    int vcpu_id, kicked = 0;
> >>>>> +
> >>>>> +    /* Loop over vcpus in WAIT state. */
> >>>>> +    for (vcpu_id = find_first_bit(fi->idle_mask, online_vcpus);
> >>>>> +         /* Until all pending ISCs have a vcpu open for airqs. */
> >>>>> +         (~kick_mask & ipm) && vcpu_id < online_vcpus;
> >>>>> +         vcpu_id = find_next_bit(fi->idle_mask, online_vcpus,
> >>>>> vcpu_id)) {
> >>>>> +        vcpu = kvm_get_vcpu(kvm, vcpu_id);
> >>>>> +        if (psw_ioint_disabled(vcpu))
> >>>>> +            continue;
> >>>>> +        ioint_mask = (u8)(vcpu->arch.sie_block->gcr[6] >> 24);
> >>>>> +        for (isc_mask = 0x80; isc_mask; isc_mask >>= 1) {
> >>>>> +            /* ISC pending in IPM ? */
> >>>>> +            if (!(ipm & isc_mask))
> >>>>> +                continue;
> >>>>> +            /* vcpu for this ISC already found ? */
> >>>>> +            if (kick_mask & isc_mask)
> >>>>> +                continue;
> >>>>> +            /* vcpu open for airq of this ISC ? */
> >>>>> +            if (!(ioint_mask & isc_mask))
> >>>>> +                continue;
> >>>>> +            /* use this vcpu (for all ISCs in ioint_mask) */
> >>>>> +            kick_mask |= ioint_mask;
> >>>>> +            kick_vcpu[kicked++] = vcpu;
> >>>>
> >>>> Assuming that the vcpu can/will take all ISCs it's currently open for
> >>>> does not seem right. We kind of rely on this assumption here, or?
> >>
> >> why does it not seem right?
> >>
> > 
> > When an interrupt is delivered a psw-swap takes place. The new-psw
> > may fence IO interrupts. Thus for example if we have the vcpu open for
> > all ISCs and 0, 1 and 2 pending, we may end up only delivering 0, if the
> > psw-swap corresponding to delivering 0 closes the vcpu for IO
> > interrupts. After guest has control, we don't have control over the rest
> > of the story.
> 
> OK I think I understand your concern, waking up a single waiting vCPU 
> per ISC is not enough.
> We must wake all vCPU in wait state having at least one matching ISC bit.
> 

That is not what I was trying to say, and IMHO generally it also ain't
true that we must. But I may be missing something.

Regards,
Halil

[PATCH v5 14/15] 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 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   | 36 ++++++++++++++++++++++++++++++++++--
 4 files changed, 37 insertions(+), 2 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 03e7ba4f215a..79b9c262479b 100644
--- a/arch/s390/kvm/interrupt.c
+++ b/arch/s390/kvm/interrupt.c
@@ -26,6 +26,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"
@@ -3043,7 +3044,7 @@ static void __floating_airqs_kick(struct kvm *kvm)
 #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;
@@ -3091,7 +3092,10 @@ void kvm_s390_gisa_clear(struct kvm *kvm)
 {
 	if (!kvm->arch.gisa)
 		return;
+	if (set_iam(kvm->arch.gisa, 0) == -EBUSY)
+		process_gib_alert_list();
 	nullify_gisa(kvm->arch.gisa);
+	set_iam(kvm->arch.gisa, kvm->arch.iam);
 	VM_EVENT(kvm, 3, "gisa 0x%pK cleared", kvm->arch.gisa);
 }
 
@@ -3111,6 +3115,8 @@ void kvm_s390_gisa_destroy(struct kvm *kvm)
 {
 	if (!kvm->arch.gisa)
 		return;
+	if (set_iam(kvm->arch.gisa, 0) == -EBUSY)
+		process_gib_alert_list();
 	kvm->arch.gisa = NULL;
 }
 
@@ -3159,11 +3165,23 @@ 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;
 }
@@ -3183,16 +3201,30 @@ int kvm_s390_gib_init(u8 nisc)
 		goto out;
 	}
 
+	gib_alert_irq.isc = nisc;
+	if (register_adapter_interrupt(&gib_alert_irq)) {
+		pr_err("Registering the GIB alert interruption handler failed\n");
+		rc = -EIO;
+		goto out_free;
+	}
+
 	gib->nisc = nisc;
 	if (chsc_sgib((u32)(u64)gib)) {
 		pr_err("Associating the GIB with the AIV facility failed\n");
 		free_page((unsigned long)gib);
 		gib = NULL;
 		rc = -EIO;
-		goto out;
+		goto out_unreg;
 	}
 
 	KVM_EVENT(3, "gib 0x%pK (nisc=%d) initialized", gib, gib->nisc);
+	goto out;
+
+out_unreg:
+	unregister_adapter_interrupt(&gib_alert_irq);
+out_free:
+	free_page((unsigned long)gib);
+	gib = NULL;
 out:
 	return rc;
 }
-- 
2.13.4

Re: [PATCH v5 14/15] KVM: s390: add and wire function gib_alert_irq_handler()

[Pierre Morel]

On 19/12/2018 20:17, Michael Mueller wrote:
> The patch implements a handler for GIB alert interruptions
> on the host. Its task is to alert 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   | 36 ++++++++++++++++++++++++++++++++++--
>   4 files changed, 37 insertions(+), 2 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 03e7ba4f215a..79b9c262479b 100644
> --- a/arch/s390/kvm/interrupt.c
> +++ b/arch/s390/kvm/interrupt.c
> @@ -26,6 +26,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"
> @@ -3043,7 +3044,7 @@ static void __floating_airqs_kick(struct kvm *kvm)
>   #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;
> @@ -3091,7 +3092,10 @@ void kvm_s390_gisa_clear(struct kvm *kvm)
>   {
>   	if (!kvm->arch.gisa)
>   		return;
> +	if (set_iam(kvm->arch.gisa, 0) == -EBUSY)
> +		process_gib_alert_list();

We call process_gib_alert_list() from different contexts shouldn't we 
protect the calls?


>   	nullify_gisa(kvm->arch.gisa);
> +	set_iam(kvm->arch.gisa, kvm->arch.iam);
>   	VM_EVENT(kvm, 3, "gisa 0x%pK cleared", kvm->arch.gisa);
>   }
>   
> @@ -3111,6 +3115,8 @@ void kvm_s390_gisa_destroy(struct kvm *kvm)
>   {
>   	if (!kvm->arch.gisa)
>   		return;
> +	if (set_iam(kvm->arch.gisa, 0) == -EBUSY)
> +		process_gib_alert_list();

idem.

>   	kvm->arch.gisa = NULL;
>   }
>   
> @@ -3159,11 +3165,23 @@ 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();

idem.

> +}
> +
> +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;
>   }
> @@ -3183,16 +3201,30 @@ int kvm_s390_gib_init(u8 nisc)
>   		goto out;
>   	}
>   
> +	gib_alert_irq.isc = nisc;
> +	if (register_adapter_interrupt(&gib_alert_irq)) {
> +		pr_err("Registering the GIB alert interruption handler failed\n");
> +		rc = -EIO;
> +		goto out_free;
> +	}
> +
>   	gib->nisc = nisc;
>   	if (chsc_sgib((u32)(u64)gib)) {
>   		pr_err("Associating the GIB with the AIV facility failed\n");
>   		free_page((unsigned long)gib);
>   		gib = NULL;
>   		rc = -EIO;
> -		goto out;
> +		goto out_unreg;
>   	}
>   
>   	KVM_EVENT(3, "gib 0x%pK (nisc=%d) initialized", gib, gib->nisc);
> +	goto out;
> +
> +out_unreg:
> +	unregister_adapter_interrupt(&gib_alert_irq);
> +out_free:
> +	free_page((unsigned long)gib);
> +	gib = NULL;
>   out:
>   	return rc;
>   }
> 


-- 
Pierre Morel
Linux/KVM/QEMU in Böblingen - Germany

Re: [PATCH v5 14/15] KVM: s390: add and wire function gib_alert_irq_handler()

[Michael Mueller]

On 03.01.19 16:16, Pierre Morel wrote:
> On 19/12/2018 20:17, Michael Mueller wrote:
>> The patch implements a handler for GIB alert interruptions
>> on the host. Its task is to alert 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   | 36 ++++++++++++++++++++++++++++++++++--
>>   4 files changed, 37 insertions(+), 2 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 03e7ba4f215a..79b9c262479b 100644
>> --- a/arch/s390/kvm/interrupt.c
>> +++ b/arch/s390/kvm/interrupt.c
>> @@ -26,6 +26,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"
>> @@ -3043,7 +3044,7 @@ static void __floating_airqs_kick(struct kvm *kvm)
>>   #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;
>> @@ -3091,7 +3092,10 @@ void kvm_s390_gisa_clear(struct kvm *kvm)
>>   {
>>       if (!kvm->arch.gisa)
>>           return;
>> +    if (set_iam(kvm->arch.gisa, 0) == -EBUSY)
>> +        process_gib_alert_list();
> 
> We call process_gib_alert_list() from different contexts shouldn't we 
> protect the calls?

That should not be necessary as the xcgh() guarantees that both
instances will work on gib alert lists with disjunctive gisas.

> 
> 
>>       nullify_gisa(kvm->arch.gisa);
>> +    set_iam(kvm->arch.gisa, kvm->arch.iam);
>>       VM_EVENT(kvm, 3, "gisa 0x%pK cleared", kvm->arch.gisa);
>>   }
>> @@ -3111,6 +3115,8 @@ void kvm_s390_gisa_destroy(struct kvm *kvm)
>>   {
>>       if (!kvm->arch.gisa)
>>           return;
>> +    if (set_iam(kvm->arch.gisa, 0) == -EBUSY)
>> +        process_gib_alert_list();
> 
> idem.
> 
>>       kvm->arch.gisa = NULL;
>>   }
>> @@ -3159,11 +3165,23 @@ 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();
> 
> idem.
> 
>> +}
>> +
>> +static struct airq_struct gib_alert_irq = {
>> +    .handler = gib_alert_irq_handler,
>> +    .lsi_ptr = &gib_alert_irq.lsi_mask,
>> +};
>> +

> 

Re: [PATCH v5 14/15] KVM: s390: add and wire function gib_alert_irq_handler()

[Pierre Morel]

On 08/01/2019 11:06, Michael Mueller wrote:
> 
> 
> On 03.01.19 16:16, Pierre Morel wrote:
>> On 19/12/2018 20:17, Michael Mueller wrote:
>>> The patch implements a handler for GIB alert interruptions
>>> on the host. Its task is to alert guests that interrupts are
>>> pending for them.
>>>

...snip...

>>>   {
>>>       u32 final, next_alert, origin = 0UL;
>>>       struct kvm_s390_gisa *gisa;
>>> @@ -3091,7 +3092,10 @@ void kvm_s390_gisa_clear(struct kvm *kvm)
>>>   {
>>>       if (!kvm->arch.gisa)
>>>           return;
>>> +    if (set_iam(kvm->arch.gisa, 0) == -EBUSY)
>>> +        process_gib_alert_list();
>>
>> We call process_gib_alert_list() from different contexts shouldn't we 
>> protect the calls?
> 
> That should not be necessary as the xcgh() guarantees that both
> instances will work on gib alert lists with disjunctive gisas.

Here is how I see the problem:

A CPU get the GAL IRQ and start processing the ALERT list.

On another guest we clear floating interrupt...
we call gisa_clear()
we return from set_iam with -EBUSY, meaning the GISA is in alert list.
   -> we call process_gib_alert_list()
   -> since the list has been disjunct by the GAL IRQ routine we return
      immediately
   -> we nullify the GISA while it has not been handled by the IRQ
      routine

!! if my assumption is right, we loose all GISA following the GISA we 
just nullified.


Regards,
Pierre



-- 
Pierre Morel
Linux/KVM/QEMU in Böblingen - Germany

[PATCH v5 15/15] KVM: s390: start using the GIB

[Michael Mueller]

By initializing the GIB, it will be used by the kvm host.

Signed-off-by: Michael Mueller <mimu@linux.ibm.com>
---
 arch/s390/kvm/kvm-s390.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
index 2d10e175862c..777c8a87d81c 100644
--- a/arch/s390/kvm/kvm-s390.c
+++ b/arch/s390/kvm/kvm-s390.c
@@ -428,16 +428,22 @@ int kvm_arch_init(void *opaque)
 		goto out_debug_unreg;
 	}
 
+	rc = kvm_s390_gib_init(GAL_ISC);
+	if (rc)
+		goto out_debug_unreg;
+
 	kvm_s390_cpu_feat_init();
 
 	/* Register floating interrupt controller interface. */
 	rc = kvm_register_device_ops(&kvm_flic_ops, KVM_DEV_TYPE_FLIC);
 	if (rc) {
 		pr_err("Failed to register FLIC rc=%d\n", rc);
-		goto out_debug_unreg;
+		goto out_gib_destroy;
 	}
 	return 0;
 
+out_gib_destroy:
+	kvm_s390_gib_destroy();
 out_debug_unreg:
 	debug_unregister(kvm_s390_dbf);
 	return rc;
-- 
2.13.4

Re: [PATCH v5 15/15] KVM: s390: start using the GIB

[Pierre Morel]

On 19/12/2018 20:17, Michael Mueller wrote:
> By initializing the GIB, it will be used by the kvm host.
> 
> Signed-off-by: Michael Mueller <mimu@linux.ibm.com>
> ---
>   arch/s390/kvm/kvm-s390.c | 8 +++++++-
>   1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
> index 2d10e175862c..777c8a87d81c 100644
> --- a/arch/s390/kvm/kvm-s390.c
> +++ b/arch/s390/kvm/kvm-s390.c
> @@ -428,16 +428,22 @@ int kvm_arch_init(void *opaque)
>   		goto out_debug_unreg;
>   	}
>   
> +	rc = kvm_s390_gib_init(GAL_ISC);
> +	if (rc)
> +		goto out_debug_unreg;
> +

Since it is the last introduced I have a preference to put the gib 
initialization after any existing initialization.
it does not have influence them.

>   	kvm_s390_cpu_feat_init();
>   
>   	/* Register floating interrupt controller interface. */
>   	rc = kvm_register_device_ops(&kvm_flic_ops, KVM_DEV_TYPE_FLIC);
>   	if (rc) {
>   		pr_err("Failed to register FLIC rc=%d\n", rc);
> -		goto out_debug_unreg;
> +		goto out_gib_destroy;

It is not a big problem but would make the patch more smaller by 
gathering all changes.

>   	}
>   	return 0;
>   
> +out_gib_destroy:
> +	kvm_s390_gib_destroy();
>   out_debug_unreg:
>   	debug_unregister(kvm_s390_dbf);
>   	return rc;
> 

with these changes.
Reviewed-by: Pierre Morel<pmorel@linux.ibm.com>




-- 
Pierre Morel
Linux/KVM/QEMU in Böblingen - Germany

Re: [PATCH v5 15/15] KVM: s390: start using the GIB

[Michael Mueller]

On 02.01.19 18:45, Pierre Morel wrote:
> On 19/12/2018 20:17, Michael Mueller wrote:
>> By initializing the GIB, it will be used by the kvm host.
>>
>> Signed-off-by: Michael Mueller <mimu@linux.ibm.com>
>> ---
>>   arch/s390/kvm/kvm-s390.c | 8 +++++++-
>>   1 file changed, 7 insertions(+), 1 deletion(-)
>>
>> diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
>> index 2d10e175862c..777c8a87d81c 100644
>> --- a/arch/s390/kvm/kvm-s390.c
>> +++ b/arch/s390/kvm/kvm-s390.c
>> @@ -428,16 +428,22 @@ int kvm_arch_init(void *opaque)
>>           goto out_debug_unreg;
>>       }
>> +    rc = kvm_s390_gib_init(GAL_ISC);
>> +    if (rc)
>> +        goto out_debug_unreg;
>> +
> 
> Since it is the last introduced I have a preference to put the gib 
> initialization after any existing initialization.
> it does not have influence them.
> 
>>       kvm_s390_cpu_feat_init();
>>       /* Register floating interrupt controller interface. */
>>       rc = kvm_register_device_ops(&kvm_flic_ops, KVM_DEV_TYPE_FLIC);
>>       if (rc) {
>>           pr_err("Failed to register FLIC rc=%d\n", rc);
>> -        goto out_debug_unreg;
>> +        goto out_gib_destroy;
> 
> It is not a big problem but would make the patch more smaller by 
> gathering all changes.

I will move that behind the FLIC registraion.

> 
>>       }
>>       return 0;
>> +out_gib_destroy:
>> +    kvm_s390_gib_destroy();
>>   out_debug_unreg:
>>       debug_unregister(kvm_s390_dbf);
>>       return rc;
>>
> 
> with these changes.
> Reviewed-by: Pierre Morel<pmorel@linux.ibm.com>
> 
> 
> 
>