[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [PATCH v2 3/3] x86/vmx: implement Notify VM Exit
On 26.05.2022 13:11, Roger Pau Monne wrote: > Under certain conditions guests can get the CPU stuck in an unbounded > loop without the possibility of an interrupt window to occur on > instruction boundary. This was the case with the scenarios described > in XSA-156. > > Make use of the Notify VM Exit mechanism, that will trigger a VM Exit > if no interrupt window occurs for a specified amount of time. Note > that using the Notify VM Exit avoids having to trap #AC and #DB > exceptions, as Xen is guaranteed to get a VM Exit even if the guest > puts the CPU in a loop without an interrupt window, as such disable > the intercepts if the feature is available and enabled. > > Setting the notify VM exit window to 0 is safe because there's a > threshold added by the hardware in order to have a sane window value. > > Suggested-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx> > Signed-off-by: Roger Pau Monné <roger.pau@xxxxxxxxxx> > --- > Changes since v1: > - Properly update debug state when using notify VM exit. > - Reword commit message. > --- > This change enables the notify VM exit by default, KVM however doesn't > seem to enable it by default, and there's the following note in the > commit message: > > "- There's a possibility, however small, that a notify VM exit happens > with VM_CONTEXT_INVALID set in exit qualification. In this case, the > vcpu can no longer run. To avoid killing a well-behaved guest, set > notify window as -1 to disable this feature by default." > > It's not obviously clear to me whether the comment was meant to be: > "There's a possibility, however small, that a notify VM exit _wrongly_ > happens with VM_CONTEXT_INVALID". > > It's also not clear whether such wrong hardware behavior only affects > a specific set of hardware, in a way that we could avoid enabling > notify VM exit there. > > There's a discussion in one of the Linux patches that 128K might be > the safer value in order to prevent false positives, but I have no > formal confirmation about this. Maybe our Intel maintainers can > provide some more feedback on a suitable notify VM exit window > value. This certainly wants sorting one way or another before I, for one, would consider giving an R-b here. > --- a/xen/arch/x86/hvm/vmx/vmcs.c > +++ b/xen/arch/x86/hvm/vmx/vmcs.c > @@ -67,6 +67,9 @@ integer_param("ple_gap", ple_gap); > static unsigned int __read_mostly ple_window = 4096; > integer_param("ple_window", ple_window); > > +static unsigned int __ro_after_init vm_notify_window; > +integer_param("vm-notify-window", vm_notify_window); Could even be a runtime param, I guess. Albeit I realize this would complicate things further down. > --- a/xen/arch/x86/hvm/vmx/vmx.c > +++ b/xen/arch/x86/hvm/vmx/vmx.c > @@ -1419,10 +1419,19 @@ static void cf_check vmx_update_host_cr3(struct vcpu > *v) > > void vmx_update_debug_state(struct vcpu *v) > { > + unsigned int mask = 1u << TRAP_int3; > + > + if ( !cpu_has_monitor_trap_flag && cpu_has_vmx_notify_vm_exiting ) I'm puzzled by the lack of symmetry between this and ... > + /* > + * Only allow toggling TRAP_debug if notify VM exit is enabled, as > + * unconditionally setting TRAP_debug is part of the XSA-156 fix. > + */ > + mask |= 1u << TRAP_debug; > + > if ( v->arch.hvm.debug_state_latch ) > - v->arch.hvm.vmx.exception_bitmap |= 1U << TRAP_int3; > + v->arch.hvm.vmx.exception_bitmap |= mask; > else > - v->arch.hvm.vmx.exception_bitmap &= ~(1U << TRAP_int3); > + v->arch.hvm.vmx.exception_bitmap &= ~mask; > > vmx_vmcs_enter(v); > vmx_update_exception_bitmap(v); > @@ -4155,6 +4164,9 @@ void vmx_vmexit_handler(struct cpu_user_regs *regs) > switch ( vector ) > { > case TRAP_debug: > + if ( cpu_has_monitor_trap_flag && cpu_has_vmx_notify_vm_exiting ) > + goto exit_and_crash; ... this condition. Shouldn't one be the inverse of the other (and then it's the one down here which wants adjusting)? > @@ -126,5 +126,6 @@ PERFCOUNTER(realmode_exits, "vmexits from realmode") > PERFCOUNTER(pauseloop_exits, "vmexits from Pause-Loop Detection") > > PERFCOUNTER(buslock, "Bus Locks Detected") > +PERFCOUNTER(vmnotify_crash, "domains crashed by Notify VM Exit") I think the text is not entirely correct and would want to be "domain crashes by ...". Multiple vCPU-s of a single domain can experience this in parallel (granted this would require "good" timing). Jan
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |