On Thu, 31 Jan 2019 09:52:43 +0100
Michael Mueller <mimu@xxxxxxxxxxxxx> wrote:
> Function kvm_s390_gisa_clear() now clears the Interruption
> Pending Mask of the GISA asap. If the GISA is in the alert
> list at this time it stays in the list but is removed by
> process_gib_alert_list().
>
> Signed-off-by: Michael Mueller <mimu@xxxxxxxxxxxxx>
> ---
> arch/s390/kvm/interrupt.c | 25 ++++++++++++++++++++++---
> 1 file changed, 22 insertions(+), 3 deletions(-)
>
> diff --git a/arch/s390/kvm/interrupt.c b/arch/s390/kvm/interrupt.c
> index f37dfb01c63c..341664b94491 100644
> --- a/arch/s390/kvm/interrupt.c
> +++ b/arch/s390/kvm/interrupt.c
> @@ -249,6 +249,25 @@ static inline int gisa_set_iam(struct kvm_s390_gisa *gisa, u8 iam)
> return 0;
> }
>
> +/**
> + * gisa_clear_ipm - clear the GISA interruption pending mask
> + *
> + * @gisa: gisa to operate on
> + *
> + * Clear the IPM atomically with the next alert address and the IAM
> + * of the GISA unconditionally. All three fields are located in the
> + * first long word of the GISA.
> + */
> +static inline void gisa_clear_ipm(struct kvm_s390_gisa *gisa)
> +{
> + u64 word, _word;
> +
> + do {
> + word = READ_ONCE(gisa->u64.word[0]);
> + _word = word & ~(0xffUL << 24);
> + } while (cmpxchg(&gisa->u64.word[0], word, _word) != word);
> +}
> +
> static inline void gisa_set_ipm_gisc(struct kvm_s390_gisa *gisa, u32 gisc)
> {
> set_bit_inv(IPM_BIT_OFFSET + gisc, (unsigned long *) gisa);
> @@ -2926,8 +2945,7 @@ void kvm_s390_gisa_clear(struct kvm *kvm)
>
> if (!gi->origin)
> return;
> - memset(gi->origin, 0, sizeof(struct kvm_s390_gisa));
> - gi->origin->next_alert = (u32)(u64)gi->origin;
> + gisa_clear_ipm(gi->origin);
> VM_EVENT(kvm, 3, "gisa 0x%pK cleared", gi->origin);
> }
I'm a bit confused. Now all kvm_s390_gisa_clear() does to the gisa is
gisa_clear_ipm(). The only usage I see is triggered by
KVM_DEV_FLIC_CLEAR_IRQS. And in that context this seems appropriate.
However gisa can hold other stuff than the ipm, and in that sense
the name kvm_s390_gisa_clear() may be misleading. I ask myself
questions like who would need to reset simm and nimm on system
reset. At the moment we don't use these AFAICT, so I guess it does not
matter.
Thus not a showstopper to me:
Acked-by: Halil Pasic <pasic@xxxxxxxxxxxxx>
>
> @@ -2940,7 +2958,8 @@ void kvm_s390_gisa_init(struct kvm *kvm)
> gi->origin = &kvm->arch.sie_page2->gisa;
> gi->alert.mask = 0;
> spin_lock_init(&gi->alert.ref_lock);
> - kvm_s390_gisa_clear(kvm);
> + memset(gi->origin, 0, sizeof(struct kvm_s390_gisa));
> + gi->origin->next_alert = (u32)(u64)gi->origin;
> VM_EVENT(kvm, 3, "gisa 0x%pK initialized", gi->origin);
> }
>
