On 10/16/2025 6:12 PM, Michael Kelley wrote: > From: Nuno Das Neves <[email protected]> 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 <[email protected]> >> --- >> 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
