Re: [PATCH v1] KVM: s390: pv: remove misleading gaddr output from KVM_PV_DUMP_CONFIG_STOR_STATE

[Christian Borntraeger <borntraeger@linux.ibm.com>]

Am 28.10.26 um 12:16 schrieb Gautam Gala:
> A comment in kvm_s390_pv_dmp() suggested gaddr is an output parameter
> for KVM_PV_DUMP_CONFIG_STOR_STATE, and kvm_s390_pv_dump_stor_state()
> wrote back uvcb.gaddr through the pointer accordingly.
> 
> However, api.rst does not document that KVM_PV_DUMP_CONFIG_STOR_STATE
> writes back dump.gaddr and QEMU does not use it. Furthermore, the
> write-back would not have reached userspace in any case: dmp was passed
> by value to kvm_s390_pv_dmp() so copy_to_user() in the caller always
> copied the unmodified input gaddr, not the updated one.
> 
> Drop the write-back and the misleading comment, and change the gaddr
> parameter of kvm_s390_pv_dump_stor_state() to u64 to reflect its
> input-only role.
> 
> Fixes: 0460eb35b443 ("KVM: s390: Add configuration dump functionality")

I will remove this, no need to backport.

> Reported-by: Christian Borntraeger <borntraeger@linux.ibm.com>
> Signed-off-by: Gautam Gala <ggala@linux.ibm.com>
> Reviewed-by: Janosch Frank <frankja@linux.ibm.com>



Reviewed-by: Christian Borntraeger <borntraeger@linux.ibm.com>


thanks applied.
> ---
>   arch/s390/kvm/s390/pv.c   | 16 ++++++----------
>   arch/s390/kvm/s390/s390.c |  7 +------
>   arch/s390/kvm/s390/s390.h |  2 +-
>   3 files changed, 8 insertions(+), 17 deletions(-)
> 
> diff --git a/arch/s390/kvm/s390/pv.c b/arch/s390/kvm/s390/pv.c
> index b18abd0e29ef..a9bc32d5be6f 100644
> --- a/arch/s390/kvm/s390/pv.c
> +++ b/arch/s390/kvm/s390/pv.c
> @@ -901,10 +901,9 @@ int kvm_s390_pv_dump_cpu(struct kvm_vcpu *vcpu, void *buff, u16 *rc, u16 *rrc)
>    *
>    * Stores buff_len bytes of tweak component values to buff_user
>    * starting with the 1MB block specified by the absolute guest address
> - * (gaddr). The gaddr pointer will be updated with the last address
> - * for which data was written when returning to userspace. buff_user
> - * might be written to even if an error rc is returned. For instance
> - * if we encounter a fault after writing the first page of data.
> + * (gaddr). buff_user might be written to even if an error rc is
> + * returned. For instance if we encounter a fault after writing the
> + * first page of data.
>    *
>    * Context: kvm->lock needs to be held
>    *
> @@ -917,13 +916,13 @@ int kvm_s390_pv_dump_cpu(struct kvm_vcpu *vcpu, void *buff, u16 *rc, u16 *rrc)
>    *  -EFAULT if copying the result to buff_user failed
>    */
>   int kvm_s390_pv_dump_stor_state(struct kvm *kvm, void __user *buff_user,
> -				u64 *gaddr, u64 buff_user_len, u16 *rc, u16 *rrc)
> +				u64 gaddr, u64 buff_user_len, u16 *rc, u16 *rrc)
>   {
>   	struct uv_cb_dump_stor_state uvcb = {
>   		.header.cmd = UVC_CMD_DUMP_CONF_STOR_STATE,
>   		.header.len = sizeof(uvcb),
>   		.config_handle = kvm->arch.pv.handle,
> -		.gaddr = *gaddr,
> +		.gaddr = gaddr,
>   		.dump_area_origin = 0,
>   	};
>   	const u64 increment_len = uv_info.conf_dump_storage_state_len;
> @@ -934,7 +933,7 @@ int kvm_s390_pv_dump_stor_state(struct kvm *kvm, void __user *buff_user,
>   
>   	ret = -EINVAL;
>   	/* UV call processes 1MB guest storage chunks at a time */
> -	if (!IS_ALIGNED(*gaddr, HPAGE_SIZE))
> +	if (!IS_ALIGNED(gaddr, HPAGE_SIZE))
>   		goto out;
>   
>   	/*
> @@ -989,9 +988,6 @@ int kvm_s390_pv_dump_stor_state(struct kvm *kvm, void __user *buff_user,
>   		}
>   	} while (buff_user_len);
>   
> -	/* Report back where we ended dumping */
> -	*gaddr = uvcb.gaddr;
> -
>   	/* Lets only log errors, we don't want to spam */
>   out:
>   	if (ret)
> diff --git a/arch/s390/kvm/s390/s390.c b/arch/s390/kvm/s390/s390.c
> index e4912db67adc..1f975ebeca82 100644
> --- a/arch/s390/kvm/s390/s390.c
> +++ b/arch/s390/kvm/s390/s390.c
> @@ -2522,12 +2522,7 @@ static int kvm_s390_pv_dmp(struct kvm *kvm, struct kvm_pv_cmd *cmd,
>   		if (!kvm->arch.pv.dumping)
>   			break;
>   
> -		/*
> -		 * gaddr is an output parameter since we might stop
> -		 * early. As dmp will be copied back in our caller, we
> -		 * don't need to do it ourselves.
> -		 */
> -		r = kvm_s390_pv_dump_stor_state(kvm, result_buff, &dmp.gaddr, dmp.buff_len,
> +		r = kvm_s390_pv_dump_stor_state(kvm, result_buff, dmp.gaddr, dmp.buff_len,
>   						&cmd->rc, &cmd->rrc);
>   		break;
>   	}
> diff --git a/arch/s390/kvm/s390/s390.h b/arch/s390/kvm/s390/s390.h
> index aa0d1d062f8d..5d6c36c6ba34 100644
> --- a/arch/s390/kvm/s390/s390.h
> +++ b/arch/s390/kvm/s390/s390.h
> @@ -316,7 +316,7 @@ int kvm_s390_pv_unpack(struct kvm *kvm, unsigned long addr, unsigned long size,
>   int kvm_s390_pv_set_cpu_state(struct kvm_vcpu *vcpu, u8 state);
>   int kvm_s390_pv_dump_cpu(struct kvm_vcpu *vcpu, void *buff, u16 *rc, u16 *rrc);
>   int kvm_s390_pv_dump_stor_state(struct kvm *kvm, void __user *buff_user,
> -				u64 *gaddr, u64 buff_user_len, u16 *rc, u16 *rrc);
> +				u64 gaddr, u64 buff_user_len, u16 *rc, u16 *rrc);
>   int kvm_s390_pv_dump_complete(struct kvm *kvm, void __user *buff_user,
>   			      u16 *rc, u16 *rrc);
>   int kvm_s390_pv_destroy_page(struct kvm *kvm, unsigned long gaddr);