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

[Heiko Carstens <heiko.carstens@de.ibm.com>]

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.