Re: [GIT PULL 10/11] KVM: s390: handle pending local interrupts via bitmap
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 majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [GIT PULL 10/11] KVM: s390: handle pending local interrupts via bitmap
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
--
To unsubscribe from this list: send the line unsubscribe kvm in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
