RE: [PATCH v2 3/3] x86/vmx: implement Notify VM Exit

["Tian, Kevin" <kevin.tian@xxxxxxxxx>]

> From: Roger Pau Monné
> Sent: Tuesday, June 7, 2022 6:06 PM
> 
> On Tue, Jun 07, 2022 at 09:43:25AM +0200, Jan Beulich wrote:
> > On 03.06.2022 16:46, Roger Pau Monné wrote:
> > > On Fri, Jun 03, 2022 at 02:49:54PM +0200, Jan Beulich wrote:
> > >> On 26.05.2022 13:11, Roger Pau Monne wrote:
> > >>> --- 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.
> >
> > Hmm, yes, I've been misguided by you not altering the existing setting
> > of v->arch.hvm.vmx.exception_bitmap in construct_vmcs(). Instead you
> > add an entirely new block of code near the bottom of the function. Is
> > there any chance you could move up that adjustment, perhaps along the
> > lines of
> >
> >     v->arch.hvm.vmx.exception_bitmap = HVM_TRAP_MASK
> >               | (paging_mode_hap(d) ? 0 : (1U << TRAP_page_fault))
> >               | (v->arch.fully_eager_fpu ? 0 : (1U << TRAP_no_device));
> >     if ( cpu_has_vmx_notify_vm_exiting )
> >     {
> >         __vmwrite(NOTIFY_WINDOW, vm_notify_window);
> >         /*
> >          * Disable #AC and #DB interception: by using VM Notify Xen is
> >          * guaranteed to get a VM exit even if the guest manages to lock the
> >          * CPU.
> >          */
> >         v->arch.hvm.vmx.exception_bitmap &= ~((1U << TRAP_debug) |
> >                                               (1U << TRAP_alignment_check));
> >     }
> >     vmx_update_exception_bitmap(v);
> 
> Sure, will move up when posting a new version then.  I will wait for
> feedback from Jun or Kevin regarding the default window size before
> doing so.
> 

let me check internally.