From mboxrd@z Thu Jan  1 00:00:00 1970
From: Andrew Cooper <andrew.cooper3@citrix.com>
Subject: Re: [PATCH] x86/vmx: Add command line option to enable
 EPT without PAT
Date: Wed, 16 Apr 2014 23:12:10 +0100
Message-ID: <534F003A.2060505@citrix.com>
References: <1397682915-9084-1-git-send-email-aravindp@cisco.com>
Mime-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
Return-path: <xen-devel-bounces@lists.xen.org>
Received: from mail6.bemta4.messagelabs.com ([85.158.143.247])
	by lists.xen.org with esmtp (Exim 4.72)
	(envelope-from <amc96@hermes.cam.ac.uk>) id 1WaY3z-0002NC-QU
	for xen-devel@lists.xenproject.org; Wed, 16 Apr 2014 22:12:16 +0000
In-Reply-To: <1397682915-9084-1-git-send-email-aravindp@cisco.com>
List-Unsubscribe: <http://lists.xen.org/cgi-bin/mailman/options/xen-devel>,
	<mailto:xen-devel-request@lists.xen.org?subject=unsubscribe>
List-Post: <mailto:xen-devel@lists.xen.org>
List-Help: <mailto:xen-devel-request@lists.xen.org?subject=help>
List-Subscribe: <http://lists.xen.org/cgi-bin/mailman/listinfo/xen-devel>,
	<mailto:xen-devel-request@lists.xen.org?subject=subscribe>
Sender: xen-devel-bounces@lists.xen.org
Errors-To: xen-devel-bounces@lists.xen.org
To: Aravindh Puthiyaparambil <aravindp@cisco.com>, xen-devel@lists.xenproject.org
Cc: Kevin Tian <kevin.tian@intel.com>, Eddie Dong <eddie.dong@intel.com>, Jun Nakajima <jun.nakajima@intel.com>
List-Id: xen-devel@lists.xenproject.org

On 16/04/2014 22:15, Aravindh Puthiyaparambil wrote:
> The fix for XSA-60 disables EPT if PAT is not available. This patch
> adds a command line option called "ept_without_pat", that allows EPT to
> be enabled even when PAT is not present. This is to enable Xen to run as
> a nested guest with EPT on hypervisors that have nested EPT but not
> nested PAT.
>
> Signed-off-by: Aravindh Puthiyaparambil <aravindp@cisco.com>
> Cc: Jun Nakajima <jun.nakajima@intel.com>
> Cc: Eddie Dong <eddie.dong@intel.com>
> Cc: Kevin Tian <kevin.tian@intel.com>
> ---
>  docs/misc/xen-command-line.markdown | 11 +++++++++++
>  xen/arch/x86/hvm/vmx/vmx.c          |  5 ++++-
>  2 files changed, 15 insertions(+), 1 deletion(-)
>
> diff --git a/docs/misc/xen-command-line.markdown b/docs/misc/xen-command-line.markdown
> index 87de2dc..9dc501b 100644
> --- a/docs/misc/xen-command-line.markdown
> +++ b/docs/misc/xen-command-line.markdown
> @@ -523,6 +523,17 @@ Either force retrieval of monitor EDID information via VESA DDC, or
>  disable it (edid=no). This option should not normally be required
>  except for debugging purposes.
>  
> +### ept_without_pat

Need to escape underscores with a backslash so markdown doesn't try to
italicise 'without'

Also, this in an Intel-specific option so should be annotated.  See the
documentation for 'vpid' as an example.

> +> `= <boolean>`
> +
> +Allow EPT to be enabled when PAT is not present. 
> +
> +*Warning:*
> +This is an unsupported option and should be used only to allow Xen to run with
> +EPT as a nested guest on hypervisors that do not have nested PAT.

I would not necessarily describe it as an unsupported option.  The
reason for the PAT requirement is because XSA-60 was a DoS attack with
HVM guests switching CR0.CD in combination with PCIPassthrough.

In the case that the administrator has weighed the risks, it need not be
unsupported.  In an environment without PCIPassthrough then it should be
unconditionally safe as flipping CR0.CD should turn into a noop, and the
benefit is the addition of nested EPT.  As a result, I might word the
paragraph a little more like this:

*Warning:*
Due to CVE-2013-2212, PAT is by default required as a prerequisite for
using EPT.  If you are not PCI Passthrough, or trust the guest
administrator who would be using passthrough, then the PAT requirement
can be relaxed.  This option is useful for nested virtualisation cases
where the outer hypervisor does not expose PAT functionality to Xen.

Or words to that effect, subject to taste.

> +
> +> Default: `false`

Default statement should be ahead of the description.

> +
>  ### extra\_guest\_irqs
>  > `= [<domU number>][,<dom0 number>]`
>  
> diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c
> index 180cf6c..a308a93 100644
> --- a/xen/arch/x86/hvm/vmx/vmx.c
> +++ b/xen/arch/x86/hvm/vmx/vmx.c
> @@ -58,6 +58,9 @@
>  #include <asm/hvm/nestedhvm.h>
>  #include <asm/event.h>
>  
> +static bool_t __initdata opt_ept_without_pat= 0;

space before =, but the assignment of 0 is redundant and can be dropped.

~Andrew

> +boolean_param("ept_without_pat", opt_ept_without_pat);
> +
>  enum handler_return { HNDL_done, HNDL_unhandled, HNDL_exception_raised };
>  
>  static void vmx_ctxt_switch_from(struct vcpu *v);
> @@ -1724,7 +1727,7 @@ const struct hvm_function_table * __init start_vmx(void)
>       * Do not enable EPT when (!cpu_has_vmx_pat), to prevent security hole
>       * (refer to http://xenbits.xen.org/xsa/advisory-60.html).
>       */
> -    if ( cpu_has_vmx_ept && cpu_has_vmx_pat )
> +    if ( cpu_has_vmx_ept && (cpu_has_vmx_pat || opt_ept_without_pat) )
>      {
>          vmx_function_table.hap_supported = 1;
>