| Message ID | 20180116200217.211897-5-borntraeger@de.ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 16.01.2018 21:02, Christian Borntraeger wrote: > From: Michael Mueller <mimu@linux.vnet.ibm.com> > > 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@linux.vnet.ibm.com> > Reviewed-by: Pierre Morel <pmorel@linux.vnet.ibm.com> > Reviewed-by: Halil Pasic <pasic@linux.vnet.ibm.com> > Reviewed-by: Christian Borntraeger <borntraeger@de.ibm.com> > Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com> > --- > 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); +} + > + > +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? > 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); >
On 17.01.18 15:35, David Hildenbrand wrote: > On 16.01.2018 21:02, Christian Borntraeger wrote: >> From: Michael Mueller <mimu@linux.vnet.ibm.com> >> >> 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@linux.vnet.ibm.com> >> Reviewed-by: Pierre Morel <pmorel@linux.vnet.ibm.com> >> Reviewed-by: Halil Pasic <pasic@linux.vnet.ibm.com> >> Reviewed-by: Christian Borntraeger <borntraeger@de.ibm.com> >> Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com> >> --- >> 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 >
On 18.01.2018 15:29, Michael Mueller wrote: > > > On 17.01.18 15:35, David Hildenbrand wrote: >> On 16.01.2018 21:02, Christian Borntraeger wrote: >>> From: Michael Mueller <mimu@linux.vnet.ibm.com> >>> >>> 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@linux.vnet.ibm.com> >>> Reviewed-by: Pierre Morel <pmorel@linux.vnet.ibm.com> >>> Reviewed-by: Halil Pasic <pasic@linux.vnet.ibm.com> >>> Reviewed-by: Christian Borntraeger <borntraeger@de.ibm.com> >>> Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com> >>> --- >>> 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. > Using unsigned long * bitmap operations to modify an u8 type atomically just seems very very wrong.
On 18.01.18 15:33, David Hildenbrand wrote: > On 18.01.2018 15:29, Michael Mueller wrote: >> >> >> On 17.01.18 15:35, David Hildenbrand wrote: >>> On 16.01.2018 21:02, Christian Borntraeger wrote: >>>> From: Michael Mueller <mimu@linux.vnet.ibm.com> >>>> >>>> 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@linux.vnet.ibm.com> >>>> Reviewed-by: Pierre Morel <pmorel@linux.vnet.ibm.com> >>>> Reviewed-by: Halil Pasic <pasic@linux.vnet.ibm.com> >>>> Reviewed-by: Christian Borntraeger <borntraeger@de.ibm.com> >>>> Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com> >>>> --- >>>> 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. >> > > Using unsigned long * bitmap operations to modify an u8 type atomically > just seems very very wrong > I have to reconsider this... >
On 18.01.2018 16:58, Michael Mueller wrote: > > > On 18.01.18 15:33, David Hildenbrand wrote: >> On 18.01.2018 15:29, Michael Mueller wrote: >>> >>> >>> On 17.01.18 15:35, David Hildenbrand wrote: >>>> On 16.01.2018 21:02, Christian Borntraeger wrote: >>>>> From: Michael Mueller <mimu@linux.vnet.ibm.com> >>>>> >>>>> 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@linux.vnet.ibm.com> >>>>> Reviewed-by: Pierre Morel <pmorel@linux.vnet.ibm.com> >>>>> Reviewed-by: Halil Pasic <pasic@linux.vnet.ibm.com> >>>>> Reviewed-by: Christian Borntraeger <borntraeger@de.ibm.com> >>>>> Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com> >>>>> --- >>>>> 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. >>> >> >> Using unsigned long * bitmap operations to modify an u8 type atomically >> just seems very very wrong > > > I have to reconsider this... > >> > Actually, the problem is there are no atomic byte-based operations on s390x without Interlocked-Access-Facility 2. Even __sync_fetch_and_or(&gisa->ipm, value) falls back to a Compare-And-Swap loop. And Compare-And-Swap also operates at least on 32bit. So I assume there isn't too much we can do about it. As storage locations following the u8 are also written - but in an atomic matter, it should in general not matter. But can we avoid starting the bitmap at the beginning of the gisa? What about something like this: +void kvm_s390_gisa_set_ipm_gisc(struct kvm_s390_gisa *gisa, u8 gisc) +{ + set_bit_inv(gisc, (unsigned long *) &gisa->ipm); +}
On Thu, Jan 18, 2018 at 09:45:03PM +0100, David Hildenbrand wrote: > Actually, the problem is there are no atomic byte-based operations on > s390x without Interlocked-Access-Facility 2. > > Even __sync_fetch_and_or(&gisa->ipm, value) falls back to a > Compare-And-Swap loop. And Compare-And-Swap also operates at least on 32bit. > > So I assume there isn't too much we can do about it. As storage > locations following the u8 are also written - but in an atomic matter, > it should in general not matter. > > But can we avoid starting the bitmap at the beginning of the gisa? > > What about something like this: > > +void kvm_s390_gisa_set_ipm_gisc(struct kvm_s390_gisa *gisa, u8 gisc) > +{ > + set_bit_inv(gisc, (unsigned long *) &gisa->ipm); > +} set_bit_inv() may use a csg instruction which requires an 8 byte alignment of the operand. What you propose would crash immediately. The code written by Michael is fine as-is.
On 19.01.2018 11:11, Heiko Carstens wrote: > On Thu, Jan 18, 2018 at 09:45:03PM +0100, David Hildenbrand wrote: >> Actually, the problem is there are no atomic byte-based operations on >> s390x without Interlocked-Access-Facility 2. >> >> Even __sync_fetch_and_or(&gisa->ipm, value) falls back to a >> Compare-And-Swap loop. And Compare-And-Swap also operates at least on 32bit. >> >> So I assume there isn't too much we can do about it. As storage >> locations following the u8 are also written - but in an atomic matter, >> it should in general not matter. >> >> But can we avoid starting the bitmap at the beginning of the gisa? >> >> What about something like this: >> >> +void kvm_s390_gisa_set_ipm_gisc(struct kvm_s390_gisa *gisa, u8 gisc) >> +{ >> + set_bit_inv(gisc, (unsigned long *) &gisa->ipm); >> +} > > set_bit_inv() may use a csg instruction which requires an 8 byte alignment > of the operand. What you propose would crash immediately. > > The code written by Michael is fine as-is. > That's unfortunate... and still looks hacky to me :) But if it works ...
On 01/19/2018 11:16 AM, David Hildenbrand wrote: > On 19.01.2018 11:11, Heiko Carstens wrote: >> On Thu, Jan 18, 2018 at 09:45:03PM +0100, David Hildenbrand wrote: >>> Actually, the problem is there are no atomic byte-based operations on >>> s390x without Interlocked-Access-Facility 2. >>> >>> Even __sync_fetch_and_or(&gisa->ipm, value) falls back to a >>> Compare-And-Swap loop. And Compare-And-Swap also operates at least on 32bit. >>> >>> So I assume there isn't too much we can do about it. As storage >>> locations following the u8 are also written - but in an atomic matter, >>> it should in general not matter. >>> >>> But can we avoid starting the bitmap at the beginning of the gisa? >>> >>> What about something like this: >>> >>> +void kvm_s390_gisa_set_ipm_gisc(struct kvm_s390_gisa *gisa, u8 gisc) >>> +{ >>> + set_bit_inv(gisc, (unsigned long *) &gisa->ipm); >>> +} >> >> set_bit_inv() may use a csg instruction which requires an 8 byte alignment >> of the operand. What you propose would crash immediately. >> >> The code written by Michael is fine as-is. >> > > That's unfortunate... and still looks hacky to me :) But if it works ... I had looked at the same place, but the alignment thing makes this really hard and I did not find a better solution.
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) + +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); +} + 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);