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@xxxxxxxxxxxxx>
> > ---
> > Â 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 */