[PATCH v3 0/3] Make hvm_fep available to non-debug build

[PATCH v3 0/3] Make hvm_fep available to non-debug build

[Wei Liu]

Wei Liu (3):
  xen: add warning infrastructure
  console: use warning infrastructure for sync console warning
  xen: make available hvm_fep to non-debug build as well

 docs/misc/xen-command-line.markdown |  8 ++++--
 xen/arch/x86/Kconfig                | 17 +++++++++++++
 xen/arch/x86/hvm/hvm.c              | 15 ++++++++++-
 xen/arch/x86/setup.c                |  3 +++
 xen/common/Makefile                 |  1 +
 xen/common/kernel.c                 |  6 +++--
 xen/common/warning.c                | 50 +++++++++++++++++++++++++++++++++++++
 xen/drivers/char/console.c          | 38 ++++++++--------------------
 xen/include/asm-x86/hvm/hvm.h       |  2 +-
 xen/include/xen/lib.h               |  1 +
 xen/include/xen/warning.h           |  7 ++++++
 11 files changed, 114 insertions(+), 34 deletions(-)
 create mode 100644 xen/common/warning.c
 create mode 100644 xen/include/xen/warning.h

-- 
2.1.4


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

[PATCH v3 1/3] xen: add warning infrastructure

[Wei Liu]

Use an array to keep track of warning text, provide a function to add
warning text to track.  Print warnings (if any) right before finishing
using the console.

Signed-off-by: Wei Liu <wei.liu2@citrix.com>
---
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
---
 xen/arch/x86/setup.c      |  3 +++
 xen/common/Makefile       |  1 +
 xen/common/warning.c      | 50 +++++++++++++++++++++++++++++++++++++++++++++++
 xen/include/xen/warning.h |  7 +++++++
 4 files changed, 61 insertions(+)
 create mode 100644 xen/common/warning.c
 create mode 100644 xen/include/xen/warning.h

diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
index 6f6a6a7..3ef8c43 100644
--- a/xen/arch/x86/setup.c
+++ b/xen/arch/x86/setup.c
@@ -28,6 +28,7 @@
 #include <xen/tmem_xen.h>
 #include <xen/virtual_region.h>
 #include <xen/watchdog.h>
+#include <xen/warning.h>
 #include <public/version.h>
 #include <compat/platform.h>
 #include <compat/xen.h>
@@ -1582,6 +1583,8 @@ void __init noreturn __start_xen(unsigned long mbi_p)
 
     init_constructors();
 
+    warning_print();
+
     console_endboot();
 
     /* Hide UART from DOM0 if we're using it */
diff --git a/xen/common/Makefile b/xen/common/Makefile
index e9893e2..600b5ce 100644
--- a/xen/common/Makefile
+++ b/xen/common/Makefile
@@ -59,6 +59,7 @@ obj-y += vm_event.o
 obj-y += vmap.o
 obj-y += vsprintf.o
 obj-y += wait.o
+obj-y += warning.o
 obj-$(CONFIG_XENOPROF) += xenoprof.o
 obj-y += xmalloc_tlsf.o
 
diff --git a/xen/common/warning.c b/xen/common/warning.c
new file mode 100644
index 0000000..8a3e904
--- /dev/null
+++ b/xen/common/warning.c
@@ -0,0 +1,50 @@
+#include <xen/delay.h>
+#include <xen/init.h>
+#include <xen/lib.h>
+#include <xen/softirq.h>
+#include <xen/warning.h>
+
+#define WARNING_ARRAY_SIZE 20
+static unsigned int __initdata nr_warnings;
+static const char __initdata *warnings[WARNING_ARRAY_SIZE];
+
+void __init warning_add(const char *warning)
+{
+    if ( nr_warnings >= WARNING_ARRAY_SIZE )
+        panic("Too many pieces of warning text.");
+
+    warnings[nr_warnings] = warning;
+    nr_warnings++;
+}
+
+void __init warning_print(void)
+{
+    unsigned int i, j;
+
+    if ( !nr_warnings )
+        return;
+
+    for ( i = 0; i < nr_warnings; i++ )
+        printk("%s", warnings[i]);
+
+    for ( i = 0; i < 3; i++ )
+    {
+        printk("%u... ", 3 - i);
+        for ( j = 0; j < 100; j++ )
+        {
+            process_pending_softirqs();
+            mdelay(10);
+        }
+    }
+    printk("\n");
+}
+
+/*
+ * Local variables:
+ * mode: C
+ * c-file-style: "BSD"
+ * c-basic-offset: 4
+ * tab-width: 4
+ * indent-tabs-mode: nil
+ * End:
+ */
diff --git a/xen/include/xen/warning.h b/xen/include/xen/warning.h
new file mode 100644
index 0000000..c0661d5
--- /dev/null
+++ b/xen/include/xen/warning.h
@@ -0,0 +1,7 @@
+#ifndef _WARNING_H_
+#define _WARNING_H_
+
+void warning_add(const char *warning);
+void warning_print(void);
+
+#endif
-- 
2.1.4


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

[PATCH v3 2/3] console: use warning infrastructure for sync console warning

[Wei Liu]

Move the warning text to a static variable and marked that as initconst
data. Call warning_add in console_init_preirq. Finally remove all
unused bits.

Signed-off-by: Wei Liu <wei.liu2@citrix.com>
---
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
---
 xen/drivers/char/console.c | 38 ++++++++++----------------------------
 1 file changed, 10 insertions(+), 28 deletions(-)

diff --git a/xen/drivers/char/console.c b/xen/drivers/char/console.c
index f4f6141..6c771dc 100644
--- a/xen/drivers/char/console.c
+++ b/xen/drivers/char/console.c
@@ -18,7 +18,6 @@
 #include <xen/serial.h>
 #include <xen/softirq.h>
 #include <xen/keyhandler.h>
-#include <xen/delay.h>
 #include <xen/guest_access.h>
 #include <xen/watchdog.h>
 #include <xen/shutdown.h>
@@ -29,6 +28,7 @@
 #include <asm/div64.h>
 #include <xen/hypercall.h> /* for do_console_io */
 #include <xen/early_printk.h>
+#include <xen/warning.h>
 
 /* console: comma-separated list of console outputs. */
 static char __initdata opt_console[30] = OPT_CONSOLE_STR;
@@ -44,6 +44,14 @@ string_param("conswitch", opt_conswitch);
 /* sync_console: force synchronous console output (useful for debugging). */
 static bool_t __initdata opt_sync_console;
 boolean_param("sync_console", opt_sync_console);
+static const char __initconst *warning_sync_console =
+    "**********************************************\n"
+    "******* WARNING: CONSOLE OUTPUT IS SYNCHRONOUS\n"
+    "******* This option is intended to aid debugging of Xen by ensuring\n"
+    "******* that all output is synchronously delivered on the serial line.\n"
+    "******* However it can introduce SIGNIFICANT latencies and affect\n"
+    "******* timekeeping. It is NOT recommended for production use!\n"
+    "**********************************************\n";
 
 /* console_to_ring: send guest (incl. dom 0) console data to console ring. */
 static bool_t __read_mostly opt_console_to_ring;
@@ -739,6 +747,7 @@ void __init console_init_preirq(void)
         serial_start_sync(sercon_handle);
         add_taint(TAINT_SYNC_CONSOLE);
         printk("Console output is synchronous.\n");
+        warning_add(warning_sync_console);
     }
 }
 
@@ -786,8 +795,6 @@ void __init console_init_postirq(void)
 
 void __init console_endboot(void)
 {
-    int i, j;
-
     printk("Std. Loglevel: %s", loglvl_str(xenlog_lower_thresh));
     if ( xenlog_upper_thresh != xenlog_lower_thresh )
         printk(" (Rate-limited: %s)", loglvl_str(xenlog_upper_thresh));
@@ -796,31 +803,6 @@ void __init console_endboot(void)
         printk(" (Rate-limited: %s)", loglvl_str(xenlog_guest_upper_thresh));
     printk("\n");
 
-    if ( opt_sync_console )
-    {
-        printk("**********************************************\n");
-        printk("******* WARNING: CONSOLE OUTPUT IS SYNCHRONOUS\n");
-        printk("******* This option is intended to aid debugging "
-               "of Xen by ensuring\n");
-        printk("******* that all output is synchronously delivered "
-               "on the serial line.\n");
-        printk("******* However it can introduce SIGNIFICANT latencies "
-               "and affect\n");
-        printk("******* timekeeping. It is NOT recommended for "
-               "production use!\n");
-        printk("**********************************************\n");
-        for ( i = 0; i < 3; i++ )
-        {
-            printk("%d... ", 3-i);
-            for ( j = 0; j < 100; j++ )
-            {
-                process_pending_softirqs();
-                mdelay(10);
-            }
-        }
-        printk("\n");
-    }
-
     video_endboot();
 
     /*
-- 
2.1.4


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

[PATCH v3 3/3] xen: make available hvm_fep to non-debug build as well

[Wei Liu]

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 feature is enabled.

Add a kconfig option under x86 to configure hvm_fep.

Signed-off-by: Wei Liu <wei.liu2@citrix.com>
---
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Doug Goldstein <cardoe@cardoe.com>

v3:
1. Make config HVM_FEP an expert option and default to DEBUG.
2. Change some ifdefs
3. Update docs
4. Use the new warning infrastructure

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                | 17 +++++++++++++++++
 xen/arch/x86/hvm/hvm.c              | 15 ++++++++++++++-
 xen/common/kernel.c                 |  6 ++++--
 xen/include/asm-x86/hvm/hvm.h       |  2 +-
 xen/include/xen/lib.h               |  1 +
 6 files changed, 43 insertions(+), 6 deletions(-)

diff --git a/docs/misc/xen-command-line.markdown b/docs/misc/xen-command-line.markdown
index fed732c..8956e02 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 arbitrary
+instruction from an HVM guest, don't 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..c1e9279 100644
--- a/xen/arch/x86/Kconfig
+++ b/xen/arch/x86/Kconfig
@@ -59,6 +59,23 @@ config BIGMEM
 
 	  If unsure, say N.
 
+config HVM_FEP
+	bool "HVM Forced Emulation Prefix support" if EXPERT = "y"
+	default DEBUG
+	---help---
+
+	  Compiles in a feature that allows HVM guest to arbitrarily
+	  exercise the instruction emulator.
+
+	  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.
+
+	  This is strictly for testing purposes, and not appropriate
+	  for use in production.
+
+	  If unsure, say N.
 endmenu
 
 source "common/Kconfig"
diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
index 22f045e..52d66d4 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/warning.h>
 #include <asm/shadow.h>
 #include <asm/hap.h>
 #include <asm/current.h>
@@ -97,9 +98,17 @@ boolean_param("hap", opt_hap_enabled);
 
 #ifndef opt_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
+static const char __initconst *warning_hvm_fep =
+    "**********************************************\n"
+    "******* WARNING: HVM FORCED EMULATION PREFIX IS AVAILABLE\n"
+    "******* This option is *ONLY* intended to aid testing of Xen.\n"
+    "******* It has implications on the security of the system.\n"
+    "******* Please *DO NOT* use this in production.\n"
+    "**********************************************\n";
+
 
 /* Xen command-line option to enable altp2m */
 static bool_t __initdata opt_altp2m_enabled = 0;
@@ -182,6 +191,9 @@ static int __init hvm_enable(void)
     if ( !opt_altp2m_enabled )
         hvm_funcs.altp2m_supported = 0;
 
+    if ( opt_hvm_fep )
+        warning_add(warning_hvm_fep);
+
     /*
      * Allow direct access to the PC debug ports 0x80 and 0xed (they are
      * often used for I/O delays, but the vmexits simply slow things down).
@@ -3913,6 +3925,7 @@ void hvm_ud_intercept(struct cpu_user_regs *regs)
         {
             regs->eip += sizeof(sig);
             regs->eflags &= ~X86_EFLAGS_RF;
+            add_taint(TAINT_HVM_FEP);
         }
     }
 
diff --git a/xen/common/kernel.c b/xen/common/kernel.c
index dae7e35..5bf77aa 100644
--- a/xen/common/kernel.c
+++ b/xen/common/kernel.c
@@ -175,6 +175,7 @@ int __init parse_bool(const char *s)
  *  'M' - Machine had a machine check experience.
  *  'B' - System has hit bad_page.
  *  'C' - Console output is synchronous.
+ *  'H' - HVM forced emulation prefix is permitted.
  *
  *      The string is overwritten by the next call to print_taint().
  */
@@ -182,11 +183,12 @@ char *print_tainted(char *str)
 {
     if ( tainted )
     {
-        snprintf(str, TAINT_STRING_MAX_LEN, "Tainted: %c%c%c%c",
+        snprintf(str, TAINT_STRING_MAX_LEN, "Tainted: %c%c%c%c%c",
                  tainted & TAINT_UNSAFE_SMP ? 'S' : ' ',
                  tainted & TAINT_MACHINE_CHECK ? 'M' : ' ',
                  tainted & TAINT_BAD_PAGE ? 'B' : ' ',
-                 tainted & TAINT_SYNC_CONSOLE ? 'C' : ' ');
+                 tainted & TAINT_SYNC_CONSOLE ? 'C' : ' ',
+                 tainted & TAINT_HVM_FEP ? 'H' : ' ');
     }
     else
     {
diff --git a/xen/include/asm-x86/hvm/hvm.h b/xen/include/asm-x86/hvm/hvm.h
index f486ee9..3c8aca8 100644
--- a/xen/include/asm-x86/hvm/hvm.h
+++ b/xen/include/asm-x86/hvm/hvm.h
@@ -27,7 +27,7 @@
 #include <public/hvm/save.h>
 #include <xen/mm.h>
 
-#ifndef NDEBUG
+#ifdef CONFIG_HVM_FEP
 /* Permit use of the Forced Emulation Prefix in HVM guests */
 extern bool_t opt_hvm_fep;
 #else
diff --git a/xen/include/xen/lib.h b/xen/include/xen/lib.h
index 1c652bb..b1b0fb2 100644
--- a/xen/include/xen/lib.h
+++ b/xen/include/xen/lib.h
@@ -142,6 +142,7 @@ uint64_t muldiv64(uint64_t a, uint32_t b, uint32_t c);
 #define TAINT_BAD_PAGE                  (1<<2)
 #define TAINT_SYNC_CONSOLE              (1<<3)
 #define TAINT_ERROR_INJECT              (1<<4)
+#define TAINT_HVM_FEP                   (1<<5)
 extern int tainted;
 #define TAINT_STRING_MAX_LEN            20
 extern char *print_tainted(char *str);
-- 
2.1.4


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

Re: [PATCH v3 1/3] xen: add warning infrastructure

[Jan Beulich]

>>> On 20.06.16 at 18:30, <wei.liu2@citrix.com> wrote:
> @@ -1582,6 +1583,8 @@ void __init noreturn __start_xen(unsigned long mbi_p)
>  
>      init_constructors();
>  
> +    warning_print();
> +
>      console_endboot();

What about an ARM equivalent? Perhaps put this in console_endboot()?

> --- a/xen/common/Makefile
> +++ b/xen/common/Makefile
> @@ -59,6 +59,7 @@ obj-y += vm_event.o
>  obj-y += vmap.o
>  obj-y += vsprintf.o
>  obj-y += wait.o
> +obj-y += warning.o

And perhaps better to put the new code into the console code too?
In any event if you want to keep this in a separate file, considering
that all of it is code/data contributions to .init.*, this wants to be

obj-bin-y += warning.init.o

so that namely string literals get moved to .init.* too.

> --- /dev/null
> +++ b/xen/common/warning.c
> @@ -0,0 +1,50 @@
> +#include <xen/delay.h>
> +#include <xen/init.h>
> +#include <xen/lib.h>
> +#include <xen/softirq.h>
> +#include <xen/warning.h>
> +
> +#define WARNING_ARRAY_SIZE 20
> +static unsigned int __initdata nr_warnings;
> +static const char __initdata *warnings[WARNING_ARRAY_SIZE];

The __initdata belongs after the *.

> +void __init warning_add(const char *warning)
> +{
> +    if ( nr_warnings >= WARNING_ARRAY_SIZE )
> +        panic("Too many pieces of warning text.");

panic() seems a bit harsh, but this shouldn't trigger anyway.

> +    warnings[nr_warnings] = warning;
> +    nr_warnings++;
> +}
> +
> +void __init warning_print(void)
> +{
> +    unsigned int i, j;
> +
> +    if ( !nr_warnings )
> +        return;

Perhaps a single instance of the ***** separators could be printed
here ...

> +    for ( i = 0; i < nr_warnings; i++ )
> +        printk("%s", warnings[i]);

... and here, avoiding each caller to add such?

Jan


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

Re: [PATCH v3 2/3] console: use warning infrastructure for sync console warning

[Jan Beulich]

>>> On 20.06.16 at 18:30, <wei.liu2@citrix.com> wrote:
> @@ -44,6 +44,14 @@ string_param("conswitch", opt_conswitch);
>  /* sync_console: force synchronous console output (useful for debugging). */
>  static bool_t __initdata opt_sync_console;
>  boolean_param("sync_console", opt_sync_console);
> +static const char __initconst *warning_sync_console =

static const char __initconst warning_sync_console[] =

Jan


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

Re: [PATCH v3 3/3] xen: make available hvm_fep to non-debug build as well

[Jan Beulich]

>>> On 20.06.16 at 18:30, <wei.liu2@citrix.com> wrote:
> @@ -97,9 +98,17 @@ boolean_param("hap", opt_hap_enabled);
>  
>  #ifndef opt_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
> +static const char __initconst *warning_hvm_fep =
> +    "**********************************************\n"
> +    "******* WARNING: HVM FORCED EMULATION PREFIX IS AVAILABLE\n"
> +    "******* This option is *ONLY* intended to aid testing of Xen.\n"
> +    "******* It has implications on the security of the system.\n"
> +    "******* Please *DO NOT* use this in production.\n"
> +    "**********************************************\n";
> +
>  

Along with the same comment as on patch 2, here even more than
there I wonder whether this string wouldn't better be a static local
in hvm_enable() (or even the scope therein where warning_add()
gets invoked).

Also adding a stray blank line.

Jan


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

Re: [PATCH v3 1/3] xen: add warning infrastructure

[Wei Liu]

On Wed, Jun 22, 2016 at 09:35:51AM -0600, Jan Beulich wrote:
> >>> On 20.06.16 at 18:30, <wei.liu2@citrix.com> wrote:
> > @@ -1582,6 +1583,8 @@ void __init noreturn __start_xen(unsigned long mbi_p)
> >  
> >      init_constructors();
> >  
> > +    warning_print();
> > +
> >      console_endboot();
> 
> What about an ARM equivalent? Perhaps put this in console_endboot()?
> 

Fine with putting this in console_endboot.

> > --- a/xen/common/Makefile
> > +++ b/xen/common/Makefile
> > @@ -59,6 +59,7 @@ obj-y += vm_event.o
> >  obj-y += vmap.o
> >  obj-y += vsprintf.o
> >  obj-y += wait.o
> > +obj-y += warning.o
> 
> And perhaps better to put the new code into the console code too?
> In any event if you want to keep this in a separate file, considering
> that all of it is code/data contributions to .init.*, this wants to be
> 

I will keep a separate file, and ...

> obj-bin-y += warning.init.o
> 

... use this.

> so that namely string literals get moved to .init.* too.
> 
> > --- /dev/null
> > +++ b/xen/common/warning.c
> > @@ -0,0 +1,50 @@
> > +#include <xen/delay.h>
> > +#include <xen/init.h>
> > +#include <xen/lib.h>
> > +#include <xen/softirq.h>
> > +#include <xen/warning.h>
> > +
> > +#define WARNING_ARRAY_SIZE 20
> > +static unsigned int __initdata nr_warnings;
> > +static const char __initdata *warnings[WARNING_ARRAY_SIZE];
> 
> The __initdata belongs after the *.
> 

Fixed.

> > +void __init warning_add(const char *warning)
> > +{
> > +    if ( nr_warnings >= WARNING_ARRAY_SIZE )
> > +        panic("Too many pieces of warning text.");
> 
> panic() seems a bit harsh, but this shouldn't trigger anyway.
> 
> > +    warnings[nr_warnings] = warning;
> > +    nr_warnings++;
> > +}
> > +
> > +void __init warning_print(void)
> > +{
> > +    unsigned int i, j;
> > +
> > +    if ( !nr_warnings )
> > +        return;
> 
> Perhaps a single instance of the ***** separators could be printed
> here ...
> 
> > +    for ( i = 0; i < nr_warnings; i++ )
> > +        printk("%s", warnings[i]);
> 
> ... and here, avoiding each caller to add such?
> 

The warning text (multiple lines) is added as one single string, which
means we can't trivially add leading stars at the beginning of each
line.

I don't feel like arguing over how the text would look like, so if
something like:

    ************************************************
    WARNING 1
    ************************************************
    WARNING 2
    ************************************************
    ...
    ************************************************
    WARNING N
    ************************************************

is ok to you, I'm fine with using that format, too.

Wei.

> Jan
> 

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

Re: [PATCH v3 2/3] console: use warning infrastructure for sync console warning

[Wei Liu]

On Wed, Jun 22, 2016 at 09:37:55AM -0600, Jan Beulich wrote:
> >>> On 20.06.16 at 18:30, <wei.liu2@citrix.com> wrote:
> > @@ -44,6 +44,14 @@ string_param("conswitch", opt_conswitch);
> >  /* sync_console: force synchronous console output (useful for debugging). */
> >  static bool_t __initdata opt_sync_console;
> >  boolean_param("sync_console", opt_sync_console);
> > +static const char __initconst *warning_sync_console =
> 
> static const char __initconst warning_sync_console[] =
> 

Fixed.

Wei.

> Jan
> 

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

Re: [PATCH v3 3/3] xen: make available hvm_fep to non-debug build as well

[Wei Liu]

On Wed, Jun 22, 2016 at 09:42:38AM -0600, Jan Beulich wrote:
> >>> On 20.06.16 at 18:30, <wei.liu2@citrix.com> wrote:
> > @@ -97,9 +98,17 @@ boolean_param("hap", opt_hap_enabled);
> >  
> >  #ifndef opt_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
> > +static const char __initconst *warning_hvm_fep =
> > +    "**********************************************\n"
> > +    "******* WARNING: HVM FORCED EMULATION PREFIX IS AVAILABLE\n"
> > +    "******* This option is *ONLY* intended to aid testing of Xen.\n"
> > +    "******* It has implications on the security of the system.\n"
> > +    "******* Please *DO NOT* use this in production.\n"
> > +    "**********************************************\n";
> > +
> >  
> 
> Along with the same comment as on patch 2, here even more than
> there I wonder whether this string wouldn't better be a static local
> in hvm_enable() (or even the scope therein where warning_add()
> gets invoked).
> 

I would rather the text stays next to where the option is defined so
it is obvious to anyone who touches this area of code.

> Also adding a stray blank line.
> 

Fixed. Also fixed the issue mentioned in patch 2.

Wei.

> Jan
> 

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

Re: [PATCH v3 1/3] xen: add warning infrastructure

[Wei Liu]

On Thu, Jun 23, 2016 at 11:37:43AM +0100, Wei Liu wrote:
[...]
> The warning text (multiple lines) is added as one single string, which
> means we can't trivially add leading stars at the beginning of each
> line.
> 
> I don't feel like arguing over how the text would look like, so if
> something like:
> 
>     ************************************************
>     WARNING 1
>     ************************************************
>     WARNING 2
>     ************************************************
>     ...
>     ************************************************
>     WARNING N
>     ************************************************
> 
> is ok to you, I'm fine with using that format, too.

Here is an example for how it looks like at the moment. Feels OK to me.

(XEN) ***************************************************
(XEN) WARNING: CONSOLE OUTPUT IS SYNCHRONOUS
(XEN) This option is intended to aid debugging of Xen by ensuring
(XEN) that all output is synchronously delivered on the serial line.
(XEN) However it can introduce SIGNIFICANT latencies and affect
(XEN) timekeeping. It is NOT recommended for production use!
(XEN) ***************************************************
(XEN) WARNING: HVM FORCED EMULATION PREFIX IS AVAILABLE
(XEN) This option is *ONLY* intended to aid testing of Xen.
(XEN) It has implications on the security of the system.
(XEN) Please *DO NOT* use this in production.
(XEN) ***************************************************


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

Re: [PATCH v3 1/3] xen: add warning infrastructure

[Andrew Cooper]

On 23/06/16 12:17, Wei Liu wrote:
> On Thu, Jun 23, 2016 at 11:37:43AM +0100, Wei Liu wrote:
> [...]
>> The warning text (multiple lines) is added as one single string, which
>> means we can't trivially add leading stars at the beginning of each
>> line.
>>
>> I don't feel like arguing over how the text would look like, so if
>> something like:
>>
>>     ************************************************
>>     WARNING 1
>>     ************************************************
>>     WARNING 2
>>     ************************************************
>>     ...
>>     ************************************************
>>     WARNING N
>>     ************************************************
>>
>> is ok to you, I'm fine with using that format, too.
> Here is an example for how it looks like at the moment. Feels OK to me.
>
> (XEN) ***************************************************
> (XEN) WARNING: CONSOLE OUTPUT IS SYNCHRONOUS
> (XEN) This option is intended to aid debugging of Xen by ensuring
> (XEN) that all output is synchronously delivered on the serial line.
> (XEN) However it can introduce SIGNIFICANT latencies and affect
> (XEN) timekeeping. It is NOT recommended for production use!
> (XEN) ***************************************************
> (XEN) WARNING: HVM FORCED EMULATION PREFIX IS AVAILABLE
> (XEN) This option is *ONLY* intended to aid testing of Xen.
> (XEN) It has implications on the security of the system.
> (XEN) Please *DO NOT* use this in production.
> (XEN) ***************************************************

LGTM.

~Andrew

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

Re: [PATCH v3 1/3] xen: add warning infrastructure

[Jan Beulich]

>>> On 23.06.16 at 13:17, <wei.liu2@citrix.com> wrote:
> On Thu, Jun 23, 2016 at 11:37:43AM +0100, Wei Liu wrote:
> [...]
>> The warning text (multiple lines) is added as one single string, which
>> means we can't trivially add leading stars at the beginning of each
>> line.
>> 
>> I don't feel like arguing over how the text would look like, so if
>> something like:
>> 
>>     ************************************************
>>     WARNING 1
>>     ************************************************
>>     WARNING 2
>>     ************************************************
>>     ...
>>     ************************************************
>>     WARNING N
>>     ************************************************
>> 
>> is ok to you, I'm fine with using that format, too.
> 
> Here is an example for how it looks like at the moment. Feels OK to me.
> 
> (XEN) ***************************************************
> (XEN) WARNING: CONSOLE OUTPUT IS SYNCHRONOUS
> (XEN) This option is intended to aid debugging of Xen by ensuring
> (XEN) that all output is synchronously delivered on the serial line.
> (XEN) However it can introduce SIGNIFICANT latencies and affect
> (XEN) timekeeping. It is NOT recommended for production use!
> (XEN) ***************************************************
> (XEN) WARNING: HVM FORCED EMULATION PREFIX IS AVAILABLE
> (XEN) This option is *ONLY* intended to aid testing of Xen.
> (XEN) It has implications on the security of the system.
> (XEN) Please *DO NOT* use this in production.
> (XEN) ***************************************************

Looks good, thanks.

Jan


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

Re: [PATCH v3 3/3] xen: make available hvm_fep to non-debug build as well

[Jan Beulich]

>>> On 23.06.16 at 12:50, <wei.liu2@citrix.com> wrote:
> On Wed, Jun 22, 2016 at 09:42:38AM -0600, Jan Beulich wrote:
>> >>> On 20.06.16 at 18:30, <wei.liu2@citrix.com> wrote:
>> > @@ -97,9 +98,17 @@ boolean_param("hap", opt_hap_enabled);
>> >  
>> >  #ifndef opt_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
>> > +static const char __initconst *warning_hvm_fep =
>> > +    "**********************************************\n"
>> > +    "******* WARNING: HVM FORCED EMULATION PREFIX IS AVAILABLE\n"
>> > +    "******* This option is *ONLY* intended to aid testing of Xen.\n"
>> > +    "******* It has implications on the security of the system.\n"
>> > +    "******* Please *DO NOT* use this in production.\n"
>> > +    "**********************************************\n";
>> > +
>> >  
>> 
>> Along with the same comment as on patch 2, here even more than
>> there I wonder whether this string wouldn't better be a static local
>> in hvm_enable() (or even the scope therein where warning_add()
>> gets invoked).
> 
> I would rather the text stays next to where the option is defined so
> it is obvious to anyone who touches this area of code.

Makes sense. But then - shouldn't it move inside the #ifndef?

Jan


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

Re: [PATCH v3 3/3] xen: make available hvm_fep to non-debug build as well

[Wei Liu]

On Thu, Jun 23, 2016 at 06:20:38AM -0600, Jan Beulich wrote:
> >>> On 23.06.16 at 12:50, <wei.liu2@citrix.com> wrote:
> > On Wed, Jun 22, 2016 at 09:42:38AM -0600, Jan Beulich wrote:
> >> >>> On 20.06.16 at 18:30, <wei.liu2@citrix.com> wrote:
> >> > @@ -97,9 +98,17 @@ boolean_param("hap", opt_hap_enabled);
> >> >  
> >> >  #ifndef opt_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
> >> > +static const char __initconst *warning_hvm_fep =
> >> > +    "**********************************************\n"
> >> > +    "******* WARNING: HVM FORCED EMULATION PREFIX IS AVAILABLE\n"
> >> > +    "******* This option is *ONLY* intended to aid testing of Xen.\n"
> >> > +    "******* It has implications on the security of the system.\n"
> >> > +    "******* Please *DO NOT* use this in production.\n"
> >> > +    "**********************************************\n";
> >> > +
> >> >  
> >> 
> >> Along with the same comment as on patch 2, here even more than
> >> there I wonder whether this string wouldn't better be a static local
> >> in hvm_enable() (or even the scope therein where warning_add()
> >> gets invoked).
> > 
> > I would rather the text stays next to where the option is defined so
> > it is obvious to anyone who touches this area of code.
> 
> Makes sense. But then - shouldn't it move inside the #ifndef?
> 

That would then require the warning_add() call to be wrapped in ifdef,
too. That looks a bit cumbersome to me.

Wei.

> Jan
> 

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

Re: [PATCH v3 3/3] xen: make available hvm_fep to non-debug build as well

[Andrew Cooper]

On 23/06/16 13:44, Wei Liu wrote:
> On Thu, Jun 23, 2016 at 06:20:38AM -0600, Jan Beulich wrote:
>>>>> On 23.06.16 at 12:50, <wei.liu2@citrix.com> wrote:
>>> On Wed, Jun 22, 2016 at 09:42:38AM -0600, Jan Beulich wrote:
>>>>>>> On 20.06.16 at 18:30, <wei.liu2@citrix.com> wrote:
>>>>> @@ -97,9 +98,17 @@ boolean_param("hap", opt_hap_enabled);
>>>>>  
>>>>>  #ifndef opt_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
>>>>> +static const char __initconst *warning_hvm_fep =
>>>>> +    "**********************************************\n"
>>>>> +    "******* WARNING: HVM FORCED EMULATION PREFIX IS AVAILABLE\n"
>>>>> +    "******* This option is *ONLY* intended to aid testing of Xen.\n"
>>>>> +    "******* It has implications on the security of the system.\n"
>>>>> +    "******* Please *DO NOT* use this in production.\n"
>>>>> +    "**********************************************\n";
>>>>> +
>>>>>  
>>>> Along with the same comment as on patch 2, here even more than
>>>> there I wonder whether this string wouldn't better be a static local
>>>> in hvm_enable() (or even the scope therein where warning_add()
>>>> gets invoked).
>>> I would rather the text stays next to where the option is defined so
>>> it is obvious to anyone who touches this area of code.
>> Makes sense. But then - shouldn't it move inside the #ifndef?
>>
> That would then require the warning_add() call to be wrapped in ifdef,
> too. That looks a bit cumbersome to me.

You can do something like:

#ifndef $FOO
#define warning_text NULL
#else
const static __initdata warning_text[] = "BAR";
#endif

This works in the same way as the current opt_hvm_fep define.

~Andrew

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

Re: [PATCH v3 3/3] xen: make available hvm_fep to non-debug build as well

[Wei Liu]

On Thu, Jun 23, 2016 at 01:48:35PM +0100, Andrew Cooper wrote:
> On 23/06/16 13:44, Wei Liu wrote:
> > On Thu, Jun 23, 2016 at 06:20:38AM -0600, Jan Beulich wrote:
> >>>>> On 23.06.16 at 12:50, <wei.liu2@citrix.com> wrote:
> >>> On Wed, Jun 22, 2016 at 09:42:38AM -0600, Jan Beulich wrote:
> >>>>>>> On 20.06.16 at 18:30, <wei.liu2@citrix.com> wrote:
> >>>>> @@ -97,9 +98,17 @@ boolean_param("hap", opt_hap_enabled);
> >>>>>  
> >>>>>  #ifndef opt_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
> >>>>> +static const char __initconst *warning_hvm_fep =
> >>>>> +    "**********************************************\n"
> >>>>> +    "******* WARNING: HVM FORCED EMULATION PREFIX IS AVAILABLE\n"
> >>>>> +    "******* This option is *ONLY* intended to aid testing of Xen.\n"
> >>>>> +    "******* It has implications on the security of the system.\n"
> >>>>> +    "******* Please *DO NOT* use this in production.\n"
> >>>>> +    "**********************************************\n";
> >>>>> +
> >>>>>  
> >>>> Along with the same comment as on patch 2, here even more than
> >>>> there I wonder whether this string wouldn't better be a static local
> >>>> in hvm_enable() (or even the scope therein where warning_add()
> >>>> gets invoked).
> >>> I would rather the text stays next to where the option is defined so
> >>> it is obvious to anyone who touches this area of code.
> >> Makes sense. But then - shouldn't it move inside the #ifndef?
> >>
> > That would then require the warning_add() call to be wrapped in ifdef,
> > too. That looks a bit cumbersome to me.
> 
> You can do something like:
> 
> #ifndef $FOO
> #define warning_text NULL
> #else
> const static __initdata warning_text[] = "BAR";
> #endif
> 
> This works in the same way as the current opt_hvm_fep define.
> 

Sure, that works for me, too.

Wei.

> ~Andrew

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

Re: [PATCH v3 3/3] xen: make available hvm_fep to non-debug build as well

[Jan Beulich]

>>> On 23.06.16 at 14:44, <wei.liu2@citrix.com> wrote:
> On Thu, Jun 23, 2016 at 06:20:38AM -0600, Jan Beulich wrote:
>> >>> On 23.06.16 at 12:50, <wei.liu2@citrix.com> wrote:
>> > On Wed, Jun 22, 2016 at 09:42:38AM -0600, Jan Beulich wrote:
>> >> >>> On 20.06.16 at 18:30, <wei.liu2@citrix.com> wrote:
>> >> > @@ -97,9 +98,17 @@ boolean_param("hap", opt_hap_enabled);
>> >> >  
>> >> >  #ifndef opt_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
>> >> > +static const char __initconst *warning_hvm_fep =
>> >> > +    "**********************************************\n"
>> >> > +    "******* WARNING: HVM FORCED EMULATION PREFIX IS AVAILABLE\n"
>> >> > +    "******* This option is *ONLY* intended to aid testing of Xen.\n"
>> >> > +    "******* It has implications on the security of the system.\n"
>> >> > +    "******* Please *DO NOT* use this in production.\n"
>> >> > +    "**********************************************\n";
>> >> > +
>> >> >  
>> >> 
>> >> Along with the same comment as on patch 2, here even more than
>> >> there I wonder whether this string wouldn't better be a static local
>> >> in hvm_enable() (or even the scope therein where warning_add()
>> >> gets invoked).
>> > 
>> > I would rather the text stays next to where the option is defined so
>> > it is obvious to anyone who touches this area of code.
>> 
>> Makes sense. But then - shouldn't it move inside the #ifndef?
>> 
> 
> That would then require the warning_add() call to be wrapped in ifdef,
> too. That looks a bit cumbersome to me.

Ah, right. Well, never mind then - the compiler will (hopefully) take
care of eliminating the unused string then.

Jan


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