[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


  • To: Roger Pau Monne <roger.pau@xxxxxxxxxx>, "xen-devel@xxxxxxxxxxxxxxxxxxxx" <xen-devel@xxxxxxxxxxxxxxxxxxxx>
  • From: Andrew Cooper <Andrew.Cooper3@xxxxxxxxxx>
  • Date: Thu, 19 May 2022 00:10:24 +0000
  • Accept-language: en-GB, en-US
  • Arc-authentication-results: i=1; mx.microsoft.com 1; spf=pass smtp.mailfrom=citrix.com; dmarc=pass action=none header.from=citrix.com; dkim=pass header.d=citrix.com; arc=none
  • Arc-message-signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=microsoft.com; s=arcselector9901; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-AntiSpam-MessageData-ChunkCount:X-MS-Exchange-AntiSpam-MessageData-0:X-MS-Exchange-AntiSpam-MessageData-1; bh=6dpfS5q1/RhgUhIsCh+xD8cLhfSbXpcFzJM/39TRqC4=; b=CbqjzE3zd2FmW+VCGEUyiOhNLjQ7/ruyCjxENYVJ/jw5W7vUDXvciCPFcWroRFqB35/4Ep/RdJ3+NG+sqvXUy3tRc8tzkLC3Qr3B1coMZw/SdxDhENEobzDSaA2gFrA2GSDGjrqkGmYuIJ53yEa3JvkwrciV5URo99QLITGGA6IYx6bjmC/2V9s5WlIEW9p/Lrx1xI/2PHwDb70eDkEEh8hVNWAlMydIQEHMDQXG6j+8HL/7kxPbZMpeQYGV8dOMCoXOrdhBHvP6jLGBLXmgmF/DiBX512pL4W2u4OyWwZCxAZH/VPkcJv9A1F9lo5zH+OFFcaQ62OdnbTaWZe6kGg==
  • Arc-seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=DmqGAhbih2k0LBi/jM/cYGLD6l308QTrgEBHD71ellPhGc3ymc855CdVRaqRauN84Rhp0I4vhyDSdf0elCnqmgM6pj+bTJuSDvpkXPrbvlaSQ3UOvN2cYBa7FR1Q7ZhDjcF4tjFnv6IwA5MQ0ujSmXsmRniUYd4YBC5UaIXAAbh8dqWRG7qr7DotscLr4vNwU/govvshXTUVnTKYayBwUfSX12/mktFwA3stYF9/PA67c0sxVK9FpBw0v8NChG3RRq2Tfj5aL6NIY2DvbvXNh8MVIzEuFx9t/QEjnmPpkt+8YFlxQj7pClPWwUU1fdNXPRk5Ir7brKBF/Got4LCYvQ==
  • Authentication-results: dkim=none (message not signed) header.d=none;dmarc=none action=none header.from=citrix.com;
  • Cc: George Dunlap <George.Dunlap@xxxxxxxxxx>, Jan Beulich <jbeulich@xxxxxxxx>, Julien Grall <julien@xxxxxxx>, Stefano Stabellini <sstabellini@xxxxxxxxxx>, Wei Liu <wl@xxxxxxx>, Jun Nakajima <jun.nakajima@xxxxxxxxx>, Kevin Tian <kevin.tian@xxxxxxxxx>
  • Delivery-date: Thu, 19 May 2022 00:10:51 +0000
  • Ironport-data: A9a23:lf0PVarlXS6SqPVVXHWtr3OGiJxeBmLRZBIvgKrLsJaIsI4StFCzt garIBmHafiKM2ukfNgja46w8B4CusTVzNY2HVM6ri43E34RpJuZCYyVIHmrMnLJJKUvbq7GA +byyDXkBJppJpMJjk71atANlVEliefQAOCU5NfsYkidfyc9IMsaoU8lyrdRbrJA24DjWVvQ4 IOq+KUzBXf+s9JKGjNMg068gEsHUMTa4Fv0aXRnOJinFHeH/5UkJMp3yZOZdhMUcaENdgKOf M7RzanRw4/s10xF5uVJMFrMWhZirrb6ZWBig5fNMkSoqkAqSicais7XOBeAAKv+Zvrgc91Zk b1wWZKMpQgBEqKWxsgnXRhjLTguALF21rLeC1OEmJnGp6HGWyOEL/RGKmgTZNRd0cEuRGZE+ LofNSwHaQ2Fi6Su2rWnR+Jwh8Mlas72IIcYvXImxjbcZRokacmbH+OWupkFgnFp2Z4m8fX2P qL1bRJGahjabgIJEVAQEJ8kx8+jh2Xlci0eo1WQzUYyyzeKklEsiOWwWDbTUvizau5ExlzBn WTb7WC6GSMDEsey6xPQpxpAgceKx0sXQrk6BLC+s/JnnlCX7mgSEwENE0u2p+GjjUyzUM4ZL FYbkgIsp6Uv8E2gTvHmQga15nWDu3Y0WcdUEuA8wBGAzOzT+QnxLnMfUjdLZdgitck3bT8nz FmEm5XuHzMHmKKRYWKQ8PGTtzzaESoIKykEbCwNTwoA6vHipp0+ilTESdMLOK24kNzzXy3xy jairS4iirFVhskOv5hX5njCijOo45POFwg841yOWnr/t1wnIom4e4av9F7Xq+5aK5qURUWAu 35CnNWC6OcJDteGkynlrPgxIYxFLs2taFX06WOD1bF4n9hx0xZPpbxt3Qw=
  • Ironport-hdrordr: A9a23:UDSCeKG5W4658tnEpLqFt5LXdLJyesId70hD6qkvc3Fom52j/f xGws5x6fatskdrZJkh8erwW5Vp2RvnhNFICPoqTM2ftW7dySWVxeBZnMffKljbdxEWmdQtsp uIH5IeNDS0NykDsS+Y2nj4Lz9D+qjgzEnAv463oBlQpENRGthdBmxCe2Sm+zhNNW177O0CZf +hD6R8xwaISDAyVICWF3MFV+/Mq5ngj5T9eyMLABYh9U2nkS6owKSSKWna4j4uFxd0hZsy+2 nMlAL0oo+5teug9xPa32jPq7xLhdrazMdZDsDksLlWFtyssHfsWG1SYczEgNkHmpDo1L/sqq iUn/4UBbU215oWRBDsnfKi4Xi67N9k0Q6e9bbRuwqenSW+fkN7NyMJv/MmTvOSgXBQw+1Uwe ZF2XmUuIFQCg6FlCPh58LQXxUvjUasp2E++NRjxEC3/rFuGoO5gLZvtX+9Kq1wVB4SKbpXZd VGHYXZ/rJbYFmaZ3fWsi1mx8GtRG06GlODTlIZssKY3jBKlDQhpnFoifA3jzMF7tYwWpNE7+ PLPuBhk6xPVNYfaeZ4CP0aScW6B2TRSVbHMX6UI17gCKYbUki94aLf8fEw/qWnaZYIxJw9lN DIV05Zr3c7fwb0BciHzPRwg2bwqaWGLEPQI+1lluhEU+fHNcvW2AW4OSMTutrlpekDCcvGXP v2MI5KApbYXB/TJbo=
  • List-id: Xen developer discussion <xen-devel.lists.xenproject.org>
  • Thread-index: AQHYafEfUx0WfCMplEq6U+wPIyhlOa0lVccA
  • Thread-topic: [PATCH 2/2] x86/vmx: implement Notify VM Exit

On 17/05/2022 14:21, Roger Pau Monne wrote:
> Under certain conditions guests can get the CPU stuck in an infinite
> loop without the possibility of an interrupt window to occur.

instruction boundary.

It's trivial to create an infinite loop without an interrupt window :)

Also, I'd probably phrase that as an unbounded loop, because not all
problem cases are truly infinite.

>   This
> was the case with the scenarios described in XSA-156.

Case in point, both of these can be broken by something else (another
vCPU, or coherent DMA write) editing the IDT and e.g. making the #AC/#DB
vectors not present, which will yield #NP instead.

>
> 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>
> ---
> 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".

TBH, I read that as a get-out clause for "we have no idea what to set
for a default window", and it's not a decision reasonable to defer to
users, because they have even less of an idea than us.

All CPUs with Notify VM Exit have the TSC crystal information in CPUID,
so I'd suggest that we trust CPUID to be accurate, and program for maybe
10us?  That's 1/3 of a default timeslice.



> 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.
>
> I've tested with 0 (the proposed default in the patch) and I don't
> seem to be able to trigger notify VM exits under normal guest
> operation.  Note that even in that case the guest won't be destroyed
> unless the context is corrupt.

Huh... There's nothing in the manual about that, but obviously hardware
has some minimum safe value if 0 appears to work in practice.

> diff --git a/xen/arch/x86/hvm/vmx/vmcs.c b/xen/arch/x86/hvm/vmx/vmcs.c
> index d388e6729c..5685a5523e 100644
> --- 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 int __read_mostly vm_notify_window;
> +integer_param("vm-notify-window", vm_notify_window);

Part of me is loath to keep on adding new top-level options for this.

I was about to suggest having a vmx= option, but I've just noticed that
ple_{window,gap} are wired up to cmdline options on Intel, and fixed
constants on AMD.

Thoughts on a suitable name?

> @@ -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).

> +    }
> +
>   out:
>      vmx_vmcs_exit(v);
>  
> diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c
> index 02cc7a2023..9c37790c36 100644
> --- a/xen/arch/x86/hvm/vmx/vmx.c
> +++ b/xen/arch/x86/hvm/vmx/vmx.c
> @@ -4567,6 +4567,30 @@ void vmx_vmexit_handler(struct cpu_user_regs *regs)
>           */
>          break;
>  
> +    case EXIT_REASON_NOTIFY:
> +        __vmread(EXIT_QUALIFICATION, &exit_qualification);
> +
> +        if ( exit_qualification & NOTIFY_VM_CONTEXT_INVALID )
> +        {
> +            perfc_incr(vmnotify_crash);
> +            gprintk(XENLOG_ERR, "invalid VM context after notify vmexit\n");
> +            domain_crash(v->domain);
> +            break;
> +        }
> +
> +        if ( cpu_has_vmx_vnmi &&
> +             (exit_qualification & INTR_INFO_NMI_UNBLOCKED_BY_IRET) )
> +        {
> +            unsigned long guest_info;
> +
> +            /* Exit was incident to an execution of IRET that unblocked 
> NMIs. */
> +            __vmread(GUEST_INTERRUPTIBILITY_INFO, &guest_info);
> +            __vmwrite(GUEST_INTERRUPTIBILITY_INFO,
> +                      guest_info | VMX_INTR_SHADOW_NMI);

I am saddened by how irritating it is having the UNBLOCKED_BY_IRET (in
the first place...) but moving between the exit qualification and the
vmexit intr info fields.  The constant probably ought to be renamed to
lose the INTR_INFO prefix.

I'd suggest a prereq patch to also break

static void undo_nmis_unblocked_by_iret(void)
{
    ...
}

out to avoid opencoding it in several places.  There's one other
instance in our code (general fault intercept), but we're buggy on
PML-full, APIC-access and EPT violation all of which Xen handles.

I don't think you need the vnmi check, because the bit is 0 otherwise.

~Andrew

 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.