From mboxrd@z Thu Jan 1 00:00:00 1970 From: Michael Mueller Subject: Re: [PATCH 04/12] KVM: s390: implement GISA IPM related primitives Date: Thu, 18 Jan 2018 15:29:17 +0100 Message-ID: References: <20180116200217.211897-1-borntraeger@de.ibm.com> <20180116200217.211897-5-borntraeger@de.ibm.com> <249b3566-b8f3-5825-1f2b-6e0662874ca0@redhat.com> Reply-To: mimu@linux.vnet.ibm.com Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit Cc: KVM , linux-s390 , Janosch Frank To: David Hildenbrand , Christian Borntraeger , Cornelia Huck Return-path: Received: from mx0b-001b2d01.pphosted.com ([148.163.158.5]:39938 "EHLO mx0a-001b2d01.pphosted.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1756503AbeARO3Y (ORCPT ); Thu, 18 Jan 2018 09:29:24 -0500 Received: from pps.filterd (m0098417.ppops.net [127.0.0.1]) by mx0a-001b2d01.pphosted.com (8.16.0.22/8.16.0.22) with SMTP id w0IEPBLI136682 for ; Thu, 18 Jan 2018 09:29:23 -0500 Received: from e06smtp14.uk.ibm.com (e06smtp14.uk.ibm.com [195.75.94.110]) by mx0a-001b2d01.pphosted.com with ESMTP id 2fjw6jgp5p-1 (version=TLSv1.2 cipher=AES256-SHA bits=256 verify=NOT) for ; Thu, 18 Jan 2018 09:29:22 -0500 Received: from localhost by e06smtp14.uk.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Thu, 18 Jan 2018 14:29:20 -0000 In-Reply-To: <249b3566-b8f3-5825-1f2b-6e0662874ca0@redhat.com> Content-Language: en-US Sender: kvm-owner@vger.kernel.org List-ID: On 17.01.18 15:35, David Hildenbrand wrote: > On 16.01.2018 21:02, Christian Borntraeger wrote: >> From: Michael Mueller >> >> 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 >> Reviewed-by: Pierre Morel >> Reviewed-by: Halil Pasic >> Reviewed-by: Christian Borntraeger >> Signed-off-by: Christian Borntraeger >> --- >> 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 >