[PATCH v2 0/1] KVM: s390: interrupt: fix virtual-physical confusion for next alert GISA

[PATCH v2 0/1] KVM: s390: interrupt: fix virtual-physical confusion for next alert GISA

[Nico Boehr]

v2:
---
* improve commit message (thanks Janosch)



Nico Boehr (1):
  KVM: s390: interrupt: fix virtual-physical confusion for next alert
    GISA

 arch/s390/kvm/interrupt.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

-- 
2.39.1

[PATCH v2 1/1] KVM: s390: interrupt: fix virtual-physical confusion for next alert GISA

[Nico Boehr]

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 and currently works, because virtual and physical
addresses are the same.

Signed-off-by: Nico Boehr <nrb@linux.ibm.com>
Reviewed-by: Janosch Frank <frankja@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);
 }
 
-- 
2.39.1

Re: [PATCH v2 1/1] KVM: s390: interrupt: fix virtual-physical confusion for next alert GISA

[Claudio Imbrenda]

On Fri, 24 Feb 2023 15:09:08 +0100
Nico Boehr <nrb@linux.ibm.com> wrote:

> 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 and currently works, because virtual and physical
> addresses are the same.
> 
> Signed-off-by: Nico Boehr <nrb@linux.ibm.com>
> Reviewed-by: Janosch Frank <frankja@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);

is gisa always allocated below 4G? (I assume 2G actually)
should we check if things are proper?
the cast to (u32) might hide bugs if gisa is above 4G (which it
shouldn't be, obviously)

or do we not care?

>  }
>  
>  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);

same here

>  	VM_EVENT(kvm, 3, "gisa 0x%pK initialized", gi->origin);
>  }
>  

Re: [PATCH v2 1/1] KVM: s390: interrupt: fix virtual-physical confusion for next alert GISA

[Nico Boehr]

Quoting Claudio Imbrenda (2023-02-28 18:16:33)
> On Fri, 24 Feb 2023 15:09:08 +0100
> Nico Boehr <nrb@linux.ibm.com> wrote:
> 
> > 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 and currently works, because virtual and physical
> > addresses are the same.
> > 
> > Signed-off-by: Nico Boehr <nrb@linux.ibm.com>
> > Reviewed-by: Janosch Frank <frankja@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);
> 
> is gisa always allocated below 4G? (I assume 2G actually)
>
> should we check if things are proper?
> the cast to (u32) might hide bugs if gisa is above 4G (which it
> shouldn't be, obviously)
> 
> or do we not care?

Yes, the gisa is always below 2 GB since it's part of the sie_page2.

I don't mind getting rid of the u32 cast really, but if it is allocated above 2 GB, it already is broken as it is and I didn't want to introduce unrelated changes. Also note that there is a few other places where we currently don't verify things really are below 2 GB, so you already need to be careful when allocating.

Also not sure if this is the right place to do this check, since we've already given the address to firmware/hardware anyways in the CHSC SGIB call, in the sie_block etc... so if we want to check this we should maybe look for a better place to check this...

Re: [PATCH v2 1/1] KVM: s390: interrupt: fix virtual-physical confusion for next alert GISA

[Claudio Imbrenda]

On Mon, 06 Mar 2023 11:50:32 +0100
Nico Boehr <nrb@linux.ibm.com> wrote:

> Quoting Claudio Imbrenda (2023-02-28 18:16:33)
> > On Fri, 24 Feb 2023 15:09:08 +0100
> > Nico Boehr <nrb@linux.ibm.com> wrote:
> >   
> > > 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 and currently works, because virtual and physical
> > > addresses are the same.
> > > 
> > > Signed-off-by: Nico Boehr <nrb@linux.ibm.com>
> > > Reviewed-by: Janosch Frank <frankja@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);  
> > 
> > is gisa always allocated below 4G? (I assume 2G actually)
> >
> > should we check if things are proper?
> > the cast to (u32) might hide bugs if gisa is above 4G (which it
> > shouldn't be, obviously)
> > 
> > or do we not care?  
> 
> Yes, the gisa is always below 2 GB since it's part of the sie_page2.
> 
> I don't mind getting rid of the u32 cast really, but if it is allocated above 2 GB, it already is broken as it is and I didn't want to introduce unrelated changes. Also note that there is a few other places where we currently don't verify things really are below 2 GB, so you already need to be careful when allocating.
> 
> Also not sure if this is the right place to do this check, since we've already given the address to firmware/hardware anyways in the CHSC SGIB call, in the sie_block etc... so if we want to check this we should maybe look for a better place to check this...

fair enough