Re: [PATCH] mshv: Fix deposit memory in MSHV_ROOT_HVCALL

[Nuno Das Neves <nunodasneves@linux.microsoft.com>]

On 10/16/2025 6:12 PM, Michael Kelley wrote:
> From: Nuno Das Neves <nunodasneves@linux.microsoft.com> Sent: Thursday, October 16, 2025 12:54 PM
>>
>> When the MSHV_ROOT_HVCALL ioctl is executing a hypercall, and gets
>> HV_STATUS_INSUFFICIENT_MEMORY, it deposits memory and then returns
>> -EAGAIN to userspace.
>>
>> However, it's much easier and efficient if the driver simply deposits
>> memory on demand and immediately retries the hypercall as is done with
>> all the other hypercall helper functions.
>>
>> But unlike those, in MSHV_ROOT_HVCALL the input is opaque to the
>> kernel. This is problematic for rep hypercalls, because the next part
>> of the input list can't be copied on each loop after depositing pages
>> (this was the original reason for returning -EAGAIN in this case).
>>
>> Introduce hv_do_rep_hypercall_ex(), which adds a 'rep_start'
>> parameter. This solves the issue, allowing the deposit loop in
>> MSHV_ROOT_HVCALL to restart a rep hypercall after depositing pages
>> partway through.
> 
>>From reading the above, I'm pretty sure this code change is an
> optimization that lets user space avoid having to deal with the
> -EAGAIN result by resubmitting the ioctl with a different
> starting point for a rep hypercall. As such, I'd suggest the patch
> title should be "Improve deposit memory ...." (or something similar).
> The word "Fix" makes it sound like a bug fix.
> 
> Or is user space code currently faulty in its handling of -EAGAIN, and
> this really is an indirect bug fix to make things work? If so, do you
> want a Fixes: tag so the change is backported?
> 

It's the latter case, userspace doesn't handle it correctly, so I
consider it a fix more than just an improvement.

I'll add a Fixes: tag pointing back to the original /dev/mshv patch.

>>
>> Signed-off-by: Nuno Das Neves <nunodasneves@linux.microsoft.com>
>> ---
>>  drivers/hv/mshv_root_main.c    | 52 ++++++++++++++++++++--------------
>>  include/asm-generic/mshyperv.h | 14 +++++++--
>>  2 files changed, 42 insertions(+), 24 deletions(-)
>>
>> diff --git a/drivers/hv/mshv_root_main.c b/drivers/hv/mshv_root_main.c
>> index 9ae67c6e9f60..731ec8cbbd63 100644
>> --- a/drivers/hv/mshv_root_main.c
>> +++ b/drivers/hv/mshv_root_main.c
>> @@ -159,6 +159,7 @@ static int mshv_ioctl_passthru_hvcall(struct mshv_partition *partition,
>>  	unsigned int pages_order;
>>  	void *input_pg = NULL;
>>  	void *output_pg = NULL;
>> +	u16 reps_completed;
>>
>>  	if (copy_from_user(&args, user_args, sizeof(args)))
>>  		return -EFAULT;
>> @@ -210,28 +211,35 @@ static int mshv_ioctl_passthru_hvcall(struct mshv_partition *partition,
>>  	 */
>>  	*(u64 *)input_pg = partition->pt_id;
>>
>> -	if (args.reps)
>> -		status = hv_do_rep_hypercall(args.code, args.reps, 0,
>> -					     input_pg, output_pg);
>> -	else
>> -		status = hv_do_hypercall(args.code, input_pg, output_pg);
>> -
>> -	if (hv_result(status) == HV_STATUS_CALL_PENDING) {
>> -		if (is_async) {
>> -			mshv_async_hvcall_handler(partition, &status);
>> -		} else { /* Paranoia check. This shouldn't happen! */
>> -			ret = -EBADFD;
>> -			goto free_pages_out;
>> +	reps_completed = 0;
>> +	do {
>> +		if (args.reps) {
>> +			status = hv_do_rep_hypercall_ex(args.code, args.reps,
>> +							0, reps_completed,
>> +							input_pg, output_pg);
>> +			reps_completed = hv_repcomp(status);
>> +		} else {
>> +			status = hv_do_hypercall(args.code, input_pg, output_pg);
>>  		}
>> -	}
>>
>> -	if (hv_result(status) == HV_STATUS_INSUFFICIENT_MEMORY) {
>> -		ret = hv_call_deposit_pages(NUMA_NO_NODE, partition->pt_id, 1);
>> -		if (!ret)
>> -			ret = -EAGAIN;
>> -	} else if (!hv_result_success(status)) {
>> -		ret = hv_result_to_errno(status);
>> -	}
>> +		if (hv_result(status) == HV_STATUS_CALL_PENDING) {
>> +			if (is_async) {
>> +				mshv_async_hvcall_handler(partition, &status);
>> +			} else { /* Paranoia check. This shouldn't happen! */
>> +				ret = -EBADFD;
>> +				goto free_pages_out;
>> +			}
>> +		}
>> +
>> +		if (hv_result_success(status))
>> +			break;
>> +
>> +		if (hv_result(status) != HV_STATUS_INSUFFICIENT_MEMORY)
>> +			ret = hv_result_to_errno(status);
>> +		else
>> +			ret = hv_call_deposit_pages(NUMA_NO_NODE,
>> +						    partition->pt_id, 1);
>> +	} while (!ret);
>>
>>  	/*
>>  	 * Always return the status and output data regardless of result.
> 
> This comment about always returning the output data is now incorrect.
> 

Thanks, I'll fix it

>> @@ -240,11 +248,11 @@ static int mshv_ioctl_passthru_hvcall(struct mshv_partition *partition,
>>  	 * succeeded.
>>  	 */
>>  	args.status = hv_result(status);
>> -	args.reps = args.reps ? hv_repcomp(status) : 0;
>> +	args.reps = reps_completed;
>>  	if (copy_to_user(user_args, &args, sizeof(args)))
>>  		ret = -EFAULT;
>>
>> -	if (output_pg &&
>> +	if (!ret && output_pg &&
>>  	    copy_to_user((void __user *)args.out_ptr, output_pg, args.out_sz))
>>  		ret = -EFAULT;
>>
>> diff --git a/include/asm-generic/mshyperv.h b/include/asm-generic/mshyperv.h
>> index ebf458dbcf84..31a209f0e18f 100644
>> --- a/include/asm-generic/mshyperv.h
>> +++ b/include/asm-generic/mshyperv.h
>> @@ -128,8 +128,9 @@ static inline unsigned int hv_repcomp(u64 status)
>>   * Rep hypercalls. Callers of this functions are supposed to ensure that
>>   * rep_count and varhead_size comply with Hyper-V hypercall definition.
> 
> Nit: This comment could be updated to include the new "rep_start"
> parameter.
> 

Thanks, will add

>>   */
>> -static inline u64 hv_do_rep_hypercall(u16 code, u16 rep_count, u16 varhead_size,
>> -				      void *input, void *output)
>> +static inline u64 hv_do_rep_hypercall_ex(u16 code, u16 rep_count,
>> +					 u16 varhead_size, u16 rep_start,
>> +					 void *input, void *output)
>>  {
>>  	u64 control = code;
>>  	u64 status;
>> @@ -137,6 +138,7 @@ static inline u64 hv_do_rep_hypercall(u16 code, u16 rep_count, u16 varhead_size,
>>
>>  	control |= (u64)varhead_size << HV_HYPERCALL_VARHEAD_OFFSET;
>>  	control |= (u64)rep_count << HV_HYPERCALL_REP_COMP_OFFSET;
>> +	control |= (u64)rep_start << HV_HYPERCALL_REP_START_OFFSET;
>>
>>  	do {
>>  		status = hv_do_hypercall(control, input, output);
>> @@ -154,6 +156,14 @@ static inline u64 hv_do_rep_hypercall(u16 code, u16 rep_count, u16 varhead_size,
>>  	return status;
>>  }
>>
>> +/* For the typical case where rep_start is 0 */
>> +static inline u64 hv_do_rep_hypercall(u16 code, u16 rep_count, u16 varhead_size,
>> +				      void *input, void *output)
>> +{
>> +	return hv_do_rep_hypercall_ex(code, rep_count, varhead_size, 0,
>> +				      input, output);
>> +}
>> +
>>  /* Generate the guest OS identifier as described in the Hyper-V TLFS */
>>  static inline u64 hv_generate_guest_id(u64 kernel_version)
>>  {
> 
> Overall, this looks good to me. I don't see any issues with the code.
> 
> Michael