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.
> -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.
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
