Re: [GIT PULL 10/11] KVM: s390: handle pending local interrupts via bitmap

[Jens Freimann <jfrei@linux.vnet.ibm.com>]

Excerpts from Heiko Carstens's message of 2014-12-01 09:19:16 +0100:
> On Fri, Nov 28, 2014 at 02:25:38PM +0100, Christian Borntraeger wrote:
> > +static int __must_check __deliver_mchk_floating(struct kvm_vcpu *vcpu,
> > +                       struct kvm_s390_interrupt_info *inti)
> > +{
> > +    struct kvm_s390_mchk_info *mchk = &inti->mchk;
> > +    int rc;
> > +
> > +    VCPU_EVENT(vcpu, 4, "interrupt: machine check mcic=%llx",
> > +           mchk->mcic);
> > +    trace_kvm_s390_deliver_interrupt(vcpu->vcpu_id, KVM_S390_MCHK,
> > +                     mchk->cr14, mchk->mcic);
> > +
> > +    rc  = kvm_s390_vcpu_store_status(vcpu, KVM_S390_STORE_STATUS_PREFIXED);
> > +    rc |= put_guest_lc(vcpu, mchk->mcic,
> > +            (u64 __user *) __LC_MCCK_CODE);
> > +    rc |= put_guest_lc(vcpu, mchk->failing_storage_address,
> > +            (u64 __user *) __LC_MCCK_FAIL_STOR_ADDR);
> > +    rc |= write_guest_lc(vcpu, __LC_PSW_SAVE_AREA,
> > +                 &mchk->fixed_logout, sizeof(mchk->fixed_logout));
> > +    rc |= write_guest_lc(vcpu, __LC_MCK_OLD_PSW,
> > +                 &vcpu->arch.sie_block->gpsw, sizeof(psw_t));
> > +    rc |= read_guest_lc(vcpu, __LC_MCK_NEW_PSW,
> > +                &vcpu->arch.sie_block->gpsw, sizeof(psw_t));
> > +    return rc;
> > +}
> 
> FWIW, rc handling seems to be a bit fragile.
> The usual return statement for such a pattern is
>     return rc ? -EWHATEVER : 0;
> so we don't get random or'ed return values.

Ok, I'll change this to return -EFAULT (need to double check) if rc is set.

> 
> > -static int __inject_prog_irq(struct kvm_vcpu *vcpu,
> > -                 struct kvm_s390_interrupt_info *inti)
> > +static int __inject_prog(struct kvm_vcpu *vcpu, struct kvm_s390_irq *irq)
> >  {
> >      struct kvm_s390_local_interrupt *li = &vcpu->arch.local_int;
> >  
> > -    list_add(&inti->list, &li->list);
> > -    atomic_set(&li->active, 1);
> > +    li->irq.pgm = irq->u.pgm;
> > +    __set_bit(IRQ_PEND_PROG, &li->pending_irqs);
> 
>         ^^^^^^^^^
> 
> > +static int __inject_pfault_init(struct kvm_vcpu *vcpu, struct kvm_s390_irq *irq)
> >  {
> >      struct kvm_s390_local_interrupt *li = &vcpu->arch.local_int;
> >  
> > -    inti->ext.ext_params2 = s390int->parm64;
> > -    list_add_tail(&inti->list, &li->list);
> > -    atomic_set(&li->active, 1);
> > +    VCPU_EVENT(vcpu, 3, "inject: external irq params:%x, params2:%llx",
> > +           irq->u.ext.ext_params, irq->u.ext.ext_params2);
> > +    trace_kvm_s390_inject_vcpu(vcpu->vcpu_id, KVM_S390_INT_PFAULT_INIT,
> > +                   irq->u.ext.ext_params,
> > +                   irq->u.ext.ext_params2, 2);
> > +
> > +    li->irq.ext = irq->u.ext;
> > +    set_bit(IRQ_PEND_PFAULT_INIT, &li->pending_irqs);
> 
>         ^^^^^^^
> 
> You're using atomic and non-atomic bitops all over the place on the same
> object(s). It would be very good to have some consistency here.
> And as far as I remember the non-atomic variant is good enough anyway.

I think you are right. The non-atomic bitops are sufficient here. I'll fix this.

Paolo, is a follow-up patch good enough or should we fix this one?


regards
Jens