On 5/20/2025 6:50 AM, Paolo Bonzini wrote:
> On 5/20/25 13:30, Magnus Kulke wrote:
>> + int ret;
>> + hv_message exit_msg = { 0 };
>
> You probably don't want to fill 512 bytes on every vmentry. Maybe pass
> &exit_msg up from mshv_cpu_exec()?
>
>> + /*
>> + * Read cpu->exit_request before KVM_RUN reads run->immediate_exit.
>> + * Matching barrier in kvm_eat_signals.
>> + */
>> + smp_rmb();
>
> The comment is obviously wrong; unfortunately, the code is wrong too:
>
> 1) qemu_cpu_kick_self() is only needed for an old KVM API. In that API the
> signal handler is blocked while QEMU runs. In your case,
> qemu_cpu_kick_self() is an expensive way to do nothing.
>
> 2) Because of this, there's a race condition between delivering the signal
> and entering MSHV_RUN_VP
>
Hi Paolo,
I might be misunderstanding something here, but isn't there a race condition
regardless of where this check is made?
i.e., checking a flag in userspace, like the above:
if (qatomic_read(&cpu->exit_request)) {
vs checking the flag in the kernel, are effectively doing the same thing.
The signal can still come just after the check is made (in the kernel) and the
VP will dispatch.
The virtual "explicit suspend" register in the VP seems to solve this problem -
it can be used for manually kicking the VP
while it is running. But, it can also be set before dispatching the VP, and the
dispatch hypercall will return immediately
in that case.
Thanks
Nuno
> You need support in the hypervisor for this: KVM and HVF both have it.
>
> There are two ways to do it, for both cases the hypervisor side for the
> latter can be something like this:
>
> diff --git a/drivers/hv/mshv_root_main.c b/drivers/hv/mshv_root_main.c
> index 72df774e410a..627afece4046 100644
> --- a/drivers/hv/mshv_root_main.c
> +++ b/drivers/hv/mshv_root_main.c
> @@ -530,7 +530,7 @@ static long mshv_run_vp_with_root_scheduler(
> struct hv_output_dispatch_vp output;
>
> ret = mshv_pre_guest_mode_work(vp);
> - if (ret)
> + if (ret || vp->run.flags.immediate_exit)
> break;
>
> if (vp->run.flags.intercept_suspend)
> @@ -585,6 +585,7 @@
> }
> } while (!vp->run.flags.intercept_suspend);
>
> + vp->run.flags.immediate_exit = 0;
> return ret;
> }
>
>
> Instead of calling qemu_cpu_kick_self(), your signal handler would invoke a
> new MSHV ioctl that sets vp->run.flags.immediate_exit = 1.
>
> And then you also don't need the barrier, by the way, because all
> inter-thread communication is mediated by the signal handler.
>
> Paolo
>
>
>
