Re: [PATCH 04/12] KVM: s390: implement GISA IPM related primitives

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 





On 17.01.18 15:35, David Hildenbrand wrote:
On 16.01.2018 21:02, Christian Borntraeger wrote:
From: Michael Mueller <mimu@xxxxxxxxxxxxxxxxxx>

The patch implements routines to access the GISA to test and modify
its Interruption Pending Mask (IPM) from the host side.

Signed-off-by: Michael Mueller <mimu@xxxxxxxxxxxxxxxxxx>
Reviewed-by: Pierre Morel <pmorel@xxxxxxxxxxxxxxxxxx>
Reviewed-by: Halil Pasic <pasic@xxxxxxxxxxxxxxxxxx>
Reviewed-by: Christian Borntraeger <borntraeger@xxxxxxxxxx>
Signed-off-by: Christian Borntraeger <borntraeger@xxxxxxxxxx>
---
  arch/s390/kvm/interrupt.c | 23 +++++++++++++++++++++++
  arch/s390/kvm/kvm-s390.h  |  4 ++++
  2 files changed, 27 insertions(+)

diff --git a/arch/s390/kvm/interrupt.c b/arch/s390/kvm/interrupt.c
index b94173560dcf..dfdecff302d2 100644
--- a/arch/s390/kvm/interrupt.c
+++ b/arch/s390/kvm/interrupt.c
@@ -2682,3 +2682,26 @@ int kvm_s390_get_irq_state(struct kvm_vcpu *vcpu, __u8 __user *buf, int len)
return n;
  }
+
+#define IPM_BIT_OFFSET (offsetof(struct kvm_s390_gisa, ipm) * 8)

8 -> BITS_PER_BYTE, but ...

Am I wrong or can we only modify the 8 ipm bits this way? But we
want/have to do it in an atomic fashion?

Using an unsigned long seems wrong, because we "rewrite" more than we
should. Esp. everything beyond ipm. oi / ni and friends are not
available on older machines.

What about something as simple as the following


+void kvm_s390_gisa_set_ipm_gisc(struct kvm_s390_gisa *gisa, u8 gisc)
+{
+       u8 value = (0x80 >> gisc);
+
+       __sync_fetch_and_or(&gisa->ipm, value);
+}
+


Nobody is using this compiler build-in in the kernel and a quick compile shows that it produces an ORK and CS instead of a LAOG beside a lot of supporting instructions. Unfortunately it's not saving what you promise ... ;) Will not change.


+
+void kvm_s390_gisa_set_ipm_gisc(struct kvm_s390_gisa *gisa, u32 gisc)
+{
+	set_bit_inv(IPM_BIT_OFFSET + gisc, (unsigned long *) gisa);
+}
+
+u8 kvm_s390_gisa_get_ipm(struct kvm_s390_gisa *gisa)
+{
+	return (u8) READ_ONCE(gisa->ipm);
+}
+
+void kvm_s390_gisa_clear_ipm_gisc(struct kvm_s390_gisa *gisa, u32 gisc)
+{
+	clear_bit_inv(IPM_BIT_OFFSET + gisc, (unsigned long *) gisa);
+}
+
+int kvm_s390_gisa_tac_ipm_gisc(struct kvm_s390_gisa *gisa, u32 gisc)
+{
+	return test_and_clear_bit_inv(IPM_BIT_OFFSET + gisc, (unsigned long *) gisa);
+}
+

shouldn't these be static inline instead?

Well, I get requests in both directions for that... my opinion is also yes and I will change it a very last time!


diff --git a/arch/s390/kvm/kvm-s390.h b/arch/s390/kvm/kvm-s390.h
index 8877116f0159..b17e4dea7ea5 100644
--- a/arch/s390/kvm/kvm-s390.h
+++ b/arch/s390/kvm/kvm-s390.h
@@ -367,6 +367,10 @@ int kvm_s390_set_irq_state(struct kvm_vcpu *vcpu,
  			   void __user *buf, int len);
  int kvm_s390_get_irq_state(struct kvm_vcpu *vcpu,
  			   __u8 __user *buf, int len);
+void kvm_s390_gisa_set_ipm_gisc(struct kvm_s390_gisa *gisa, u32 gisc);
+u8 kvm_s390_gisa_get_ipm(struct kvm_s390_gisa *gisa);
+void kvm_s390_gisa_clear_ipm_gisc(struct kvm_s390_gisa *gisa, u32 gisc);
+int kvm_s390_gisa_tac_ipm_gisc(struct kvm_s390_gisa *gisa, u32 gisc);
/* implemented in guestdbg.c */
  void kvm_s390_backup_guest_per_regs(struct kvm_vcpu *vcpu);



Thanks a lot for your comments David.

Michael



--
To unsubscribe from this list: send the line "unsubscribe linux-s390" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html



[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
[Index of Archives]     [Kernel Development]     [Kernel Newbies]     [IDE]     [Security]     [Git]     [Netfilter]     [Bugtraq]     [Yosemite Info]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux ATA RAID]     [Samba]     [Linux Media]     [Device Mapper]

  Powered by Linux