[PATCH v12 1/9] x86: add generic resource (e.g. MSR) access hypercall

[PATCH v12 1/9] x86: add generic resource (e.g. MSR) access hypercall

[Dongxiao Xu]

Add a generic resource access hypercall for tool stack or other
components, e.g., accessing MSR, port I/O, etc.

Signed-off-by: Dongxiao Xu <dongxiao.xu@intel.com>
---
 xen/arch/x86/Makefile             |    1 +
 xen/arch/x86/platform_hypercall.c |   39 ++++++++++++
 xen/arch/x86/resource.c           |  119 +++++++++++++++++++++++++++++++++++++
 xen/include/asm-x86/resource.h    |   40 +++++++++++++
 xen/include/public/platform.h     |   24 ++++++++
 xen/include/xlat.lst              |    1 +
 6 files changed, 224 insertions(+)
 create mode 100644 xen/arch/x86/resource.c
 create mode 100644 xen/include/asm-x86/resource.h

diff --git a/xen/arch/x86/Makefile b/xen/arch/x86/Makefile
index 6c90b1b..e0cee24 100644
--- a/xen/arch/x86/Makefile
+++ b/xen/arch/x86/Makefile
@@ -59,6 +59,7 @@ obj-y += crash.o
 obj-y += tboot.o
 obj-y += hpet.o
 obj-y += xstate.o
+obj-y += resource.o
 
 obj-$(crash_debug) += gdbstub.o
 
diff --git a/xen/arch/x86/platform_hypercall.c b/xen/arch/x86/platform_hypercall.c
index 2162811..da3d6c4 100644
--- a/xen/arch/x86/platform_hypercall.c
+++ b/xen/arch/x86/platform_hypercall.c
@@ -32,6 +32,7 @@
 #include <asm/setup.h>
 #include "cpu/mtrr/mtrr.h"
 #include <xsm/xsm.h>
+#include <asm/resource.h>
 
 #ifndef COMPAT
 typedef long ret_t;
@@ -601,6 +602,44 @@ ret_t do_platform_op(XEN_GUEST_HANDLE_PARAM(xen_platform_op_t) u_xenpf_op)
     }
     break;
 
+    case XENPF_resource_op:
+    {
+        struct xen_resource_access info;
+
+        info.nr = op->u.resource_op.nr;
+        info.type = op->u.resource_op.type;
+        info.data = xmalloc_array(xenpf_resource_data_t, info.nr);
+        if ( !info.data )
+        {
+            ret = -ENOMEM;
+            break;
+        }
+
+        if ( copy_from_guest(info.data, op->u.resource_op.data, info.nr) )
+        {
+            xfree(info.data);
+            ret = -EFAULT;
+            break;
+        }
+
+        ret = resource_access_helper(&info);
+        if ( ret )
+        {
+            xfree(info.data);
+            break;
+        }
+
+        if ( copy_to_guest(op->u.resource_op.data, info.data, info.nr) )
+        {
+            xfree(info.data);
+            ret = -EFAULT;
+            break;
+        }
+
+        xfree(info.data);
+    }
+    break;
+
     default:
         ret = -ENOSYS;
         break;
diff --git a/xen/arch/x86/resource.c b/xen/arch/x86/resource.c
new file mode 100644
index 0000000..cc548cd
--- /dev/null
+++ b/xen/arch/x86/resource.c
@@ -0,0 +1,119 @@
+/*
+ * resource.c: Helpers for Dom0 to access system resource
+ *
+ * Copyright (c) 2014, Intel Corporation
+ * Author: Dongxiao Xu <dongxiao.xu@intel.com>
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms and conditions of the GNU General Public License,
+ * version 2, as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
+ * more details.
+ */
+
+#include <xen/types.h>
+#include <xen/domain.h>
+#include <xen/guest_access.h>
+#include <public/platform.h>
+#include <asm/msr.h>
+#include <asm/event.h>
+#include <asm/resource.h>
+
+static int resource_access_one(uint32_t type, uint32_t cmd,
+                                uint64_t idx, uint64_t *val)
+{
+    int ret = 0;
+
+    switch ( type )
+    {
+    case XEN_RESOURCE_TYPE_MSR:
+        if ( cmd == XEN_RESOURCE_OP_READ )
+            ret = rdmsr_safe(idx, *val);
+        else if ( cmd == XEN_RESOURCE_OP_WRITE )
+            ret = wrmsr_safe(idx, *val);
+        break;
+
+    default:
+        gdprintk(XENLOG_WARNING, "unsupported resource type: %d\n", type);
+        ret = -ENOSYS;
+        break;
+    }
+
+    return ret;
+}
+
+static void resource_access_multi(void *param)
+{
+    struct xen_resource_access *info = param;
+    unsigned int i;
+    int ret = 0;
+
+    for ( i = 0; i < info->nr; i++ )
+    {
+        if ( !is_idle_vcpu(current) && hypercall_preempt_check() )
+        {
+            ret = -ERESTART;
+            break;
+        }
+        ret = resource_access_one(info->type, info->data[i].cmd,
+                                  info->data[i].idx, &info->data[i].val);
+        if ( ret )
+            break;
+    }
+
+    info->ret = ret;
+}
+
+int resource_access_helper(struct xen_resource_access *info)
+{
+    struct xen_resource_access iter;
+    unsigned int i, last_cpu = ~0;
+
+    iter.ret = 0;
+    iter.nr = 0;
+    iter.type = info->type;
+    iter.data = info->data;
+
+    for ( i = 0; i < info->nr; i++ )
+    {
+        if ( iter.nr && info->data[i].cpu != last_cpu )
+        {
+            if ( last_cpu == smp_processor_id() )
+                resource_access_multi(&iter);
+            else
+                /* Set wait=1 to ensure the access order  */
+                on_selected_cpus(cpumask_of(last_cpu),
+                                 resource_access_multi, &iter, 1);
+
+            if ( iter.ret )
+                return iter.ret;
+
+            iter.nr = 0;
+            iter.data = &info->data[i];
+        }
+
+        last_cpu = info->data[i].cpu;
+        iter.nr++;
+    }
+
+    if ( last_cpu == smp_processor_id() )
+        resource_access_multi(&iter);
+    else
+        on_selected_cpus(cpumask_of(last_cpu),
+                         resource_access_multi, &iter, 1);
+
+    return iter.ret;
+}
+
+/*
+ * 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/asm-x86/resource.h b/xen/include/asm-x86/resource.h
new file mode 100644
index 0000000..74b46be
--- /dev/null
+++ b/xen/include/asm-x86/resource.h
@@ -0,0 +1,40 @@
+/*
+ * resource.h: Helpers for Dom0 to access system resource
+ *
+ * Copyright (c) 2014, Intel Corporation
+ * Author: Dongxiao Xu <dongxiao.xu@intel.com>
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms and conditions of the GNU General Public License,
+ * version 2, as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
+ * more details.
+ */
+#ifndef __ASM_RESOURCE_H__
+#define __ASM_RESOURCE_H__
+
+#include <public/platform.h>
+
+struct xen_resource_access {
+    int32_t ret;
+    uint32_t nr;
+    uint32_t type;
+    xenpf_resource_data_t *data;
+};
+
+int resource_access_helper(struct xen_resource_access *info);
+
+#endif /* __ASM_RESOURCE_H__ */
+
+/*
+ * 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/public/platform.h b/xen/include/public/platform.h
index 053b9fa..eafdc8a 100644
--- a/xen/include/public/platform.h
+++ b/xen/include/public/platform.h
@@ -527,6 +527,29 @@ struct xenpf_core_parking {
 typedef struct xenpf_core_parking xenpf_core_parking_t;
 DEFINE_XEN_GUEST_HANDLE(xenpf_core_parking_t);
 
+#define XENPF_resource_op   61
+
+#define XEN_RESOURCE_OP_READ  0
+#define XEN_RESOURCE_OP_WRITE 1
+
+#define XEN_RESOURCE_TYPE_MSR  0
+
+struct xenpf_resource_data {
+    uint32_t cmd;       /* XEN_RESOURCE_OP_* */
+    uint32_t cpu;
+    uint64_t idx;
+    uint64_t val;
+};
+typedef struct xenpf_resource_data xenpf_resource_data_t;
+DEFINE_XEN_GUEST_HANDLE(xenpf_resource_data_t);
+struct xenpf_resource_op {
+    uint32_t nr;
+    uint32_t type;      /* XEN_RESOURCE_TYPE_* */
+    XEN_GUEST_HANDLE(xenpf_resource_data_t) data;
+};
+typedef struct xenpf_resource_op xenpf_resource_op_t;
+DEFINE_XEN_GUEST_HANDLE(xenpf_resource_op_t);
+
 /*
  * ` enum neg_errnoval
  * ` HYPERVISOR_platform_op(const struct xen_platform_op*);
@@ -553,6 +576,7 @@ struct xen_platform_op {
         struct xenpf_cpu_hotadd        cpu_add;
         struct xenpf_mem_hotadd        mem_add;
         struct xenpf_core_parking      core_parking;
+        struct xenpf_resource_op       resource_op;
         uint8_t                        pad[128];
     } u;
 };
diff --git a/xen/include/xlat.lst b/xen/include/xlat.lst
index 9a35dd7..06ed7b9 100644
--- a/xen/include/xlat.lst
+++ b/xen/include/xlat.lst
@@ -88,6 +88,7 @@
 ?	xenpf_enter_acpi_sleep		platform.h
 ?	xenpf_pcpuinfo			platform.h
 ?	xenpf_pcpu_version		platform.h
+?	xenpf_resource_op		platform.h
 !	sched_poll			sched.h
 ?	sched_remote_shutdown		sched.h
 ?	sched_shutdown			sched.h
-- 
1.7.9.5

Re: [PATCH v12 1/9] x86: add generic resource (e.g. MSR) access hypercall

[Andrew Cooper]

On 04/07/14 09:34, Dongxiao Xu wrote:
> Add a generic resource access hypercall for tool stack or other
> components, e.g., accessing MSR, port I/O, etc.
>
> Signed-off-by: Dongxiao Xu <dongxiao.xu@intel.com>

This still permits a user of the hypercalls to play with EFER or
SYSENTER_EIP, which obviously is a very bad thing.

There needs to be a whitelist of permitted MSRs which can be accessed.

~Andrew

> ---
>  xen/arch/x86/Makefile             |    1 +
>  xen/arch/x86/platform_hypercall.c |   39 ++++++++++++
>  xen/arch/x86/resource.c           |  119 +++++++++++++++++++++++++++++++++++++
>  xen/include/asm-x86/resource.h    |   40 +++++++++++++
>  xen/include/public/platform.h     |   24 ++++++++
>  xen/include/xlat.lst              |    1 +
>  6 files changed, 224 insertions(+)
>  create mode 100644 xen/arch/x86/resource.c
>  create mode 100644 xen/include/asm-x86/resource.h
>
> diff --git a/xen/arch/x86/Makefile b/xen/arch/x86/Makefile
> index 6c90b1b..e0cee24 100644
> --- a/xen/arch/x86/Makefile
> +++ b/xen/arch/x86/Makefile
> @@ -59,6 +59,7 @@ obj-y += crash.o
>  obj-y += tboot.o
>  obj-y += hpet.o
>  obj-y += xstate.o
> +obj-y += resource.o
>  
>  obj-$(crash_debug) += gdbstub.o
>  
> diff --git a/xen/arch/x86/platform_hypercall.c b/xen/arch/x86/platform_hypercall.c
> index 2162811..da3d6c4 100644
> --- a/xen/arch/x86/platform_hypercall.c
> +++ b/xen/arch/x86/platform_hypercall.c
> @@ -32,6 +32,7 @@
>  #include <asm/setup.h>
>  #include "cpu/mtrr/mtrr.h"
>  #include <xsm/xsm.h>
> +#include <asm/resource.h>
>  
>  #ifndef COMPAT
>  typedef long ret_t;
> @@ -601,6 +602,44 @@ ret_t do_platform_op(XEN_GUEST_HANDLE_PARAM(xen_platform_op_t) u_xenpf_op)
>      }
>      break;
>  
> +    case XENPF_resource_op:
> +    {
> +        struct xen_resource_access info;
> +
> +        info.nr = op->u.resource_op.nr;
> +        info.type = op->u.resource_op.type;
> +        info.data = xmalloc_array(xenpf_resource_data_t, info.nr);
> +        if ( !info.data )
> +        {
> +            ret = -ENOMEM;
> +            break;
> +        }
> +
> +        if ( copy_from_guest(info.data, op->u.resource_op.data, info.nr) )
> +        {
> +            xfree(info.data);
> +            ret = -EFAULT;
> +            break;
> +        }
> +
> +        ret = resource_access_helper(&info);
> +        if ( ret )
> +        {
> +            xfree(info.data);
> +            break;
> +        }
> +
> +        if ( copy_to_guest(op->u.resource_op.data, info.data, info.nr) )
> +        {
> +            xfree(info.data);
> +            ret = -EFAULT;
> +            break;
> +        }
> +
> +        xfree(info.data);
> +    }
> +    break;
> +
>      default:
>          ret = -ENOSYS;
>          break;
> diff --git a/xen/arch/x86/resource.c b/xen/arch/x86/resource.c
> new file mode 100644
> index 0000000..cc548cd
> --- /dev/null
> +++ b/xen/arch/x86/resource.c
> @@ -0,0 +1,119 @@
> +/*
> + * resource.c: Helpers for Dom0 to access system resource
> + *
> + * Copyright (c) 2014, Intel Corporation
> + * Author: Dongxiao Xu <dongxiao.xu@intel.com>
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms and conditions of the GNU General Public License,
> + * version 2, as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope it will be useful, but WITHOUT
> + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
> + * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
> + * more details.
> + */
> +
> +#include <xen/types.h>
> +#include <xen/domain.h>
> +#include <xen/guest_access.h>
> +#include <public/platform.h>
> +#include <asm/msr.h>
> +#include <asm/event.h>
> +#include <asm/resource.h>
> +
> +static int resource_access_one(uint32_t type, uint32_t cmd,
> +                                uint64_t idx, uint64_t *val)
> +{
> +    int ret = 0;
> +
> +    switch ( type )
> +    {
> +    case XEN_RESOURCE_TYPE_MSR:
> +        if ( cmd == XEN_RESOURCE_OP_READ )
> +            ret = rdmsr_safe(idx, *val);
> +        else if ( cmd == XEN_RESOURCE_OP_WRITE )
> +            ret = wrmsr_safe(idx, *val);
> +        break;
> +
> +    default:
> +        gdprintk(XENLOG_WARNING, "unsupported resource type: %d\n", type);
> +        ret = -ENOSYS;
> +        break;
> +    }
> +
> +    return ret;
> +}
> +
> +static void resource_access_multi(void *param)
> +{
> +    struct xen_resource_access *info = param;
> +    unsigned int i;
> +    int ret = 0;
> +
> +    for ( i = 0; i < info->nr; i++ )
> +    {
> +        if ( !is_idle_vcpu(current) && hypercall_preempt_check() )
> +        {
> +            ret = -ERESTART;
> +            break;
> +        }
> +        ret = resource_access_one(info->type, info->data[i].cmd,
> +                                  info->data[i].idx, &info->data[i].val);
> +        if ( ret )
> +            break;
> +    }
> +
> +    info->ret = ret;
> +}
> +
> +int resource_access_helper(struct xen_resource_access *info)
> +{
> +    struct xen_resource_access iter;
> +    unsigned int i, last_cpu = ~0;
> +
> +    iter.ret = 0;
> +    iter.nr = 0;
> +    iter.type = info->type;
> +    iter.data = info->data;
> +
> +    for ( i = 0; i < info->nr; i++ )
> +    {
> +        if ( iter.nr && info->data[i].cpu != last_cpu )
> +        {
> +            if ( last_cpu == smp_processor_id() )
> +                resource_access_multi(&iter);
> +            else
> +                /* Set wait=1 to ensure the access order  */
> +                on_selected_cpus(cpumask_of(last_cpu),
> +                                 resource_access_multi, &iter, 1);
> +
> +            if ( iter.ret )
> +                return iter.ret;
> +
> +            iter.nr = 0;
> +            iter.data = &info->data[i];
> +        }
> +
> +        last_cpu = info->data[i].cpu;
> +        iter.nr++;
> +    }
> +
> +    if ( last_cpu == smp_processor_id() )
> +        resource_access_multi(&iter);
> +    else
> +        on_selected_cpus(cpumask_of(last_cpu),
> +                         resource_access_multi, &iter, 1);
> +
> +    return iter.ret;
> +}
> +
> +/*
> + * 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/asm-x86/resource.h b/xen/include/asm-x86/resource.h
> new file mode 100644
> index 0000000..74b46be
> --- /dev/null
> +++ b/xen/include/asm-x86/resource.h
> @@ -0,0 +1,40 @@
> +/*
> + * resource.h: Helpers for Dom0 to access system resource
> + *
> + * Copyright (c) 2014, Intel Corporation
> + * Author: Dongxiao Xu <dongxiao.xu@intel.com>
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms and conditions of the GNU General Public License,
> + * version 2, as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope it will be useful, but WITHOUT
> + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
> + * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
> + * more details.
> + */
> +#ifndef __ASM_RESOURCE_H__
> +#define __ASM_RESOURCE_H__
> +
> +#include <public/platform.h>
> +
> +struct xen_resource_access {
> +    int32_t ret;
> +    uint32_t nr;
> +    uint32_t type;
> +    xenpf_resource_data_t *data;
> +};
> +
> +int resource_access_helper(struct xen_resource_access *info);
> +
> +#endif /* __ASM_RESOURCE_H__ */
> +
> +/*
> + * 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/public/platform.h b/xen/include/public/platform.h
> index 053b9fa..eafdc8a 100644
> --- a/xen/include/public/platform.h
> +++ b/xen/include/public/platform.h
> @@ -527,6 +527,29 @@ struct xenpf_core_parking {
>  typedef struct xenpf_core_parking xenpf_core_parking_t;
>  DEFINE_XEN_GUEST_HANDLE(xenpf_core_parking_t);
>  
> +#define XENPF_resource_op   61
> +
> +#define XEN_RESOURCE_OP_READ  0
> +#define XEN_RESOURCE_OP_WRITE 1
> +
> +#define XEN_RESOURCE_TYPE_MSR  0
> +
> +struct xenpf_resource_data {
> +    uint32_t cmd;       /* XEN_RESOURCE_OP_* */
> +    uint32_t cpu;
> +    uint64_t idx;
> +    uint64_t val;
> +};
> +typedef struct xenpf_resource_data xenpf_resource_data_t;
> +DEFINE_XEN_GUEST_HANDLE(xenpf_resource_data_t);
> +struct xenpf_resource_op {
> +    uint32_t nr;
> +    uint32_t type;      /* XEN_RESOURCE_TYPE_* */
> +    XEN_GUEST_HANDLE(xenpf_resource_data_t) data;
> +};
> +typedef struct xenpf_resource_op xenpf_resource_op_t;
> +DEFINE_XEN_GUEST_HANDLE(xenpf_resource_op_t);
> +
>  /*
>   * ` enum neg_errnoval
>   * ` HYPERVISOR_platform_op(const struct xen_platform_op*);
> @@ -553,6 +576,7 @@ struct xen_platform_op {
>          struct xenpf_cpu_hotadd        cpu_add;
>          struct xenpf_mem_hotadd        mem_add;
>          struct xenpf_core_parking      core_parking;
> +        struct xenpf_resource_op       resource_op;
>          uint8_t                        pad[128];
>      } u;
>  };
> diff --git a/xen/include/xlat.lst b/xen/include/xlat.lst
> index 9a35dd7..06ed7b9 100644
> --- a/xen/include/xlat.lst
> +++ b/xen/include/xlat.lst
> @@ -88,6 +88,7 @@
>  ?	xenpf_enter_acpi_sleep		platform.h
>  ?	xenpf_pcpuinfo			platform.h
>  ?	xenpf_pcpu_version		platform.h
> +?	xenpf_resource_op		platform.h
>  !	sched_poll			sched.h
>  ?	sched_remote_shutdown		sched.h
>  ?	sched_shutdown			sched.h

Re: [PATCH v12 1/9] x86: add generic resource (e.g. MSR) access hypercall

[Jan Beulich]

>>> On 04.07.14 at 11:40, <andrew.cooper3@citrix.com> wrote:
> On 04/07/14 09:34, Dongxiao Xu wrote:
>> Add a generic resource access hypercall for tool stack or other
>> components, e.g., accessing MSR, port I/O, etc.
>>
>> Signed-off-by: Dongxiao Xu <dongxiao.xu@intel.com>
> 
> This still permits a user of the hypercalls to play with EFER or
> SYSENTER_EIP, which obviously is a very bad thing.
> 
> There needs to be a whitelist of permitted MSRs which can be accessed.

Hmm, I'm not sure. One particular purpose I see here is to allow the
tool stack (or Dom0) access to MSRs Xen may not know about (yet).
Furthermore, this being a platform op, only the hardware domain
should ever have access, and it certainly ought to know what it's
doing. So the sum of these two considerations is: If at all, we may
want a black list here.

Jan

Re: [PATCH v12 1/9] x86: add generic resource (e.g. MSR) access hypercall

[Jan Beulich]

>>> On 04.07.14 at 10:34, <dongxiao.xu@intel.com> wrote:
> Add a generic resource access hypercall for tool stack or other
> components, e.g., accessing MSR, port I/O, etc.

Sigh - you're still allocating an unbounded amount of memory for
passing around the input arguments, despite it being possible (and
having been suggested) to read these from the original buffer on
each iteration. You're still not properly checking for preemption
between iterations. And you're still not making use of
continue_hypercall_on_cpu(). Plus you now silently ignore the
upper 32-bits of the passing in "idx" value as well as not
understood XEN_RESOURCE_OP_* values.

I also doubt that this warrants a new source file to be introduced,
the helper functions (if indeed needed) can very well live in
platform_hypercall.c.

Jan

Re: [PATCH v12 1/9] x86: add generic resource (e.g. MSR) access hypercall

[Andrew Cooper]

On 04/07/14 11:30, Jan Beulich wrote:
>>>> On 04.07.14 at 11:40, <andrew.cooper3@citrix.com> wrote:
>> On 04/07/14 09:34, Dongxiao Xu wrote:
>>> Add a generic resource access hypercall for tool stack or other
>>> components, e.g., accessing MSR, port I/O, etc.
>>>
>>> Signed-off-by: Dongxiao Xu <dongxiao.xu@intel.com>
>> This still permits a user of the hypercalls to play with EFER or
>> SYSENTER_EIP, which obviously is a very bad thing.
>>
>> There needs to be a whitelist of permitted MSRs which can be accessed.
> Hmm, I'm not sure. One particular purpose I see here is to allow the
> tool stack (or Dom0) access to MSRs Xen may not know about (yet).
> Furthermore, this being a platform op, only the hardware domain
> should ever have access, and it certainly ought to know what it's
> doing. So the sum of these two considerations is: If at all, we may
> want a black list here.
>
> Jan
>

I don't think it is safe for the toolstack to ever be playing with MSRs
which Xen is completely unaware of.  There is no guarentee whatsoever
that a new MSR which Xen is unaware of doesn't have security
implications if the toolstack were to play with it.

Adding entries to a whitelist is easy and could be considered a
maintenance activity similar to keeping the model/stepping information
up-to-date.

~Andrew

Re: [PATCH v12 1/9] x86: add generic resource (e.g. MSR) access hypercall

[Xu, Dongxiao]

> -----Original Message-----
> From: Andrew Cooper [mailto:andrew.cooper3@citrix.com]
> Sent: Friday, July 04, 2014 6:53 PM
> To: Jan Beulich; Xu, Dongxiao; xen-devel@lists.xen.org
> Cc: Ian.Campbell@citrix.com; George.Dunlap@eu.citrix.com;
> Ian.Jackson@eu.citrix.com; stefano.stabellini@eu.citrix.com;
> konrad.wilk@oracle.com; dgdegra@tycho.nsa.gov; keir@xen.org
> Subject: Re: [PATCH v12 1/9] x86: add generic resource (e.g. MSR) access
> hypercall
> 
> On 04/07/14 11:30, Jan Beulich wrote:
> >>>> On 04.07.14 at 11:40, <andrew.cooper3@citrix.com> wrote:
> >> On 04/07/14 09:34, Dongxiao Xu wrote:
> >>> Add a generic resource access hypercall for tool stack or other
> >>> components, e.g., accessing MSR, port I/O, etc.
> >>>
> >>> Signed-off-by: Dongxiao Xu <dongxiao.xu@intel.com>
> >> This still permits a user of the hypercalls to play with EFER or
> >> SYSENTER_EIP, which obviously is a very bad thing.
> >>
> >> There needs to be a whitelist of permitted MSRs which can be accessed.
> > Hmm, I'm not sure. One particular purpose I see here is to allow the
> > tool stack (or Dom0) access to MSRs Xen may not know about (yet).
> > Furthermore, this being a platform op, only the hardware domain
> > should ever have access, and it certainly ought to know what it's
> > doing. So the sum of these two considerations is: If at all, we may
> > want a black list here.
> >
> > Jan
> >
> 
> I don't think it is safe for the toolstack to ever be playing with MSRs
> which Xen is completely unaware of.  There is no guarentee whatsoever
> that a new MSR which Xen is unaware of doesn't have security
> implications if the toolstack were to play with it.
> 
> Adding entries to a whitelist is easy and could be considered a
> maintenance activity similar to keeping the model/stepping information
> up-to-date.

This resource access mechanism is useful according to some conversation with IPDC customers. Per their description, once the machine and VMs are online, they will not be turned off. Sometimes administrators may need to dynamically modify some resource values to fix/workaround certain issues, and our patch may serve this purpose.

Adding the white/black list will bring certain constraints for the above use case. Also considering that the tool stack resides in dom0, I think it is not so dangerous.

Thanks,
Dongxiao

> 
> ~Andrew

Re: [PATCH v12 1/9] x86: add generic resource (e.g. MSR) access hypercall

[George Dunlap]

On 07/04/2014 11:52 AM, Andrew Cooper wrote:
> On 04/07/14 11:30, Jan Beulich wrote:
>>>>> On 04.07.14 at 11:40, <andrew.cooper3@citrix.com> wrote:
>>> On 04/07/14 09:34, Dongxiao Xu wrote:
>>>> Add a generic resource access hypercall for tool stack or other
>>>> components, e.g., accessing MSR, port I/O, etc.
>>>>
>>>> Signed-off-by: Dongxiao Xu <dongxiao.xu@intel.com>
>>> This still permits a user of the hypercalls to play with EFER or
>>> SYSENTER_EIP, which obviously is a very bad thing.
>>>
>>> There needs to be a whitelist of permitted MSRs which can be accessed.
>> Hmm, I'm not sure. One particular purpose I see here is to allow the
>> tool stack (or Dom0) access to MSRs Xen may not know about (yet).
>> Furthermore, this being a platform op, only the hardware domain
>> should ever have access, and it certainly ought to know what it's
>> doing. So the sum of these two considerations is: If at all, we may
>> want a black list here.
>>
>> Jan
>>
>
> I don't think it is safe for the toolstack to ever be playing with MSRs
> which Xen is completely unaware of.  There is no guarentee whatsoever
> that a new MSR which Xen is unaware of doesn't have security
> implications if the toolstack were to play with it.

But the toolstack is part of the trusted base; it should be thinking 
about the security implications as much as Xen should.

  -George

Re: [PATCH v12 1/9] x86: add generic resource (e.g. MSR) access hypercall

[Andrew Cooper]

On 08/07/14 08:06, Xu, Dongxiao wrote:
>> -----Original Message-----
>> From: Andrew Cooper [mailto:andrew.cooper3@citrix.com]
>> Sent: Friday, July 04, 2014 6:53 PM
>> To: Jan Beulich; Xu, Dongxiao; xen-devel@lists.xen.org
>> Cc: Ian.Campbell@citrix.com; George.Dunlap@eu.citrix.com;
>> Ian.Jackson@eu.citrix.com; stefano.stabellini@eu.citrix.com;
>> konrad.wilk@oracle.com; dgdegra@tycho.nsa.gov; keir@xen.org
>> Subject: Re: [PATCH v12 1/9] x86: add generic resource (e.g. MSR) access
>> hypercall
>>
>> On 04/07/14 11:30, Jan Beulich wrote:
>>>>>> On 04.07.14 at 11:40, <andrew.cooper3@citrix.com> wrote:
>>>> On 04/07/14 09:34, Dongxiao Xu wrote:
>>>>> Add a generic resource access hypercall for tool stack or other
>>>>> components, e.g., accessing MSR, port I/O, etc.
>>>>>
>>>>> Signed-off-by: Dongxiao Xu <dongxiao.xu@intel.com>
>>>> This still permits a user of the hypercalls to play with EFER or
>>>> SYSENTER_EIP, which obviously is a very bad thing.
>>>>
>>>> There needs to be a whitelist of permitted MSRs which can be accessed.
>>> Hmm, I'm not sure. One particular purpose I see here is to allow the
>>> tool stack (or Dom0) access to MSRs Xen may not know about (yet).
>>> Furthermore, this being a platform op, only the hardware domain
>>> should ever have access, and it certainly ought to know what it's
>>> doing. So the sum of these two considerations is: If at all, we may
>>> want a black list here.
>>>
>>> Jan
>>>
>> I don't think it is safe for the toolstack to ever be playing with MSRs
>> which Xen is completely unaware of.  There is no guarentee whatsoever
>> that a new MSR which Xen is unaware of doesn't have security
>> implications if the toolstack were to play with it.
>>
>> Adding entries to a whitelist is easy and could be considered a
>> maintenance activity similar to keeping the model/stepping information
>> up-to-date.
> This resource access mechanism is useful according to some conversation with IPDC customers. Per their description, once the machine and VMs are online, they will not be turned off. Sometimes administrators may need to dynamically modify some resource values to fix/workaround certain issues, and our patch may serve this purpose.
>
> Adding the white/black list will bring certain constraints for the above use case. Also considering that the tool stack resides in dom0, I think it is not so dangerous.

The whole purpose of XSM is to split the toolstack up so it isn't all in
dom0.

Extending a whitelist is trivial, and requires a positive action on
behalf of someone to decide that the added MSR *is* safe.  Anything else
is a security bug waiting to happen.

~Andrew

Re: [PATCH v12 1/9] x86: add generic resource (e.g. MSR) access hypercall

[Andrew Cooper]

On 08/07/14 09:57, George Dunlap wrote:
> On 07/04/2014 11:52 AM, Andrew Cooper wrote:
>> On 04/07/14 11:30, Jan Beulich wrote:
>>>>>> On 04.07.14 at 11:40, <andrew.cooper3@citrix.com> wrote:
>>>> On 04/07/14 09:34, Dongxiao Xu wrote:
>>>>> Add a generic resource access hypercall for tool stack or other
>>>>> components, e.g., accessing MSR, port I/O, etc.
>>>>>
>>>>> Signed-off-by: Dongxiao Xu <dongxiao.xu@intel.com>
>>>> This still permits a user of the hypercalls to play with EFER or
>>>> SYSENTER_EIP, which obviously is a very bad thing.
>>>>
>>>> There needs to be a whitelist of permitted MSRs which can be accessed.
>>> Hmm, I'm not sure. One particular purpose I see here is to allow the
>>> tool stack (or Dom0) access to MSRs Xen may not know about (yet).
>>> Furthermore, this being a platform op, only the hardware domain
>>> should ever have access, and it certainly ought to know what it's
>>> doing. So the sum of these two considerations is: If at all, we may
>>> want a black list here.
>>>
>>> Jan
>>>
>>
>> I don't think it is safe for the toolstack to ever be playing with MSRs
>> which Xen is completely unaware of.  There is no guarentee whatsoever
>> that a new MSR which Xen is unaware of doesn't have security
>> implications if the toolstack were to play with it.
>
> But the toolstack is part of the trusted base; it should be thinking
> about the security implications as much as Xen should.
>  -George
>

No - it very much isn't.  It has more privileges than a standard Xen
domain, and in some cases has powers to shoot itself in the foot, but
all these powers are all behind the Xen API which does provide
restrictions on what dom0/toolstack is permitted to do.

~Andrew

Re: [PATCH v12 1/9] x86: add generic resource (e.g. MSR) access hypercall

[Jürgen Groß]

On 07/08/2014 11:07 AM, Andrew Cooper wrote:
> On 08/07/14 08:06, Xu, Dongxiao wrote:
>>> -----Original Message-----
>>> From: Andrew Cooper [mailto:andrew.cooper3@citrix.com]
>>> Sent: Friday, July 04, 2014 6:53 PM
>>> To: Jan Beulich; Xu, Dongxiao; xen-devel@lists.xen.org
>>> Cc: Ian.Campbell@citrix.com; George.Dunlap@eu.citrix.com;
>>> Ian.Jackson@eu.citrix.com; stefano.stabellini@eu.citrix.com;
>>> konrad.wilk@oracle.com; dgdegra@tycho.nsa.gov; keir@xen.org
>>> Subject: Re: [PATCH v12 1/9] x86: add generic resource (e.g. MSR) access
>>> hypercall
>>>
>>> On 04/07/14 11:30, Jan Beulich wrote:
>>>>>>> On 04.07.14 at 11:40, <andrew.cooper3@citrix.com> wrote:
>>>>> On 04/07/14 09:34, Dongxiao Xu wrote:
>>>>>> Add a generic resource access hypercall for tool stack or other
>>>>>> components, e.g., accessing MSR, port I/O, etc.
>>>>>>
>>>>>> Signed-off-by: Dongxiao Xu <dongxiao.xu@intel.com>
>>>>> This still permits a user of the hypercalls to play with EFER or
>>>>> SYSENTER_EIP, which obviously is a very bad thing.
>>>>>
>>>>> There needs to be a whitelist of permitted MSRs which can be accessed.
>>>> Hmm, I'm not sure. One particular purpose I see here is to allow the
>>>> tool stack (or Dom0) access to MSRs Xen may not know about (yet).
>>>> Furthermore, this being a platform op, only the hardware domain
>>>> should ever have access, and it certainly ought to know what it's
>>>> doing. So the sum of these two considerations is: If at all, we may
>>>> want a black list here.
>>>>
>>>> Jan
>>>>
>>> I don't think it is safe for the toolstack to ever be playing with MSRs
>>> which Xen is completely unaware of.  There is no guarentee whatsoever
>>> that a new MSR which Xen is unaware of doesn't have security
>>> implications if the toolstack were to play with it.
>>>
>>> Adding entries to a whitelist is easy and could be considered a
>>> maintenance activity similar to keeping the model/stepping information
>>> up-to-date.
>> This resource access mechanism is useful according to some conversation with IPDC customers. Per their description, once the machine and VMs are online, they will not be turned off. Sometimes administrators may need to dynamically modify some resource values to fix/workaround certain issues, and our patch may serve this purpose.
>>
>> Adding the white/black list will bring certain constraints for the above use case. Also considering that the tool stack resides in dom0, I think it is not so dangerous.
>
> The whole purpose of XSM is to split the toolstack up so it isn't all in
> dom0.
>
> Extending a whitelist is trivial, and requires a positive action on
> behalf of someone to decide that the added MSR *is* safe.  Anything else
> is a security bug waiting to happen.

Why not adding a whitelist which is tested as default and make the test
switchable via a boot parameter?

Juergen

Re: [PATCH v12 1/9] x86: add generic resource (e.g. MSR) access hypercall

[Xu, Dongxiao]

> -----Original Message-----
> From: Andrew Cooper [mailto:andrew.cooper3@citrix.com]
> Sent: Tuesday, July 08, 2014 5:08 PM
> To: Xu, Dongxiao; Jan Beulich; xen-devel@lists.xen.org
> Cc: keir@xen.org; Ian.Campbell@citrix.com; George.Dunlap@eu.citrix.com;
> stefano.stabellini@eu.citrix.com; Ian.Jackson@eu.citrix.com;
> dgdegra@tycho.nsa.gov
> Subject: Re: [Xen-devel] [PATCH v12 1/9] x86: add generic resource (e.g. MSR)
> access hypercall
> 
> On 08/07/14 08:06, Xu, Dongxiao wrote:
> >> -----Original Message-----
> >> From: Andrew Cooper [mailto:andrew.cooper3@citrix.com]
> >> Sent: Friday, July 04, 2014 6:53 PM
> >> To: Jan Beulich; Xu, Dongxiao; xen-devel@lists.xen.org
> >> Cc: Ian.Campbell@citrix.com; George.Dunlap@eu.citrix.com;
> >> Ian.Jackson@eu.citrix.com; stefano.stabellini@eu.citrix.com;
> >> konrad.wilk@oracle.com; dgdegra@tycho.nsa.gov; keir@xen.org
> >> Subject: Re: [PATCH v12 1/9] x86: add generic resource (e.g. MSR) access
> >> hypercall
> >>
> >> On 04/07/14 11:30, Jan Beulich wrote:
> >>>>>> On 04.07.14 at 11:40, <andrew.cooper3@citrix.com> wrote:
> >>>> On 04/07/14 09:34, Dongxiao Xu wrote:
> >>>>> Add a generic resource access hypercall for tool stack or other
> >>>>> components, e.g., accessing MSR, port I/O, etc.
> >>>>>
> >>>>> Signed-off-by: Dongxiao Xu <dongxiao.xu@intel.com>
> >>>> This still permits a user of the hypercalls to play with EFER or
> >>>> SYSENTER_EIP, which obviously is a very bad thing.
> >>>>
> >>>> There needs to be a whitelist of permitted MSRs which can be accessed.
> >>> Hmm, I'm not sure. One particular purpose I see here is to allow the
> >>> tool stack (or Dom0) access to MSRs Xen may not know about (yet).
> >>> Furthermore, this being a platform op, only the hardware domain
> >>> should ever have access, and it certainly ought to know what it's
> >>> doing. So the sum of these two considerations is: If at all, we may
> >>> want a black list here.
> >>>
> >>> Jan
> >>>
> >> I don't think it is safe for the toolstack to ever be playing with MSRs
> >> which Xen is completely unaware of.  There is no guarentee whatsoever
> >> that a new MSR which Xen is unaware of doesn't have security
> >> implications if the toolstack were to play with it.
> >>
> >> Adding entries to a whitelist is easy and could be considered a
> >> maintenance activity similar to keeping the model/stepping information
> >> up-to-date.
> > This resource access mechanism is useful according to some conversation with
> IPDC customers. Per their description, once the machine and VMs are online,
> they will not be turned off. Sometimes administrators may need to dynamically
> modify some resource values to fix/workaround certain issues, and our patch
> may serve this purpose.
> >
> > Adding the white/black list will bring certain constraints for the above use case.
> Also considering that the tool stack resides in dom0, I think it is not so dangerous.
> 
> The whole purpose of XSM is to split the toolstack up so it isn't all in
> dom0.

We limited this resource operation in domain 0 through xsm policies.

> 
> Extending a whitelist is trivial, and requires a positive action on
> behalf of someone to decide that the added MSR *is* safe.  Anything else
> is a security bug waiting to happen.

Considering that today's QEMU could even map all guest's memory and it is also resides in dom0.
Not sure how dangerous this resource operation is if we didn't add such whitelist or blacklist...

Thanks,
Dongxiao

> 
> ~Andrew

Re: [PATCH v12 1/9] x86: add generic resource (e.g. MSR) access hypercall

[Daniel De Graaf]

On 07/08/2014 10:06 PM, Xu, Dongxiao wrote:
>> -----Original Message-----
>> From: Andrew Cooper [mailto:andrew.cooper3@citrix.com]
>> Sent: Tuesday, July 08, 2014 5:08 PM
>> To: Xu, Dongxiao; Jan Beulich; xen-devel@lists.xen.org
>> Cc: keir@xen.org; Ian.Campbell@citrix.com; George.Dunlap@eu.citrix.com;
>> stefano.stabellini@eu.citrix.com; Ian.Jackson@eu.citrix.com;
>> dgdegra@tycho.nsa.gov
>> Subject: Re: [Xen-devel] [PATCH v12 1/9] x86: add generic resource (e.g. MSR)
>> access hypercall
>>
>> On 08/07/14 08:06, Xu, Dongxiao wrote:
>>>> -----Original Message-----
>>>> From: Andrew Cooper [mailto:andrew.cooper3@citrix.com]
>>>> Sent: Friday, July 04, 2014 6:53 PM
>>>> To: Jan Beulich; Xu, Dongxiao; xen-devel@lists.xen.org
>>>> Cc: Ian.Campbell@citrix.com; George.Dunlap@eu.citrix.com;
>>>> Ian.Jackson@eu.citrix.com; stefano.stabellini@eu.citrix.com;
>>>> konrad.wilk@oracle.com; dgdegra@tycho.nsa.gov; keir@xen.org
>>>> Subject: Re: [PATCH v12 1/9] x86: add generic resource (e.g. MSR) access
>>>> hypercall
>>>>
>>>> On 04/07/14 11:30, Jan Beulich wrote:
>>>>>>>> On 04.07.14 at 11:40, <andrew.cooper3@citrix.com> wrote:
>>>>>> On 04/07/14 09:34, Dongxiao Xu wrote:
>>>>>>> Add a generic resource access hypercall for tool stack or other
>>>>>>> components, e.g., accessing MSR, port I/O, etc.
>>>>>>>
>>>>>>> Signed-off-by: Dongxiao Xu <dongxiao.xu@intel.com>
>>>>>> This still permits a user of the hypercalls to play with EFER or
>>>>>> SYSENTER_EIP, which obviously is a very bad thing.
>>>>>>
>>>>>> There needs to be a whitelist of permitted MSRs which can be accessed.
>>>>> Hmm, I'm not sure. One particular purpose I see here is to allow the
>>>>> tool stack (or Dom0) access to MSRs Xen may not know about (yet).
>>>>> Furthermore, this being a platform op, only the hardware domain
>>>>> should ever have access, and it certainly ought to know what it's
>>>>> doing. So the sum of these two considerations is: If at all, we may
>>>>> want a black list here.
>>>>>
>>>>> Jan
>>>>>
>>>> I don't think it is safe for the toolstack to ever be playing with MSRs
>>>> which Xen is completely unaware of.  There is no guarentee whatsoever
>>>> that a new MSR which Xen is unaware of doesn't have security
>>>> implications if the toolstack were to play with it.
>>>>
>>>> Adding entries to a whitelist is easy and could be considered a
>>>> maintenance activity similar to keeping the model/stepping information
>>>> up-to-date.
>>> This resource access mechanism is useful according to some conversation with
>> IPDC customers. Per their description, once the machine and VMs are online,
>> they will not be turned off. Sometimes administrators may need to dynamically
>> modify some resource values to fix/workaround certain issues, and our patch
>> may serve this purpose.
>>>
>>> Adding the white/black list will bring certain constraints for the above use case.
>> Also considering that the tool stack resides in dom0, I think it is not so dangerous.
>>
>> The whole purpose of XSM is to split the toolstack up so it isn't all in
>> dom0.
>
> We limited this resource operation in domain 0 through xsm policies.

In this case, I expect a security-conscious administrator will need to
block access to this hypercall completely.  XSM does make this possible,
but that loses all the benefits of adding this feature.

>>
>> Extending a whitelist is trivial, and requires a positive action on
>> behalf of someone to decide that the added MSR *is* safe.  Anything else
>> is a security bug waiting to happen.
>
> Considering that today's QEMU could even map all guest's memory and it is also resides in dom0.
> Not sure how dangerous this resource operation is if we didn't add such whitelist or blacklist...
>
> Thanks,
> Dongxiao

The toolstack domain can only map all guest memory for all guests as
long as the toolstack is not disaggregated.  Even without diaggregation,
it is possible to set up domains whose memory dom0 cannot map: dom0
merely needs to be trusted to set the XSM label after domain creation,
which can be done from an initrd or before accepting logins/commands.

The resource operation has already been identified to be fully
dangerous: modifying SYSENTER_EIP allows a userspace program in domain 0
to run code in hypervisor context.  This bypasses any security features
that the Linux kernel in dom0 may be trying to provide in addition to
those that Xen provides.

-- 
Daniel De Graaf
National Security Agency

Re: [PATCH v12 1/9] x86: add generic resource (e.g. MSR) access hypercall

[Xu, Dongxiao]

> -----Original Message-----
> From: Jan Beulich [mailto:JBeulich@suse.com]
> Sent: Friday, July 04, 2014 6:44 PM
> To: Xu, Dongxiao
> Cc: andrew.cooper3@citrix.com; Ian.Campbell@citrix.com;
> George.Dunlap@eu.citrix.com; Ian.Jackson@eu.citrix.com;
> stefano.stabellini@eu.citrix.com; xen-devel@lists.xen.org;
> konrad.wilk@oracle.com; dgdegra@tycho.nsa.gov; keir@xen.org
> Subject: Re: [PATCH v12 1/9] x86: add generic resource (e.g. MSR) access
> hypercall
> 
> >>> On 04.07.14 at 10:34, <dongxiao.xu@intel.com> wrote:
> > Add a generic resource access hypercall for tool stack or other
> > components, e.g., accessing MSR, port I/O, etc.
> 
> Sigh - you're still allocating an unbounded amount of memory for
> passing around the input arguments, despite it being possible (and
> having been suggested) to read these from the original buffer on
> each iteration. You're still not properly checking for preemption
> between iterations. And you're still not making use of
> continue_hypercall_on_cpu(). Plus you now silently ignore the
> upper 32-bits of the passing in "idx" value as well as not
> understood XEN_RESOURCE_OP_* values.

continue_hypercall_on_cpu() is asynchronized, which requires the "data" field always points to the right place before the hypercall returns.
However in our function, we have a "for" loop to cover multiple operations, so the "data" field will be modified in each iteration, which cannot meet the continue_hypercall_on_cpu() requirements...

For the preemption check, what about the following? Here the preemption is checked within each resource_access_one() function.

static int resource_access_one(uint32_t type, uint32_t cmd,
                                uint64_t idx, uint64_t *val)
{
    int ret;

    if ( !is_idle_vcpu(current) && hypercall_preempt_check() )
    {
        ret = -ERESTART;
        break;
    }

    /* Handle the resource access code. */
    ...

    return ret;
}

int resource_access_helper(struct xenpf_resource_op *op)
{
    ...
    for ( i = 0; i < op->nr; i++ )
    {
        ...
        if ( data.cpu == smp_processor_id() )
            resource_access_one(&data);
        else
            on_selected_cpus(cpumask_of(last_cpu),
                             resource_access_one, &data, 1); 
    }

    ...
}

> 
> I also doubt that this warrants a new source file to be introduced,
> the helper functions (if indeed needed) can very well live in
> platform_hypercall.c.

Okay, I will put the resource related function in platform_hypercall.c.

Thanks,
Dongxiao

> 
> Jan

Re: [PATCH v12 1/9] x86: add generic resource (e.g. MSR) access hypercall

[Andrew Cooper]

On 11/07/14 05:29, Xu, Dongxiao wrote:
>> -----Original Message-----
>> From: Jan Beulich [mailto:JBeulich@suse.com]
>> Sent: Friday, July 04, 2014 6:44 PM
>> To: Xu, Dongxiao
>> Cc: andrew.cooper3@citrix.com; Ian.Campbell@citrix.com;
>> George.Dunlap@eu.citrix.com; Ian.Jackson@eu.citrix.com;
>> stefano.stabellini@eu.citrix.com; xen-devel@lists.xen.org;
>> konrad.wilk@oracle.com; dgdegra@tycho.nsa.gov; keir@xen.org
>> Subject: Re: [PATCH v12 1/9] x86: add generic resource (e.g. MSR) access
>> hypercall
>>
>>>>> On 04.07.14 at 10:34, <dongxiao.xu@intel.com> wrote:
>>> Add a generic resource access hypercall for tool stack or other
>>> components, e.g., accessing MSR, port I/O, etc.
>> Sigh - you're still allocating an unbounded amount of memory for
>> passing around the input arguments, despite it being possible (and
>> having been suggested) to read these from the original buffer on
>> each iteration. You're still not properly checking for preemption
>> between iterations. And you're still not making use of
>> continue_hypercall_on_cpu(). Plus you now silently ignore the
>> upper 32-bits of the passing in "idx" value as well as not
>> understood XEN_RESOURCE_OP_* values.
> continue_hypercall_on_cpu() is asynchronized, which requires the "data" field always points to the right place before the hypercall returns.
> However in our function, we have a "for" loop to cover multiple operations, so the "data" field will be modified in each iteration, which cannot meet the continue_hypercall_on_cpu() requirements...

This is because you are still copying all resource data at once from the
hypercall.

As Jan points out, this is an unbounded allocation in Xen which must be
addresses.  If instead you were to copy each element one at a time, you
would avoid this allocation entirely and be able to correctly use
continue_hypercall_on_cpu().


>
> For the preemption check, what about the following? Here the preemption is checked within each resource_access_one() function.

None of this preemption works.

In the case a hypercall gets preempted, you need to increment the guest
handle along to the next element to process, and decrement the count by
the number of elements processed in *the guest context*.

That way, when the hypercall continues in Xen, it shall pick up with the
next action to perform rather than restarting from the beginning.

~Andrew

Re: [PATCH v12 1/9] x86: add generic resource (e.g. MSR) access hypercall

[Xu, Dongxiao]

> -----Original Message-----
> From: Andrew Cooper [mailto:andrew.cooper3@citrix.com]
> Sent: Friday, July 11, 2014 5:25 PM
> To: Xu, Dongxiao; Jan Beulich
> Cc: Ian.Campbell@citrix.com; George.Dunlap@eu.citrix.com;
> Ian.Jackson@eu.citrix.com; stefano.stabellini@eu.citrix.com;
> xen-devel@lists.xen.org; konrad.wilk@oracle.com; dgdegra@tycho.nsa.gov;
> keir@xen.org
> Subject: Re: [PATCH v12 1/9] x86: add generic resource (e.g. MSR) access
> hypercall
> 
> On 11/07/14 05:29, Xu, Dongxiao wrote:
> >> -----Original Message-----
> >> From: Jan Beulich [mailto:JBeulich@suse.com]
> >> Sent: Friday, July 04, 2014 6:44 PM
> >> To: Xu, Dongxiao
> >> Cc: andrew.cooper3@citrix.com; Ian.Campbell@citrix.com;
> >> George.Dunlap@eu.citrix.com; Ian.Jackson@eu.citrix.com;
> >> stefano.stabellini@eu.citrix.com; xen-devel@lists.xen.org;
> >> konrad.wilk@oracle.com; dgdegra@tycho.nsa.gov; keir@xen.org
> >> Subject: Re: [PATCH v12 1/9] x86: add generic resource (e.g. MSR) access
> >> hypercall
> >>
> >>>>> On 04.07.14 at 10:34, <dongxiao.xu@intel.com> wrote:
> >>> Add a generic resource access hypercall for tool stack or other
> >>> components, e.g., accessing MSR, port I/O, etc.
> >> Sigh - you're still allocating an unbounded amount of memory for
> >> passing around the input arguments, despite it being possible (and
> >> having been suggested) to read these from the original buffer on
> >> each iteration. You're still not properly checking for preemption
> >> between iterations. And you're still not making use of
> >> continue_hypercall_on_cpu(). Plus you now silently ignore the
> >> upper 32-bits of the passing in "idx" value as well as not
> >> understood XEN_RESOURCE_OP_* values.
> > continue_hypercall_on_cpu() is asynchronized, which requires the "data" field
> always points to the right place before the hypercall returns.
> > However in our function, we have a "for" loop to cover multiple operations, so
> the "data" field will be modified in each iteration, which cannot meet the
> continue_hypercall_on_cpu() requirements...
> 
> This is because you are still copying all resource data at once from the
> hypercall.
> 
> As Jan points out, this is an unbounded allocation in Xen which must be
> addresses.  If instead you were to copy each element one at a time, you
> would avoid this allocation entirely and be able to correctly use
> continue_hypercall_on_cpu().

I've accepted the idea to copy the element one by one, however it seems that it doesn't help on continue_hypercall_on_cpu().
The full code looks like the following, where the variable "ra" will be updated on every "for" loop, and couldn't be used in continue_hypercall_on_cpu().
Do you have idea on how to solve this issue and use continue_hypercall_on_cpu() here?

static int resource_access_helper(struct xenpf_resource_op *op)
{
    struct xen_resource_access ra;
    unsigned int i;
    int ret = 0;

    for ( i = 0; i < op->nr; i++ )
    {
        if ( copy_from_guest_offset(&ra.data, op->data, i, 1) )
        {
            ret = -EFAULT;
            break;
        }

        if ( ra.data.cpu == smp_processor_id() )
            resource_access_one(&ra);
        else
            on_selected_cpus(cpumask_of(ra.data.cpu),
                             resource_access_one, &ra, 1);

        if ( copy_to_guest_offset(op->data, i, &ra.data, 1) )
        {
            ret = -EFAULT;
            break;
        }
    }

    return ret;
}

> 
> 
> >
> > For the preemption check, what about the following? Here the preemption is
> checked within each resource_access_one() function.
> 
> None of this preemption works.
> 
> In the case a hypercall gets preempted, you need to increment the guest
> handle along to the next element to process, and decrement the count by
> the number of elements processed in *the guest context*.
> 
> That way, when the hypercall continues in Xen, it shall pick up with the
> next action to perform rather than restarting from the beginning.

Some actions (like CQM) requires the read/write the MSRs in a continuous way, if it is interrupted, this "continuity" couldn't be guaranteed. The RESTART return value indicates to re-run the operations.
BTW, I tested it in my box, and the "failure" case doesn't happen frequently.

Thanks,
Dongxiao

> 
> ~Andrew

Re: [PATCH v12 1/9] x86: add generic resource (e.g. MSR) access hypercall

[Andrew Cooper]

On 15/07/14 03:23, Xu, Dongxiao wrote:
>> -----Original Message-----
>> From: Andrew Cooper [mailto:andrew.cooper3@citrix.com]
>> Sent: Friday, July 11, 2014 5:25 PM
>> To: Xu, Dongxiao; Jan Beulich
>> Cc: Ian.Campbell@citrix.com; George.Dunlap@eu.citrix.com;
>> Ian.Jackson@eu.citrix.com; stefano.stabellini@eu.citrix.com;
>> xen-devel@lists.xen.org; konrad.wilk@oracle.com; dgdegra@tycho.nsa.gov;
>> keir@xen.org
>> Subject: Re: [PATCH v12 1/9] x86: add generic resource (e.g. MSR) access
>> hypercall
>>
>> On 11/07/14 05:29, Xu, Dongxiao wrote:
>>>> -----Original Message-----
>>>> From: Jan Beulich [mailto:JBeulich@suse.com]
>>>> Sent: Friday, July 04, 2014 6:44 PM
>>>> To: Xu, Dongxiao
>>>> Cc: andrew.cooper3@citrix.com; Ian.Campbell@citrix.com;
>>>> George.Dunlap@eu.citrix.com; Ian.Jackson@eu.citrix.com;
>>>> stefano.stabellini@eu.citrix.com; xen-devel@lists.xen.org;
>>>> konrad.wilk@oracle.com; dgdegra@tycho.nsa.gov; keir@xen.org
>>>> Subject: Re: [PATCH v12 1/9] x86: add generic resource (e.g. MSR) access
>>>> hypercall
>>>>
>>>>>>> On 04.07.14 at 10:34, <dongxiao.xu@intel.com> wrote:
>>>>> Add a generic resource access hypercall for tool stack or other
>>>>> components, e.g., accessing MSR, port I/O, etc.
>>>> Sigh - you're still allocating an unbounded amount of memory for
>>>> passing around the input arguments, despite it being possible (and
>>>> having been suggested) to read these from the original buffer on
>>>> each iteration. You're still not properly checking for preemption
>>>> between iterations. And you're still not making use of
>>>> continue_hypercall_on_cpu(). Plus you now silently ignore the
>>>> upper 32-bits of the passing in "idx" value as well as not
>>>> understood XEN_RESOURCE_OP_* values.
>>> continue_hypercall_on_cpu() is asynchronized, which requires the "data" field
>> always points to the right place before the hypercall returns.
>>> However in our function, we have a "for" loop to cover multiple operations, so
>> the "data" field will be modified in each iteration, which cannot meet the
>> continue_hypercall_on_cpu() requirements...
>>
>> This is because you are still copying all resource data at once from the
>> hypercall.
>>
>> As Jan points out, this is an unbounded allocation in Xen which must be
>> addresses.  If instead you were to copy each element one at a time, you
>> would avoid this allocation entirely and be able to correctly use
>> continue_hypercall_on_cpu().
> I've accepted the idea to copy the element one by one, however it seems that it doesn't help on continue_hypercall_on_cpu().
> The full code looks like the following, where the variable "ra" will be updated on every "for" loop, and couldn't be used in continue_hypercall_on_cpu().
> Do you have idea on how to solve this issue and use continue_hypercall_on_cpu() here?

Hmm - I am very sorry for pushing at continue_hypercall_on_cpu(), but
looking at the code, there is no possible way it can be made to work in
its current form.  It will BUG() at the second attempt to nest, making
it inappropriate to use.

For now, the on_selected_cpus() solution is acceptable, although I still
have some comments.

> static int resource_access_helper(struct xenpf_resource_op *op)
> {
>     struct xen_resource_access ra;
>     unsigned int i;
>     int ret = 0;
>
>     for ( i = 0; i < op->nr; i++ )
>     {
>         if ( copy_from_guest_offset(&ra.data, op->data, i, 1) )
>         {
>             ret = -EFAULT;
>             break;
>         }
>
>         if ( ra.data.cpu == smp_processor_id() )

The call to smp_processor_id() is expensive.  Please read once outside
the loop.

>             resource_access_one(&ra);
>         else
>             on_selected_cpus(cpumask_of(ra.data.cpu),

ra.data.cpu needs validating otherwise Xen could walk off the end of the
cpu_bit_bitmap array.

>                              resource_access_one, &ra, 1);
>
>         if ( copy_to_guest_offset(op->data, i, &ra.data, 1) )
>         {
>             ret = -EFAULT;
>             break;
>         }

You still need a preemption check here, and to possibly create a
continuation.  See the uses of hypercall_create_continuation()
(do_set_trap_table() is a particularly relevant example)

>     }
>
>     return ret;
> }
>
>>
>>> For the preemption check, what about the following? Here the preemption is
>> checked within each resource_access_one() function.
>>
>> None of this preemption works.
>>
>> In the case a hypercall gets preempted, you need to increment the guest
>> handle along to the next element to process, and decrement the count by
>> the number of elements processed in *the guest context*.
>>
>> That way, when the hypercall continues in Xen, it shall pick up with the
>> next action to perform rather than restarting from the beginning.
> Some actions (like CQM) requires the read/write the MSRs in a continuous way, if it is interrupted, this "continuity" couldn't be guaranteed. The RESTART return value indicates to re-run the operations.
> BTW, I tested it in my box, and the "failure" case doesn't happen frequently.
>
> Thanks,
> Dongxiao

-ERESTART should never be returned to userspace.  It is purely for
internal accounting.  -EAGAIN is also inappropriate in this case.

Xen needs some way of knowing about the indirect MSR accesses.  This
either means it needs knowledge of the MSRs themselves (inflexible in
this circumstance), or userspace could set a "dont preempt yet" flag in
a xen_resource_access struct when it knows the following access has to
be adjacent.

To prevent misuse of the "don't preempt" flag (name subject to
improvement), improper use of it ("don't preempt" flags on consecutive
'ra's, or a "don't preempt" flag between a pair of 'ra's which switch
cpu) should result in a hypercall failure.

~Andrew

Re: [PATCH v12 1/9] x86: add generic resource (e.g. MSR) access hypercall

[Jan Beulich]

>>> On 15.07.14 at 12:00, <andrew.cooper3@citrix.com> wrote:
> Hmm - I am very sorry for pushing at continue_hypercall_on_cpu(), but
> looking at the code, there is no possible way it can be made to work in
> its current form.  It will BUG() at the second attempt to nest, making
> it inappropriate to use.

Not sure why - look at microcode.c's use of it, which also makes
use of it more than once for a single hypercall.

Jan

Re: [PATCH v12 1/9] x86: add generic resource (e.g. MSR) access hypercall

[Andrew Cooper]

On 23/07/14 08:45, Jan Beulich wrote:
>>>> On 15.07.14 at 12:00, <andrew.cooper3@citrix.com> wrote:
>> Hmm - I am very sorry for pushing at continue_hypercall_on_cpu(), but
>> looking at the code, there is no possible way it can be made to work in
>> its current form.  It will BUG() at the second attempt to nest, making
>> it inappropriate to use.
> Not sure why - look at microcode.c's use of it, which also makes
> use of it more than once for a single hypercall.
>
> Jan
>

Eugh.  That highlights several more issues.

Any two continue_hypercall_on_cpu()'s which target the same cpu may fall
over the BUG_ON(info->nest != 0).  A microcode update and this proposed
hypercall run a very real risk of intersecting.

The vcpu_pause_nosync(curr); in continue_hypercall_on_cpu() is bogus
given the do_microcode_update() example, as it will be pausing the wrong
vcpu.

As a result, the vcpu which made the hypercall will be unpaused after
the first continue_hypercall_on_cpu() returns, which is before the
hypercall has actually completed.

Irrespective of that, copying data back to guest context from the
continue_hypercall_tasklet is in the wrong vcpu context, and likely even
the wrong domain context.

Finally, we have seen in the past that using AMD SVM with mismatched
microcode patch levels can result in hardware lockup.  The only safe way
to apply microcode in these circumstances would be to rendezvous in Xen
and update every core at once, ensuring that no SVM operations are being
performed on sibling processors.

~Andrew

Re: [PATCH v12 1/9] x86: add generic resource (e.g. MSR) access hypercall

[Jan Beulich]

>>> On 23.07.14 at 11:09, <andrew.cooper3@citrix.com> wrote:
> On 23/07/14 08:45, Jan Beulich wrote:
>>>>> On 15.07.14 at 12:00, <andrew.cooper3@citrix.com> wrote:
>>> Hmm - I am very sorry for pushing at continue_hypercall_on_cpu(), but
>>> looking at the code, there is no possible way it can be made to work in
>>> its current form.  It will BUG() at the second attempt to nest, making
>>> it inappropriate to use.
>> Not sure why - look at microcode.c's use of it, which also makes
>> use of it more than once for a single hypercall.
> 
> Eugh.  That highlights several more issues.
> 
> Any two continue_hypercall_on_cpu()'s which target the same cpu may fall
> over the BUG_ON(info->nest != 0).  A microcode update and this proposed
> hypercall run a very real risk of intersecting.

I don't think so: info is this_cpu(continue_info) or a freshly allocated
one, but never per_cpu(continue_info, cpu).

> The vcpu_pause_nosync(curr); in continue_hypercall_on_cpu() is bogus
> given the do_microcode_update() example, as it will be pausing the wrong
> vcpu.

And again no: continue_hypercall_tasklet_handler() sets its CPU's
continue_info to the respective struct migrate_info instance (and
only for the duration of the callout). Consequently a nesting call to
continue_hypercall_on_cpu() won't come through the if() branch
leading to said vcpu_pause_nosync().

> As a result, the vcpu which made the hypercall will be unpaused after
> the first continue_hypercall_on_cpu() returns, which is before the
> hypercall has actually completed.
> 
> Irrespective of that, copying data back to guest context from the
> continue_hypercall_tasklet is in the wrong vcpu context, and likely even
> the wrong domain context.

Yes, that's an issue that would need addressing - the microcode use
simply doesn't need to do any copying (nor do any of the other current
ones afaict).

> Finally, we have seen in the past that using AMD SVM with mismatched
> microcode patch levels can result in hardware lockup.  The only safe way
> to apply microcode in these circumstances would be to rendezvous in Xen
> and update every core at once, ensuring that no SVM operations are being
> performed on sibling processors.

But even that doesn't guarantee all CPUs to get updated at _exactly_
the same moment. Such a problem needs to be eliminated in hardware,
or there would need to be an extremely precise list of operations that
are permitted while microcode on different cores is out of sync.

Jan