[v2,06/12] KVM: s390: exploit GISA and AIV for emulated interrupts

Commit Message

Christian Borntraeger Jan. 25, 2018, 1:28 p.m. UTC

From: Michael Mueller <[e-mail]>

The adapter interruption virtualization (AIV) facility is an
optional facility that comes with functionality expected to increase
the performance of adapter interrupt handling for both emulated and
passed-through adapter interrupts. With AIV, adapter interrupts can be
delivered to the guest without exiting SIE.

This patch provides some preparations for using AIV for emulated adapter
interrupts (inclusive virtio) if it's available. When using AIV, the
interrupts are delivered at the so called GISA by setting the bit
corresponding to its Interruption Subclass (ISC) in the Interruption
Pending Mask (IPM) instead of inserting a node into the floating interrupt
list.

To keep the change reasonably small, the handling of this new state is
deferred in get_all_floating_irqs and handle_tpi. This patch concentrates
on the code handling enqueuement of emulated adapter interrupts, and their
delivery to the guest.

Note that care is still required for adapter interrupts using AIV,
because there is no guarantee that AIV is going to deliver the adapter
interrupts pending at the GISA (consider all vcpus idle). When delivering
GISA adapter interrupts by the host (usual mechanism) special attention
is required to honor interrupt priorities.

Empirical results show that the time window between making an interrupt
pending at the GISA and doing kvm_s390_deliver_pending_interrupts is
sufficient for a guest with at least moderate cpu activity to get adapter
interrupts delivered within the SIE, and potentially save some SIE exits
(if not other deliverable interrupts).

The code will be activated with a follow-up patch.

Signed-off-by: Michael Mueller <[e-mail]>
Acked-by: Christian Borntraeger <[e-mail]>
Signed-off-by: Christian Borntraeger <[e-mail]>
---
 arch/s390/include/asm/kvm_host.h |   1 +
 arch/s390/kvm/interrupt.c        | 106 +++++++++++++++++++++++++++++++--------
 arch/s390/kvm/kvm-s390.c         |   8 +++
 arch/s390/kvm/kvm-s390.h         |   3 ++
 4 files changed, 98 insertions(+), 20 deletions(-)

Comments

David Hildenbrand Jan. 25, 2018, 2:20 p.m. UTC | #1

On 25.01.2018 14:28, Christian Borntraeger wrote:
> From: Michael Mueller <[e-mail]>
> 
> The adapter interruption virtualization (AIV) facility is an
> optional facility that comes with functionality expected to increase
> the performance of adapter interrupt handling for both emulated and
> passed-through adapter interrupts. With AIV, adapter interrupts can be
> delivered to the guest without exiting SIE.
> 
> This patch provides some preparations for using AIV for emulated adapter
> interrupts (inclusive virtio) if it's available. When using AIV, the
> interrupts are delivered at the so called GISA by setting the bit
> corresponding to its Interruption Subclass (ISC) in the Interruption
> Pending Mask (IPM) instead of inserting a node into the floating interrupt
> list.
> 
> To keep the change reasonably small, the handling of this new state is
> deferred in get_all_floating_irqs and handle_tpi. This patch concentrates
> on the code handling enqueuement of emulated adapter interrupts, and their
> delivery to the guest.
> 
> Note that care is still required for adapter interrupts using AIV,
> because there is no guarantee that AIV is going to deliver the adapter
> interrupts pending at the GISA (consider all vcpus idle). When delivering
> GISA adapter interrupts by the host (usual mechanism) special attention
> is required to honor interrupt priorities.
> 
> Empirical results show that the time window between making an interrupt
> pending at the GISA and doing kvm_s390_deliver_pending_interrupts is
> sufficient for a guest with at least moderate cpu activity to get adapter
> interrupts delivered within the SIE, and potentially save some SIE exits
> (if not other deliverable interrupts).
> 
> The code will be activated with a follow-up patch.
> 
> Signed-off-by: Michael Mueller <[e-mail]>
> Acked-by: Christian Borntraeger <[e-mail]>
> Signed-off-by: Christian Borntraeger <[e-mail]>
> ---
>  arch/s390/include/asm/kvm_host.h |   1 +
>  arch/s390/kvm/interrupt.c        | 106 +++++++++++++++++++++++++++++++--------
>  arch/s390/kvm/kvm-s390.c         |   8 +++
>  arch/s390/kvm/kvm-s390.h         |   3 ++
>  4 files changed, 98 insertions(+), 20 deletions(-)
> 
> diff --git a/arch/s390/include/asm/kvm_host.h b/arch/s390/include/asm/kvm_host.h
> index 77acade..6802d5d 100644
> --- a/arch/s390/include/asm/kvm_host.h
> +++ b/arch/s390/include/asm/kvm_host.h
> @@ -772,6 +772,7 @@ struct kvm_arch{
>  	struct kvm_s390_migration_state *migration_state;
>  	/* 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;
>  };
>  
>  #define KVM_HVA_ERR_BAD		(-1UL)
> diff --git a/arch/s390/kvm/interrupt.c b/arch/s390/kvm/interrupt.c
> index d559776..263cd10 100644
> --- a/arch/s390/kvm/interrupt.c
> +++ b/arch/s390/kvm/interrupt.c
> @@ -228,7 +228,8 @@ static inline int kvm_s390_gisa_tac_ipm_gisc(struct kvm_s390_gisa *gisa, u32 gis
>  static inline unsigned long pending_irqs(struct kvm_vcpu *vcpu)
>  {
>  	return vcpu->kvm->arch.float_int.pending_irqs |
> -	       vcpu->arch.local_int.pending_irqs;
> +		vcpu->arch.local_int.pending_irqs |
> +		kvm_s390_gisa_get_ipm(vcpu->kvm->arch.gisa) << IRQ_PEND_IO_ISC_7;
>  }
>  
>  static inline int isc_to_irq_type(unsigned long isc)
> @@ -918,18 +919,38 @@ static int __must_check __deliver_virtio(struct kvm_vcpu *vcpu)
>  	return rc ? -EFAULT : 0;
>  }
>  
> +static int __do_deliver_io(struct kvm_vcpu *vcpu, struct kvm_s390_io_info *io)
> +{
> +	int rc;
> +
> +	rc  = put_guest_lc(vcpu, io->subchannel_id, (u16 *)__LC_SUBCHANNEL_ID);
> +	rc |= put_guest_lc(vcpu, io->subchannel_nr, (u16 *)__LC_SUBCHANNEL_NR);
> +	rc |= put_guest_lc(vcpu, io->io_int_parm, (u32 *)__LC_IO_INT_PARM);
> +	rc |= put_guest_lc(vcpu, io->io_int_word, (u32 *)__LC_IO_INT_WORD);
> +	rc |= write_guest_lc(vcpu, __LC_IO_OLD_PSW,
> +			     &vcpu->arch.sie_block->gpsw,
> +			     sizeof(psw_t));
> +	rc |= read_guest_lc(vcpu, __LC_IO_NEW_PSW,
> +			    &vcpu->arch.sie_block->gpsw,
> +			    sizeof(psw_t));

These should now it into less lines. Can you factor that change out into
a separate patch?

> +	return rc ? -EFAULT : 0;
> +}
> +
>  static int __must_check __deliver_io(struct kvm_vcpu *vcpu,
>  				     unsigned long irq_type)
>  {
>  	struct list_head *isc_list;
>  	struct kvm_s390_float_interrupt *fi;
>  	struct kvm_s390_interrupt_info *inti = NULL;
> +	struct kvm_s390_io_info io;
> +	u32 isc;
>  	int rc = 0;
>  
>  	fi = &vcpu->kvm->arch.float_int;
>  
>  	spin_lock(&fi->lock);
> -	isc_list = &fi->lists[irq_type_to_isc(irq_type)];
> +	isc = irq_type_to_isc(irq_type);
> +	isc_list = &fi->lists[isc];
>  	inti = list_first_entry_or_null(isc_list,
>  					struct kvm_s390_interrupt_info,
>  					list);
> @@ -957,24 +978,32 @@ static int __must_check __deliver_io(struct kvm_vcpu *vcpu,
>  	spin_unlock(&fi->lock);
>  
>  	if (inti) {
> -		rc  = put_guest_lc(vcpu, inti->io.subchannel_id,
> -				(u16 *)__LC_SUBCHANNEL_ID);
> -		rc |= put_guest_lc(vcpu, inti->io.subchannel_nr,
> -				(u16 *)__LC_SUBCHANNEL_NR);
> -		rc |= put_guest_lc(vcpu, inti->io.io_int_parm,
> -				(u32 *)__LC_IO_INT_PARM);
> -		rc |= put_guest_lc(vcpu, inti->io.io_int_word,
> -				(u32 *)__LC_IO_INT_WORD);
> -		rc |= write_guest_lc(vcpu, __LC_IO_OLD_PSW,
> -				&vcpu->arch.sie_block->gpsw,
> -				sizeof(psw_t));
> -		rc |= read_guest_lc(vcpu, __LC_IO_NEW_PSW,
> -				&vcpu->arch.sie_block->gpsw,
> -				sizeof(psw_t));
> +		rc = __do_deliver_io(vcpu, &(inti->io));
>  		kfree(inti);
> +		goto out;
>  	}
>  
> -	return rc ? -EFAULT : 0;
> +	if (vcpu->kvm->arch.gisa) {
> +		if (kvm_s390_gisa_tac_ipm_gisc(vcpu->kvm->arch.gisa, isc)) {


if (vcpu->kvm->arch.gisa &&
    kvm_s390_gisa_tac_ipm_gisc(vcpu->kvm->arch.gisa, isc) {

avoids one nesting level

> +			/*
> +			 * in case an adapter interrupt was not delivered
> +			 * in SIE context KVM will handle the delivery
> +			 */
> +			VCPU_EVENT(vcpu, 4, "%s isc %u", "deliver: I/O (AI/gisa)", isc);
> +			memset(&io, 0, sizeof(io));
> +			io.io_int_word = (isc << 27) | 0x80000000;
> +			vcpu->stat.deliver_io_int++;
> +			trace_kvm_s390_deliver_interrupt(vcpu->vcpu_id,
> +				KVM_S390_INT_IO(1, 0, 0, 0),
> +				((__u32)io.subchannel_id << 16) |
> +				io.subchannel_nr,
> +				((__u64)io.io_int_parm << 32) |
> +				io.io_int_word);
> +			rc = __do_deliver_io(vcpu, &io);
> +		}
> +	}
> +out:
> +	return rc;
>  }
>  
>  typedef int (*deliver_irq_t)(struct kvm_vcpu *vcpu);
> @@ -1537,12 +1566,23 @@ static int __inject_io(struct kvm *kvm, struct kvm_s390_interrupt_info *inti)
>  	struct kvm_s390_float_interrupt *fi;
>  	struct list_head *list;
>  	int isc;
> +	int rc = 0;
> +
> +	isc = int_word_to_isc(inti->io.io_int_word);
> +
> +	if (kvm->arch.gisa && inti->type & KVM_S390_INT_IO_AI_MASK) {

&& (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);

Can there only be one pending at a time? (e.g. what happens if the bit
is already set?)

> +		kfree(inti);
> +		goto out;

Why not simply return 0? ...

> +	}
>  
>  	fi = &kvm->arch.float_int;
>  	spin_lock(&fi->lock);
>  	if (fi->counters[FIRQ_CNTR_IO] >= KVM_S390_MAX_FLOAT_IRQS) {
>  		spin_unlock(&fi->lock);
> -		return -EBUSY;
> +		rc = -EBUSY;
> +		goto out;

... avoids this change ...

>  	}
>  	fi->counters[FIRQ_CNTR_IO] += 1;
>  
> @@ -1553,12 +1593,12 @@ static int __inject_io(struct kvm *kvm, struct kvm_s390_interrupt_info *inti)
>  			inti->io.subchannel_id >> 8,
>  			inti->io.subchannel_id >> 1 & 0x3,
>  			inti->io.subchannel_nr);
> -	isc = int_word_to_isc(inti->io.io_int_word);
>  	list = &fi->lists[FIRQ_LIST_IO_ISC_0 + isc];
>  	list_add_tail(&inti->list, list);
>  	set_bit(isc_to_irq_type(isc), &fi->pending_irqs);
>  	spin_unlock(&fi->lock);
> -	return 0;
> +out:
> +	return rc;

... and this.

>  }
>  
>  /*
> @@ -2705,3 +2745,29 @@ int kvm_s390_get_irq_state(struct kvm_vcpu *vcpu, __u8 __user *buf, int len)
>  	return n;
>  }
>  
> +void kvm_s390_gisa_clear(struct kvm *kvm)

can we instead call this "gisa_setup" or move all that directly into the
init function? (because it essentially inits the gisa)

> +{
> +	if (kvm->arch.gisa) {

This check is not needed: guaranteed to be set by the caller.

> +		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 (1 || !css_general_characteristics.aiv)
> +		kvm->arch.gisa = NULL;

1 || ... ? This will always trigger. -> gisa never active with this patch

> +	else {
> +		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)
> +{
> +	if (!kvm->arch.gisa)
> +		return;
> +	kvm->arch.gisa = NULL;

You can do this unconditionally. Nevertheless, this function is not
really useful: only called when we destroy the VM.

> +}
> diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
> index 48f0099..68d7eef 100644
> --- a/arch/s390/kvm/kvm-s390.c
> +++ b/arch/s390/kvm/kvm-s390.c
> @@ -1986,6 +1986,7 @@ int kvm_arch_init_vm(struct kvm *kvm, unsigned long type)
>  
>  	spin_lock_init(&kvm->arch.start_stop_lock);
>  	kvm_s390_vsie_init(kvm);
> +	kvm_s390_gisa_init(kvm);
>  	KVM_EVENT(3, "vm 0x%pK created by pid %u", kvm, current->pid);
>  
>  	return 0;
> @@ -2048,6 +2049,7 @@ void kvm_arch_destroy_vm(struct kvm *kvm)
>  	kvm_free_vcpus(kvm);
>  	sca_dispose(kvm);
>  	debug_unregister(kvm->arch.dbf);
> +	kvm_s390_gisa_destroy(kvm);
>  	free_page((unsigned long)kvm->arch.sie_page2);
>  	if (!kvm_is_ucontrol(kvm))
>  		gmap_remove(kvm->arch.gmap);
> @@ -2458,6 +2460,11 @@ int kvm_arch_vcpu_setup(struct kvm_vcpu *vcpu)
>  	if (test_kvm_facility(vcpu->kvm, 139))
>  		vcpu->arch.sie_block->ecd |= ECD_MEF;
>  
> +	if (vcpu->arch.sie_block->gd) {
> +		vcpu->arch.sie_block->eca |= ECA_AIV;
> +		VCPU_EVENT(vcpu, 3, "AIV gisa format-%u enabled for cpu %03u",
> +			   vcpu->arch.sie_block->gd & 0x3, vcpu->vcpu_id);
> +	}
>  	vcpu->arch.sie_block->sdnxo = ((unsigned long) &vcpu->run->s.regs.sdnx)
>  					| SDNXC;
>  	vcpu->arch.sie_block->riccbd = (unsigned long) &vcpu->run->s.regs.riccb;
> @@ -2510,6 +2517,7 @@ struct kvm_vcpu *kvm_arch_vcpu_create(struct kvm *kvm,
>  
>  	vcpu->arch.sie_block->icpua = id;
>  	spin_lock_init(&vcpu->arch.local_int.lock);
> +	vcpu->arch.sie_block->gd = (u32)(u64)kvm->arch.gisa;
>  	seqcount_init(&vcpu->arch.cputm_seqcount);
>  
>  	rc = kvm_vcpu_init(vcpu, kvm, id);
> diff --git a/arch/s390/kvm/kvm-s390.h b/arch/s390/kvm/kvm-s390.h
> index 8877116..05269a9 100644
> --- a/arch/s390/kvm/kvm-s390.h
> +++ b/arch/s390/kvm/kvm-s390.h
> @@ -367,6 +367,9 @@ int kvm_s390_set_irq_state(struct kvm_vcpu *vcpu,
>  			   void __user *buf, int len);
>  int kvm_s390_get_irq_state(struct kvm_vcpu *vcpu,
>  			   __u8 __user *buf, int len);
> +void kvm_s390_gisa_init(struct kvm *kvm);
> +void kvm_s390_gisa_clear(struct kvm *kvm);
> +void kvm_s390_gisa_destroy(struct kvm *kvm);
>  
>  /* implemented in guestdbg.c */
>  void kvm_s390_backup_guest_per_regs(struct kvm_vcpu *vcpu);
>

Christian Borntraeger Jan. 25, 2018, 2:32 p.m. UTC | #2

On 01/25/2018 03:20 PM, David Hildenbrand wrote:

>> +void kvm_s390_gisa_init(struct kvm *kvm)
>> +{
>> +	if (1 || !css_general_characteristics.aiv)
>> +		kvm->arch.gisa = NULL;
> 
> 1 || ... ? This will always trigger. -> gisa never active with this patch

See patch 10.

David Hildenbrand Jan. 25, 2018, 2:42 p.m. UTC | #3

On 25.01.2018 15:32, Christian Borntraeger wrote:
> 
> 
> On 01/25/2018 03:20 PM, David Hildenbrand wrote:
> 
>>> +void kvm_s390_gisa_init(struct kvm *kvm)
>>> +{
>>> +	if (1 || !css_general_characteristics.aiv)
>>> +		kvm->arch.gisa = NULL;
>>
>> 1 || ... ? This will always trigger. -> gisa never active with this patch
> 
> See patch 10. 
> 

Well than this is just ugly this way.

Christian Borntraeger Jan. 25, 2018, 2:45 p.m. UTC | #4

On 01/25/2018 03:42 PM, David Hildenbrand wrote:
> On 25.01.2018 15:32, Christian Borntraeger wrote:
>>
>>
>> On 01/25/2018 03:20 PM, David Hildenbrand wrote:
>>
>>>> +void kvm_s390_gisa_init(struct kvm *kvm)
>>>> +{
>>>> +	if (1 || !css_general_characteristics.aiv)
>>>> +		kvm->arch.gisa = NULL;
>>>
>>> 1 || ... ? This will always trigger. -> gisa never active with this patch
>>
>> See patch 10. 
>>
> 
> Well than this is just ugly this way.

Reshuffling this is hard due to code dependencies. I can try to minimize this.
The alternative is to merge patches 6,7,8,9 and 10, but this split makes it
easier to review the parts.

Cornelia Huck Jan. 25, 2018, 3:05 p.m. UTC | #5

On Thu, 25 Jan 2018 15:45:14 +0100
Christian Borntraeger <[e-mail]> wrote:

> On 01/25/2018 03:42 PM, David Hildenbrand wrote:
> > On 25.01.2018 15:32, Christian Borntraeger wrote:  
> >>
> >>
> >> On 01/25/2018 03:20 PM, David Hildenbrand wrote:
> >>  
> >>>> +void kvm_s390_gisa_init(struct kvm *kvm)
> >>>> +{
> >>>> +	if (1 || !css_general_characteristics.aiv)
> >>>> +		kvm->arch.gisa = NULL;  
> >>>
> >>> 1 || ... ? This will always trigger. -> gisa never active with this patch  
> >>
> >> See patch 10. 
> >>  
> > 
> > Well than this is just ugly this way.  
> 
> Reshuffling this is hard due to code dependencies. I can try to minimize this.
> The alternative is to merge patches 6,7,8,9 and 10, but this split makes it
> easier to review the parts. 
> 

if (1 /* disabled for now */ || !css_general_characteristics.aiv)

?

I dunno, I'm not really bothered by this.

David Hildenbrand Jan. 25, 2018, 3:27 p.m. UTC | #6

On 25.01.2018 16:05, Cornelia Huck wrote:
> On Thu, 25 Jan 2018 15:45:14 +0100
> Christian Borntraeger <[e-mail]> wrote:
> 
>> On 01/25/2018 03:42 PM, David Hildenbrand wrote:
>>> On 25.01.2018 15:32, Christian Borntraeger wrote:  
>>>>
>>>>
>>>> On 01/25/2018 03:20 PM, David Hildenbrand wrote:
>>>>  
>>>>>> +void kvm_s390_gisa_init(struct kvm *kvm)
>>>>>> +{
>>>>>> +	if (1 || !css_general_characteristics.aiv)
>>>>>> +		kvm->arch.gisa = NULL;  
>>>>>
>>>>> 1 || ... ? This will always trigger. -> gisa never active with this patch  
>>>>
>>>> See patch 10. 
>>>>  
>>>
>>> Well than this is just ugly this way.  
>>
>> Reshuffling this is hard due to code dependencies. I can try to minimize this.
>> The alternative is to merge patches 6,7,8,9 and 10, but this split makes it
>> easier to review the parts. 
>>
> 
> if (1 /* disabled for now */ || !css_general_characteristics.aiv)
> 
> ?
> 
> I dunno, I'm not really bothered by this.
> 

Or add the enabling code when you can enable it?

Christian Borntraeger Jan. 25, 2018, 3:31 p.m. UTC | #7

On 01/25/2018 04:27 PM, David Hildenbrand wrote:
> On 25.01.2018 16:05, Cornelia Huck wrote:
>> On Thu, 25 Jan 2018 15:45:14 +0100
>> Christian Borntraeger <[e-mail]> wrote:
>>
>>> On 01/25/2018 03:42 PM, David Hildenbrand wrote:
>>>> On 25.01.2018 15:32, Christian Borntraeger wrote:  
>>>>>
>>>>>
>>>>> On 01/25/2018 03:20 PM, David Hildenbrand wrote:
>>>>>  
>>>>>>> +void kvm_s390_gisa_init(struct kvm *kvm)
>>>>>>> +{
>>>>>>> +	if (1 || !css_general_characteristics.aiv)
>>>>>>> +		kvm->arch.gisa = NULL;  
>>>>>>
>>>>>> 1 || ... ? This will always trigger. -> gisa never active with this patch  
>>>>>
>>>>> See patch 10. 
>>>>>  
>>>>
>>>> Well than this is just ugly this way.  
>>>
>>> Reshuffling this is hard due to code dependencies. I can try to minimize this.
>>> The alternative is to merge patches 6,7,8,9 and 10, but this split makes it
>>> easier to review the parts. 
>>>
>>
>> if (1 /* disabled for now */ || !css_general_characteristics.aiv)
>>
>> ?
>>
>> I dunno, I'm not really bothered by this.
>>
> 
> Or add the enabling code when you can enable it?

I will do that.

Christian Borntraeger Jan. 25, 2018, 4:32 p.m. UTC | #8

On 01/25/2018 03:20 PM, David Hildenbrand wrote:
[...]
>> @@ -918,18 +919,38 @@ static int __must_check __deliver_virtio(struct kvm_vcpu *vcpu)
>>  	return rc ? -EFAULT : 0;
>>  }
>>  
>> +static int __do_deliver_io(struct kvm_vcpu *vcpu, struct kvm_s390_io_info *io)
>> +{
>> +	int rc;
>> +
>> +	rc  = put_guest_lc(vcpu, io->subchannel_id, (u16 *)__LC_SUBCHANNEL_ID);
>> +	rc |= put_guest_lc(vcpu, io->subchannel_nr, (u16 *)__LC_SUBCHANNEL_NR);
>> +	rc |= put_guest_lc(vcpu, io->io_int_parm, (u32 *)__LC_IO_INT_PARM);
>> +	rc |= put_guest_lc(vcpu, io->io_int_word, (u32 *)__LC_IO_INT_WORD);
>> +	rc |= write_guest_lc(vcpu, __LC_IO_OLD_PSW,
>> +			     &vcpu->arch.sie_block->gpsw,
>> +			     sizeof(psw_t));
>> +	rc |= read_guest_lc(vcpu, __LC_IO_NEW_PSW,
>> +			    &vcpu->arch.sie_block->gpsw,
>> +			    sizeof(psw_t));
> 
> These should now it into less lines.

The last two lines are way beyond 80. 

Can you factor that change out into
> a separate patch?


Unless Conny agrees that this is absolutely mandatory I would like to avoid that.

> 
>> +	return rc ? -EFAULT : 0;
>> +}
>> +
>>  static int __must_check __deliver_io(struct kvm_vcpu *vcpu,
>>  				     unsigned long irq_type)
>>  {
>>  	struct list_head *isc_list;
>>  	struct kvm_s390_float_interrupt *fi;
>>  	struct kvm_s390_interrupt_info *inti = NULL;
>> +	struct kvm_s390_io_info io;
>> +	u32 isc;
>>  	int rc = 0;
>>  
>>  	fi = &vcpu->kvm->arch.float_int;
>>  
>>  	spin_lock(&fi->lock);
>> -	isc_list = &fi->lists[irq_type_to_isc(irq_type)];
>> +	isc = irq_type_to_isc(irq_type);
>> +	isc_list = &fi->lists[isc];
>>  	inti = list_first_entry_or_null(isc_list,
>>  					struct kvm_s390_interrupt_info,
>>  					list);
>> @@ -957,24 +978,32 @@ static int __must_check __deliver_io(struct kvm_vcpu *vcpu,
>>  	spin_unlock(&fi->lock);
>>  
>>  	if (inti) {
>> -		rc  = put_guest_lc(vcpu, inti->io.subchannel_id,
>> -				(u16 *)__LC_SUBCHANNEL_ID);
>> -		rc |= put_guest_lc(vcpu, inti->io.subchannel_nr,
>> -				(u16 *)__LC_SUBCHANNEL_NR);
>> -		rc |= put_guest_lc(vcpu, inti->io.io_int_parm,
>> -				(u32 *)__LC_IO_INT_PARM);
>> -		rc |= put_guest_lc(vcpu, inti->io.io_int_word,
>> -				(u32 *)__LC_IO_INT_WORD);
>> -		rc |= write_guest_lc(vcpu, __LC_IO_OLD_PSW,
>> -				&vcpu->arch.sie_block->gpsw,
>> -				sizeof(psw_t));
>> -		rc |= read_guest_lc(vcpu, __LC_IO_NEW_PSW,
>> -				&vcpu->arch.sie_block->gpsw,
>> -				sizeof(psw_t));
>> +		rc = __do_deliver_io(vcpu, &(inti->io));
>>  		kfree(inti);
>> +		goto out;
>>  	}
>>  
>> -	return rc ? -EFAULT : 0;
>> +	if (vcpu->kvm->arch.gisa) {
>> +		if (kvm_s390_gisa_tac_ipm_gisc(vcpu->kvm->arch.gisa, isc)) {
> 
> 
> if (vcpu->kvm->arch.gisa &&
>     kvm_s390_gisa_tac_ipm_gisc(vcpu->kvm->arch.gisa, isc) {
> 
> avoids one nesting level

agreed.


> 
>> +			/*
>> +			 * in case an adapter interrupt was not delivered
>> +			 * in SIE context KVM will handle the delivery
>> +			 */
>> +			VCPU_EVENT(vcpu, 4, "%s isc %u", "deliver: I/O (AI/gisa)", isc);
>> +			memset(&io, 0, sizeof(io));
>> +			io.io_int_word = (isc << 27) | 0x80000000;
>> +			vcpu->stat.deliver_io_int++;
>> +			trace_kvm_s390_deliver_interrupt(vcpu->vcpu_id,
>> +				KVM_S390_INT_IO(1, 0, 0, 0),
>> +				((__u32)io.subchannel_id << 16) |
>> +				io.subchannel_nr,
>> +				((__u64)io.io_int_parm << 32) |
>> +				io.io_int_word);
>> +			rc = __do_deliver_io(vcpu, &io);
>> +		}
>> +	}
>> +out:
>> +	return rc;
>>  }
>>  
>>  typedef int (*deliver_irq_t)(struct kvm_vcpu *vcpu);
>> @@ -1537,12 +1566,23 @@ static int __inject_io(struct kvm *kvm, struct kvm_s390_interrupt_info *inti)
>>  	struct kvm_s390_float_interrupt *fi;
>>  	struct list_head *list;
>>  	int isc;
>> +	int rc = 0;
>> +
>> +	isc = int_word_to_isc(inti->io.io_int_word);
>> +
>> +	if (kvm->arch.gisa && inti->type & KVM_S390_INT_IO_AI_MASK) {
> 
> && (inti->type & KVM_S390_INT_IO_AI_MASK)) {

not necessary, & binds stronger than &&. So why?



> 
>> +		VM_EVENT(kvm, 4, "%s isc %1u", "inject: I/O (AI/gisa)", isc);
>> +		kvm_s390_gisa_set_ipm_gisc(kvm->arch.gisa, isc);
> 
> Can there only be one pending at a time? (e.g. what happens if the bit
> is already set?)

Yes there can be only one per ISC and the multiplexing is done on device
specific summary and queue indicator bits. (e.g. look at the virtio-ccw 
stuff)


> 
>> +		kfree(inti);
>> +		goto out;
> 
> Why not simply return 0? ...
> 
>> +	}
>>  
>>  	fi = &kvm->arch.float_int;
>>  	spin_lock(&fi->lock);
>>  	if (fi->counters[FIRQ_CNTR_IO] >= KVM_S390_MAX_FLOAT_IRQS) {
>>  		spin_unlock(&fi->lock);
>> -		return -EBUSY;
>> +		rc = -EBUSY;
>> +		goto out;
> 
> ... avoids this change ...
> 
>>  	}
>>  	fi->counters[FIRQ_CNTR_IO] += 1;
>>  
>> @@ -1553,12 +1593,12 @@ static int __inject_io(struct kvm *kvm, struct kvm_s390_interrupt_info *inti)
>>  			inti->io.subchannel_id >> 8,
>>  			inti->io.subchannel_id >> 1 & 0x3,
>>  			inti->io.subchannel_nr);
>> -	isc = int_word_to_isc(inti->io.io_int_word);
>>  	list = &fi->lists[FIRQ_LIST_IO_ISC_0 + isc];
>>  	list_add_tail(&inti->list, list);
>>  	set_bit(isc_to_irq_type(isc), &fi->pending_irqs);
>>  	spin_unlock(&fi->lock);
>> -	return 0;
>> +out:
>> +	return rc;
> 
> ... and this.
> 
>>  }

Will double check but at the moment I agree to 3 comments.





>>  
>>  /*
>> @@ -2705,3 +2745,29 @@ int kvm_s390_get_irq_state(struct kvm_vcpu *vcpu, __u8 __user *buf, int len)
>>  	return n;
>>  }
>>  
>> +void kvm_s390_gisa_clear(struct kvm *kvm)
> 
> can we instead call this "gisa_setup" or move all that directly into the
> init function? (because it essentially inits the gisa)

It is also called in other places (kvm_s390_clear_float_irqs)

> 
>> +{
>> +	if (kvm->arch.gisa) {
> 
> This check is not needed: guaranteed to be set by the caller.

Huh? kvm->arch.gisa can be NULL, also called by kvm_s390_clear_float_irqs
> 
>> +		memset(kvm->arch.gisa, 0, sizeof(struct kvm_s390_gisa));



>> +
>> +void kvm_s390_gisa_destroy(struct kvm *kvm)
>> +{
>> +	if (!kvm->arch.gisa)
>> +		return;
>> +	kvm->arch.gisa = NULL;
> 
> You can do this unconditionally. Nevertheless, this function is not
> really useful: only called when we destroy the VM.

It can be useful for later support of passthrough, so I would like to
keep it as counter part of gisa_init

Cornelia Huck Jan. 25, 2018, 4:39 p.m. UTC | #9

On Thu, 25 Jan 2018 17:32:29 +0100
Christian Borntraeger <[e-mail]> wrote:

> On 01/25/2018 03:20 PM, David Hildenbrand wrote:
> [...]
> >> @@ -918,18 +919,38 @@ static int __must_check __deliver_virtio(struct kvm_vcpu *vcpu)
> >>  	return rc ? -EFAULT : 0;
> >>  }
> >>  
> >> +static int __do_deliver_io(struct kvm_vcpu *vcpu, struct kvm_s390_io_info *io)
> >> +{
> >> +	int rc;
> >> +
> >> +	rc  = put_guest_lc(vcpu, io->subchannel_id, (u16 *)__LC_SUBCHANNEL_ID);
> >> +	rc |= put_guest_lc(vcpu, io->subchannel_nr, (u16 *)__LC_SUBCHANNEL_NR);
> >> +	rc |= put_guest_lc(vcpu, io->io_int_parm, (u32 *)__LC_IO_INT_PARM);
> >> +	rc |= put_guest_lc(vcpu, io->io_int_word, (u32 *)__LC_IO_INT_WORD);
> >> +	rc |= write_guest_lc(vcpu, __LC_IO_OLD_PSW,
> >> +			     &vcpu->arch.sie_block->gpsw,
> >> +			     sizeof(psw_t));
> >> +	rc |= read_guest_lc(vcpu, __LC_IO_NEW_PSW,
> >> +			    &vcpu->arch.sie_block->gpsw,
> >> +			    sizeof(psw_t));  
> > 
> > These should now it into less lines.  
> 
> The last two lines are way beyond 80. 
> 
> Can you factor that change out into
> > a separate patch?  
> 
> 
> Unless Conny agrees that this is absolutely mandatory I would like to avoid that.

I don't think factoring this out would be very useful.

David Hildenbrand Jan. 25, 2018, 4:47 p.m. UTC | #10

On 25.01.2018 17:32, Christian Borntraeger wrote:
> 
> 
> On 01/25/2018 03:20 PM, David Hildenbrand wrote:
> [...]
>>> @@ -918,18 +919,38 @@ static int __must_check __deliver_virtio(struct kvm_vcpu *vcpu)
>>>  	return rc ? -EFAULT : 0;
>>>  }
>>>  
>>> +static int __do_deliver_io(struct kvm_vcpu *vcpu, struct kvm_s390_io_info *io)
>>> +{
>>> +	int rc;
>>> +
>>> +	rc  = put_guest_lc(vcpu, io->subchannel_id, (u16 *)__LC_SUBCHANNEL_ID);
>>> +	rc |= put_guest_lc(vcpu, io->subchannel_nr, (u16 *)__LC_SUBCHANNEL_NR);
>>> +	rc |= put_guest_lc(vcpu, io->io_int_parm, (u32 *)__LC_IO_INT_PARM);
>>> +	rc |= put_guest_lc(vcpu, io->io_int_word, (u32 *)__LC_IO_INT_WORD);
>>> +	rc |= write_guest_lc(vcpu, __LC_IO_OLD_PSW,
>>> +			     &vcpu->arch.sie_block->gpsw,
>>> +			     sizeof(psw_t));
>>> +	rc |= read_guest_lc(vcpu, __LC_IO_NEW_PSW,
>>> +			    &vcpu->arch.sie_block->gpsw,
>>> +			    sizeof(psw_t));
>>
>> These should now it into less lines.
> 
> The last two lines are way beyond 80. 
> 

Huh? At least in my world, I can reduce 3 to 2 lines (for both).

> Can you factor that change out into
>> a separate patch?
> 
> 
[...]

>>
>>> +			/*
>>> +			 * in case an adapter interrupt was not delivered
>>> +			 * in SIE context KVM will handle the delivery
>>> +			 */
>>> +			VCPU_EVENT(vcpu, 4, "%s isc %u", "deliver: I/O (AI/gisa)", isc);
>>> +			memset(&io, 0, sizeof(io));
>>> +			io.io_int_word = (isc << 27) | 0x80000000;
>>> +			vcpu->stat.deliver_io_int++;
>>> +			trace_kvm_s390_deliver_interrupt(vcpu->vcpu_id,
>>> +				KVM_S390_INT_IO(1, 0, 0, 0),
>>> +				((__u32)io.subchannel_id << 16) |
>>> +				io.subchannel_nr,
>>> +				((__u64)io.io_int_parm << 32) |
>>> +				io.io_int_word);
>>> +			rc = __do_deliver_io(vcpu, &io);
>>> +		}
>>> +	}
>>> +out:
>>> +	return rc;
>>>  }
>>>  
>>>  typedef int (*deliver_irq_t)(struct kvm_vcpu *vcpu);
>>> @@ -1537,12 +1566,23 @@ static int __inject_io(struct kvm *kvm, struct kvm_s390_interrupt_info *inti)
>>>  	struct kvm_s390_float_interrupt *fi;
>>>  	struct list_head *list;
>>>  	int isc;
>>> +	int rc = 0;
>>> +
>>> +	isc = int_word_to_isc(inti->io.io_int_word);
>>> +
>>> +	if (kvm->arch.gisa && inti->type & KVM_S390_INT_IO_AI_MASK) {
>>
>> && (inti->type & KVM_S390_INT_IO_AI_MASK)) {
> 
> not necessary, & binds stronger than &&. So why?

You're right, I thought we usually do that because of "suggest
parentheses around comparison in operand of ..." warnings from GCC. But
it only seems to apply when using e.g. == / !=.

> 
> 
> 
>>
>>> +		VM_EVENT(kvm, 4, "%s isc %1u", "inject: I/O (AI/gisa)", isc);
>>> +		kvm_s390_gisa_set_ipm_gisc(kvm->arch.gisa, isc);
>>
>> Can there only be one pending at a time? (e.g. what happens if the bit
>> is already set?)
> 
> Yes there can be only one per ISC and the multiplexing is done on device
> specific summary and queue indicator bits. (e.g. look at the virtio-ccw 
> stuff)
> 
[...]

> 
>>>  
>>>  /*
>>> @@ -2705,3 +2745,29 @@ int kvm_s390_get_irq_state(struct kvm_vcpu *vcpu, __u8 __user *buf, int len)
>>>  	return n;
>>>  }
>>>  
>>> +void kvm_s390_gisa_clear(struct kvm *kvm)
>>
>> can we instead call this "gisa_setup" or move all that directly into the
>> init function? (because it essentially inits the gisa)
> 
> It is also called in other places (kvm_s390_clear_float_irqs)
> 
>>
>>> +{
>>> +	if (kvm->arch.gisa) {
>>
>> This check is not needed: guaranteed to be set by the caller.
> 
> Huh? kvm->arch.gisa can be NULL, also called by kvm_s390_clear_float_irqs

Okay, not clear when reviewing this patch on its own. But maybe just I
find the order in which things are added challenging to review :)

>>
>>> +		memset(kvm->arch.gisa, 0, sizeof(struct kvm_s390_gisa));
> 
> 
> 
>>> +
>>> +void kvm_s390_gisa_destroy(struct kvm *kvm)
>>> +{
>>> +	if (!kvm->arch.gisa)
>>> +		return;
>>> +	kvm->arch.gisa = NULL;
>>
>> You can do this unconditionally. Nevertheless, this function is not
>> really useful: only called when we destroy the VM.
> 
> It can be useful for later support of passthrough, so I would like to
> keep it as counter part of gisa_init

Makes sense.

Christian Borntraeger Jan. 25, 2018, 4:50 p.m. UTC | #11

On 01/25/2018 05:47 PM, David Hildenbrand wrote:
> On 25.01.2018 17:32, Christian Borntraeger wrote:
>>
>>
>> On 01/25/2018 03:20 PM, David Hildenbrand wrote:
>> [...]
>>>> @@ -918,18 +919,38 @@ static int __must_check __deliver_virtio(struct kvm_vcpu *vcpu)
>>>>  	return rc ? -EFAULT : 0;
>>>>  }
>>>>  
>>>> +static int __do_deliver_io(struct kvm_vcpu *vcpu, struct kvm_s390_io_info *io)
>>>> +{
>>>> +	int rc;
>>>> +
>>>> +	rc  = put_guest_lc(vcpu, io->subchannel_id, (u16 *)__LC_SUBCHANNEL_ID);
>>>> +	rc |= put_guest_lc(vcpu, io->subchannel_nr, (u16 *)__LC_SUBCHANNEL_NR);
>>>> +	rc |= put_guest_lc(vcpu, io->io_int_parm, (u32 *)__LC_IO_INT_PARM);
>>>> +	rc |= put_guest_lc(vcpu, io->io_int_word, (u32 *)__LC_IO_INT_WORD);
>>>> +	rc |= write_guest_lc(vcpu, __LC_IO_OLD_PSW,
>>>> +			     &vcpu->arch.sie_block->gpsw,
>>>> +			     sizeof(psw_t));
>>>> +	rc |= read_guest_lc(vcpu, __LC_IO_NEW_PSW,
>>>> +			    &vcpu->arch.sie_block->gpsw,
>>>> +			    sizeof(psw_t));
>>>
>>> These should now it into less lines.
>>
>> The last two lines are way beyond 80. 
>>
> 
> Huh? At least in my world, I can reduce 3 to 2 lines (for both).

Yes, but if I have to break the line, then I prefer to have one parameter per line. (all or
nothing)

Patch

diff --git a/arch/s390/include/asm/kvm_host.h b/arch/s390/include/asm/kvm_host.h
index 77acade..6802d5d 100644
--- a/arch/s390/include/asm/kvm_host.h
+++ b/arch/s390/include/asm/kvm_host.h
@@ -772,6 +772,7 @@  struct kvm_arch{
 	struct kvm_s390_migration_state *migration_state;
 	/* 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;
 };
 
 #define KVM_HVA_ERR_BAD		(-1UL)
diff --git a/arch/s390/kvm/interrupt.c b/arch/s390/kvm/interrupt.c
index d559776..263cd10 100644
--- a/arch/s390/kvm/interrupt.c
+++ b/arch/s390/kvm/interrupt.c
@@ -228,7 +228,8 @@  static inline int kvm_s390_gisa_tac_ipm_gisc(struct kvm_s390_gisa *gisa, u32 gis
 static inline unsigned long pending_irqs(struct kvm_vcpu *vcpu)
 {
 	return vcpu->kvm->arch.float_int.pending_irqs |
-	       vcpu->arch.local_int.pending_irqs;
+		vcpu->arch.local_int.pending_irqs |
+		kvm_s390_gisa_get_ipm(vcpu->kvm->arch.gisa) << IRQ_PEND_IO_ISC_7;
 }
 
 static inline int isc_to_irq_type(unsigned long isc)
@@ -918,18 +919,38 @@  static int __must_check __deliver_virtio(struct kvm_vcpu *vcpu)
 	return rc ? -EFAULT : 0;
 }
 
+static int __do_deliver_io(struct kvm_vcpu *vcpu, struct kvm_s390_io_info *io)
+{
+	int rc;
+
+	rc  = put_guest_lc(vcpu, io->subchannel_id, (u16 *)__LC_SUBCHANNEL_ID);
+	rc |= put_guest_lc(vcpu, io->subchannel_nr, (u16 *)__LC_SUBCHANNEL_NR);
+	rc |= put_guest_lc(vcpu, io->io_int_parm, (u32 *)__LC_IO_INT_PARM);
+	rc |= put_guest_lc(vcpu, io->io_int_word, (u32 *)__LC_IO_INT_WORD);
+	rc |= write_guest_lc(vcpu, __LC_IO_OLD_PSW,
+			     &vcpu->arch.sie_block->gpsw,
+			     sizeof(psw_t));
+	rc |= read_guest_lc(vcpu, __LC_IO_NEW_PSW,
+			    &vcpu->arch.sie_block->gpsw,
+			    sizeof(psw_t));
+	return rc ? -EFAULT : 0;
+}
+
 static int __must_check __deliver_io(struct kvm_vcpu *vcpu,
 				     unsigned long irq_type)
 {
 	struct list_head *isc_list;
 	struct kvm_s390_float_interrupt *fi;
 	struct kvm_s390_interrupt_info *inti = NULL;
+	struct kvm_s390_io_info io;
+	u32 isc;
 	int rc = 0;
 
 	fi = &vcpu->kvm->arch.float_int;
 
 	spin_lock(&fi->lock);
-	isc_list = &fi->lists[irq_type_to_isc(irq_type)];
+	isc = irq_type_to_isc(irq_type);
+	isc_list = &fi->lists[isc];
 	inti = list_first_entry_or_null(isc_list,
 					struct kvm_s390_interrupt_info,
 					list);
@@ -957,24 +978,32 @@  static int __must_check __deliver_io(struct kvm_vcpu *vcpu,
 	spin_unlock(&fi->lock);
 
 	if (inti) {
-		rc  = put_guest_lc(vcpu, inti->io.subchannel_id,
-				(u16 *)__LC_SUBCHANNEL_ID);
-		rc |= put_guest_lc(vcpu, inti->io.subchannel_nr,
-				(u16 *)__LC_SUBCHANNEL_NR);
-		rc |= put_guest_lc(vcpu, inti->io.io_int_parm,
-				(u32 *)__LC_IO_INT_PARM);
-		rc |= put_guest_lc(vcpu, inti->io.io_int_word,
-				(u32 *)__LC_IO_INT_WORD);
-		rc |= write_guest_lc(vcpu, __LC_IO_OLD_PSW,
-				&vcpu->arch.sie_block->gpsw,
-				sizeof(psw_t));
-		rc |= read_guest_lc(vcpu, __LC_IO_NEW_PSW,
-				&vcpu->arch.sie_block->gpsw,
-				sizeof(psw_t));
+		rc = __do_deliver_io(vcpu, &(inti->io));
 		kfree(inti);
+		goto out;
 	}
 
-	return rc ? -EFAULT : 0;
+	if (vcpu->kvm->arch.gisa) {
+		if (kvm_s390_gisa_tac_ipm_gisc(vcpu->kvm->arch.gisa, isc)) {
+			/*
+			 * in case an adapter interrupt was not delivered
+			 * in SIE context KVM will handle the delivery
+			 */
+			VCPU_EVENT(vcpu, 4, "%s isc %u", "deliver: I/O (AI/gisa)", isc);
+			memset(&io, 0, sizeof(io));
+			io.io_int_word = (isc << 27) | 0x80000000;
+			vcpu->stat.deliver_io_int++;
+			trace_kvm_s390_deliver_interrupt(vcpu->vcpu_id,
+				KVM_S390_INT_IO(1, 0, 0, 0),
+				((__u32)io.subchannel_id << 16) |
+				io.subchannel_nr,
+				((__u64)io.io_int_parm << 32) |
+				io.io_int_word);
+			rc = __do_deliver_io(vcpu, &io);
+		}
+	}
+out:
+	return rc;
 }
 
 typedef int (*deliver_irq_t)(struct kvm_vcpu *vcpu);
@@ -1537,12 +1566,23 @@  static int __inject_io(struct kvm *kvm, struct kvm_s390_interrupt_info *inti)
 	struct kvm_s390_float_interrupt *fi;
 	struct list_head *list;
 	int isc;
+	int rc = 0;
+
+	isc = int_word_to_isc(inti->io.io_int_word);
+
+	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);
+		kfree(inti);
+		goto out;
+	}
 
 	fi = &kvm->arch.float_int;
 	spin_lock(&fi->lock);
 	if (fi->counters[FIRQ_CNTR_IO] >= KVM_S390_MAX_FLOAT_IRQS) {
 		spin_unlock(&fi->lock);
-		return -EBUSY;
+		rc = -EBUSY;
+		goto out;
 	}
 	fi->counters[FIRQ_CNTR_IO] += 1;
 
@@ -1553,12 +1593,12 @@  static int __inject_io(struct kvm *kvm, struct kvm_s390_interrupt_info *inti)
 			inti->io.subchannel_id >> 8,
 			inti->io.subchannel_id >> 1 & 0x3,
 			inti->io.subchannel_nr);
-	isc = int_word_to_isc(inti->io.io_int_word);
 	list = &fi->lists[FIRQ_LIST_IO_ISC_0 + isc];
 	list_add_tail(&inti->list, list);
 	set_bit(isc_to_irq_type(isc), &fi->pending_irqs);
 	spin_unlock(&fi->lock);
-	return 0;
+out:
+	return rc;
 }
 
 /*
@@ -2705,3 +2745,29 @@  int kvm_s390_get_irq_state(struct kvm_vcpu *vcpu, __u8 __user *buf, int len)
 	return n;
 }
 
+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);
+	}
+}
+
+void kvm_s390_gisa_init(struct kvm *kvm)
+{
+	if (1 || !css_general_characteristics.aiv)
+		kvm->arch.gisa = NULL;
+	else {
+		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)
+{
+	if (!kvm->arch.gisa)
+		return;
+	kvm->arch.gisa = NULL;
+}
diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
index 48f0099..68d7eef 100644
--- a/arch/s390/kvm/kvm-s390.c
+++ b/arch/s390/kvm/kvm-s390.c
@@ -1986,6 +1986,7 @@  int kvm_arch_init_vm(struct kvm *kvm, unsigned long type)
 
 	spin_lock_init(&kvm->arch.start_stop_lock);
 	kvm_s390_vsie_init(kvm);
+	kvm_s390_gisa_init(kvm);
 	KVM_EVENT(3, "vm 0x%pK created by pid %u", kvm, current->pid);
 
 	return 0;
@@ -2048,6 +2049,7 @@  void kvm_arch_destroy_vm(struct kvm *kvm)
 	kvm_free_vcpus(kvm);
 	sca_dispose(kvm);
 	debug_unregister(kvm->arch.dbf);
+	kvm_s390_gisa_destroy(kvm);
 	free_page((unsigned long)kvm->arch.sie_page2);
 	if (!kvm_is_ucontrol(kvm))
 		gmap_remove(kvm->arch.gmap);
@@ -2458,6 +2460,11 @@  int kvm_arch_vcpu_setup(struct kvm_vcpu *vcpu)
 	if (test_kvm_facility(vcpu->kvm, 139))
 		vcpu->arch.sie_block->ecd |= ECD_MEF;
 
+	if (vcpu->arch.sie_block->gd) {
+		vcpu->arch.sie_block->eca |= ECA_AIV;
+		VCPU_EVENT(vcpu, 3, "AIV gisa format-%u enabled for cpu %03u",
+			   vcpu->arch.sie_block->gd & 0x3, vcpu->vcpu_id);
+	}
 	vcpu->arch.sie_block->sdnxo = ((unsigned long) &vcpu->run->s.regs.sdnx)
 					| SDNXC;
 	vcpu->arch.sie_block->riccbd = (unsigned long) &vcpu->run->s.regs.riccb;
@@ -2510,6 +2517,7 @@  struct kvm_vcpu *kvm_arch_vcpu_create(struct kvm *kvm,
 
 	vcpu->arch.sie_block->icpua = id;
 	spin_lock_init(&vcpu->arch.local_int.lock);
+	vcpu->arch.sie_block->gd = (u32)(u64)kvm->arch.gisa;
 	seqcount_init(&vcpu->arch.cputm_seqcount);
 
 	rc = kvm_vcpu_init(vcpu, kvm, id);
diff --git a/arch/s390/kvm/kvm-s390.h b/arch/s390/kvm/kvm-s390.h
index 8877116..05269a9 100644
--- a/arch/s390/kvm/kvm-s390.h
+++ b/arch/s390/kvm/kvm-s390.h
@@ -367,6 +367,9 @@  int kvm_s390_set_irq_state(struct kvm_vcpu *vcpu,
 			   void __user *buf, int len);
 int kvm_s390_get_irq_state(struct kvm_vcpu *vcpu,
 			   __u8 __user *buf, int len);
+void kvm_s390_gisa_init(struct kvm *kvm);
+void kvm_s390_gisa_clear(struct kvm *kvm);
+void kvm_s390_gisa_destroy(struct kvm *kvm);
 
 /* implemented in guestdbg.c */
 void kvm_s390_backup_guest_per_regs(struct kvm_vcpu *vcpu);