Re: [Xen-devel] [PATCH v2 2/2] xen: make available hvm_fep to non-debug build as well

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 17/06/16 12:05, Wei Liu wrote:
> Originally hvm_fep was guarded by NDEBUG, which means it was only
> available to debug builds.
>
> However there is value to have it for non-debug builds as well. User can
> use that to run tests in setup that replicates production setup.
>
> Make it clear with a sync_console style warning that this option can't
> be used in production setup. Update command line documentation
> accordingly. Finally mark Xen as tainted when this option is enabled.
>
> Add a kconfig option under x86 to configure hvm_fep.
>
> Signed-off-by: Wei Liu <wei.liu2@xxxxxxxxxx>
> ---
> Cc: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
> Cc: Jan Beulich <jbeulich@xxxxxxxx>
> Cc: Doug Goldstein <cardoe@xxxxxxxxxx>
>
> v2:
> 1. unsigned -> unsigned int
> 2. %d -> %u
> 3. Add spaces around "-"
> 4. Update warning message
> 5. Only taint hv when fep is used
> 6. Add kconfig option
> ---
>  docs/misc/xen-command-line.markdown |  8 ++++++--
>  xen/arch/x86/Kconfig                | 14 ++++++++++++++
>  xen/arch/x86/hvm/hvm.c              | 28 ++++++++++++++++++++++++++--
>  xen/common/kernel.c                 |  6 ++++--
>  xen/include/asm-x86/hvm/hvm.h       |  2 +-
>  xen/include/xen/lib.h               |  1 +
>  6 files changed, 52 insertions(+), 7 deletions(-)
>
> diff --git a/docs/misc/xen-command-line.markdown 
> b/docs/misc/xen-command-line.markdown
> index fed732c..dc53e24 100644
> --- a/docs/misc/xen-command-line.markdown
> +++ b/docs/misc/xen-command-line.markdown
> @@ -878,8 +878,12 @@ Recognized in debug builds of the hypervisor only.
>  Allow use of the Forced Emulation Prefix in HVM guests, to allow emulation of
>  arbitrary instructions.
>  
> -This option is intended for development purposes, and is only available in
> -debug builds of the hypervisor.
> +This option is intended for development and testing purposes.
> +
> +*Warning*
> +As this feature opens up the instruction emulator to HVM guest, don't

"to arbitrary instruction from an HVM guest,".  It is an important
distinction, because all guest can enter the emulator in other ways as
part of normal operation.

> +use this in production system. No security support is provided when
> +this flag is set.
>  
>  ### hvm\_port80
>  > `= <boolean>`
> diff --git a/xen/arch/x86/Kconfig b/xen/arch/x86/Kconfig
> index 73f79cc..5e3b04a 100644
> --- a/xen/arch/x86/Kconfig
> +++ b/xen/arch/x86/Kconfig
> @@ -59,6 +59,20 @@ config BIGMEM
>  
>         If unsure, say N.
>  
> +config HVM_FEP
> +     bool "HVM Forced Emulation Prefix support"
> +     default y
> +     ---help---
> +
> +       Compiles in a feature that allows HVM guest to enter
> +       instruction emulator with forced emulation prefix.

"allows HVM guests to arbitrarily exercise the instruction emulator." 
There are other ways in which guests can enter the instruction emulator,
but they are specifically limited in nature.

I would also have a separate paragraph stating:

"This is strictly for testing purposes, and not appropriate for use in
production."

> +
> +       This feature can only be enabled during boot time with
> +       appropriate hypervisor command line option. Please read
> +       hypervisor command line documentation before trying to use
> +       this feature.
> +
> +       If unsure, say Y.
>  endmenu
>  
>  source "common/Kconfig"
> diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
> index 78db903..373b78e 100644
> --- a/xen/arch/x86/hvm/hvm.c
> +++ b/xen/arch/x86/hvm/hvm.c
> @@ -37,6 +37,7 @@
>  #include <xen/mem_access.h>
>  #include <xen/rangeset.h>
>  #include <xen/vm_event.h>
> +#include <xen/delay.h>
>  #include <asm/shadow.h>
>  #include <asm/hap.h>
>  #include <asm/current.h>
> @@ -95,9 +96,9 @@ unsigned long __section(".bss.page_aligned")
>  static bool_t __initdata opt_hap_enabled = 1;
>  boolean_param("hap", opt_hap_enabled);
>  
> -#ifndef opt_hvm_fep
> +#if CONFIG_HVM_FEP
>  /* Permit use of the Forced Emulation Prefix in HVM guests */
> -bool_t opt_hvm_fep;
> +bool_t __read_mostly opt_hvm_fep;
>  boolean_param("hvm_fep", opt_hvm_fep);
>  #endif
>  
> @@ -182,6 +183,28 @@ static int __init hvm_enable(void)
>      if ( !opt_altp2m_enabled )
>          hvm_funcs.altp2m_supported = 0;
>  
> +    if ( opt_hvm_fep )
> +    {
> +        unsigned int i, j;
> +
> +        printk("**********************************************\n");
> +        printk("******* WARNING: HVM FORCED EMULATION PREFIX IS 
> AVAILABLE\n");
> +        printk("******* This option is *ONLY* intended to aid testing of 
> Xen.\n");
> +        printk("******* It has implications on the security of the 
> system.\n");
> +        printk("******* Please *DO NOT* use this in production.\n");
> +        printk("**********************************************\n");
> +        for ( i = 0; i < 3; i++ )
> +        {
> +            printk("%u... ", 3 - i);
> +            for ( j = 0; j < 100; j++ )
> +            {
> +                process_pending_softirqs();
> +                mdelay(10);
> +            }
> +        }

I would drop this 3s wait, and I plan to drop it from sync_console as
well.  It isn't of any practical use, even if you are watching the
serial console on boot, and just leads to an unnecessary delay.  Worse,
the two delays are cumulative.

~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel