| Message ID | 20230223162236.51569-1-nrb@linux.ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v1] KVM: s390: interrupt: fix virtual-physical confusion for next alert GISA | expand |
On 2/23/23 17:22, Nico Boehr wrote: > We sometimes put a virtual address in next_alert, which should always be > a physical address, since it is shared with hardware. > > This currently works, because virtual and physical addresses are > the same. I'd replace that with something like: The gisa next alert address is defined as a host absolute address so let's use virt_to_phys() to make sure we always write an absolute address to this hardware structure. This is not a bug since we're currently still running as a virtual == physical kernel but plan to move away from that. > > Add phys_to_virt() to resolve the virtual-physical confusion. Reviewed-by: Janosch Frank <frankja@linux.ibm.com> > > Signed-off-by: Nico Boehr <nrb@linux.ibm.com> > --- > arch/s390/kvm/interrupt.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/arch/s390/kvm/interrupt.c b/arch/s390/kvm/interrupt.c > index ab26aa53ee37..20743c5b000a 100644 > --- a/arch/s390/kvm/interrupt.c > +++ b/arch/s390/kvm/interrupt.c > @@ -305,7 +305,7 @@ static inline u8 gisa_get_ipm_or_restore_iam(struct kvm_s390_gisa_interrupt *gi) > > static inline int gisa_in_alert_list(struct kvm_s390_gisa *gisa) > { > - return READ_ONCE(gisa->next_alert) != (u32)(u64)gisa; > + return READ_ONCE(gisa->next_alert) != (u32)virt_to_phys(gisa); > } > > static inline void gisa_set_ipm_gisc(struct kvm_s390_gisa *gisa, u32 gisc) > @@ -3167,7 +3167,7 @@ void kvm_s390_gisa_init(struct kvm *kvm) > hrtimer_init(&gi->timer, CLOCK_MONOTONIC, HRTIMER_MODE_REL); > gi->timer.function = gisa_vcpu_kicker; > memset(gi->origin, 0, sizeof(struct kvm_s390_gisa)); > - gi->origin->next_alert = (u32)(u64)gi->origin; > + gi->origin->next_alert = (u32)virt_to_phys(gi->origin); > VM_EVENT(kvm, 3, "gisa 0x%pK initialized", gi->origin); > } >
On 23.02.23 17:22, Nico Boehr wrote: > We sometimes put a virtual address in next_alert, which should always be > a physical address, since it is shared with hardware. > > This currently works, because virtual and physical addresses are > the same. > > Add phys_to_virt() to resolve the virtual-physical confusion. > > Signed-off-by: Nico Boehr <nrb@linux.ibm.com> > --- > arch/s390/kvm/interrupt.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/arch/s390/kvm/interrupt.c b/arch/s390/kvm/interrupt.c > index ab26aa53ee37..20743c5b000a 100644 > --- a/arch/s390/kvm/interrupt.c > +++ b/arch/s390/kvm/interrupt.c > @@ -305,7 +305,7 @@ static inline u8 gisa_get_ipm_or_restore_iam(struct kvm_s390_gisa_interrupt *gi) > > static inline int gisa_in_alert_list(struct kvm_s390_gisa *gisa) > { > - return READ_ONCE(gisa->next_alert) != (u32)(u64)gisa; > + return READ_ONCE(gisa->next_alert) != (u32)virt_to_phys(gisa); > } > > static inline void gisa_set_ipm_gisc(struct kvm_s390_gisa *gisa, u32 gisc) > @@ -3167,7 +3167,7 @@ void kvm_s390_gisa_init(struct kvm *kvm) > hrtimer_init(&gi->timer, CLOCK_MONOTONIC, HRTIMER_MODE_REL); > gi->timer.function = gisa_vcpu_kicker; > memset(gi->origin, 0, sizeof(struct kvm_s390_gisa)); > - gi->origin->next_alert = (u32)(u64)gi->origin; > + gi->origin->next_alert = (u32)virt_to_phys(gi->origin); > VM_EVENT(kvm, 3, "gisa 0x%pK initialized", gi->origin); > } > Here is my Signed-off-by: Michael Mueller <mimu@linux.ibm.com> I ran hades tests as well. Thanks.
On 3/7/23 14:53, Michael Mueller wrote: > > > On 23.02.23 17:22, Nico Boehr wrote: >> We sometimes put a virtual address in next_alert, which should always be >> a physical address, since it is shared with hardware. >> >> This currently works, because virtual and physical addresses are >> the same. >> >> Add phys_to_virt() to resolve the virtual-physical confusion. >> >> Signed-off-by: Nico Boehr <nrb@linux.ibm.com> >> --- >> arch/s390/kvm/interrupt.c | 4 ++-- >> 1 file changed, 2 insertions(+), 2 deletions(-) >> >> diff --git a/arch/s390/kvm/interrupt.c b/arch/s390/kvm/interrupt.c >> index ab26aa53ee37..20743c5b000a 100644 >> --- a/arch/s390/kvm/interrupt.c >> +++ b/arch/s390/kvm/interrupt.c >> @@ -305,7 +305,7 @@ static inline u8 gisa_get_ipm_or_restore_iam(struct kvm_s390_gisa_interrupt *gi) >> >> static inline int gisa_in_alert_list(struct kvm_s390_gisa *gisa) >> { >> - return READ_ONCE(gisa->next_alert) != (u32)(u64)gisa; >> + return READ_ONCE(gisa->next_alert) != (u32)virt_to_phys(gisa); >> } >> >> static inline void gisa_set_ipm_gisc(struct kvm_s390_gisa *gisa, u32 gisc) >> @@ -3167,7 +3167,7 @@ void kvm_s390_gisa_init(struct kvm *kvm) >> hrtimer_init(&gi->timer, CLOCK_MONOTONIC, HRTIMER_MODE_REL); >> gi->timer.function = gisa_vcpu_kicker; >> memset(gi->origin, 0, sizeof(struct kvm_s390_gisa)); >> - gi->origin->next_alert = (u32)(u64)gi->origin; >> + gi->origin->next_alert = (u32)virt_to_phys(gi->origin); >> VM_EVENT(kvm, 3, "gisa 0x%pK initialized", gi->origin); >> } >> > > Here is my > > Signed-off-by: Michael Mueller <mimu@linux.ibm.com> After a consultation with Michael I'm now a 100% sure this is a rev-by and not a s-o-b. I've picked the patch with his rev-by.
On 23.02.23 17:22, Nico Boehr wrote: > We sometimes put a virtual address in next_alert, which should always be > a physical address, since it is shared with hardware. > > This currently works, because virtual and physical addresses are > the same. > > Add phys_to_virt() to resolve the virtual-physical confusion. > > Signed-off-by: Nico Boehr <nrb@linux.ibm.com> > --- > arch/s390/kvm/interrupt.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/arch/s390/kvm/interrupt.c b/arch/s390/kvm/interrupt.c > index ab26aa53ee37..20743c5b000a 100644 > --- a/arch/s390/kvm/interrupt.c > +++ b/arch/s390/kvm/interrupt.c > @@ -305,7 +305,7 @@ static inline u8 gisa_get_ipm_or_restore_iam(struct kvm_s390_gisa_interrupt *gi) > > static inline int gisa_in_alert_list(struct kvm_s390_gisa *gisa) > { > - return READ_ONCE(gisa->next_alert) != (u32)(u64)gisa; > + return READ_ONCE(gisa->next_alert) != (u32)virt_to_phys(gisa); > } > > static inline void gisa_set_ipm_gisc(struct kvm_s390_gisa *gisa, u32 gisc) > @@ -3167,7 +3167,7 @@ void kvm_s390_gisa_init(struct kvm *kvm) > hrtimer_init(&gi->timer, CLOCK_MONOTONIC, HRTIMER_MODE_REL); > gi->timer.function = gisa_vcpu_kicker; > memset(gi->origin, 0, sizeof(struct kvm_s390_gisa)); > - gi->origin->next_alert = (u32)(u64)gi->origin; > + gi->origin->next_alert = (u32)virt_to_phys(gi->origin); > VM_EVENT(kvm, 3, "gisa 0x%pK initialized", gi->origin); > } > I ran hades tests as well. Thanks. Here is my Reviewed-by: Michael Mueller <mimu@linux.ibm.com>
diff --git a/arch/s390/kvm/interrupt.c b/arch/s390/kvm/interrupt.c index ab26aa53ee37..20743c5b000a 100644 --- a/arch/s390/kvm/interrupt.c +++ b/arch/s390/kvm/interrupt.c @@ -305,7 +305,7 @@ static inline u8 gisa_get_ipm_or_restore_iam(struct kvm_s390_gisa_interrupt *gi) static inline int gisa_in_alert_list(struct kvm_s390_gisa *gisa) { - return READ_ONCE(gisa->next_alert) != (u32)(u64)gisa; + return READ_ONCE(gisa->next_alert) != (u32)virt_to_phys(gisa); } static inline void gisa_set_ipm_gisc(struct kvm_s390_gisa *gisa, u32 gisc) @@ -3167,7 +3167,7 @@ void kvm_s390_gisa_init(struct kvm *kvm) hrtimer_init(&gi->timer, CLOCK_MONOTONIC, HRTIMER_MODE_REL); gi->timer.function = gisa_vcpu_kicker; memset(gi->origin, 0, sizeof(struct kvm_s390_gisa)); - gi->origin->next_alert = (u32)(u64)gi->origin; + gi->origin->next_alert = (u32)virt_to_phys(gi->origin); VM_EVENT(kvm, 3, "gisa 0x%pK initialized", gi->origin); }
We sometimes put a virtual address in next_alert, which should always be a physical address, since it is shared with hardware. This currently works, because virtual and physical addresses are the same. Add phys_to_virt() to resolve the virtual-physical confusion. Signed-off-by: Nico Boehr <nrb@linux.ibm.com> --- arch/s390/kvm/interrupt.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)