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

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

On 10/17/2025 10:33 AM, Michael Kelley wrote:
> From: Nuno Das Neves <nunodasneves@linux.microsoft.com> Sent: Friday, October 17, 2025 10:26 AM
>>
>> 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.
> 
> OK, thanks. You might want to tweak the commit message a bit
> to clarify that this really is a bug fix and why. I was leaning
> toward the wrong conclusion based on the current commit
> message.
> 

Is this clearer?

"
mshv: Fix deposit memory in MSHV_ROOT_HVCALL

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. The expectation is that the VMM will retry.

However, some VMM code in the wild doesn't do this and simply fails.
Rather than force the VMM to retry, change the ioctl to deposit
memory on demand and immediately retry the hypercall as is done with
all the other hypercall helper functions.

In addition to making the ioctl easier to use, removing the need for
multiple syscalls improves performance.

There is a complication: unlike the other hypercall helper functions,
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.
"

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