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

[Claudio Imbrenda <imbrenda@linux.ibm.com>]

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