[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [PATCH v2 3/3] x86/vmx: implement Notify VM Exit
On Fri, Jun 03, 2022 at 02:49:54PM +0200, Jan Beulich wrote: > 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. I was hoping for either Kevin or Jun (now moved from Cc to To) to provide some guidance here on what a suitable default value would be. > > --- 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)? The condition in vmx_update_debug_state() sets the mask so that TRAP_debug will only be added or removed from the bitmap if !cpu_has_monitor_trap_flag && cpu_has_vmx_notify_vm_exiting (note that otherwise TRAP_debug is unconditionally set if !cpu_has_vmx_notify_vm_exiting). Hence it's impossible to get a VMExit TRAP_debug with cpu_has_monitor_trap_flag && cpu_has_vmx_notify_vm_exiting because TRAP_debug will never be set by vmx_update_debug_state() in that case. > > > @@ -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). Sure, thanks for the review. Roger.
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |