Re: [PATCH 2/2] x86/vmx: implement Notify VM Exit

From: "Roger Pau Monné" <roger.pau@citrix.com>
To: Andrew Cooper <Andrew.Cooper3@citrix.com>,
	Jan Beulich <jbeulich@suse.com>
Cc: "xen-devel@lists.xenproject.org" <xen-devel@lists.xenproject.org>,
	George Dunlap <George.Dunlap@citrix.com>,
	Julien Grall <julien@xen.org>,
	Stefano Stabellini <sstabellini@kernel.org>, Wei Liu <wl@xen.org>,
	Jun Nakajima <jun.nakajima@intel.com>,
	Kevin Tian <kevin.tian@intel.com>
Subject: Re: [PATCH 2/2] x86/vmx: implement Notify VM Exit
Date: Fri, 20 May 2022 12:08:50 +0200	[thread overview]
Message-ID: <YodosqK1YK5fx45P@Air-de-Roger> (raw)
In-Reply-To: <YoZYAAG5YbHMyxMx@Air-de-Roger>

On Thu, May 19, 2022 at 04:45:20PM +0200, Roger Pau Monné wrote:
> On Thu, May 19, 2022 at 12:10:24AM +0000, Andrew Cooper wrote:
> > On 17/05/2022 14:21, Roger Pau Monne wrote:
> > > @@ -1333,6 +1338,19 @@ static int construct_vmcs(struct vcpu *v)
> > >          rc = vmx_add_msr(v, MSR_FLUSH_CMD, FLUSH_CMD_L1D,
> > >                           VMX_MSR_GUEST_LOADONLY);
> > >  
> > > +    if ( cpu_has_vmx_notify_vm_exiting && vm_notify_window >= 0 )
> > > +    {
> > > +        __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);
> > 
> > IIRC, it's not quite this easy.  There are conditions, e.g. attaching
> > gdbsx, where #DB interception wants turning on/off dynamically, and the
> > logic got simplified to nothing following XSA-156, so will need
> > reintroducing.
> > 
> > AMD Milan (Zen3) actually has NoNestedDataBp in CPUID.80000021.eax[0]
> > which allows us to not intercept #DB, so perhaps that might offer an
> > easier way of adjusting the interception logic.  (Or maybe not.  I can't
> > remember).
> 
> OK, will look into it.

So after taking a look, I think we need to modify vmx_update_debug_state() so it's:

void vmx_update_debug_state(struct vcpu *v)
{
    unsigned int mask = 1u << TRAP_int3;

    if ( v->arch.hvm.vmx.secondary_exec_control &
         SECONDARY_EXEC_NOTIFY_VM_EXITING )
        /*
         * 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 |= mask;
    else
        v->arch.hvm.vmx.exception_bitmap &= ~mask;

[...]

I'm however confused by the usage of cpu_has_monitor_trap_flag
previous to XSA-156, which was:

void vmx_update_debug_state(struct vcpu *v)
{
    unsigned long mask;

    mask = 1u << TRAP_int3;
    if ( !cpu_has_monitor_trap_flag )
        mask |= 1u << TRAP_debug;

Was it fine to not set TRAP_debug only if cpu_has_monitor_trap_flag
is supported by the CPU? (even if not currently set on
secondary_exec_control)?

Thanks, Roger.