On Sat, Apr 16, 2022 at 12:09:46PM -0400, Dave Voutila wrote:
> This tucks all the spinout paranoia behind `#ifdef MP_LOCKDEBUG` and
> uses the same approach used in amd64's pmap's TLB shootdown code.
>
> Part of me wants to remove this altogether, but I'm not sure it's
> outlived its usefulness quite yet.
>
> Three areas that busy wait on ipi's are modified:
>
> 1. vmm_start - performs ipi to enable vmm on all cpus
> 2. vmm_stop - performs ipi to disable vmm on all cpus
> 3. vmx_remote_vmclear - performs ipi to vmclear a cpu (only pertinent to
> Intel hosts)
>
> (3) is the most likely to spin out and prior to bumping the spinout to
> the current value (based on __mp_lock_spinout) we had reports from users
> of hitting it on slower/older MP hardware.
>
> For vmm_{start, stop}, I moved the current cpu start/stop routine to
> before performing the ipi broadcast because if we're going to fail to
> (dis)enable vmm we should fail fast. If we fail, there's no need to
> broadcast the ipi. This simplifies the code paths and removes a local
> variable.
>
> All three migrate to infinite busy waits and only have a spinout if
> built with MP_LOCKDEBUG. On a spinout, we enter ddb.
>
> Compiled on amd64 GENERIC, GENERIC.MP, and GENERIC.MP with
> MP_LOCKDEBUG. (This time I won't break GENERIC :)
>
> OK?
>
> -dv
Sorry for the delay. ok mlarkin. I've had this on a few machines for
the better part of a month and haven't seen any problems.
-ml
>
> Index: sys/arch/amd64/amd64/vmm.c
> ===================================================================
> RCS file: /opt/cvs/src/sys/arch/amd64/amd64/vmm.c,v
> retrieving revision 1.305
> diff -u -p -r1.305 vmm.c
> --- sys/arch/amd64/amd64/vmm.c 28 Mar 2022 06:28:47 -0000 1.305
> +++ sys/arch/amd64/amd64/vmm.c 16 Apr 2022 18:49:01 -0000
> @@ -43,6 +43,11 @@
> #include <dev/isa/isareg.h>
> #include <dev/pv/pvreg.h>
>
> +#ifdef MP_LOCKDEBUG
> +#include <ddb/db_output.h>
> +extern int __mp_lock_spinout;
> +#endif /* MP_LOCKDEBUG */
> +
> /* #define VMM_DEBUG */
>
> void *l1tf_flush_region;
> @@ -1328,17 +1333,26 @@ int
> vmm_start(void)
> {
> struct cpu_info *self = curcpu();
> - int ret = 0;
> #ifdef MULTIPROCESSOR
> struct cpu_info *ci;
> CPU_INFO_ITERATOR cii;
> - int i;
> -#endif
> +#ifdef MP_LOCKDEBUG
> + int nticks;
> +#endif /* MP_LOCKDEBUG */
> +#endif /* MULTIPROCESSOR */
>
> /* VMM is already running */
> if (self->ci_flags & CPUF_VMM)
> return (0);
>
> + /* Start VMM on this CPU */
> + start_vmm_on_cpu(self);
> + if (!(self->ci_flags & CPUF_VMM)) {
> + printf("%s: failed to enter VMM mode\n",
> + self->ci_dev->dv_xname);
> + return (EIO);
> + }
> +
> #ifdef MULTIPROCESSOR
> /* Broadcast start VMM IPI */
> x86_broadcast_ipi(X86_IPI_START_VMM);
> @@ -1346,25 +1360,23 @@ vmm_start(void)
> CPU_INFO_FOREACH(cii, ci) {
> if (ci == self)
> continue;
> - for (i = 100000; (!(ci->ci_flags & CPUF_VMM)) && i>0;i--)
> - delay(10);
> - if (!(ci->ci_flags & CPUF_VMM)) {
> - printf("%s: failed to enter VMM mode\n",
> - ci->ci_dev->dv_xname);
> - ret = EIO;
> +#ifdef MP_LOCKDEBUG
> + nticks = __mp_lock_spinout;
> +#endif /* MP_LOCKDEBUG */
> + while (!(ci->ci_flags & CPUF_VMM)) {
> + CPU_BUSY_CYCLE();
> +#ifdef MP_LOCKDEBUG
> + if (--nticks <= 0) {
> + db_printf("%s: spun out", __func__);
> + db_enter();
> + nticks = __mp_lock_spinout;
> + }
> +#endif /* MP_LOCKDEBUG */
> }
> }
> #endif /* MULTIPROCESSOR */
>
> - /* Start VMM on this CPU */
> - start_vmm_on_cpu(self);
> - if (!(self->ci_flags & CPUF_VMM)) {
> - printf("%s: failed to enter VMM mode\n",
> - self->ci_dev->dv_xname);
> - ret = EIO;
> - }
> -
> - return (ret);
> + return (0);
> }
>
> /*
> @@ -1376,17 +1388,26 @@ int
> vmm_stop(void)
> {
> struct cpu_info *self = curcpu();
> - int ret = 0;
> #ifdef MULTIPROCESSOR
> struct cpu_info *ci;
> CPU_INFO_ITERATOR cii;
> - int i;
> -#endif
> +#ifdef MP_LOCKDEBUG
> + int nticks;
> +#endif /* MP_LOCKDEBUG */
> +#endif /* MULTIPROCESSOR */
>
> /* VMM is not running */
> if (!(self->ci_flags & CPUF_VMM))
> return (0);
>
> + /* Stop VMM on this CPU */
> + stop_vmm_on_cpu(self);
> + if (self->ci_flags & CPUF_VMM) {
> + printf("%s: failed to exit VMM mode\n",
> + self->ci_dev->dv_xname);
> + return (EIO);
> + }
> +
> #ifdef MULTIPROCESSOR
> /* Stop VMM on other CPUs */
> x86_broadcast_ipi(X86_IPI_STOP_VMM);
> @@ -1394,25 +1415,23 @@ vmm_stop(void)
> CPU_INFO_FOREACH(cii, ci) {
> if (ci == self)
> continue;
> - for (i = 100000; (ci->ci_flags & CPUF_VMM) && i>0 ;i--)
> - delay(10);
> - if (ci->ci_flags & CPUF_VMM) {
> - printf("%s: failed to exit VMM mode\n",
> - ci->ci_dev->dv_xname);
> - ret = EIO;
> +#ifdef MP_LOCKDEBUG
> + nticks = __mp_lock_spinout;
> +#endif /* MP_LOCKDEBUG */
> + while ((ci->ci_flags & CPUF_VMM)) {
> + CPU_BUSY_CYCLE();
> +#ifdef MP_LOCKDEBUG
> + if (--nticks <= 0) {
> + db_printf("%s: spunout", __func__);
> + db_enter();
> + nticks = __mp_lock_spinout;
> + }
> +#endif /* MP_LOCKDEBUG */
> }
> }
> #endif /* MULTIPROCESSOR */
>
> - /* Stop VMM on this CPU */
> - stop_vmm_on_cpu(self);
> - if (self->ci_flags & CPUF_VMM) {
> - printf("%s: failed to exit VMM mode\n",
> - self->ci_dev->dv_xname);
> - ret = EIO;
> - }
> -
> - return (ret);
> + return (0);
> }
>
> /*
> @@ -1536,7 +1555,9 @@ vmclear_on_cpu(struct cpu_info *ci)
> static int
> vmx_remote_vmclear(struct cpu_info *ci, struct vcpu *vcpu)
> {
> - int ret = 0, nticks = 200000000;
> +#ifdef MP_LOCKDEBUG
> + int nticks = __mp_lock_spinout;
> +#endif /* MP_LOCKDEBUG */
>
> rw_enter_write(&ci->ci_vmcs_lock);
> atomic_swap_ulong(&ci->ci_vmcs_pa, vcpu->vc_control_pa);
> @@ -1544,16 +1565,18 @@ vmx_remote_vmclear(struct cpu_info *ci,
>
> while (ci->ci_vmcs_pa != VMX_VMCS_PA_CLEAR) {
> CPU_BUSY_CYCLE();
> +#ifdef MP_LOCKDEBUG
> if (--nticks <= 0) {
> - printf("%s: spun out\n", __func__);
> - ret = 1;
> - break;
> + db_printf("%s: spun out\n", __func__);
> + db_enter();
> + nticks = __mp_lock_spinout;
> }
> +#endif /* MP_LOCKDEBUG */
> }
> atomic_swap_uint(&vcpu->vc_vmx_vmcs_state, VMCS_CLEARED);
> rw_exit_write(&ci->ci_vmcs_lock);
>
> - return (ret);
> + return (0);
> }
> #endif /* MULTIPROCESSOR */
>
