[PATCH v12 0/9] enable Cache QoS Monitoring (CQM) feature

[PATCH v12 0/9] enable Cache QoS Monitoring (CQM) feature

[Dongxiao Xu]

Changes from v11:
 - Turn off pqos and pqos_monitor in Xen command line by default.
 - Modify the original specific MSR access hypercall into a generic
   resource access hypercall. This hypercall could be used to access
   MSR, Port I/O, etc. Use platform_op to replace sysctl so that both
   dom0 kernel and userspace could use this hypercall.
 - Address various comments from Jan, Ian, Konrad, and Daniel.

Changes from v10:
 - Re-design and re-implement the whole logic. In this version,
   hypervisor provides basic mechanisms (like access MSRs) while all
   policies are put in user space.
   patch 1-3 provide a generic MSR hypercall for toolstack to access
   patch 4-9 implement the cache QoS monitoring feature

Changes from v9:
 - Revise the readonly mapping mechanism to share data between Xen and
   userspace. We create L3C buffer for each socket, share both buffer
   address MFNs and buffer MFNs to userspace.
 - Split the pqos.c into pqos.c and cqm.c for better code structure.
 - Show the total L3 cache size when issueing xl pqos-list cqm command.
 - Abstract a libxl_getcqminfo() function to fetch cqm data from Xen.
 - Several coding style fixes.

Changes from v8:
 - Address comments from Ian Campbell, including:
   * Modify the return handling for xc_sysctl();
   * Add man page items for platform QoS related commands.
   * Fix typo in commit message.

Changes from v7:
 - Address comments from Andrew Cooper, including:
   * Check CQM capability before allocating cpumask memory.
   * Move one function declaration into the correct patch.

Changes from v6:
 - Address comments from Jan Beulich, including:
   * Remove the unnecessary CPUID feature check.
   * Remove the unnecessary socket_cpu_map.
   * Spin_lock related changes, avoid spin_lock_irqsave().
   * Use readonly mapping to pass cqm data between Xen/Userspace,
     to avoid data copying.
   * Optimize RDMSR/WRMSR logic to avoid unnecessary calls.
   * Misc fixes including __read_mostly prefix, return value, etc.

Changes from v5:
 - Address comments from Dario Faggioli, including:
   * Define a new libxl_cqminfo structure to avoid reference of xc
     structure in libxl functions.
   * Use LOGE() instead of the LIBXL__LOG() functions.

Changes from v4:
 - When comparing xl cqm parameter, use strcmp instead of strncmp,
   otherwise, "xl pqos-attach cqmabcd domid" will be considered as
   a valid command line.
 - Address comments from Andrew Cooper, including:
   * Adjust the pqos parameter parsing function.
   * Modify the pqos related documentation.
   * Add a check for opt_cqm_max_rmid in initialization code.
   * Do not IPI CPU that is in same socket with current CPU.
 - Address comments from Dario Faggioli, including:
   * Fix an typo in export symbols.
   * Return correct libxl error code for qos related functions.
   * Abstract the error printing logic into a function.
 - Address comment from Daniel De Graaf, including:
   * Add return value in for pqos related check.
 - Address comments from Konrad Rzeszutek Wilk, including:
   * Modify the GPLv2 related file header, remove the address.

Changes from v3:
 - Use structure to better organize CQM related global variables.
 - Address comments from Andrew Cooper, including:
   * Remove the domain creation flag for CQM RMID allocation.
   * Adjust the boot parameter format, use custom_param().
   * Add documentation for the new added boot parameter.
   * Change QoS type flag to be uint64_t.
   * Initialize the per socket cpu bitmap in system boot time.
   * Remove get_cqm_avail() function.
   * Misc of format changes.
 - Address comment from Daniel De Graaf, including:
   * Use avc_current_has_perm() for XEN2__PQOS_OP that belongs to SECCLASS_XEN2.

Changes from v2:
 - Address comments from Andrew Cooper, including:
   * Merging tools stack changes into one patch.
   * Reduce the IPI number to one per socket.
   * Change structures for CQM data exchange between tools and Xen.
   * Misc of format/variable/function name changes.
 - Address comments from Konrad Rzeszutek Wilk, including:
   * Simplify the error printing logic.
   * Add xsm check for the new added hypercalls.

Changes from v1:
 - Address comments from Andrew Cooper, including:
   * Change function names, e.g., alloc_cqm_rmid(), system_supports_cqm(), etc.
   * Change some structure element order to save packing cost.
   * Correct some function's return value.
   * Some programming styles change.
   * ...

Future generations of Intel Xeon processor may offer monitoring capability in
each logical processor to measure specific quality-of-service metric,
for example, the Cache QoS Monitoring to get L3 cache occupancy.
Detailed information please refer to Intel SDM chapter 17.14.

Cache QoS Monitoring provides a layer of abstraction between applications and
logical processors through the use of Resource Monitoring IDs (RMIDs).
In Xen design, each guest in the system can be assigned an RMID independently,
while RMID=0 is reserved for monitoring domains that doesn't enable CQM service.
When any of the domain's vcpu is scheduled on a logical processor, the domain's
RMID will be activated by programming the value into one specific MSR, and when
the vcpu is scheduled out, a RMID=0 will be programmed into that MSR.
The Cache QoS Hardware tracks cache utilization of memory accesses according to
the RMIDs and reports monitored data via a counter register. With this solution,
we can get the knowledge how much L3 cache is used by a certain guest.

To attach QoS monitoring service to a certain guest:
xl pqos-monitor-attach domid

To detached CQM service from a guest:
xl pqos-monitor-detach domid

To get the L3 cache usage:
$ xl pqos-monitor-show cache_occupancy <domid>

The below data is just an example showing how the CQM related data is exposed to
end user.

[root@localhost]# xl pqos-monitor-show cache_occupancy
Total RMID: 55
Per-Socket L3 Cache Size: 35840 KB
Name                                        ID  SocketID        L3C_Usage       SocketID        L3C_Usage
Domain-0                                     0         0         31920 KB              1         28728 KB
ExampleHVMDomain                             1         0           504 KB              1          6160 KB

Dongxiao Xu (9):
  x86: add generic resource (e.g. MSR) access hypercall
  xsm: add resource operation related xsm policy
  tools: provide interface for generic MSR access
  x86: detect and initialize Platform QoS Monitoring feature
  x86: dynamically attach/detach QoS monitoring service for a guest
  x86: collect global QoS monitoring information
  x86: enable QoS monitoring for each domain RMID
  xsm: add platform QoS related xsm policies
  tools: CMDs and APIs for Platform QoS Monitoring

 docs/man/xl.pod.1                            |   24 +++
 docs/misc/xen-command-line.markdown          |   14 ++
 tools/flask/policy/policy/modules/xen/xen.if |    2 +-
 tools/flask/policy/policy/modules/xen/xen.te |    6 +-
 tools/libxc/Makefile                         |    2 +
 tools/libxc/xc_msr_x86.h                     |   36 +++++
 tools/libxc/xc_pqos.c                        |  219 ++++++++++++++++++++++++++
 tools/libxc/xc_private.h                     |   31 ++++
 tools/libxc/xc_resource.c                    |   53 +++++++
 tools/libxc/xenctrl.h                        |   23 +++
 tools/libxl/Makefile                         |    2 +-
 tools/libxl/libxl.h                          |   17 ++
 tools/libxl/libxl_pqos.c                     |  171 ++++++++++++++++++++
 tools/libxl/libxl_types.idl                  |    4 +
 tools/libxl/xl.h                             |    3 +
 tools/libxl/xl_cmdimpl.c                     |  131 +++++++++++++++
 tools/libxl/xl_cmdtable.c                    |   17 ++
 xen/arch/x86/Makefile                        |    2 +
 xen/arch/x86/domain.c                        |    8 +
 xen/arch/x86/domctl.c                        |   29 ++++
 xen/arch/x86/platform_hypercall.c            |   39 +++++
 xen/arch/x86/pqos.c                          |  193 +++++++++++++++++++++++
 xen/arch/x86/resource.c                      |  119 ++++++++++++++
 xen/arch/x86/setup.c                         |    3 +
 xen/arch/x86/sysctl.c                        |   56 +++++++
 xen/include/asm-x86/cpufeature.h             |    1 +
 xen/include/asm-x86/domain.h                 |    2 +
 xen/include/asm-x86/msr-index.h              |    3 +
 xen/include/asm-x86/pqos.h                   |   65 ++++++++
 xen/include/asm-x86/resource.h               |   40 +++++
 xen/include/public/domctl.h                  |   12 ++
 xen/include/public/platform.h                |   24 +++
 xen/include/public/sysctl.h                  |   14 ++
 xen/include/xlat.lst                         |    1 +
 xen/xsm/flask/hooks.c                        |   11 ++
 xen/xsm/flask/policy/access_vectors          |   18 ++-
 xen/xsm/flask/policy/security_classes        |    1 +
 37 files changed, 1390 insertions(+), 6 deletions(-)
 create mode 100644 tools/libxc/xc_msr_x86.h
 create mode 100644 tools/libxc/xc_pqos.c
 create mode 100644 tools/libxc/xc_resource.c
 create mode 100644 tools/libxl/libxl_pqos.c
 create mode 100644 xen/arch/x86/pqos.c
 create mode 100644 xen/arch/x86/resource.c
 create mode 100644 xen/include/asm-x86/pqos.h
 create mode 100644 xen/include/asm-x86/resource.h

-- 
1.7.9.5

[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

[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

[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

[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

[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 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

[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

[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

[PATCH v12 2/9] xsm: add resource operation related xsm policy

[Dongxiao Xu]

Add xsm policies for resource access related hypercall, such as MSR
access, port I/O read/write, and other related resource operations.

Signed-off-by: Dongxiao Xu <dongxiao.xu@intel.com>
---
 tools/flask/policy/policy/modules/xen/xen.te |    3 +++
 xen/xsm/flask/hooks.c                        |    4 ++++
 xen/xsm/flask/policy/access_vectors          |   14 +++++++++++---
 xen/xsm/flask/policy/security_classes        |    1 +
 4 files changed, 19 insertions(+), 3 deletions(-)

diff --git a/tools/flask/policy/policy/modules/xen/xen.te b/tools/flask/policy/policy/modules/xen/xen.te
index bb59fe8..562b8df 100644
--- a/tools/flask/policy/policy/modules/xen/xen.te
+++ b/tools/flask/policy/policy/modules/xen/xen.te
@@ -64,6 +64,9 @@ allow dom0_t xen_t:xen {
 	getidle debug getcpuinfo heap pm_op mca_op lockprof cpupool_op tmem_op
 	tmem_control getscheduler setscheduler
 };
+allow dom0_t xen_t:xen2 {
+    resource_op
+};
 allow dom0_t xen_t:mmu memorymap;
 
 # Allow dom0 to use these domctls on itself. For domctls acting on other
diff --git a/xen/xsm/flask/hooks.c b/xen/xsm/flask/hooks.c
index f2f59ea..fcfed25 100644
--- a/xen/xsm/flask/hooks.c
+++ b/xen/xsm/flask/hooks.c
@@ -1383,6 +1383,10 @@ static int flask_platform_op(uint32_t op)
     case XENPF_get_cpuinfo:
         return domain_has_xen(current->domain, XEN__GETCPUINFO);
 
+    case XENPF_resource_op:
+        return avc_current_has_perm(SECINITSID_XEN, SECCLASS_XEN2,
+                                    XEN2__RESOURCE_OP, NULL);
+
     default:
         printk("flask_platform_op: Unknown op %d\n", op);
         return -EPERM;
diff --git a/xen/xsm/flask/policy/access_vectors b/xen/xsm/flask/policy/access_vectors
index 32371a9..b606441 100644
--- a/xen/xsm/flask/policy/access_vectors
+++ b/xen/xsm/flask/policy/access_vectors
@@ -3,9 +3,9 @@
 #
 # class class_name { permission_name ... }
 
-# Class xen consists of dom0-only operations dealing with the hypervisor itself.
-# Unless otherwise specified, the source is the domain executing the hypercall,
-# and the target is the xen initial sid (type xen_t).
+# Class xen and xen2 consists of dom0-only operations dealing with the
+# hypervisor itself. Unless otherwise specified, the source is the domain
+# executing the hypercall, and the target is the xen initial sid (type xen_t).
 class xen
 {
 # XENPF_settime
@@ -75,6 +75,14 @@ class xen
     setscheduler
 }
 
+# This is a continuation of class xen, since only 32 permissions can be
+# defined per class
+class xen2
+{
+# XENPF_resource_op
+    resource_op
+}
+
 # Classes domain and domain2 consist of operations that a domain performs on
 # another domain or on itself.  Unless otherwise specified, the source is the
 # domain executing the hypercall, and the target is the domain being operated on
diff --git a/xen/xsm/flask/policy/security_classes b/xen/xsm/flask/policy/security_classes
index ef134a7..ca191db 100644
--- a/xen/xsm/flask/policy/security_classes
+++ b/xen/xsm/flask/policy/security_classes
@@ -8,6 +8,7 @@
 # for userspace object managers
 
 class xen
+class xen2
 class domain
 class domain2
 class hvm
-- 
1.7.9.5

Re: [PATCH v12 2/9] xsm: add resource operation related xsm policy

[Daniel De Graaf]

On 07/04/2014 04:34 AM, Dongxiao Xu wrote:
> Add xsm policies for resource access related hypercall, such as MSR
> access, port I/O read/write, and other related resource operations.
>
> Signed-off-by: Dongxiao Xu <dongxiao.xu@intel.com>

This is correct as far as a permission check, but I think the name
should be changed to reflect the contents of the white-list for the
access: pqos_monitor_op would work for the two MSRs used in #9.

If arbitrary access to MSRs is permitted without a white-list or other
categorization in the hypervisor, then the XSM policy needs to be able
to label individual MSRs and allow the security policy author to create
their own white- or black-lists.  This handles the use case you
described at the cost of requiring XSM to be enabled to manage the lists
of MSRs permitted to a toolstack domain.  I do not think this is the
best solution, since it will leave Xen without XSM unprotected, and the
construction of an XSM policy that permits useful features (like CQM)
but denies harmful ones (SYSENTER_EIP) will be more difficult than if
the permissions were explicit (pqos_monitor_op, compromise_hypervisor_op).

-- 
Daniel De Graaf
National Security Agency

Re: [PATCH v12 2/9] xsm: add resource operation related xsm policy

[Xu, Dongxiao]

> -----Original Message-----
> From: Daniel De Graaf [mailto:dgdegra@tycho.nsa.gov]
> Sent: Wednesday, July 09, 2014 5:22 AM
> To: Xu, Dongxiao; xen-devel@lists.xen.org
> Cc: keir@xen.org; JBeulich@suse.com; Ian.Jackson@eu.citrix.com;
> Ian.Campbell@citrix.com; stefano.stabellini@eu.citrix.com;
> andrew.cooper3@citrix.com; konrad.wilk@oracle.com;
> George.Dunlap@eu.citrix.com
> Subject: Re: [PATCH v12 2/9] xsm: add resource operation related xsm policy
> 
> On 07/04/2014 04:34 AM, Dongxiao Xu wrote:
> > Add xsm policies for resource access related hypercall, such as MSR
> > access, port I/O read/write, and other related resource operations.
> >
> > Signed-off-by: Dongxiao Xu <dongxiao.xu@intel.com>
> 
> This is correct as far as a permission check, but I think the name
> should be changed to reflect the contents of the white-list for the
> access: pqos_monitor_op would work for the two MSRs used in #9.

Now the hypercall is a generic resource access mechanism, which covers MSR, port I/O, etc.
Therefore I named it as "resource_op"...

> 
> If arbitrary access to MSRs is permitted without a white-list or other
> categorization in the hypervisor, then the XSM policy needs to be able
> to label individual MSRs and allow the security policy author to create
> their own white- or black-lists.  This handles the use case you
> described at the cost of requiring XSM to be enabled to manage the lists
> of MSRs permitted to a toolstack domain.  I do not think this is the
> best solution, since it will leave Xen without XSM unprotected, and the
> construction of an XSM policy that permits useful features (like CQM)
> but denies harmful ones (SYSENTER_EIP) will be more difficult than if
> the permissions were explicit (pqos_monitor_op, compromise_hypervisor_op).

We use the xsm policy to limit the execution of this resource_op only in dom0, and suppose dom0 and its toolstack is trusted...

One concern to add the white list is that, some customers also need this resource access mechanism to dynamically adjust some resource values (MSR) without turn off the machine, and if we add white list here, they may not be able to achieve it.

Thanks,
Dongxiao

> 
> --
> Daniel De Graaf
> National Security Agency

Re: [PATCH v12 2/9] xsm: add resource operation related xsm policy

[Daniel De Graaf]

On 07/09/2014 01:28 AM, Xu, Dongxiao wrote:
>> -----Original Message-----
>> From: Daniel De Graaf [mailto:dgdegra@tycho.nsa.gov]
>> Sent: Wednesday, July 09, 2014 5:22 AM
>> To: Xu, Dongxiao; xen-devel@lists.xen.org
>> Cc: keir@xen.org; JBeulich@suse.com; Ian.Jackson@eu.citrix.com;
>> Ian.Campbell@citrix.com; stefano.stabellini@eu.citrix.com;
>> andrew.cooper3@citrix.com; konrad.wilk@oracle.com;
>> George.Dunlap@eu.citrix.com
>> Subject: Re: [PATCH v12 2/9] xsm: add resource operation related xsm policy
>>
>> On 07/04/2014 04:34 AM, Dongxiao Xu wrote:
>>> Add xsm policies for resource access related hypercall, such as MSR
>>> access, port I/O read/write, and other related resource operations.
>>>
>>> Signed-off-by: Dongxiao Xu <dongxiao.xu@intel.com>
>>
>> This is correct as far as a permission check, but I think the name
>> should be changed to reflect the contents of the white-list for the
>> access: pqos_monitor_op would work for the two MSRs used in #9.
>
> Now the hypercall is a generic resource access mechanism, which covers MSR, port I/O, etc.
> Therefore I named it as "resource_op"...
>
>>
>> If arbitrary access to MSRs is permitted without a white-list or other
>> categorization in the hypervisor, then the XSM policy needs to be able
>> to label individual MSRs and allow the security policy author to create
>> their own white- or black-lists.  This handles the use case you
>> described at the cost of requiring XSM to be enabled to manage the lists
>> of MSRs permitted to a toolstack domain.  I do not think this is the
>> best solution, since it will leave Xen without XSM unprotected, and the
>> construction of an XSM policy that permits useful features (like CQM)
>> but denies harmful ones (SYSENTER_EIP) will be more difficult than if
>> the permissions were explicit (pqos_monitor_op, compromise_hypervisor_op).
>
> We use the xsm policy to limit the execution of this resource_op only in dom0, and suppose dom0 and its toolstack is trusted...
>
> One concern to add the white list is that, some customers also need this resource access mechanism to dynamically adjust some resource values (MSR) without turn off the machine, and if we add white list here, they may not be able to achieve it.

These MSRs are documented somewhere prior to the processor's release, I assume.
In that case, they can be added to a whitelist in the hypervisor before it
is possible to start the hypervisor on the new processor, so that any dom0 run
on the new processor has access to the new MSRs.  This type of update should be
suitable for backporting to stable releases, so you won't need to require the
newest hypervisor to take advantage of these features (at least, that's my
impression given Andrew's comments on the other thread).

-- 
Daniel De Graaf
National Security Agency

[PATCH v12 3/9] tools: provide interface for generic MSR access

[Dongxiao Xu]

Xen added a new sysctl hypercall for generic MSR access, and this is the
tool side change to wrapper the hypercall into xc APIs.

Signed-off-by: Dongxiao Xu <dongxiao.xu@intel.com>
---
 tools/libxc/Makefile      |    1 +
 tools/libxc/xc_private.h  |   31 ++++++++++++++++++++++++++
 tools/libxc/xc_resource.c |   53 +++++++++++++++++++++++++++++++++++++++++++++
 tools/libxc/xenctrl.h     |    5 +++++
 4 files changed, 90 insertions(+)
 create mode 100644 tools/libxc/xc_resource.c

diff --git a/tools/libxc/Makefile b/tools/libxc/Makefile
index 63be3d3..b8b28c1 100644
--- a/tools/libxc/Makefile
+++ b/tools/libxc/Makefile
@@ -34,6 +34,7 @@ CTRL_SRCS-y       += xc_foreign_memory.c
 CTRL_SRCS-y       += xc_kexec.c
 CTRL_SRCS-y       += xtl_core.c
 CTRL_SRCS-y       += xtl_logger_stdio.c
+CTRL_SRCS-y       += xc_resource.c
 CTRL_SRCS-$(CONFIG_X86) += xc_pagetab.c
 CTRL_SRCS-$(CONFIG_Linux) += xc_linux.c xc_linux_osdep.c
 CTRL_SRCS-$(CONFIG_FreeBSD) += xc_freebsd.c xc_freebsd_osdep.c
diff --git a/tools/libxc/xc_private.h b/tools/libxc/xc_private.h
index c7730f2..b1f1afa 100644
--- a/tools/libxc/xc_private.h
+++ b/tools/libxc/xc_private.h
@@ -46,6 +46,7 @@
 #define DECLARE_SYSCTL struct xen_sysctl sysctl
 #define DECLARE_PHYSDEV_OP struct physdev_op physdev_op
 #define DECLARE_FLASK_OP struct xen_flask_op op
+#define DECLARE_PLATFORM_OP struct xen_platform_op platform_op
 
 #undef PAGE_SHIFT
 #undef PAGE_SIZE
@@ -310,6 +311,36 @@ static inline int do_sysctl(xc_interface *xch, struct xen_sysctl *sysctl)
     return ret;
 }
 
+static inline int do_platform_op(xc_interface *xch,
+                                 struct xen_platform_op *platform_op)
+{
+    int ret = -1;
+    DECLARE_HYPERCALL;
+    DECLARE_HYPERCALL_BOUNCE(platform_op, sizeof(*platform_op),
+                             XC_HYPERCALL_BUFFER_BOUNCE_BOTH);
+
+    platform_op->interface_version = XENPF_INTERFACE_VERSION;
+
+    if ( xc_hypercall_bounce_pre(xch, platform_op) )
+    {
+        PERROR("Could not bounce buffer for platform_op hypercall");
+        goto out1;
+    }
+
+    hypercall.op     = __HYPERVISOR_platform_op;
+    hypercall.arg[0] = HYPERCALL_BUFFER_AS_ARG(platform_op);
+    if ( (ret = do_xen_hypercall(xch, &hypercall)) < 0 )
+    {
+        if ( errno == EACCES )
+            DPRINTF("platform operation failed -- need to"
+                    " rebuild the user-space tool set?\n");
+    }
+
+    xc_hypercall_bounce_post(xch, platform_op);
+ out1:
+    return ret;
+}
+
 int do_memory_op(xc_interface *xch, int cmd, void *arg, size_t len);
 
 void *xc_map_foreign_ranges(xc_interface *xch, uint32_t dom,
diff --git a/tools/libxc/xc_resource.c b/tools/libxc/xc_resource.c
new file mode 100644
index 0000000..bfecfdd
--- /dev/null
+++ b/tools/libxc/xc_resource.c
@@ -0,0 +1,53 @@
+/*
+ * xc_resource.c
+ *
+ * Generic resource access API
+ *
+ * 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 of the GNU Lesser General Public License as published
+ * by the Free Software Foundation; version 2.1 only. with the special
+ * exception on linking described in file LICENSE.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU Lesser General Public License for more details.
+ */
+
+#include "xc_private.h"
+
+int xc_resource_op(xc_interface *xch, uint32_t nr, uint32_t type,
+                   xc_resource_data_t *data)
+{
+    int rc;
+    DECLARE_PLATFORM_OP;
+    DECLARE_HYPERCALL_BOUNCE(data, nr * sizeof(*data),
+                             XC_HYPERCALL_BUFFER_BOUNCE_BOTH);
+
+    if ( xc_hypercall_bounce_pre(xch, data) )
+        return -1;
+
+    platform_op.cmd = XENPF_resource_op;
+    platform_op.u.resource_op.nr = nr;
+    platform_op.u.resource_op.type = type;
+    set_xen_guest_handle(platform_op.u.resource_op.data, data);
+
+    rc = do_platform_op(xch, &platform_op);
+
+    xc_hypercall_bounce_post(xch, data);
+
+    return rc;
+}
+
+/*
+ * Local variables:
+ * mode: C
+ * c-file-style: "BSD"
+ * c-basic-offset: 4
+ * tab-width: 4
+ * indent-tabs-mode: nil
+ * End:
+ */
diff --git a/tools/libxc/xenctrl.h b/tools/libxc/xenctrl.h
index af6f249..ca0b207 100644
--- a/tools/libxc/xenctrl.h
+++ b/tools/libxc/xenctrl.h
@@ -47,6 +47,7 @@
 #include <xen/xsm/flask_op.h>
 #include <xen/tmem.h>
 #include <xen/kexec.h>
+#include <xen/platform.h>
 
 #include "xentoollog.h"
 
@@ -2603,6 +2604,10 @@ int xc_kexec_load(xc_interface *xch, uint8_t type, uint16_t arch,
  */
 int xc_kexec_unload(xc_interface *xch, int type);
 
+typedef xenpf_resource_data_t xc_resource_data_t;
+int xc_resource_op(xc_interface *xch, uint32_t nr,
+    uint32_t type, xc_resource_data_t *data);
+
 #endif /* XENCTRL_H */
 
 /*
-- 
1.7.9.5

Re: [PATCH v12 3/9] tools: provide interface for generic MSR access

[Jan Beulich]

>>> On 04.07.14 at 10:34, <dongxiao.xu@intel.com> wrote:
> Xen added a new sysctl hypercall for generic MSR access, and this is the
> tool side change to wrapper the hypercall into xc APIs.

s/sysctl/platform op/

Jan

Re: [PATCH v12 3/9] tools: provide interface for generic MSR access

[Ian Campbell]

On Fri, 2014-07-04 at 12:42 +0100, Jan Beulich wrote:
> >>> On 04.07.14 at 10:34, <dongxiao.xu@intel.com> wrote:
> > Xen added a new sysctl hypercall for generic MSR access, and this is the
> > tool side change to wrapper the hypercall into xc APIs.
> 
> s/sysctl/platform op/

is platform_op really the right umbrella for this stuff? platform_op.h
says:
 * Hardware platform operations. Intended for use by domain-0 kernel.

Which in particular I suppose has API stability implications.

Ian.

Re: [PATCH v12 3/9] tools: provide interface for generic MSR access

[Jan Beulich]

>>> On 09.07.14 at 18:58, <ian.campbell@citrix.com> wrote:
> On Fri, 2014-07-04 at 12:42 +0100, Jan Beulich wrote:
>> >>> On 04.07.14 at 10:34, <dongxiao.xu@intel.com> wrote:
>> > Xen added a new sysctl hypercall for generic MSR access, and this is the
>> > tool side change to wrapper the hypercall into xc APIs.
>> 
>> s/sysctl/platform op/
> 
> is platform_op really the right umbrella for this stuff? platform_op.h
> says:
>  * Hardware platform operations. Intended for use by domain-0 kernel.
> 
> Which in particular I suppose has API stability implications.

Yeah, I think Dongxiao earlier also got trapped by this comment. It's
origin predates thinking about disaggregation, and hence it's really
stale at this point: Hardware operations aren't necessarily limited to
Dom0 (they might be limited to hardware_domain, but that in the end
is a XSM policy decision).

Jan

Re: [PATCH v12 3/9] tools: provide interface for generic MSR access

[Xu, Dongxiao]

> -----Original Message-----
> From: Jan Beulich [mailto:JBeulich@suse.com]
> Sent: Wednesday, July 23, 2014 3:49 PM
> To: Ian Campbell
> Cc: andrew.cooper3@citrix.com; George.Dunlap@eu.citrix.com;
> Ian.Jackson@eu.citrix.com; stefano.stabellini@eu.citrix.com; Xu, Dongxiao;
> xen-devel@lists.xen.org; konrad.wilk@oracle.com; dgdegra@tycho.nsa.gov;
> keir@xen.org
> Subject: Re: [PATCH v12 3/9] tools: provide interface for generic MSR access
> 
> >>> On 09.07.14 at 18:58, <ian.campbell@citrix.com> wrote:
> > On Fri, 2014-07-04 at 12:42 +0100, Jan Beulich wrote:
> >> >>> On 04.07.14 at 10:34, <dongxiao.xu@intel.com> wrote:
> >> > Xen added a new sysctl hypercall for generic MSR access, and this is the
> >> > tool side change to wrapper the hypercall into xc APIs.
> >>
> >> s/sysctl/platform op/
> >
> > is platform_op really the right umbrella for this stuff? platform_op.h
> > says:
> >  * Hardware platform operations. Intended for use by domain-0 kernel.
> >
> > Which in particular I suppose has API stability implications.
> 
> Yeah, I think Dongxiao earlier also got trapped by this comment. It's
> origin predates thinking about disaggregation, and hence it's really
> stale at this point: Hardware operations aren't necessarily limited to
> Dom0 (they might be limited to hardware_domain, but that in the end
> is a XSM policy decision).

Hi Jan,

Considering many people in the list requires the white-list style to limit the capability for resource access (e.g. MSR read/write), so I implement such a white-list in my new version patch like following:
Does it look reasonable to you?

static unsigned int allowed_msr_list[] = {
    MSR_IA32_QOSEVTSEL,
    MSR_IA32_QMC,
};

static unsigned int allow_access(unsigned int idx, unsigned int *list, unsigned int nr)
{
    unsigned int i;

    for ( i = 0; i < nr; i++ )
        if ( list[i] == idx )
            return 1;

    return 0;
}

static void resource_access_one(void *info)
{
    ...
    switch ( ra->type )
    {
    case XEN_RESOURCE_TYPE_MSR:
        if ( !allow_access(ra->data.idx, allowed_msr_list,
             sizeof(allowed_msr_list)/sizeof(unsigned int)) )
            ret = -EACCES;
        ...
    ...
}

Thanks,
Dongxiao

> 
> Jan

Re: [PATCH v12 3/9] tools: provide interface for generic MSR access

[Jan Beulich]

>>> On 24.07.14 at 08:31, <dongxiao.xu@intel.com> wrote:
> Considering many people in the list requires the white-list style to limit 
> the capability for resource access (e.g. MSR read/write), so I implement such 
> a white-list in my new version patch like following:
> Does it look reasonable to you?

Reasonable - perhaps. Efficiently coded - no:

> static unsigned int allowed_msr_list[] = {
>     MSR_IA32_QOSEVTSEL,
>     MSR_IA32_QMC,
> };
> 
> static unsigned int allow_access(unsigned int idx, unsigned int *list, unsigned int nr)
> {
>     unsigned int i;
> 
>     for ( i = 0; i < nr; i++ )
>         if ( list[i] == idx )
>             return 1;

Using a big switch allows the compiler to (hopefully) find an optimal
translation - possibly via lookup table, but maybe via other means.
And if you were to stay with the explicit lookup table, you'd have
to fix various mechanical issues (but I take this only as a sketch,
not as something you're proposing as is).

That said I continue to not be fully convinced of the need of a white
or black list here, not the least with the consideration in mind of
extending this to port I/O.

Jan

Re: [PATCH v12 3/9] tools: provide interface for generic MSR access

[Xu, Dongxiao]

> -----Original Message-----
> From: Xu, Dongxiao
> Sent: Thursday, July 24, 2014 2:31 PM
> To: Jan Beulich; Ian Campbell
> Cc: andrew.cooper3@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 3/9] tools: provide interface for generic MSR access
> 
> > -----Original Message-----
> > From: Jan Beulich [mailto:JBeulich@suse.com]
> > Sent: Wednesday, July 23, 2014 3:49 PM
> > To: Ian Campbell
> > Cc: andrew.cooper3@citrix.com; George.Dunlap@eu.citrix.com;
> > Ian.Jackson@eu.citrix.com; stefano.stabellini@eu.citrix.com; Xu, Dongxiao;
> > xen-devel@lists.xen.org; konrad.wilk@oracle.com; dgdegra@tycho.nsa.gov;
> > keir@xen.org
> > Subject: Re: [PATCH v12 3/9] tools: provide interface for generic MSR access
> >
> > >>> On 09.07.14 at 18:58, <ian.campbell@citrix.com> wrote:
> > > On Fri, 2014-07-04 at 12:42 +0100, Jan Beulich wrote:
> > >> >>> On 04.07.14 at 10:34, <dongxiao.xu@intel.com> wrote:
> > >> > Xen added a new sysctl hypercall for generic MSR access, and this is the
> > >> > tool side change to wrapper the hypercall into xc APIs.
> > >>
> > >> s/sysctl/platform op/
> > >
> > > is platform_op really the right umbrella for this stuff? platform_op.h
> > > says:
> > >  * Hardware platform operations. Intended for use by domain-0 kernel.
> > >
> > > Which in particular I suppose has API stability implications.
> >
> > Yeah, I think Dongxiao earlier also got trapped by this comment. It's
> > origin predates thinking about disaggregation, and hence it's really
> > stale at this point: Hardware operations aren't necessarily limited to
> > Dom0 (they might be limited to hardware_domain, but that in the end
> > is a XSM policy decision).
> 
> Hi Jan,
> 
> Considering many people in the list requires the white-list style to limit the
> capability for resource access (e.g. MSR read/write), so I implement such a
> white-list in my new version patch like following:
> Does it look reasonable to you?

Forget to mention that, the following is hypervisor side code, residing in patch 1/9.

Thanks,
Dongxiao

> 
> static unsigned int allowed_msr_list[] = {
>     MSR_IA32_QOSEVTSEL,
>     MSR_IA32_QMC,
> };
> 
> static unsigned int allow_access(unsigned int idx, unsigned int *list, unsigned int
> nr)
> {
>     unsigned int i;
> 
>     for ( i = 0; i < nr; i++ )
>         if ( list[i] == idx )
>             return 1;
> 
>     return 0;
> }
> 
> static void resource_access_one(void *info)
> {
>     ...
>     switch ( ra->type )
>     {
>     case XEN_RESOURCE_TYPE_MSR:
>         if ( !allow_access(ra->data.idx, allowed_msr_list,
>              sizeof(allowed_msr_list)/sizeof(unsigned int)) )
>             ret = -EACCES;
>         ...
>     ...
> }
> 
> Thanks,
> Dongxiao
> 
> >
> > Jan

Re: [PATCH v12 3/9] tools: provide interface for generic MSR access

[Ian Campbell]

On Fri, 2014-07-04 at 16:34 +0800, Dongxiao Xu wrote:
> Xen added a new sysctl hypercall for generic MSR access, and this is the
> tool side change to wrapper the hypercall into xc APIs.
> 
> Signed-off-by: Dongxiao Xu <dongxiao.xu@intel.com>

These look like correct wrappers around the given interfaces, so in that
regard I would Ack it. But see my other mail since I'm not really sure
that platform_op is the right choice.

So iff the hypervisor guys can confirm that they are happy with this new
interface being a platform_op:
        Acked-by: Ian Campbell <ian.campbell@citrix.com>

Ian

[PATCH v12 4/9] x86: detect and initialize Platform QoS Monitoring feature

[Dongxiao Xu]

Detect platform QoS feature status and enumerate the resource types,
one of which is to monitor the L3 cache occupancy.

Also introduce a Xen command line parameter to control the QoS
feature status.

Signed-off-by: Dongxiao Xu <dongxiao.xu@intel.com>
---
 docs/misc/xen-command-line.markdown |   14 +++++
 xen/arch/x86/Makefile               |    1 +
 xen/arch/x86/pqos.c                 |  117 +++++++++++++++++++++++++++++++++++
 xen/arch/x86/setup.c                |    3 +
 xen/include/asm-x86/cpufeature.h    |    1 +
 xen/include/asm-x86/pqos.h          |   56 +++++++++++++++++
 6 files changed, 192 insertions(+)
 create mode 100644 xen/arch/x86/pqos.c
 create mode 100644 xen/include/asm-x86/pqos.h

diff --git a/docs/misc/xen-command-line.markdown b/docs/misc/xen-command-line.markdown
index 94f04bd..cf02884 100644
--- a/docs/misc/xen-command-line.markdown
+++ b/docs/misc/xen-command-line.markdown
@@ -863,6 +863,20 @@ This option can be specified more than once (up to 8 times at present).
 ### ple\_window
 > `= <integer>`
 
+### pqos (Intel)
+> `= List of ( <boolean> | pqos_monitor:<boolean> | rmid_max:<integer> )`
+
+> Default: `pqos=1,pqos_monitor:1,rmid_max:255`
+
+Configure platform QoS services, which are available in some of the new
+Intel processors.
+
+`pqos` instructs Xen to enable/disable platform QoS service.
+
+`pqos_monitor` instructs Xen to enable/disable QoS monitoring service.
+
+`rmid_max` indicates the max value for rmid.
+
 ### reboot
 > `= t[riple] | k[bd] | n[o] [, [w]arm | [c]old]`
 
diff --git a/xen/arch/x86/Makefile b/xen/arch/x86/Makefile
index e0cee24..a508bc6 100644
--- a/xen/arch/x86/Makefile
+++ b/xen/arch/x86/Makefile
@@ -60,6 +60,7 @@ obj-y += tboot.o
 obj-y += hpet.o
 obj-y += xstate.o
 obj-y += resource.o
+obj-y += pqos.o
 
 obj-$(crash_debug) += gdbstub.o
 
diff --git a/xen/arch/x86/pqos.c b/xen/arch/x86/pqos.c
new file mode 100644
index 0000000..513dd0f
--- /dev/null
+++ b/xen/arch/x86/pqos.c
@@ -0,0 +1,117 @@
+/*
+ * pqos.c: Platform QoS related service for guest.
+ *
+ * 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/init.h>
+#include <xen/cpu.h>
+#include <asm/pqos.h>
+
+struct pqos_monitor *__read_mostly pqosm = NULL;
+static bool_t __initdata opt_pqos = 0;
+static bool_t __initdata opt_pqos_monitor = 0;
+static unsigned int __initdata opt_rmid_max = 255;
+
+static void __init parse_pqos_param(char *s)
+{
+    char *ss, *val_str;
+    int val;
+
+    do {
+        ss = strchr(s, ',');
+        if ( ss )
+            *ss = '\0';
+
+        val = parse_bool(s);
+        if ( val >= 0 )
+            opt_pqos = val;
+        else
+        {
+            val = !!strncmp(s, "no-", 3);
+            if ( !val )
+                s += 3;
+
+            val_str = strchr(s, ':');
+            if ( val_str )
+                *val_str++ = '\0';
+
+            if ( val_str && !strcmp(s, "pqos_monitor") &&
+                 (val = parse_bool(val_str)) >= 0 )
+                opt_pqos_monitor = val;
+            else if ( val_str && !strcmp(s, "rmid_max") )
+                opt_rmid_max = simple_strtoul(val_str, NULL, 0);
+        }
+
+        s = ss + 1;
+    } while ( ss );
+}
+
+custom_param("pqos", parse_pqos_param);
+static void __init init_pqos_monitor(unsigned int rmid_max)
+{
+    unsigned int eax, ebx, ecx, edx;
+    unsigned int rmid;
+
+    if ( !boot_cpu_has(X86_FEATURE_QOSM) )
+        return;
+
+    cpuid_count(0xf, 0, &eax, &ebx, &ecx, &edx);
+    if ( !edx )
+        return;
+
+    pqosm = xzalloc(struct pqos_monitor);
+    if ( !pqosm )
+        return;
+
+    pqosm->qm_features = edx;
+    pqosm->rmid_mask = ~(~0ull << get_count_order(ebx));
+    pqosm->rmid_inuse = 0;
+    pqosm->rmid_min = 1;
+    pqosm->rmid_max = min(rmid_max, ebx);
+
+    if ( pqosm->qm_features & QOS_MONITOR_TYPE_L3 ) {
+        cpuid_count(0xf, 1, &eax, &ebx, &ecx, &edx);
+        pqosm->l3m.upscaling_factor = ebx;
+        pqosm->l3m.rmid_max = ecx;
+        pqosm->l3m.l3_features = edx;
+    }
+
+    pqosm->rmid_max = min(rmid_max, pqosm->l3m.rmid_max);
+    pqosm->rmid_to_dom = xmalloc_array(domid_t, pqosm->rmid_max + 1);
+    if ( !pqosm->rmid_to_dom ) {
+        xfree(pqosm);
+        return;
+    }
+    /* Reserve RMID 0 for all domains not being monitored */
+    pqosm->rmid_to_dom[0] = DOMID_XEN;
+    for ( rmid = pqosm->rmid_min; rmid <= pqosm->rmid_max; rmid++ )
+        pqosm->rmid_to_dom[rmid] = DOMID_INVALID;
+
+    printk(XENLOG_INFO "Platform QoS Monitoring Enabled.\n");
+}
+
+void __init init_platform_qos(void)
+{
+    if ( opt_pqos && opt_pqos_monitor && opt_rmid_max )
+        init_pqos_monitor(opt_rmid_max);
+}
+
+/*
+ * Local variables:
+ * mode: C
+ * c-file-style: "BSD"
+ * c-basic-offset: 4
+ * tab-width: 4
+ * indent-tabs-mode: nil
+ * End:
+ */
diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
index 599cf04..2bbabd2 100644
--- a/xen/arch/x86/setup.c
+++ b/xen/arch/x86/setup.c
@@ -49,6 +49,7 @@
 #include <xen/cpu.h>
 #include <asm/nmi.h>
 #include <asm/alternative.h>
+#include <asm/pqos.h>
 
 /* opt_nosmp: If true, secondary processors are ignored. */
 static bool_t __initdata opt_nosmp;
@@ -1441,6 +1442,8 @@ void __init noreturn __start_xen(unsigned long mbi_p)
 
     domain_unpause_by_systemcontroller(dom0);
 
+    init_platform_qos();
+
     reset_stack_and_jump(init_done);
 }
 
diff --git a/xen/include/asm-x86/cpufeature.h b/xen/include/asm-x86/cpufeature.h
index 8014241..eb1dd27 100644
--- a/xen/include/asm-x86/cpufeature.h
+++ b/xen/include/asm-x86/cpufeature.h
@@ -148,6 +148,7 @@
 #define X86_FEATURE_ERMS	(7*32+ 9) /* Enhanced REP MOVSB/STOSB */
 #define X86_FEATURE_INVPCID	(7*32+10) /* Invalidate Process Context ID */
 #define X86_FEATURE_RTM 	(7*32+11) /* Restricted Transactional Memory */
+#define X86_FEATURE_QOSM	(7*32+12) /* Platform QoS monitoring capability */
 #define X86_FEATURE_NO_FPU_SEL 	(7*32+13) /* FPU CS/DS stored as zero */
 #define X86_FEATURE_MPX		(7*32+14) /* Memory Protection Extensions */
 #define X86_FEATURE_RDSEED	(7*32+18) /* RDSEED instruction */
diff --git a/xen/include/asm-x86/pqos.h b/xen/include/asm-x86/pqos.h
new file mode 100644
index 0000000..1ec7970
--- /dev/null
+++ b/xen/include/asm-x86/pqos.h
@@ -0,0 +1,56 @@
+/*
+ * pqos.h: Platform QoS related service for guest.
+ *
+ * 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_PQOS_H__
+#define __ASM_PQOS_H__
+
+#include <public/xen.h>
+
+/* QoS Resource Type Enumeration */
+#define QOS_MONITOR_TYPE_L3            0x2
+
+/* L3 Monitoring Features */
+#define L3_FEATURE_OCCUPANCY           0x1
+
+struct pqos_monitor_l3 {
+    unsigned int l3_features;
+    unsigned int upscaling_factor;
+    unsigned int rmid_max;
+};
+
+struct pqos_monitor {
+    unsigned long rmid_mask;
+    unsigned int rmid_min;
+    unsigned int rmid_max;
+    unsigned int rmid_inuse;
+    unsigned int qm_features;
+    domid_t *rmid_to_dom;
+    struct pqos_monitor_l3 l3m;
+};
+extern struct pqos_monitor *pqosm;
+
+void __init init_platform_qos(void);
+
+#endif /* __ASM_PQOS_H__ */
+
+/*
+ * Local variables:
+ * mode: C
+ * c-file-style: "BSD"
+ * c-basic-offset: 4
+ * tab-width: 4
+ * indent-tabs-mode: nil
+ * End:
+ */
-- 
1.7.9.5

Re: [PATCH v12 4/9] x86: detect and initialize Platform QoS Monitoring feature

[Jan Beulich]

>>> On 04.07.14 at 10:34, <dongxiao.xu@intel.com> wrote:
> --- a/docs/misc/xen-command-line.markdown
> +++ b/docs/misc/xen-command-line.markdown
> @@ -863,6 +863,20 @@ This option can be specified more than once (up to 8 times at present).
>  ### ple\_window
>  > `= <integer>`
>  
> +### pqos (Intel)
> +> `= List of ( <boolean> | pqos_monitor:<boolean> | rmid_max:<integer> )`
> +
> +> Default: `pqos=1,pqos_monitor:1,rmid_max:255`

Please keep this in sync with the actual code.

> +struct pqos_monitor *__read_mostly pqosm = NULL;
> +static bool_t __initdata opt_pqos = 0;
> +static bool_t __initdata opt_pqos_monitor = 0;

So as said in response to the overview mail already - I think it's fine to
have the sub-option enabled by default, and just keeping the master
one off for now. In any event you don't need explicit 0 initializers for
static data.

> +static unsigned int __initdata opt_rmid_max = 255;
> +
> +static void __init parse_pqos_param(char *s)
> +{
> +    char *ss, *val_str;
> +    int val;
> +
> +    do {
> +        ss = strchr(s, ',');
> +        if ( ss )
> +            *ss = '\0';
> +
> +        val = parse_bool(s);
> +        if ( val >= 0 )
> +            opt_pqos = val;
> +        else
> +        {
> +            val = !!strncmp(s, "no-", 3);
> +            if ( !val )
> +                s += 3;
> +
> +            val_str = strchr(s, ':');
> +            if ( val_str )
> +                *val_str++ = '\0';
> +
> +            if ( val_str && !strcmp(s, "pqos_monitor") &&
> +                 (val = parse_bool(val_str)) >= 0 )
> +                opt_pqos_monitor = val;
> +            else if ( val_str && !strcmp(s, "rmid_max") )
> +                opt_rmid_max = simple_strtoul(val_str, NULL, 0);
> +        }
> +
> +        s = ss + 1;
> +    } while ( ss );
> +}
> +
> +custom_param("pqos", parse_pqos_param);
> +static void __init init_pqos_monitor(unsigned int rmid_max)

The blank line should be below custom_param() rather than above it.

> +{
> +    unsigned int eax, ebx, ecx, edx;
> +    unsigned int rmid;
> +
> +    if ( !boot_cpu_has(X86_FEATURE_QOSM) )
> +        return;
> +
> +    cpuid_count(0xf, 0, &eax, &ebx, &ecx, &edx);
> +    if ( !edx )
> +        return;
> +
> +    pqosm = xzalloc(struct pqos_monitor);
> +    if ( !pqosm )
> +        return;
> +
> +    pqosm->qm_features = edx;
> +    pqosm->rmid_mask = ~(~0ull << get_count_order(ebx));
> +    pqosm->rmid_inuse = 0;
> +    pqosm->rmid_min = 1;
> +    pqosm->rmid_max = min(rmid_max, ebx);
> +
> +    if ( pqosm->qm_features & QOS_MONITOR_TYPE_L3 ) {

Coding style (also further down).

> --- /dev/null
> +++ b/xen/include/asm-x86/pqos.h
> @@ -0,0 +1,56 @@
> +/*
> + * pqos.h: Platform QoS related service for guest.
> + *
> + * 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_PQOS_H__
> +#define __ASM_PQOS_H__
> +
> +#include <public/xen.h>
> +
> +/* QoS Resource Type Enumeration */
> +#define QOS_MONITOR_TYPE_L3            0x2
> +
> +/* L3 Monitoring Features */
> +#define L3_FEATURE_OCCUPANCY           0x1
> +
> +struct pqos_monitor_l3 {
> +    unsigned int l3_features;
> +    unsigned int upscaling_factor;
> +    unsigned int rmid_max;
> +};
> +
> +struct pqos_monitor {
> +    unsigned long rmid_mask;
> +    unsigned int rmid_min;
> +    unsigned int rmid_max;
> +    unsigned int rmid_inuse;
> +    unsigned int qm_features;
> +    domid_t *rmid_to_dom;
> +    struct pqos_monitor_l3 l3m;
> +};

Does any of the above get used outside of pqos.c? Anything used only
locally (until the end of the series of course) should be moved there.

> +extern struct pqos_monitor *pqosm;
> +
> +void __init init_platform_qos(void);

No __init annotaion on declarations please (these only belong on
definitions), even more so without including xen/init.h.

Jan

Re: [PATCH v12 4/9] x86: detect and initialize Platform QoS Monitoring feature

[Xu, Dongxiao]

> -----Original Message-----
> From: Jan Beulich [mailto:JBeulich@suse.com]
> Sent: Friday, July 04, 2014 7:57 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 4/9] x86: detect and initialize Platform QoS Monitoring
> feature
> 
> >>> On 04.07.14 at 10:34, <dongxiao.xu@intel.com> wrote:
> > --- a/docs/misc/xen-command-line.markdown
> > +++ b/docs/misc/xen-command-line.markdown
> > @@ -863,6 +863,20 @@ This option can be specified more than once (up to 8
> times at present).
> >  ### ple\_window
> >  > `= <integer>`
> >
> > +### pqos (Intel)
> > +> `= List of ( <boolean> | pqos_monitor:<boolean> | rmid_max:<integer> )`
> > +
> > +> Default: `pqos=1,pqos_monitor:1,rmid_max:255`
> 
> Please keep this in sync with the actual code.
> 
> > +struct pqos_monitor *__read_mostly pqosm = NULL;
> > +static bool_t __initdata opt_pqos = 0;
> > +static bool_t __initdata opt_pqos_monitor = 0;
> 
> So as said in response to the overview mail already - I think it's fine to
> have the sub-option enabled by default, and just keeping the master
> one off for now. In any event you don't need explicit 0 initializers for
> static data.
> 
> > +static unsigned int __initdata opt_rmid_max = 255;
> > +
> > +static void __init parse_pqos_param(char *s)
> > +{
> > +    char *ss, *val_str;
> > +    int val;
> > +
> > +    do {
> > +        ss = strchr(s, ',');
> > +        if ( ss )
> > +            *ss = '\0';
> > +
> > +        val = parse_bool(s);
> > +        if ( val >= 0 )
> > +            opt_pqos = val;
> > +        else
> > +        {
> > +            val = !!strncmp(s, "no-", 3);
> > +            if ( !val )
> > +                s += 3;
> > +
> > +            val_str = strchr(s, ':');
> > +            if ( val_str )
> > +                *val_str++ = '\0';
> > +
> > +            if ( val_str && !strcmp(s, "pqos_monitor") &&
> > +                 (val = parse_bool(val_str)) >= 0 )
> > +                opt_pqos_monitor = val;
> > +            else if ( val_str && !strcmp(s, "rmid_max") )
> > +                opt_rmid_max = simple_strtoul(val_str, NULL, 0);
> > +        }
> > +
> > +        s = ss + 1;
> > +    } while ( ss );
> > +}
> > +
> > +custom_param("pqos", parse_pqos_param);
> > +static void __init init_pqos_monitor(unsigned int rmid_max)
> 
> The blank line should be below custom_param() rather than above it.
> 
> > +{
> > +    unsigned int eax, ebx, ecx, edx;
> > +    unsigned int rmid;
> > +
> > +    if ( !boot_cpu_has(X86_FEATURE_QOSM) )
> > +        return;
> > +
> > +    cpuid_count(0xf, 0, &eax, &ebx, &ecx, &edx);
> > +    if ( !edx )
> > +        return;
> > +
> > +    pqosm = xzalloc(struct pqos_monitor);
> > +    if ( !pqosm )
> > +        return;
> > +
> > +    pqosm->qm_features = edx;
> > +    pqosm->rmid_mask = ~(~0ull << get_count_order(ebx));
> > +    pqosm->rmid_inuse = 0;
> > +    pqosm->rmid_min = 1;
> > +    pqosm->rmid_max = min(rmid_max, ebx);
> > +
> > +    if ( pqosm->qm_features & QOS_MONITOR_TYPE_L3 ) {
> 
> Coding style (also further down).
> 
> > --- /dev/null
> > +++ b/xen/include/asm-x86/pqos.h
> > @@ -0,0 +1,56 @@
> > +/*
> > + * pqos.h: Platform QoS related service for guest.
> > + *
> > + * 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_PQOS_H__
> > +#define __ASM_PQOS_H__
> > +
> > +#include <public/xen.h>
> > +
> > +/* QoS Resource Type Enumeration */
> > +#define QOS_MONITOR_TYPE_L3            0x2
> > +
> > +/* L3 Monitoring Features */
> > +#define L3_FEATURE_OCCUPANCY           0x1
> > +
> > +struct pqos_monitor_l3 {
> > +    unsigned int l3_features;
> > +    unsigned int upscaling_factor;
> > +    unsigned int rmid_max;
> > +};
> > +
> > +struct pqos_monitor {
> > +    unsigned long rmid_mask;
> > +    unsigned int rmid_min;
> > +    unsigned int rmid_max;
> > +    unsigned int rmid_inuse;
> > +    unsigned int qm_features;
> > +    domid_t *rmid_to_dom;
> > +    struct pqos_monitor_l3 l3m;
> > +};
> 
> Does any of the above get used outside of pqos.c? Anything used only
> locally (until the end of the series of course) should be moved there.

The later patch 6/9 will reference the structure in sysctl.c.

Thanks,
Dongxiao

> 
> > +extern struct pqos_monitor *pqosm;
> > +
> > +void __init init_platform_qos(void);
> 
> No __init annotaion on declarations please (these only belong on
> definitions), even more so without including xen/init.h.
> 
> Jan

[PATCH v12 5/9] x86: dynamically attach/detach QoS monitoring service for a guest

[Dongxiao Xu]

Add hypervisor side support for dynamically attach and detach QoS
monitoring services for a certain guest.

When attach Qos monitoring service for a guest, system will allocate an
RMID for it. When detach or guest is shutdown, the RMID will be
recycled for future use.

Signed-off-by: Dongxiao Xu <dongxiao.xu@intel.com>
---
 xen/arch/x86/domain.c        |    3 +++
 xen/arch/x86/domctl.c        |   29 ++++++++++++++++++++++++
 xen/arch/x86/pqos.c          |   50 ++++++++++++++++++++++++++++++++++++++++++
 xen/include/asm-x86/domain.h |    2 ++
 xen/include/asm-x86/pqos.h   |    8 +++++++
 xen/include/public/domctl.h  |   12 ++++++++++
 6 files changed, 104 insertions(+)

diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c
index e896210..f8e0e33 100644
--- a/xen/arch/x86/domain.c
+++ b/xen/arch/x86/domain.c
@@ -60,6 +60,7 @@
 #include <xen/numa.h>
 #include <xen/iommu.h>
 #include <compat/vcpu.h>
+#include <asm/pqos.h>
 
 DEFINE_PER_CPU(struct vcpu *, curr_vcpu);
 DEFINE_PER_CPU(unsigned long, cr4);
@@ -636,6 +637,8 @@ void arch_domain_destroy(struct domain *d)
 
     free_xenheap_page(d->shared_info);
     cleanup_domain_irq_mapping(d);
+
+    pqos_monitor_free_rmid(d);
 }
 
 unsigned long pv_guest_cr4_fixup(const struct vcpu *v, unsigned long guest_cr4)
diff --git a/xen/arch/x86/domctl.c b/xen/arch/x86/domctl.c
index a4effc3..3958830 100644
--- a/xen/arch/x86/domctl.c
+++ b/xen/arch/x86/domctl.c
@@ -35,6 +35,7 @@
 #include <asm/mem_sharing.h>
 #include <asm/xstate.h>
 #include <asm/debugger.h>
+#include <asm/pqos.h>
 
 static int gdbsx_guest_mem_io(
     domid_t domid, struct xen_domctl_gdbsx_memio *iop)
@@ -1395,6 +1396,34 @@ long arch_do_domctl(
     }
     break;
 
+    case XEN_DOMCTL_pqos_monitor_op:
+        if ( !pqos_monitor_enabled() )
+        {
+            ret = -ENODEV;
+            break;
+        }
+
+        switch ( domctl->u.pqos_monitor_op.cmd )
+        {
+        case XEN_DOMCTL_PQOS_MONITOR_OP_ATTACH:
+            ret = pqos_monitor_alloc_rmid(d);
+            break;
+        case XEN_DOMCTL_PQOS_MONITOR_OP_DETACH:
+            if ( d->arch.pqos_rmid > 0 )
+                pqos_monitor_free_rmid(d);
+            else
+                ret = -ENOENT;
+            break;
+        case XEN_DOMCTL_PQOS_MONITOR_OP_QUERY_RMID:
+            domctl->u.pqos_monitor_op.data = d->arch.pqos_rmid;
+            copyback = 1;
+            break;
+        default:
+            ret = -ENOSYS;
+            break;
+        }
+        break;
+
     default:
         ret = iommu_do_domctl(domctl, d, u_domctl);
         break;
diff --git a/xen/arch/x86/pqos.c b/xen/arch/x86/pqos.c
index 513dd0f..18fd1f9 100644
--- a/xen/arch/x86/pqos.c
+++ b/xen/arch/x86/pqos.c
@@ -106,6 +106,56 @@ void __init init_platform_qos(void)
         init_pqos_monitor(opt_rmid_max);
 }
 
+int pqos_monitor_alloc_rmid(struct domain *d)
+{
+    int rc = 0;
+    unsigned int rmid;
+
+    ASSERT(pqos_monitor_enabled());
+
+    if ( d->arch.pqos_rmid > 0 )
+    {
+        rc = -EEXIST;
+        return rc;
+    }
+
+    for ( rmid = pqosm->rmid_min; rmid <= pqosm->rmid_max; rmid++ )
+    {
+        if ( pqosm->rmid_to_dom[rmid] != DOMID_INVALID)
+            continue;
+
+        pqosm->rmid_to_dom[rmid] = d->domain_id;
+        break;
+    }
+
+    /* No RMID available, assign RMID=0 by default */
+    if ( rmid > pqosm->rmid_max )
+    {
+        rmid = 0;
+        rc = -EUSERS;
+    }
+    else
+        pqosm->rmid_inuse++;
+
+    d->arch.pqos_rmid = rmid;
+
+    return rc;
+}
+
+void pqos_monitor_free_rmid(struct domain *d)
+{
+    unsigned int rmid;
+
+    rmid = d->arch.pqos_rmid;
+    /* We do not free system reserved "RMID=0" */
+    if ( rmid == 0 )
+        return;
+
+    pqosm->rmid_to_dom[rmid] = DOMID_INVALID;
+    d->arch.pqos_rmid = 0;
+    pqosm->rmid_inuse--;
+}
+
 /*
  * Local variables:
  * mode: C
diff --git a/xen/include/asm-x86/domain.h b/xen/include/asm-x86/domain.h
index abf55fb..cd53108 100644
--- a/xen/include/asm-x86/domain.h
+++ b/xen/include/asm-x86/domain.h
@@ -313,6 +313,8 @@ struct arch_domain
     /* Shared page for notifying that explicit PIRQ EOI is required. */
     unsigned long *pirq_eoi_map;
     unsigned long pirq_eoi_map_mfn;
+
+    unsigned int pqos_rmid; /* QoS monitoring RMID assigned to the domain */
 } __cacheline_aligned;
 
 #define has_arch_pdevs(d)    (!list_empty(&(d)->arch.pdev_list))
diff --git a/xen/include/asm-x86/pqos.h b/xen/include/asm-x86/pqos.h
index 1ec7970..16aeb24 100644
--- a/xen/include/asm-x86/pqos.h
+++ b/xen/include/asm-x86/pqos.h
@@ -16,6 +16,7 @@
 #ifndef __ASM_PQOS_H__
 #define __ASM_PQOS_H__
 
+#include <xen/sched.h>
 #include <public/xen.h>
 
 /* QoS Resource Type Enumeration */
@@ -41,7 +42,14 @@ struct pqos_monitor {
 };
 extern struct pqos_monitor *pqosm;
 
+static inline bool_t pqos_monitor_enabled(void)
+{
+    return !!pqosm;
+}
+
 void __init init_platform_qos(void);
+int pqos_monitor_alloc_rmid(struct domain *d);
+void pqos_monitor_free_rmid(struct domain *d);
 
 #endif /* __ASM_PQOS_H__ */
 
diff --git a/xen/include/public/domctl.h b/xen/include/public/domctl.h
index 5b11bbf..af9e775 100644
--- a/xen/include/public/domctl.h
+++ b/xen/include/public/domctl.h
@@ -936,6 +936,16 @@ typedef struct xen_domctl_vcpu_msrs xen_domctl_vcpu_msrs_t;
 DEFINE_XEN_GUEST_HANDLE(xen_domctl_vcpu_msrs_t);
 #endif
 
+struct xen_domctl_pqos_monitor_op {
+#define XEN_DOMCTL_PQOS_MONITOR_OP_DETACH         0
+#define XEN_DOMCTL_PQOS_MONITOR_OP_ATTACH         1
+#define XEN_DOMCTL_PQOS_MONITOR_OP_QUERY_RMID     2
+    uint32_t cmd;
+    uint32_t data;
+};
+typedef struct xen_domctl_pqos_op xen_domctl_pqos_op_t;
+DEFINE_XEN_GUEST_HANDLE(xen_domctl_pqos_op_t);
+
 struct xen_domctl {
     uint32_t cmd;
 #define XEN_DOMCTL_createdomain                   1
@@ -1008,6 +1018,7 @@ struct xen_domctl {
 #define XEN_DOMCTL_cacheflush                    71
 #define XEN_DOMCTL_get_vcpu_msrs                 72
 #define XEN_DOMCTL_set_vcpu_msrs                 73
+#define XEN_DOMCTL_pqos_monitor_op               74
 #define XEN_DOMCTL_gdbsx_guestmemio            1000
 #define XEN_DOMCTL_gdbsx_pausevcpu             1001
 #define XEN_DOMCTL_gdbsx_unpausevcpu           1002
@@ -1068,6 +1079,7 @@ struct xen_domctl {
         struct xen_domctl_cacheflush        cacheflush;
         struct xen_domctl_gdbsx_pauseunp_vcpu gdbsx_pauseunp_vcpu;
         struct xen_domctl_gdbsx_domstatus   gdbsx_domstatus;
+        struct xen_domctl_pqos_monitor_op   pqos_monitor_op;
         uint8_t                             pad[128];
     } u;
 };
-- 
1.7.9.5

Re: [PATCH v12 5/9] x86: dynamically attach/detach QoS monitoring service for a guest

[Jan Beulich]

>>> On 04.07.14 at 10:34, <dongxiao.xu@intel.com> wrote:
> +    case XEN_DOMCTL_pqos_monitor_op:
> +        if ( !pqos_monitor_enabled() )
> +        {
> +            ret = -ENODEV;
> +            break;
> +        }
> +
> +        switch ( domctl->u.pqos_monitor_op.cmd )
> +        {
> +        case XEN_DOMCTL_PQOS_MONITOR_OP_ATTACH:
> +            ret = pqos_monitor_alloc_rmid(d);
> +            break;

Wouldn't it make sense to return the allocated rmid from this operation?

> +int pqos_monitor_alloc_rmid(struct domain *d)
> +{
> +    int rc = 0;
> +    unsigned int rmid;
> +
> +    ASSERT(pqos_monitor_enabled());
> +
> +    if ( d->arch.pqos_rmid > 0 )
> +    {
> +        rc = -EEXIST;
> +        return rc;

"return -EEXIST" please (unless you're being paid my number of
lines of code).

> +    }
> +
> +    for ( rmid = pqosm->rmid_min; rmid <= pqosm->rmid_max; rmid++ )
> +    {
> +        if ( pqosm->rmid_to_dom[rmid] != DOMID_INVALID)
> +            continue;
> +
> +        pqosm->rmid_to_dom[rmid] = d->domain_id;
> +        break;
> +    }
> +
> +    /* No RMID available, assign RMID=0 by default */
> +    if ( rmid > pqosm->rmid_max )
> +    {
> +        rmid = 0;
> +        rc = -EUSERS;
> +    }
> +    else
> +        pqosm->rmid_inuse++;
> +
> +    d->arch.pqos_rmid = rmid;
> +
> +    return rc;
> +}
> +
> +void pqos_monitor_free_rmid(struct domain *d)
> +{
> +    unsigned int rmid;
> +
> +    rmid = d->arch.pqos_rmid;
> +    /* We do not free system reserved "RMID=0" */
> +    if ( rmid == 0 )
> +        return;
> +
> +    pqosm->rmid_to_dom[rmid] = DOMID_INVALID;
> +    d->arch.pqos_rmid = 0;
> +    pqosm->rmid_inuse--;
> +}

This pair of functions should get a brief comment added explaining
why explicit locking here isn't needed.

> --- a/xen/include/asm-x86/pqos.h
> +++ b/xen/include/asm-x86/pqos.h
> @@ -16,6 +16,7 @@
>  #ifndef __ASM_PQOS_H__
>  #define __ASM_PQOS_H__
>  
> +#include <xen/sched.h>

You'd get away with just xen/types.h here if you forward declared
struct domain below.

Jan

Re: [PATCH v12 5/9] x86: dynamically attach/detach QoS monitoring service for a guest

[Xu, Dongxiao]

> -----Original Message-----
> From: Jan Beulich [mailto:JBeulich@suse.com]
> Sent: Friday, July 04, 2014 8:06 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 5/9] x86: dynamically attach/detach QoS monitoring
> service for a guest
> 
> >>> On 04.07.14 at 10:34, <dongxiao.xu@intel.com> wrote:
> > +    case XEN_DOMCTL_pqos_monitor_op:
> > +        if ( !pqos_monitor_enabled() )
> > +        {
> > +            ret = -ENODEV;
> > +            break;
> > +        }
> > +
> > +        switch ( domctl->u.pqos_monitor_op.cmd )
> > +        {
> > +        case XEN_DOMCTL_PQOS_MONITOR_OP_ATTACH:
> > +            ret = pqos_monitor_alloc_rmid(d);
> > +            break;
> 
> Wouldn't it make sense to return the allocated rmid from this operation?

The current return type for this function is int, where 0 stands correct, and -EXXX stands for error.
However rmid is "unsigned int" type, which leads to type conflict.

The allocated rmid will be stored to domain structure inside the function.

Thanks,
Dongxiao

> 
> > +int pqos_monitor_alloc_rmid(struct domain *d)
> > +{
> > +    int rc = 0;
> > +    unsigned int rmid;
> > +
> > +    ASSERT(pqos_monitor_enabled());
> > +
> > +    if ( d->arch.pqos_rmid > 0 )
> > +    {
> > +        rc = -EEXIST;
> > +        return rc;
> 
> "return -EEXIST" please (unless you're being paid my number of
> lines of code).
> 
> > +    }
> > +
> > +    for ( rmid = pqosm->rmid_min; rmid <= pqosm->rmid_max; rmid++ )
> > +    {
> > +        if ( pqosm->rmid_to_dom[rmid] != DOMID_INVALID)
> > +            continue;
> > +
> > +        pqosm->rmid_to_dom[rmid] = d->domain_id;
> > +        break;
> > +    }
> > +
> > +    /* No RMID available, assign RMID=0 by default */
> > +    if ( rmid > pqosm->rmid_max )
> > +    {
> > +        rmid = 0;
> > +        rc = -EUSERS;
> > +    }
> > +    else
> > +        pqosm->rmid_inuse++;
> > +
> > +    d->arch.pqos_rmid = rmid;
> > +
> > +    return rc;
> > +}
> > +
> > +void pqos_monitor_free_rmid(struct domain *d)
> > +{
> > +    unsigned int rmid;
> > +
> > +    rmid = d->arch.pqos_rmid;
> > +    /* We do not free system reserved "RMID=0" */
> > +    if ( rmid == 0 )
> > +        return;
> > +
> > +    pqosm->rmid_to_dom[rmid] = DOMID_INVALID;
> > +    d->arch.pqos_rmid = 0;
> > +    pqosm->rmid_inuse--;
> > +}
> 
> This pair of functions should get a brief comment added explaining
> why explicit locking here isn't needed.
> 
> > --- a/xen/include/asm-x86/pqos.h
> > +++ b/xen/include/asm-x86/pqos.h
> > @@ -16,6 +16,7 @@
> >  #ifndef __ASM_PQOS_H__
> >  #define __ASM_PQOS_H__
> >
> > +#include <xen/sched.h>
> 
> You'd get away with just xen/types.h here if you forward declared
> struct domain below.
> 
> Jan

Re: [PATCH v12 5/9] x86: dynamically attach/detach QoS monitoring service for a guest

[Jan Beulich]

>>> On 15.07.14 at 07:31, <dongxiao.xu@intel.com> wrote:
>> From: Jan Beulich [mailto:JBeulich@suse.com]
>> Sent: Friday, July 04, 2014 8:06 PM
>> >>> On 04.07.14 at 10:34, <dongxiao.xu@intel.com> wrote:
>> > +    case XEN_DOMCTL_pqos_monitor_op:
>> > +        if ( !pqos_monitor_enabled() )
>> > +        {
>> > +            ret = -ENODEV;
>> > +            break;
>> > +        }
>> > +
>> > +        switch ( domctl->u.pqos_monitor_op.cmd )
>> > +        {
>> > +        case XEN_DOMCTL_PQOS_MONITOR_OP_ATTACH:
>> > +            ret = pqos_monitor_alloc_rmid(d);
>> > +            break;
>> 
>> Wouldn't it make sense to return the allocated rmid from this operation?
> 
> The current return type for this function is int, where 0 stands correct, 
> and -EXXX stands for error.
> However rmid is "unsigned int" type, which leads to type conflict.

Only if full 32 bits would ever be used for RMID. But anyway, if the
caller doesn't really care, or if the extra hypercall is no problem, just
leave it the way it is.

Jan

[PATCH v12 6/9] x86: collect global QoS monitoring information

[Dongxiao Xu]

This implementation tries to put all policies into user space, thus some
global QoS monitoring information needs to be exposed, such as the total
RMID count, L3 upscaling factor, etc.

Signed-off-by: Dongxiao Xu <dongxiao.xu@intel.com>
---
 xen/arch/x86/sysctl.c       |   56 +++++++++++++++++++++++++++++++++++++++++++
 xen/include/public/sysctl.h |   14 +++++++++++
 2 files changed, 70 insertions(+)

diff --git a/xen/arch/x86/sysctl.c b/xen/arch/x86/sysctl.c
index 15d4b91..0a21ad2 100644
--- a/xen/arch/x86/sysctl.c
+++ b/xen/arch/x86/sysctl.c
@@ -28,6 +28,7 @@
 #include <xen/nodemask.h>
 #include <xen/cpu.h>
 #include <xsm/xsm.h>
+#include <asm/pqos.h>
 
 #define get_xen_guest_handle(val, hnd)  do { val = (hnd).p; } while (0)
 
@@ -70,6 +71,7 @@ long arch_do_sysctl(
     struct xen_sysctl *sysctl, XEN_GUEST_HANDLE_PARAM(xen_sysctl_t) u_sysctl)
 {
     long ret = 0;
+    bool_t copyback = 0;
 
     switch ( sysctl->cmd )
     {
@@ -101,11 +103,65 @@ long arch_do_sysctl(
     }
     break;
 
+    case XEN_SYSCTL_pqos_monitor_op:
+        if ( !pqos_monitor_enabled() )
+            return -ENODEV;
+
+        switch ( sysctl->u.pqos_monitor_op.cmd )
+        {
+        case XEN_SYSCTL_PQOS_MONITOR_cqm_enabled:
+            sysctl->u.pqos_monitor_op.data =
+                (pqosm->qm_features & QOS_MONITOR_TYPE_L3) &&
+                (pqosm->l3m.l3_features & L3_FEATURE_OCCUPANCY);
+            break;
+        case XEN_SYSCTL_PQOS_MONITOR_get_total_rmid:
+            sysctl->u.pqos_monitor_op.data =
+                pqosm->rmid_max - pqosm->rmid_min + 1;
+            break;
+        case XEN_SYSCTL_PQOS_MONITOR_get_l3_upscaling_factor:
+            sysctl->u.pqos_monitor_op.data = pqosm->l3m.upscaling_factor;
+            break;
+        case XEN_SYSCTL_PQOS_MONITOR_get_l3_cache_size:
+            sysctl->u.pqos_monitor_op.data = boot_cpu_data.x86_cache_size;
+            break;
+        case XEN_SYSCTL_PQOS_MONITOR_get_socket_cpu:
+        {
+            unsigned int i, cpu;
+            unsigned int socket = sysctl->u.pqos_monitor_op.data;
+
+            for ( i = 0; i < nr_cpu_ids; i++ )
+            {
+                if ( cpu_to_socket(i) < 0 || cpu_to_socket(i) != socket )
+                    continue;
+                cpu = cpumask_any(per_cpu(cpu_core_mask, i));
+                if ( cpu < nr_cpu_ids )
+                {
+                    sysctl->u.pqos_monitor_op.data = cpu;
+                    break;
+                }
+            }
+
+            if ( i == nr_cpu_ids )
+                ret = -ESRCH;
+        }
+        break;
+
+        default:
+            sysctl->u.pqos_monitor_op.data = 0;
+            ret = -ENOSYS;
+            break;
+        }
+        copyback = 1;
+        break;
+
     default:
         ret = -ENOSYS;
         break;
     }
 
+    if ( copyback && __copy_to_guest(u_sysctl, sysctl, 1) )
+        ret = -EFAULT;
+
     return ret;
 }
 
diff --git a/xen/include/public/sysctl.h b/xen/include/public/sysctl.h
index 3588698..5ec1212 100644
--- a/xen/include/public/sysctl.h
+++ b/xen/include/public/sysctl.h
@@ -636,6 +636,18 @@ struct xen_sysctl_coverage_op {
 typedef struct xen_sysctl_coverage_op xen_sysctl_coverage_op_t;
 DEFINE_XEN_GUEST_HANDLE(xen_sysctl_coverage_op_t);
 
+#define XEN_SYSCTL_PQOS_MONITOR_get_total_rmid            0
+#define XEN_SYSCTL_PQOS_MONITOR_get_l3_upscaling_factor   1
+#define XEN_SYSCTL_PQOS_MONITOR_get_l3_cache_size         2
+#define XEN_SYSCTL_PQOS_MONITOR_get_socket_cpu            3
+#define XEN_SYSCTL_PQOS_MONITOR_cqm_enabled               4
+struct xen_sysctl_pqos_monitor_op {
+    uint32_t cmd;
+    uint64_aligned_t data;
+};
+typedef struct xen_sysctl_pqos_monitor_op xen_sysctl_pqos_monitor_op_t;
+DEFINE_XEN_GUEST_HANDLE(xen_sysctl_pqos_monitor_op_t);
+
 
 struct xen_sysctl {
     uint32_t cmd;
@@ -658,6 +670,7 @@ struct xen_sysctl {
 #define XEN_SYSCTL_cpupool_op                    18
 #define XEN_SYSCTL_scheduler_op                  19
 #define XEN_SYSCTL_coverage_op                   20
+#define XEN_SYSCTL_pqos_monitor_op               21
     uint32_t interface_version; /* XEN_SYSCTL_INTERFACE_VERSION */
     union {
         struct xen_sysctl_readconsole       readconsole;
@@ -679,6 +692,7 @@ struct xen_sysctl {
         struct xen_sysctl_cpupool_op        cpupool_op;
         struct xen_sysctl_scheduler_op      scheduler_op;
         struct xen_sysctl_coverage_op       coverage_op;
+        struct xen_sysctl_pqos_monitor_op   pqos_monitor_op;
         uint8_t                             pad[128];
     } u;
 };
-- 
1.7.9.5

Re: [PATCH v12 6/9] x86: collect global QoS monitoring information

[Jan Beulich]

>>> On 04.07.14 at 10:34, <dongxiao.xu@intel.com> wrote:
> +        case XEN_SYSCTL_PQOS_MONITOR_get_l3_cache_size:
> +            sysctl->u.pqos_monitor_op.data = boot_cpu_data.x86_cache_size;
> +            break;

ISTR having asked before - is boot_cpu_data.x86_cache_size really
always the L3 cache size?

> +        case XEN_SYSCTL_PQOS_MONITOR_get_socket_cpu:
> +        {
> +            unsigned int i, cpu;
> +            unsigned int socket = sysctl->u.pqos_monitor_op.data;
> +
> +            for ( i = 0; i < nr_cpu_ids; i++ )
> +            {
> +                if ( cpu_to_socket(i) < 0 || cpu_to_socket(i) != socket )
> +                    continue;
> +                cpu = cpumask_any(per_cpu(cpu_core_mask, i));
> +                if ( cpu < nr_cpu_ids )
> +                {
> +                    sysctl->u.pqos_monitor_op.data = cpu;
> +                    break;
> +                }
> +            }
> +
> +            if ( i == nr_cpu_ids )
> +                ret = -ESRCH;
> +        }
> +        break;

It should be possible for the tools to get this information by using
XEN_SYSCTL_topologyinfo.

> +
> +        default:
> +            sysctl->u.pqos_monitor_op.data = 0;
> +            ret = -ENOSYS;
> +            break;
> +        }
> +        copyback = 1;
> +        break;
> +
>      default:
>          ret = -ENOSYS;
>          break;
>      }
>  
> +    if ( copyback && __copy_to_guest(u_sysctl, sysctl, 1) )
> +        ret = -EFAULT;

That's not very nice here: You only ever want to copy back a single
field (sysctl->u.pqos_monitor_op.data), so please do just that at
the end of the case XEN_SYSCTL_pqos_monitor_op handling.

> +#define XEN_SYSCTL_PQOS_MONITOR_get_total_rmid            0
> +#define XEN_SYSCTL_PQOS_MONITOR_get_l3_upscaling_factor   1
> +#define XEN_SYSCTL_PQOS_MONITOR_get_l3_cache_size         2
> +#define XEN_SYSCTL_PQOS_MONITOR_get_socket_cpu            3
> +#define XEN_SYSCTL_PQOS_MONITOR_cqm_enabled               4
> +struct xen_sysctl_pqos_monitor_op {
> +    uint32_t cmd;
> +    uint64_aligned_t data;

Please explicitly add a field between the two (e.g. named "flags"),
and check it to be zero in the handler. That way we have room for
extending the interface without bumping the sysctl interface
version.

Jan

Re: [PATCH v12 6/9] x86: collect global QoS monitoring information

[Xu, Dongxiao]

> -----Original Message-----
> From: Jan Beulich [mailto:JBeulich@suse.com]
> Sent: Friday, July 04, 2014 8:14 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 6/9] x86: collect global QoS monitoring information
> 
> >>> On 04.07.14 at 10:34, <dongxiao.xu@intel.com> wrote:
> > +        case XEN_SYSCTL_PQOS_MONITOR_get_l3_cache_size:
> > +            sysctl->u.pqos_monitor_op.data =
> boot_cpu_data.x86_cache_size;
> > +            break;
> 
> ISTR having asked before - is boot_cpu_data.x86_cache_size really
> always the L3 cache size?

On current machines with CQM enabled, boot_cpu_data.x86_cache_size reflects the L3 cache size, unless if later we have more level of caches (maybe L4, L5, etc... just guessing).
I think the current solution is okay, unless you prefer to have another round CPUID enumeration here.

> 
> > +        case XEN_SYSCTL_PQOS_MONITOR_get_socket_cpu:
> > +        {
> > +            unsigned int i, cpu;
> > +            unsigned int socket = sysctl->u.pqos_monitor_op.data;
> > +
> > +            for ( i = 0; i < nr_cpu_ids; i++ )
> > +            {
> > +                if ( cpu_to_socket(i) < 0 || cpu_to_socket(i) != socket )
> > +                    continue;
> > +                cpu = cpumask_any(per_cpu(cpu_core_mask, i));
> > +                if ( cpu < nr_cpu_ids )
> > +                {
> > +                    sysctl->u.pqos_monitor_op.data = cpu;
> > +                    break;
> > +                }
> > +            }
> > +
> > +            if ( i == nr_cpu_ids )
> > +                ret = -ESRCH;
> > +        }
> > +        break;
> 
> It should be possible for the tools to get this information by using
> XEN_SYSCTL_topologyinfo.

I noticed that the XEN_SYSCTL_topologyinfo hypercall is heavy loaded, which passes more data between hypervisor and guest, while we only need one "int" result.

> 
> > +
> > +        default:
> > +            sysctl->u.pqos_monitor_op.data = 0;
> > +            ret = -ENOSYS;
> > +            break;
> > +        }
> > +        copyback = 1;
> > +        break;
> > +
> >      default:
> >          ret = -ENOSYS;
> >          break;
> >      }
> >
> > +    if ( copyback && __copy_to_guest(u_sysctl, sysctl, 1) )
> > +        ret = -EFAULT;
> 
> That's not very nice here: You only ever want to copy back a single
> field (sysctl->u.pqos_monitor_op.data), so please do just that at
> the end of the case XEN_SYSCTL_pqos_monitor_op handling.
> 
> > +#define XEN_SYSCTL_PQOS_MONITOR_get_total_rmid            0
> > +#define XEN_SYSCTL_PQOS_MONITOR_get_l3_upscaling_factor   1
> > +#define XEN_SYSCTL_PQOS_MONITOR_get_l3_cache_size         2
> > +#define XEN_SYSCTL_PQOS_MONITOR_get_socket_cpu            3
> > +#define XEN_SYSCTL_PQOS_MONITOR_cqm_enabled               4
> > +struct xen_sysctl_pqos_monitor_op {
> > +    uint32_t cmd;
> > +    uint64_aligned_t data;
> 
> Please explicitly add a field between the two (e.g. named "flags"),
> and check it to be zero in the handler. That way we have room for
> extending the interface without bumping the sysctl interface
> version.

Okay.

Thanks,
Dongxiao

> 
> Jan

Re: [PATCH v12 6/9] x86: collect global QoS monitoring information

[Jan Beulich]

>>> On 01.08.14 at 10:26, <dongxiao.xu@intel.com> wrote:
>>  -----Original Message-----
>> From: Jan Beulich [mailto:JBeulich@suse.com]
>> Sent: Friday, July 04, 2014 8:14 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 6/9] x86: collect global QoS monitoring information
>> 
>> >>> On 04.07.14 at 10:34, <dongxiao.xu@intel.com> wrote:
>> > +        case XEN_SYSCTL_PQOS_MONITOR_get_l3_cache_size:
>> > +            sysctl->u.pqos_monitor_op.data =
>> boot_cpu_data.x86_cache_size;
>> > +            break;
>> 
>> ISTR having asked before - is boot_cpu_data.x86_cache_size really
>> always the L3 cache size?
> 
> On current machines with CQM enabled, boot_cpu_data.x86_cache_size reflects 
> the L3 cache size, unless if later we have more level of caches (maybe L4, 
> L5, etc... just guessing).
> I think the current solution is okay, unless you prefer to have another 
> round CPUID enumeration here.

And as long as there aren't going to be CPUs without any L3.

In particular for future systems, if anyone ever altered
init_intel_cacheinfo(), I don't think (s)he'll remember that the QOS
code relies on x86_cache_size to be L3. So yes, I think you ought
to specifically look for the L3 cache size here.

>> > +        case XEN_SYSCTL_PQOS_MONITOR_get_socket_cpu:
>> > +        {
>> > +            unsigned int i, cpu;
>> > +            unsigned int socket = sysctl->u.pqos_monitor_op.data;
>> > +
>> > +            for ( i = 0; i < nr_cpu_ids; i++ )
>> > +            {
>> > +                if ( cpu_to_socket(i) < 0 || cpu_to_socket(i) != socket )
>> > +                    continue;
>> > +                cpu = cpumask_any(per_cpu(cpu_core_mask, i));
>> > +                if ( cpu < nr_cpu_ids )
>> > +                {
>> > +                    sysctl->u.pqos_monitor_op.data = cpu;
>> > +                    break;
>> > +                }
>> > +            }
>> > +
>> > +            if ( i == nr_cpu_ids )
>> > +                ret = -ESRCH;
>> > +        }
>> > +        break;
>> 
>> It should be possible for the tools to get this information by using
>> XEN_SYSCTL_topologyinfo.
> 
> I noticed that the XEN_SYSCTL_topologyinfo hypercall is heavy loaded, which 
> passes more data between hypervisor and guest, while we only need one "int" 
> result.

To me that's rather lame an excuse for adding a new sysctl.

Jan

[PATCH v12 7/9] x86: enable QoS monitoring for each domain RMID

[Dongxiao Xu]

If the QoS monitoring service is attached to a domain, its related RMID
will be set to hardware for monitoring when the domain's vcpu is
scheduled in. When the domain's vcpu is scheduled out, RMID 0
(system reserved) will be set for monitoring.

Signed-off-by: Dongxiao Xu <dongxiao.xu@intel.com>
---
 xen/arch/x86/domain.c           |    5 +++++
 xen/arch/x86/pqos.c             |   26 ++++++++++++++++++++++++++
 xen/include/asm-x86/msr-index.h |    3 +++
 xen/include/asm-x86/pqos.h      |    1 +
 4 files changed, 35 insertions(+)

diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c
index f8e0e33..bb9a4d2 100644
--- a/xen/arch/x86/domain.c
+++ b/xen/arch/x86/domain.c
@@ -1398,6 +1398,8 @@ static void __context_switch(void)
     {
         memcpy(&p->arch.user_regs, stack_regs, CTXT_SWITCH_STACK_BYTES);
         vcpu_save_fpu(p);
+        if ( pqos_monitor_enabled() && pqosm->rmid_inuse )
+            pqos_monitor_assoc_rmid(0);
         p->arch.ctxt_switch_from(p);
     }
 
@@ -1422,6 +1424,9 @@ static void __context_switch(void)
         }
         vcpu_restore_fpu_eager(n);
         n->arch.ctxt_switch_to(n);
+
+        if ( pqos_monitor_enabled() && n->domain->arch.pqos_rmid > 0 )
+            pqos_monitor_assoc_rmid(n->domain->arch.pqos_rmid);
     }
 
     gdt = !is_pv_32on64_vcpu(n) ? per_cpu(gdt_table, cpu) :
diff --git a/xen/arch/x86/pqos.c b/xen/arch/x86/pqos.c
index 18fd1f9..5b2ccbe 100644
--- a/xen/arch/x86/pqos.c
+++ b/xen/arch/x86/pqos.c
@@ -17,10 +17,16 @@
 #include <xen/cpu.h>
 #include <asm/pqos.h>
 
+struct pqr_assoc {
+    uint64_t val;
+    bool_t initialized;
+};
+
 struct pqos_monitor *__read_mostly pqosm = NULL;
 static bool_t __initdata opt_pqos = 0;
 static bool_t __initdata opt_pqos_monitor = 0;
 static unsigned int __initdata opt_rmid_max = 255;
+static DEFINE_PER_CPU(struct pqr_assoc, pqr_assoc);
 
 static void __init parse_pqos_param(char *s)
 {
@@ -156,6 +162,26 @@ void pqos_monitor_free_rmid(struct domain *d)
     pqosm->rmid_inuse--;
 }
 
+void pqos_monitor_assoc_rmid(unsigned int rmid)
+{
+    uint64_t val;
+    uint64_t new_val;
+
+    if ( !this_cpu(pqr_assoc).initialized )
+    {
+        rdmsrl(MSR_IA32_PQR_ASSOC, this_cpu(pqr_assoc).val);
+        this_cpu(pqr_assoc).initialized = 1;
+    }
+    val = this_cpu(pqr_assoc).val;
+
+    new_val = (val & ~pqosm->rmid_mask) | (rmid & pqosm->rmid_mask);
+    if ( val != new_val )
+    {
+        wrmsrl(MSR_IA32_PQR_ASSOC, new_val);
+        this_cpu(pqr_assoc).val = new_val;
+    }
+}
+
 /*
  * Local variables:
  * mode: C
diff --git a/xen/include/asm-x86/msr-index.h b/xen/include/asm-x86/msr-index.h
index 70a8201..f894dfa 100644
--- a/xen/include/asm-x86/msr-index.h
+++ b/xen/include/asm-x86/msr-index.h
@@ -489,4 +489,7 @@
 /* Geode defined MSRs */
 #define MSR_GEODE_BUSCONT_CONF0		0x00001900
 
+/* Platform QoS MSRs */
+#define MSR_IA32_PQR_ASSOC             0x00000c8f
+
 #endif /* __ASM_MSR_INDEX_H */
diff --git a/xen/include/asm-x86/pqos.h b/xen/include/asm-x86/pqos.h
index 16aeb24..de5b808 100644
--- a/xen/include/asm-x86/pqos.h
+++ b/xen/include/asm-x86/pqos.h
@@ -50,6 +50,7 @@ static inline bool_t pqos_monitor_enabled(void)
 void __init init_platform_qos(void);
 int pqos_monitor_alloc_rmid(struct domain *d);
 void pqos_monitor_free_rmid(struct domain *d);
+void pqos_monitor_assoc_rmid(unsigned int rmid);
 
 #endif /* __ASM_PQOS_H__ */
 
-- 
1.7.9.5

Re: [PATCH v12 7/9] x86: enable QoS monitoring for each domain RMID

[Jan Beulich]

>>> On 04.07.14 at 10:34, <dongxiao.xu@intel.com> wrote:
> If the QoS monitoring service is attached to a domain, its related RMID
> will be set to hardware for monitoring when the domain's vcpu is
> scheduled in. When the domain's vcpu is scheduled out, RMID 0
> (system reserved) will be set for monitoring.
> 
> Signed-off-by: Dongxiao Xu <dongxiao.xu@intel.com>

Acked-by: Jan Beulich <jbeulich@suse.com>

[PATCH v12 8/9] xsm: add platform QoS related xsm policies

[Dongxiao Xu]

Add xsm policies for QoS monitoring related hypercalls.

Signed-off-by: Dongxiao Xu <dongxiao.xu@intel.com>
---
 tools/flask/policy/policy/modules/xen/xen.if |    2 +-
 tools/flask/policy/policy/modules/xen/xen.te |    3 ++-
 xen/xsm/flask/hooks.c                        |    7 +++++++
 xen/xsm/flask/policy/access_vectors          |    4 ++++
 4 files changed, 14 insertions(+), 2 deletions(-)

diff --git a/tools/flask/policy/policy/modules/xen/xen.if b/tools/flask/policy/policy/modules/xen/xen.if
index dedc035..5e7042c 100644
--- a/tools/flask/policy/policy/modules/xen/xen.if
+++ b/tools/flask/policy/policy/modules/xen/xen.if
@@ -49,7 +49,7 @@ define(`create_domain_common', `
 			getdomaininfo hypercall setvcpucontext setextvcpucontext
 			getscheduler getvcpuinfo getvcpuextstate getaddrsize
 			getaffinity setaffinity };
-	allow $1 $2:domain2 { set_cpuid settsc setscheduler setclaim  set_max_evtchn };
+	allow $1 $2:domain2 { set_cpuid settsc setscheduler setclaim set_max_evtchn pqos_monitor_op };
 	allow $1 $2:security check_context;
 	allow $1 $2:shadow enable;
 	allow $1 $2:mmu { map_read map_write adjust memorymap physmap pinpage mmuext_op };
diff --git a/tools/flask/policy/policy/modules/xen/xen.te b/tools/flask/policy/policy/modules/xen/xen.te
index 562b8df..fc3fdb2 100644
--- a/tools/flask/policy/policy/modules/xen/xen.te
+++ b/tools/flask/policy/policy/modules/xen/xen.te
@@ -66,6 +66,7 @@ allow dom0_t xen_t:xen {
 };
 allow dom0_t xen_t:xen2 {
     resource_op
+    pqos_monitor_op
 };
 allow dom0_t xen_t:mmu memorymap;
 
@@ -79,7 +80,7 @@ allow dom0_t dom0_t:domain {
 	getpodtarget setpodtarget set_misc_info set_virq_handler
 };
 allow dom0_t dom0_t:domain2 {
-	set_cpuid gettsc settsc setscheduler set_max_evtchn
+	set_cpuid gettsc settsc setscheduler set_max_evtchn pqos_monitor_op
 };
 allow dom0_t dom0_t:resource { add remove };
 
diff --git a/xen/xsm/flask/hooks.c b/xen/xsm/flask/hooks.c
index fcfed25..7d3fb52 100644
--- a/xen/xsm/flask/hooks.c
+++ b/xen/xsm/flask/hooks.c
@@ -715,6 +715,9 @@ static int flask_domctl(struct domain *d, int cmd)
     case XEN_DOMCTL_cacheflush:
         return current_has_perm(d, SECCLASS_DOMAIN2, DOMAIN2__CACHEFLUSH);
 
+    case XEN_DOMCTL_pqos_monitor_op:
+        return current_has_perm(d, SECCLASS_DOMAIN2, DOMAIN2__PQOS_MONITOR_OP);
+
     default:
         printk("flask_domctl: Unknown op %d\n", cmd);
         return -EPERM;
@@ -770,6 +773,10 @@ static int flask_sysctl(int cmd)
     case XEN_SYSCTL_numainfo:
         return domain_has_xen(current->domain, XEN__PHYSINFO);
 
+    case XEN_SYSCTL_pqos_monitor_op:
+        return avc_current_has_perm(SECINITSID_XEN, SECCLASS_XEN2,
+                                    XEN2__PQOS_MONITOR_OP, NULL);
+
     default:
         printk("flask_sysctl: Unknown op %d\n", cmd);
         return -EPERM;
diff --git a/xen/xsm/flask/policy/access_vectors b/xen/xsm/flask/policy/access_vectors
index b606441..fd71ac7 100644
--- a/xen/xsm/flask/policy/access_vectors
+++ b/xen/xsm/flask/policy/access_vectors
@@ -81,6 +81,8 @@ class xen2
 {
 # XENPF_resource_op
     resource_op
+# XEN_SYSCTL_pqos_monitor_op
+    pqos_monitor_op
 }
 
 # Classes domain and domain2 consist of operations that a domain performs on
@@ -208,6 +210,8 @@ class domain2
     cacheflush
 # Creation of the hardware domain when it is not dom0
     create_hardware_domain
+# XEN_DOMCTL_pqos_monitor_op
+    pqos_monitor_op
 }
 
 # Similar to class domain, but primarily contains domctls related to HVM domains
-- 
1.7.9.5

Re: [PATCH v12 8/9] xsm: add platform QoS related xsm policies

[Daniel De Graaf]

On 07/04/2014 04:34 AM, Dongxiao Xu wrote:
> Add xsm policies for QoS monitoring related hypercalls.
>
> Signed-off-by: Dongxiao Xu <dongxiao.xu@intel.com>

Acked-by: Daniel De Graaf <dgdegra@tycho.nsa.gov>

-- 
Daniel De Graaf
National Security Agency

[PATCH v12 9/9] tools: CMDs and APIs for Platform QoS Monitoring

[Dongxiao Xu]

Introduced some new xl commands to handle the platform QoS monitoring
related services.

The following two commands is to attach/detach the QoS monitoring
service to/from a certain domain.

$ xl pqos-monitor-attach domid
$ xl pqos-monitor-detach domid

This command is to display the QoS monitoring information, such as CQM,
to show the L3 cache occupancy.

$ xl pqos-monitor-show cache_occupancy <domid>

Signed-off-by: Dongxiao Xu <dongxiao.xu@intel.com>
---
 docs/man/xl.pod.1           |   24 +++++
 tools/libxc/Makefile        |    1 +
 tools/libxc/xc_msr_x86.h    |   36 +++++++
 tools/libxc/xc_pqos.c       |  219 +++++++++++++++++++++++++++++++++++++++++++
 tools/libxc/xenctrl.h       |   18 ++++
 tools/libxl/Makefile        |    2 +-
 tools/libxl/libxl.h         |   17 ++++
 tools/libxl/libxl_pqos.c    |  171 +++++++++++++++++++++++++++++++++
 tools/libxl/libxl_types.idl |    4 +
 tools/libxl/xl.h            |    3 +
 tools/libxl/xl_cmdimpl.c    |  131 ++++++++++++++++++++++++++
 tools/libxl/xl_cmdtable.c   |   17 ++++
 12 files changed, 642 insertions(+), 1 deletion(-)
 create mode 100644 tools/libxc/xc_msr_x86.h
 create mode 100644 tools/libxc/xc_pqos.c
 create mode 100644 tools/libxl/libxl_pqos.c

diff --git a/docs/man/xl.pod.1 b/docs/man/xl.pod.1
index 30bd4bf..9480a86 100644
--- a/docs/man/xl.pod.1
+++ b/docs/man/xl.pod.1
@@ -1366,6 +1366,30 @@ Load FLASK policy from the given policy file. The initial policy is provided to
 the hypervisor as a multiboot module; this command allows runtime updates to the
 policy. Loading new security policy will reset runtime changes to device labels.
 
+=head1 PLATFORM QOS MONITORING
+
+Some new hardware may offer monitoring capability in each logical processor to
+measure specific quality-of-service metric. In Xen implementation, the
+monitoring granularity is domain level. To monitor a specific domain, just
+attach the domain id with the monitoring service. When the domain doesn't need
+to be monitored any more, detach the domain id from the monitoring service.
+
+=over 4
+
+=item B<pqos-monitor-attach> [I<domain-id>]
+
+attach: Attach the platform QoS monitoring service to a domain.
+
+=item B<pqos-monitor-detach> [I<domain-id>]
+
+detach: Detach the platform QoS monitoring service from a domain.
+
+=item B<pqos-monitor-show> [I<qos-monitor-type>] [I<domain-id>]
+
+Show monitoring data for a certain domain or all domains. Current supported
+QoS monitor types are:
+ - "cache-occupancy": showing the L3 cache occupancy.
+
 =back
 
 =head1 TO BE DOCUMENTED
diff --git a/tools/libxc/Makefile b/tools/libxc/Makefile
index b8b28c1..91e65c4 100644
--- a/tools/libxc/Makefile
+++ b/tools/libxc/Makefile
@@ -35,6 +35,7 @@ CTRL_SRCS-y       += xc_kexec.c
 CTRL_SRCS-y       += xtl_core.c
 CTRL_SRCS-y       += xtl_logger_stdio.c
 CTRL_SRCS-y       += xc_resource.c
+CTRL_SRCS-y       += xc_pqos.c
 CTRL_SRCS-$(CONFIG_X86) += xc_pagetab.c
 CTRL_SRCS-$(CONFIG_Linux) += xc_linux.c xc_linux_osdep.c
 CTRL_SRCS-$(CONFIG_FreeBSD) += xc_freebsd.c xc_freebsd_osdep.c
diff --git a/tools/libxc/xc_msr_x86.h b/tools/libxc/xc_msr_x86.h
new file mode 100644
index 0000000..1e0ee99
--- /dev/null
+++ b/tools/libxc/xc_msr_x86.h
@@ -0,0 +1,36 @@
+/*
+ * xc_msr_x86.h
+ *
+ * MSR definition macros
+ *
+ * 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 of the GNU Lesser General Public License as published
+ * by the Free Software Foundation; version 2.1 only. with the special
+ * exception on linking described in file LICENSE.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU Lesser General Public License for more details.
+ */
+
+#ifndef XC_MSR_X86_H
+#define XC_MSR_X86_H
+
+#define MSR_IA32_QOSEVTSEL      0x00000c8d
+#define MSR_IA32_QMC            0x00000c8e
+
+#endif
+
+/*
+ * Local variables:
+ * mode: C
+ * c-file-style: "BSD"
+ * c-basic-offset: 4
+ * tab-width: 4
+ * indent-tabs-mode: nil
+ * End:
+ */
diff --git a/tools/libxc/xc_pqos.c b/tools/libxc/xc_pqos.c
new file mode 100644
index 0000000..4adf7ef
--- /dev/null
+++ b/tools/libxc/xc_pqos.c
@@ -0,0 +1,219 @@
+/*
+ * xc_pqos.c
+ *
+ * platform QoS related API functions.
+ *
+ * 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 of the GNU Lesser General Public License as published
+ * by the Free Software Foundation; version 2.1 only. with the special
+ * exception on linking described in file LICENSE.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU Lesser General Public License for more details.
+ */
+
+#include "xc_private.h"
+#include "xc_msr_x86.h"
+
+#define IA32_QM_CTR_ERROR_MASK         (0x3ull << 62)
+
+#define EVTID_L3_OCCUPANCY             0x1
+
+int xc_pqos_monitor_attach(xc_interface *xch, uint32_t domid)
+{
+    DECLARE_DOMCTL;
+
+    domctl.cmd = XEN_DOMCTL_pqos_monitor_op;
+    domctl.domain = (domid_t)domid;
+    domctl.u.pqos_monitor_op.cmd = XEN_DOMCTL_PQOS_MONITOR_OP_ATTACH;
+
+    return do_domctl(xch, &domctl);
+}
+
+int xc_pqos_monitor_detach(xc_interface *xch, uint32_t domid)
+{
+    DECLARE_DOMCTL;
+
+    domctl.cmd = XEN_DOMCTL_pqos_monitor_op;
+    domctl.domain = (domid_t)domid;
+    domctl.u.pqos_monitor_op.cmd = XEN_DOMCTL_PQOS_MONITOR_OP_DETACH;
+
+    return do_domctl(xch, &domctl);
+}
+
+int xc_pqos_monitor_get_domain_rmid(xc_interface *xch, uint32_t domid,
+                                    uint32_t *rmid)
+{
+    int rc;
+    DECLARE_DOMCTL;
+
+    domctl.cmd = XEN_DOMCTL_pqos_monitor_op;
+    domctl.domain = (domid_t)domid;
+    domctl.u.pqos_monitor_op.cmd = XEN_DOMCTL_PQOS_MONITOR_OP_QUERY_RMID;
+
+    rc = do_domctl(xch, &domctl);
+
+    *rmid = domctl.u.pqos_monitor_op.data;
+
+    return rc;
+}
+
+int xc_pqos_monitor_get_total_rmid(xc_interface *xch, uint32_t *total_rmid)
+{
+    static int val = 0;
+    int rc;
+    DECLARE_SYSCTL;
+
+    if ( val )
+    {
+        *total_rmid = val;
+        return 0;
+    }
+
+    sysctl.cmd = XEN_SYSCTL_pqos_monitor_op;
+    sysctl.u.pqos_monitor_op.cmd = XEN_SYSCTL_PQOS_MONITOR_get_total_rmid;
+
+    rc = xc_sysctl(xch, &sysctl);
+    if ( !rc )
+        val = *total_rmid = sysctl.u.pqos_monitor_op.data;
+
+    return rc;
+}
+
+int xc_pqos_monitor_get_l3_upscaling_factor(xc_interface *xch,
+                                            uint32_t *upscaling_factor)
+{
+    static int val = 0;
+    int rc;
+    DECLARE_SYSCTL;
+
+    if ( val )
+    {
+        *upscaling_factor = val;
+        return 0;
+    }
+
+    sysctl.cmd = XEN_SYSCTL_pqos_monitor_op;
+    sysctl.u.pqos_monitor_op.cmd =
+        XEN_SYSCTL_PQOS_MONITOR_get_l3_upscaling_factor;
+
+    rc = xc_sysctl(xch, &sysctl);
+    if ( !rc )
+        val = *upscaling_factor = sysctl.u.pqos_monitor_op.data;
+
+    return rc;
+}
+
+int xc_pqos_monitor_get_l3_cache_size(xc_interface *xch,
+                                      uint32_t *l3_cache_size)
+{
+    static int val = 0;
+    int rc;
+    DECLARE_SYSCTL;
+
+    if ( val )
+    {
+        *l3_cache_size = val;
+        return 0;
+    }
+
+    sysctl.cmd = XEN_SYSCTL_pqos_monitor_op;
+    sysctl.u.pqos_monitor_op.cmd =
+        XEN_SYSCTL_PQOS_MONITOR_get_l3_cache_size;
+
+    rc = xc_sysctl(xch, &sysctl);
+    if ( !rc )
+        val = *l3_cache_size= sysctl.u.pqos_monitor_op.data;
+
+    return rc;
+}
+
+int xc_pqos_monitor_select_socket_cpu(xc_interface *xch, uint32_t socket)
+{
+    int rc;
+    DECLARE_SYSCTL;
+
+    sysctl.cmd = XEN_SYSCTL_pqos_monitor_op;
+    sysctl.u.pqos_monitor_op.cmd = XEN_SYSCTL_PQOS_MONITOR_get_socket_cpu;
+    sysctl.u.pqos_monitor_op.data = socket;
+
+    rc = do_sysctl(xch, &sysctl);
+    if ( !rc )
+        return sysctl.u.pqos_monitor_op.data;
+
+    return -1;
+}
+
+int xc_pqos_monitor_get_data(xc_interface *xch, uint32_t rmid,
+    uint32_t cpu, xc_pqos_monitor_type type, uint64_t *monitor_data)
+{
+    xc_resource_data_t rsc_data[2];
+    uint32_t evtid;
+    int rc;
+
+    switch ( type )
+    {
+    case XC_PQOS_MONITOR_L3_OCCUPANCY:
+        evtid = EVTID_L3_OCCUPANCY;
+        break;
+    default:
+        return -1;
+    }
+
+    rsc_data[0].cmd = XEN_RESOURCE_OP_WRITE;
+    rsc_data[0].cpu = cpu;
+    rsc_data[0].idx = MSR_IA32_QOSEVTSEL;
+    rsc_data[0].val = (uint64_t)rmid << 32 | evtid;
+    rsc_data[1].cmd = XEN_RESOURCE_OP_READ;
+    rsc_data[1].cpu = cpu;
+    rsc_data[1].idx = MSR_IA32_QMC;
+    rsc_data[1].val = 0;
+
+    rc = xc_resource_op(xch, 2, XEN_RESOURCE_TYPE_MSR, rsc_data);
+    if ( rc )
+        return rc;
+
+    if ( rsc_data[1].val & IA32_QM_CTR_ERROR_MASK )
+        return -1;
+
+    *monitor_data = rsc_data[1].val;
+
+    return 0;
+}
+
+int xc_pqos_monitor_cqm_enabled(xc_interface *xch)
+{
+    static int val = -1;
+    int rc;
+    DECLARE_SYSCTL;
+
+    if ( val >= 0 )
+        return val;
+
+    sysctl.cmd = XEN_SYSCTL_pqos_monitor_op;
+    sysctl.u.pqos_monitor_op.cmd = XEN_SYSCTL_PQOS_MONITOR_cqm_enabled;
+
+    rc = do_sysctl(xch, &sysctl);
+    if ( !rc )
+    {
+        val = sysctl.u.pqos_monitor_op.data;
+        return val;
+    }
+
+    return 0;
+}
+
+/*
+ * Local variables:
+ * mode: C
+ * c-file-style: "BSD"
+ * c-basic-offset: 4
+ * tab-width: 4
+ * indent-tabs-mode: nil
+ * End:
+ */
diff --git a/tools/libxc/xenctrl.h b/tools/libxc/xenctrl.h
index ca0b207..08ee12f 100644
--- a/tools/libxc/xenctrl.h
+++ b/tools/libxc/xenctrl.h
@@ -2608,6 +2608,24 @@ typedef xenpf_resource_data_t xc_resource_data_t;
 int xc_resource_op(xc_interface *xch, uint32_t nr,
     uint32_t type, xc_resource_data_t *data);
 
+enum xc_pqos_monitor_type {
+    XC_PQOS_MONITOR_L3_OCCUPANCY,
+};
+typedef enum xc_pqos_monitor_type xc_pqos_monitor_type;
+int xc_pqos_monitor_attach(xc_interface *xch, uint32_t domid);
+int xc_pqos_monitor_detach(xc_interface *xch, uint32_t domid);
+int xc_pqos_monitor_get_domain_rmid(xc_interface *xch, uint32_t domid,
+    uint32_t *rmid);
+int xc_pqos_monitor_get_total_rmid(xc_interface *xch, uint32_t *total_rmid);
+int xc_pqos_monitor_get_l3_upscaling_factor(xc_interface *xch,
+    uint32_t *upscaling_factor);
+int xc_pqos_monitor_get_l3_cache_size(xc_interface *xch,
+    uint32_t *l3_cache_size);
+int xc_pqos_monitor_select_socket_cpu(xc_interface *xch, uint32_t socket);
+int xc_pqos_monitor_get_data(xc_interface *xch, uint32_t rmid,
+    uint32_t cpu, uint32_t pqos_monitor_type, uint64_t *monitor_data);
+int xc_pqos_monitor_cqm_enabled(xc_interface *xch);
+
 #endif /* XENCTRL_H */
 
 /*
diff --git a/tools/libxl/Makefile b/tools/libxl/Makefile
index 7fc42c8..14cdd54 100644
--- a/tools/libxl/Makefile
+++ b/tools/libxl/Makefile
@@ -43,7 +43,7 @@ LIBXL_OBJS-y += libxl_blktap2.o
 else
 LIBXL_OBJS-y += libxl_noblktap2.o
 endif
-LIBXL_OBJS-$(CONFIG_X86) += libxl_cpuid.o libxl_x86.o
+LIBXL_OBJS-$(CONFIG_X86) += libxl_cpuid.o libxl_x86.o libxl_pqos.o
 LIBXL_OBJS-$(CONFIG_ARM) += libxl_nocpuid.o libxl_arm.o
 
 ifeq ($(CONFIG_NetBSD),y)
diff --git a/tools/libxl/libxl.h b/tools/libxl/libxl.h
index 69ceac8..f510ade 100644
--- a/tools/libxl/libxl.h
+++ b/tools/libxl/libxl.h
@@ -533,6 +533,13 @@ typedef uint8_t libxl_mac[6];
 #define LIBXL_MAC_FMTLEN ((2*6)+5) /* 6 hex bytes plus 5 colons */
 #define LIBXL_MAC_BYTES(mac) mac[0], mac[1], mac[2], mac[3], mac[4], mac[5]
 
+/*
+ * LIBXL_HAVE_PQOS_MONITOR_CQM
+ *
+ * If this is defined, the cache QoS monitoring feature is suported.
+ */
+#define LIBXL_HAVE_PQOS_MONITOR_CQM 1
+
 typedef char **libxl_string_list;
 void libxl_string_list_dispose(libxl_string_list *sl);
 int libxl_string_list_length(const libxl_string_list *sl);
@@ -1217,6 +1224,16 @@ int libxl_flask_getenforce(libxl_ctx *ctx);
 int libxl_flask_setenforce(libxl_ctx *ctx, int mode);
 int libxl_flask_loadpolicy(libxl_ctx *ctx, void *policy, uint32_t size);
 
+int libxl_pqos_monitor_attach(libxl_ctx *ctx, uint32_t domid);
+int libxl_pqos_monitor_detach(libxl_ctx *ctx, uint32_t domid);
+int libxl_pqos_monitor_domain_attached(libxl_ctx *ctx, uint32_t domid);
+int libxl_pqos_monitor_cqm_enabled(libxl_ctx *ctx);
+int libxl_pqos_monitor_get_total_rmid(libxl_ctx *ctx, uint32_t *total_rmid);
+int libxl_pqos_monitor_get_l3_cache_size(libxl_ctx *ctx,
+    uint32_t *l3_cache_size);
+int libxl_pqos_monitor_get_cache_occupancy(libxl_ctx *ctx, uint32_t domid,
+    uint32_t socketid, uint64_t *l3_cache_occupancy);
+
 /* misc */
 
 /* Each of these sets or clears the flag according to whether the
diff --git a/tools/libxl/libxl_pqos.c b/tools/libxl/libxl_pqos.c
new file mode 100644
index 0000000..8c56e67
--- /dev/null
+++ b/tools/libxl/libxl_pqos.c
@@ -0,0 +1,171 @@
+/*
+ * 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 of the GNU Lesser General Public License as published
+ * by the Free Software Foundation; version 2.1 only. with the special
+ * exception on linking described in file LICENSE.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU Lesser General Public License for more details.
+ */
+
+#include "libxl_osdeps.h" /* must come before any other headers */
+#include "libxl_internal.h"
+
+
+#define IA32_QM_CTR_ERROR_MASK         (0x3ul << 62)
+
+static const char * const msg[] = {
+    [ENOSYS] = "unsupported operation",
+    [ENODEV] = "Platform QoS Monitoring is not supported in this system",
+    [EEXIST] = "Platform QoS Monitoring is already attached to this domain",
+    [ENOENT] = "Platform QoS Monitoring is not attached to this domain",
+    [EUSERS] = "there is no free RMID available",
+    [ESRCH]  = "is this Domain ID valid?",
+    [EFAULT] = "cannot find a valid CPU belonging to this socket",
+};
+
+static void libxl_pqos_monitor_err_msg(libxl_ctx *ctx, int err)
+{
+    GC_INIT(ctx);
+
+    switch (err) {
+    case EINVAL:
+    case ENODEV:
+    case EEXIST:
+    case EUSERS:
+    case ESRCH:
+    case ENOENT:
+        LOGE(ERROR, "%s", msg[err]);
+        break;
+    default:
+        LOGE(ERROR, "errno: %d", err);
+    }
+
+    GC_FREE;
+}
+
+int libxl_pqos_monitor_attach(libxl_ctx *ctx, uint32_t domid)
+{
+    int rc;
+
+    rc = xc_pqos_monitor_attach(ctx->xch, domid);
+    if (rc < 0) {
+        libxl_pqos_monitor_err_msg(ctx, errno);
+        return ERROR_FAIL;
+    }
+
+    return 0;
+}
+
+int libxl_pqos_monitor_detach(libxl_ctx *ctx, uint32_t domid)
+{
+    int rc;
+
+    rc = xc_pqos_monitor_detach(ctx->xch, domid);
+    if (rc < 0) {
+        libxl_pqos_monitor_err_msg(ctx, errno);
+        return ERROR_FAIL;
+    }
+
+    return 0;
+}
+
+int libxl_pqos_monitor_domain_attached(libxl_ctx *ctx, uint32_t domid)
+{
+    int rc;
+    uint32_t rmid;
+
+    rc = xc_pqos_monitor_get_domain_rmid(ctx->xch, domid, &rmid);
+    if (rc < 0)
+        return 0;
+
+    return !!rmid;
+}
+
+int libxl_pqos_monitor_cqm_enabled(libxl_ctx *ctx)
+{
+    return xc_pqos_monitor_cqm_enabled(ctx->xch);
+}
+
+int libxl_pqos_monitor_get_total_rmid(libxl_ctx *ctx, uint32_t *total_rmid)
+{
+    int rc;
+
+    rc = xc_pqos_monitor_get_total_rmid(ctx->xch, total_rmid);
+    if (rc < 0) {
+        libxl_pqos_monitor_err_msg(ctx, errno);
+        return ERROR_FAIL;
+    }
+
+    return 0;
+}
+
+int libxl_pqos_monitor_get_l3_cache_size(libxl_ctx *ctx,
+                                         uint32_t *l3_cache_size)
+{
+    int rc;
+
+    rc = xc_pqos_monitor_get_l3_cache_size(ctx->xch, l3_cache_size);
+    if (rc < 0) {
+        libxl_pqos_monitor_err_msg(ctx, errno);
+        return ERROR_FAIL;
+    }
+
+    return 0;
+}
+
+int libxl_pqos_monitor_get_cache_occupancy(libxl_ctx *ctx, uint32_t domid,
+    uint32_t socketid, uint64_t *l3_cache_occupancy)
+{
+    GC_INIT(ctx);
+
+    unsigned int rmid;
+    uint32_t upscaling_factor;
+    uint64_t monitor_data;
+    int cpu, rc;
+    xc_pqos_monitor_type type;
+
+    rc = xc_pqos_monitor_get_domain_rmid(ctx->xch, domid, &rmid);
+    if (rc < 0 || rmid == 0) {
+        LOGE(ERROR, "fail to get the domain rmid, "
+            "or domain is not attached with platform QoS monitoring service");
+        return ERROR_FAIL;
+    }
+
+    cpu = xc_pqos_monitor_select_socket_cpu(ctx->xch, socketid);
+    if (cpu < 0) {
+        LOGE(ERROR, "fail to select a cpu from socket %d", socketid);
+        return ERROR_FAIL;
+    }
+
+    type = XC_PQOS_MONITOR_L3_OCCUPANCY;
+    rc = xc_pqos_monitor_get_data(ctx->xch, rmid, cpu, type, &monitor_data);
+    if (rc < 0) {
+        LOGE(ERROR, "failed to get monitoring data");
+        return ERROR_FAIL;
+    }
+
+    rc = xc_pqos_monitor_get_l3_upscaling_factor(ctx->xch, &upscaling_factor);
+    if (rc < 0) {
+        LOGE(ERROR, "failed to get L3 upscaling factor");
+        return ERROR_FAIL;
+    }
+
+    *l3_cache_occupancy = upscaling_factor * monitor_data;
+
+    GC_FREE;
+    return 0;
+}
+
+/*
+ * Local variables:
+ * mode: C
+ * c-basic-offset: 4
+ * indent-tabs-mode: nil
+ * End:
+ */
diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl
index 1018142..2e7668b 100644
--- a/tools/libxl/libxl_types.idl
+++ b/tools/libxl/libxl_types.idl
@@ -604,3 +604,7 @@ libxl_event = Struct("event",[
                                  ])),
            ("domain_create_console_available", None),
            ]))])
+
+libxl_pqos_monitor_type = Enumeration("pqos_monitor_type", [
+    (1, "CACHE_OCCUPANCY"),
+    ])
diff --git a/tools/libxl/xl.h b/tools/libxl/xl.h
index 10a2e66..4e9635a 100644
--- a/tools/libxl/xl.h
+++ b/tools/libxl/xl.h
@@ -110,6 +110,9 @@ int main_loadpolicy(int argc, char **argv);
 int main_remus(int argc, char **argv);
 #endif
 int main_devd(int argc, char **argv);
+int main_pqos_monitor_attach(int argc, char **argv);
+int main_pqos_monitor_detach(int argc, char **argv);
+int main_pqos_monitor_show(int argc, char **argv);
 
 void help(const char *command);
 
diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
index be041f2..ef049b5 100644
--- a/tools/libxl/xl_cmdimpl.c
+++ b/tools/libxl/xl_cmdimpl.c
@@ -7303,6 +7303,137 @@ out:
     return ret;
 }
 
+static int pqos_monitor_show_cache_occupancy(uint32_t first_domain,
+                                             uint32_t nr_domains,
+                                             int ignore_domain_err)
+{
+    uint32_t domid, socketid, nr_sockets, total_rmid, l3_cache_size;
+    uint64_t l3_cache_occupancy;
+    libxl_physinfo info;
+    char *domain_name;
+    int rc, print_header;
+
+    if (!libxl_pqos_monitor_cqm_enabled(ctx)) {
+        printf("CQM is not supported in the system\n");
+        return -1;
+    }
+
+    rc = libxl_get_physinfo(ctx, &info);
+    if (rc < 0) {
+        printf("failed to get system socket number\n");
+        return -1;
+    }
+    nr_sockets = info.nr_cpus / info.threads_per_core / info.cores_per_socket;
+
+    rc = libxl_pqos_monitor_get_total_rmid(ctx, &total_rmid);
+    if (rc < 0) {
+        printf("failed to get system total rmid number\n");
+        return -1;
+    }
+
+    rc = libxl_pqos_monitor_get_l3_cache_size(ctx, &l3_cache_size);
+    if (rc < 0) {
+        printf("failed to get system l3 cache size\n");
+        return -1;
+    }
+
+    printf("Total RMID: %d\n", total_rmid);
+    printf("Per-Socket L3 Cache Size: %d KB\n", l3_cache_size);
+
+    print_header = 1;
+    for (domid = first_domain; domid < first_domain + nr_domains; domid++) {
+        if (!libxl_pqos_monitor_domain_attached(ctx, domid)) {
+            if (!ignore_domain_err)
+                printf("No CQM info for Domain %d.\n", domid);
+            continue;
+        }
+        if (print_header) {
+            printf("Name                                        ID");
+            for (socketid = 0; socketid < nr_sockets; socketid++)
+                printf("\tSocketID\tL3C_Usage");
+            printf("\n");
+            print_header = 0;
+        }
+        domain_name = libxl_domid_to_name(ctx, domid);
+        printf("%-40s %5d", domain_name, domid);
+        free(domain_name);
+        for (socketid = 0; socketid < nr_sockets; socketid++) {
+            rc = libxl_pqos_monitor_get_cache_occupancy(ctx, domid, socketid,
+                                                        &l3_cache_occupancy);
+            printf("%10u %13lu KB     ", socketid, l3_cache_occupancy/1024);
+        }
+        printf("\n");
+    }
+
+    return 0;
+}
+
+int main_pqos_monitor_attach(int argc, char **argv)
+{
+    uint32_t domid;
+    int opt, ret = 0;
+
+    SWITCH_FOREACH_OPT(opt, "", NULL, "pqos-monitor-attach", 1) {
+        /* No options */
+    }
+
+    domid = find_domain(argv[optind]);
+    ret = libxl_pqos_monitor_attach(ctx, domid);
+
+    return ret;
+}
+
+int main_pqos_monitor_detach(int argc, char **argv)
+{
+    uint32_t domid;
+    int opt, ret = 0;
+
+    SWITCH_FOREACH_OPT(opt, "", NULL, "pqos-monitor-detach", 1) {
+        /* No options */
+    }
+
+    domid = find_domain(argv[optind]);
+    ret = libxl_pqos_monitor_detach(ctx, domid);
+
+    return ret;
+}
+
+int main_pqos_monitor_show(int argc, char **argv)
+{
+    int opt, ret = 0;
+    uint32_t first_domain, nr_domains;
+    libxl_pqos_monitor_type type;
+
+    SWITCH_FOREACH_OPT(opt, "", NULL, "pqos-monitor-show", 1) {
+        /* No options */
+    }
+
+    libxl_pqos_monitor_type_from_string(argv[optind], &type);
+
+    if (optind + 1 >= argc) {
+        first_domain = 0;
+        nr_domains = 1024;
+    } else if (optind + 1 == argc - 1){
+        first_domain = find_domain(argv[optind + 1]);
+        nr_domains = 1;
+    } else {
+        help("pqos-monitor-show");
+        return 2;
+    }
+
+    switch (type) {
+    case LIBXL_PQOS_MONITOR_TYPE_CACHE_OCCUPANCY:
+        ret = pqos_monitor_show_cache_occupancy(first_domain, nr_domains,
+                                                nr_domains == 1024);
+        break;
+    default:
+        help("pqos-monitor-show");
+        return 2;
+    }
+
+    return ret;
+}
+
 /*
  * Local variables:
  * mode: C
diff --git a/tools/libxl/xl_cmdtable.c b/tools/libxl/xl_cmdtable.c
index 4279b9f..a2f8e9a 100644
--- a/tools/libxl/xl_cmdtable.c
+++ b/tools/libxl/xl_cmdtable.c
@@ -500,6 +500,23 @@ struct cmd_spec cmd_table[] = {
       "[options]",
       "-F                      Run in the foreground",
     },
+    { "pqos-monitor-attach",
+      &main_pqos_monitor_attach, 0, 1,
+      "Attach platform QoS monitoring to a domain",
+      "<Domain>",
+    },
+    { "pqos-monitor-detach",
+      &main_pqos_monitor_detach, 0, 1,
+      "Detach platform QoS monitoring from a domain",
+      "<Domain>",
+    },
+    { "pqos-monitor-show",
+      &main_pqos_monitor_show, 0, 1,
+      "Show platform QoS monitoring information, current supported QoS monitor "
+      "types are:\n"
+      "\"cache_occupancy\": cache QoS monitoring, showing the L3 cache occupancy",
+      "<QoS-Monitor-Type> <Domain>",
+    },
 };
 
 int cmdtable_len = sizeof(cmd_table)/sizeof(struct cmd_spec);
-- 
1.7.9.5

Re: [PATCH v12 9/9] tools: CMDs and APIs for Platform QoS Monitoring

[Ian Campbell]

On Fri, 2014-07-04 at 16:34 +0800, Dongxiao Xu wrote:
> Introduced some new xl commands to handle the platform QoS monitoring
> related services.
> 
> The following two commands is to attach/detach the QoS monitoring
> service to/from a certain domain.

Is there a literal "service" here to attach to? Or are these operations
more like enable/disable?

> diff --git a/tools/libxc/Makefile b/tools/libxc/Makefile
> index b8b28c1..91e65c4 100644
> --- a/tools/libxc/Makefile
> +++ b/tools/libxc/Makefile
> @@ -35,6 +35,7 @@ CTRL_SRCS-y       += xc_kexec.c
>  CTRL_SRCS-y       += xtl_core.c
>  CTRL_SRCS-y       += xtl_logger_stdio.c
>  CTRL_SRCS-y       += xc_resource.c
> +CTRL_SRCS-y       += xc_pqos.c

CONFIG_X86?

>  CTRL_SRCS-$(CONFIG_X86) += xc_pagetab.c
>  CTRL_SRCS-$(CONFIG_Linux) += xc_linux.c xc_linux_osdep.c
>  CTRL_SRCS-$(CONFIG_FreeBSD) += xc_freebsd.c xc_freebsd_osdep.c

> +int xc_pqos_monitor_get_domain_rmid(xc_interface *xch, uint32_t domid,
> +                                    uint32_t *rmid)
> +{
> +    int rc;
> +    DECLARE_DOMCTL;
> +
> +    domctl.cmd = XEN_DOMCTL_pqos_monitor_op;
> +    domctl.domain = (domid_t)domid;
> +    domctl.u.pqos_monitor_op.cmd = XEN_DOMCTL_PQOS_MONITOR_OP_QUERY_RMID;
> +
> +    rc = do_domctl(xch, &domctl);
> +
> +    *rmid = domctl.u.pqos_monitor_op.data;

Only if rc indicates success.

> +
> +    return rc;
> +}
> +
> +int xc_pqos_monitor_get_total_rmid(xc_interface *xch, uint32_t *total_rmid)
> +{
> +    static int val = 0;
> +    int rc;
> +    DECLARE_SYSCTL;
> +
> +    if ( val )
> +    {
> +        *total_rmid = val;

This can never change dynamically?

Likewise for several more similar things further down.

> diff --git a/tools/libxl/libxl.h b/tools/libxl/libxl.h
> index 69ceac8..f510ade 100644
> --- a/tools/libxl/libxl.h
> +++ b/tools/libxl/libxl.h
> @@ -533,6 +533,13 @@ typedef uint8_t libxl_mac[6];
>  #define LIBXL_MAC_FMTLEN ((2*6)+5) /* 6 hex bytes plus 5 colons */
>  #define LIBXL_MAC_BYTES(mac) mac[0], mac[1], mac[2], mac[3], mac[4], mac[5]
>  
> +/*
> + * LIBXL_HAVE_PQOS_MONITOR_CQM
> + *
> + * If this is defined, the cache QoS monitoring feature is suported.

"supported"

> +int libxl_pqos_monitor_get_l3_cache_size(libxl_ctx *ctx,
> +    uint32_t *l3_cache_size);
> +int libxl_pqos_monitor_get_cache_occupancy(libxl_ctx *ctx, uint32_t
> domid,
> +    uint32_t socketid, uint64_t *l3_cache_occupancy);

What are the units of the outputs of these functions?

Why is cache size 32 bits but occupancy is 64 ?

> diff --git a/tools/libxl/libxl_pqos.c b/tools/libxl/libxl_pqos.c
> new file mode 100644
> index 0000000..8c56e67
> --- /dev/null
> +++ b/tools/libxl/libxl_pqos.c
> @@ -0,0 +1,171 @@
> +/*
> + * 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 of the GNU Lesser General Public License as published
> + * by the Free Software Foundation; version 2.1 only. with the special
> + * exception on linking described in file LICENSE.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU Lesser General Public License for more details.
> + */
> +
> +#include "libxl_osdeps.h" /* must come before any other headers */
> +#include "libxl_internal.h"
> +
> +
> +#define IA32_QM_CTR_ERROR_MASK         (0x3ul << 62)
> +
> +static const char * const msg[] = {

Could this be local to the error printing function?

> +    [ENOSYS] = "unsupported operation",
> +    [ENODEV] = "Platform QoS Monitoring is not supported in this system",
> +    [EEXIST] = "Platform QoS Monitoring is already attached to this domain",
> +    [ENOENT] = "Platform QoS Monitoring is not attached to this domain",
> +    [EUSERS] = "there is no free RMID available",
> +    [ESRCH]  = "is this Domain ID valid?",
> +    [EFAULT] = "cannot find a valid CPU belonging to this socket",
> +};
> +
> +static void libxl_pqos_monitor_err_msg(libxl_ctx *ctx, int err)
> +{
> +    GC_INIT(ctx);
> +
> +    switch (err) {
> +    case EINVAL:
> +    case ENODEV:
> +    case EEXIST:
> +    case EUSERS:
> +    case ESRCH:
> +    case ENOENT:

This enumeration of the valid errnos is bound to get out of sync I
think.

You can compare err to ARRAY_SIZE(msg) and if it is within bounds check
it for NULL.

or  perhaps
case EINVAL: msg = "unsupported operation"; break;
etc

> +
> +int libxl_pqos_monitor_domain_attached(libxl_ctx *ctx, uint32_t domid)
> +{
> +    int rc;
> +    uint32_t rmid;
> +
> +    rc = xc_pqos_monitor_get_domain_rmid(ctx->xch, domid, &rmid);
> +    if (rc < 0)
> +        return 0;

don't you mean return rc here?

> +int libxl_pqos_monitor_get_cache_occupancy(libxl_ctx *ctx, uint32_t domid,
> +    uint32_t socketid, uint64_t *l3_cache_occupancy)
> +{
> +    GC_INIT(ctx);
> +
> +    unsigned int rmid;
> +    uint32_t upscaling_factor;
> +    uint64_t monitor_data;
> +    int cpu, rc;
> +    xc_pqos_monitor_type type;
> +
> +    rc = xc_pqos_monitor_get_domain_rmid(ctx->xch, domid, &rmid);
> +    if (rc < 0 || rmid == 0) {
> +        LOGE(ERROR, "fail to get the domain rmid, "
> +            "or domain is not attached with platform QoS monitoring service");
> +        return ERROR_FAIL;

All of these error paths fail to GC_FREE. You should follow the
    if (error thing) {
        rc = ERROR_FAIL;
        goto out;
    }

    ...

    rc = 0
out:
    GC_FREE;
    return 0
idiom.

> diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl
> index 1018142..2e7668b 100644
> --- a/tools/libxl/libxl_types.idl
> +++ b/tools/libxl/libxl_types.idl
> @@ -604,3 +604,7 @@ libxl_event = Struct("event",[
>                                   ])),
>             ("domain_create_console_available", None),
>             ]))])
> +
> +libxl_pqos_monitor_type = Enumeration("pqos_monitor_type", [
> +    (1, "CACHE_OCCUPANCY"),
> +    ])

This is used purely for the libxl_pqos_monitor_type_from_string helper
in xl and never in the libxl API, is that correct?

I'm not sure we want to expose this in the libxl api just for stat.
maybe just strcmp for now and if this list grows unwieldy we can
revisit.

> +    print_header = 1;
> +    for (domid = first_domain; domid < first_domain + nr_domains; domid++) {
> +        if (!libxl_pqos_monitor_domain_attached(ctx, domid)) {
> +            if (!ignore_domain_err)
> +                printf("No CQM info for Domain %d.\n", domid);

There might be an argument for printing a line anyway with "Unknown".

> +            continue;
> +        }
> +        if (print_header) {
> +            printf("Name                                        ID");

printf("%-40s %5s", "Name", "ID") would be easier to relate to the list
below.

> +            for (socketid = 0; socketid < nr_sockets; socketid++)
> +                printf("\tSocketID\tL3C_Usage");
> +            printf("\n");
> +            print_header = 0;
> +        }

So the output on a two socket system would be:
Name                                      ID	SocketID	L3CUsage	SocketID	L3CUsage
Blahblah                                   1           0            123KB              1              456KB

?

The socketid column seems weird to me. Why not:

Name                                      ID        Socket 0	Socket 1
Blahblah                                   1           123KB       456KB


Or

Name                                      ID        Socket	L3C Usage
Blahblah                                   1             0      456KB
                                                         1      123KB

...

"xl list" takes a bunch of flags to include extra info. Maybe you want
to think about listing this stuff there instead?

> +        domain_name = libxl_domid_to_name(ctx, domid);
> +        printf("%-40s %5d", domain_name, domid);
> +        free(domain_name);
> +        for (socketid = 0; socketid < nr_sockets; socketid++) {
> +            rc = libxl_pqos_monitor_get_cache_occupancy(ctx, domid, socketid,
> +                                                        &l3_cache_occupancy);
> +            printf("%10u %13lu KB     ", socketid, l3_cache_occupancy/1024);
> +        }
> +        printf("\n");
> +    }
> +
> +    return 0;
> +}
> +
> +
> +int main_pqos_monitor_show(int argc, char **argv)
> +{
> +    int opt, ret = 0;
> +    uint32_t first_domain, nr_domains;
> +    libxl_pqos_monitor_type type;
> +
> +    SWITCH_FOREACH_OPT(opt, "", NULL, "pqos-monitor-show", 1) {
> +        /* No options */
> +    }
> +
> +    libxl_pqos_monitor_type_from_string(argv[optind], &type);
> +
> +    if (optind + 1 >= argc) {
> +        first_domain = 0;
> +        nr_domains = 1024;


When an explicit domain is not given please use libxl_list_domain() and
print the status for all of them instead of hardcoding the first 1024
domains.


> +    } else if (optind + 1 == argc - 1){
> +        first_domain = find_domain(argv[optind + 1]);
> +        nr_domains = 1;

Ian.

Re: [PATCH v12 0/9] enable Cache QoS Monitoring (CQM) feature

[Jan Beulich]

>>> On 04.07.14 at 10:34, <dongxiao.xu@intel.com> wrote:
> Changes from v11:
>  - Turn off pqos and pqos_monitor in Xen command line by default.

You may have gone a little too far here - having sub-features enabled
by default if the main feature gets enabled on the command line would
see pretty reasonable to me. All I didn't like was the overall feature to
be enabled by default while all this still is very experimental.

Jan

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