All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v4 0/3] Expose PMU to the guests
@ 2021-10-11  9:00 Michal Orzel
  2021-10-11  9:00 ` [PATCH v4 1/3] xen+tools: Introduce XEN_SYSCTL_PHYSCAP_vpmu Michal Orzel
                   ` (2 more replies)
  0 siblings, 3 replies; 11+ messages in thread
From: Michal Orzel @ 2021-10-11  9:00 UTC (permalink / raw)
  To: xen-devel
  Cc: George Dunlap, Nick Rosbrook, Ian Jackson, Wei Liu,
	Andrew Cooper, Jan Beulich, Julien Grall, Stefano Stabellini,
	Anthony PERARD, Juergen Gross, Christian Lindig, David Scott,
	Volodymyr Babchuk, bertrand.marquis

This patch series is a rework of an already pushed patch
exposing PMU to the guests. Since the second version the vpmu
parameter is common and prework in the form of reporting
availability of vPMU on the hardware is added.

The third version of the patch series removes the redundant check
from x86 code and modifies the way to define the flags XEN_DOMCTL_CDF and
XEN_SYSCTL_PHYSCAP, meaning not to define bit position and mask separately.

In the fourth version, the additional check is added so that we fail
if vpmu is set in the config file but XEN_SYSCTL_PHYSCAP_vpmu is not available.

The current status is that the PMU registers are not virtualized
and the physical registers are directly accessible when "vpmu"
parameter is enabled in the guest config file. There is no interrupt
support and Xen will not save/restore the register values on context
switches. This is to be done in the future.

Michal Orzel (3):
  xen+tools: Introduce XEN_SYSCTL_PHYSCAP_vpmu
  xen/arm: Check for PMU platform support
  xen: Expose the PMU to the guests

 docs/man/xl.cfg.5.pod.in             | 17 ++++++++++
 tools/golang/xenlight/helpers.gen.go |  8 +++++
 tools/golang/xenlight/types.gen.go   |  2 ++
 tools/include/libxl.h                | 12 +++++++
 tools/libs/light/libxl.c             |  1 +
 tools/libs/light/libxl_create.c      | 10 ++++++
 tools/libs/light/libxl_types.idl     |  3 ++
 tools/ocaml/libs/xc/xenctrl.ml       |  2 ++
 tools/ocaml/libs/xc/xenctrl.mli      |  2 ++
 tools/xl/xl_info.c                   |  5 +--
 tools/xl/xl_parse.c                  |  2 ++
 xen/arch/arm/domain.c                | 12 +++++--
 xen/arch/arm/setup.c                 |  1 +
 xen/common/domain.c                  | 12 ++++++-
 xen/common/sysctl.c                  |  3 ++
 xen/include/asm-arm/cpufeature.h     | 49 ++++++++++++++++++++++++++--
 xen/include/asm-arm/domain.h         |  1 +
 xen/include/public/domctl.h          |  4 ++-
 xen/include/public/sysctl.h          |  6 ++--
 xen/include/xen/domain.h             |  2 ++
 20 files changed, 143 insertions(+), 11 deletions(-)

-- 
2.29.0



^ permalink raw reply	[flat|nested] 11+ messages in thread
* [PATCH v4 1/3] xen+tools: Introduce XEN_SYSCTL_PHYSCAP_vpmu
  2021-10-11  9:00 [PATCH v4 0/3] Expose PMU to the guests Michal Orzel
@ 2021-10-11  9:00 ` Michal Orzel
  2021-10-11 11:02   ` Ian Jackson
  2021-10-11  9:00 ` [PATCH v4 2/3] xen/arm: Check for PMU platform support Michal Orzel
  2021-10-11  9:00 ` [PATCH v4 3/3] xen: Expose the PMU to the guests Michal Orzel
  2 siblings, 1 reply; 11+ messages in thread
From: Michal Orzel @ 2021-10-11  9:00 UTC (permalink / raw)
  To: xen-devel
  Cc: George Dunlap, Nick Rosbrook, Ian Jackson, Wei Liu,
	Andrew Cooper, Jan Beulich, Julien Grall, Stefano Stabellini,
	Anthony PERARD, Juergen Gross, Christian Lindig, David Scott,
	bertrand.marquis

Introduce flag XEN_SYSCTL_PHYSCAP_vpmu which
indicates whether the platform supports vPMU
functionality. Modify Xen and tools accordingly.

Take the opportunity and fix XEN_SYSCTL_PHYSCAP_vmtrace
definition in sysctl.h which wrongly use (1 << 6)
instead of (1u << 6).

Signed-off-by: Michal Orzel <michal.orzel@arm.com>
Reviewed-by: Bertrand Marquis <bertrand.marquis@arm.com>
Acked-by: Nick Rosbrook <rosbrookn@ainfosec.com>
Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
---
Changes since v3:
-add spaces between brackets and keyword
Changes since v2:
-do not define bit position and mask separately
Changes since v1:
-new in v2
---
 tools/golang/xenlight/helpers.gen.go | 2 ++
 tools/golang/xenlight/types.gen.go   | 1 +
 tools/include/libxl.h                | 6 ++++++
 tools/libs/light/libxl.c             | 1 +
 tools/libs/light/libxl_types.idl     | 1 +
 tools/ocaml/libs/xc/xenctrl.ml       | 1 +
 tools/ocaml/libs/xc/xenctrl.mli      | 1 +
 tools/xl/xl_info.c                   | 5 +++--
 xen/common/domain.c                  | 2 ++
 xen/common/sysctl.c                  | 3 +++
 xen/include/public/sysctl.h          | 6 ++++--
 xen/include/xen/domain.h             | 2 ++
 12 files changed, 27 insertions(+), 4 deletions(-)

diff --git a/tools/golang/xenlight/helpers.gen.go b/tools/golang/xenlight/helpers.gen.go
index bfc1e7f312..c8669837d8 100644
--- a/tools/golang/xenlight/helpers.gen.go
+++ b/tools/golang/xenlight/helpers.gen.go
@@ -3360,6 +3360,7 @@ x.CapHap = bool(xc.cap_hap)
 x.CapShadow = bool(xc.cap_shadow)
 x.CapIommuHapPtShare = bool(xc.cap_iommu_hap_pt_share)
 x.CapVmtrace = bool(xc.cap_vmtrace)
+x.CapVpmu = bool(xc.cap_vpmu)
 
  return nil}
 
@@ -3391,6 +3392,7 @@ xc.cap_hap = C.bool(x.CapHap)
 xc.cap_shadow = C.bool(x.CapShadow)
 xc.cap_iommu_hap_pt_share = C.bool(x.CapIommuHapPtShare)
 xc.cap_vmtrace = C.bool(x.CapVmtrace)
+xc.cap_vpmu = C.bool(x.CapVpmu)
 
  return nil
  }
diff --git a/tools/golang/xenlight/types.gen.go b/tools/golang/xenlight/types.gen.go
index 09a3bb67e2..45f2cba3d2 100644
--- a/tools/golang/xenlight/types.gen.go
+++ b/tools/golang/xenlight/types.gen.go
@@ -1008,6 +1008,7 @@ CapHap bool
 CapShadow bool
 CapIommuHapPtShare bool
 CapVmtrace bool
+CapVpmu bool
 }
 
 type Connectorinfo struct {
diff --git a/tools/include/libxl.h b/tools/include/libxl.h
index b9ba16d698..ec5e3badae 100644
--- a/tools/include/libxl.h
+++ b/tools/include/libxl.h
@@ -502,6 +502,12 @@
  */
 #define LIBXL_HAVE_X86_MSR_RELAXED 1
 
+/*
+ * LIBXL_HAVE_PHYSINFO_CAP_VPMU indicates that libxl_physinfo has a cap_vpmu
+ * field, which indicates the availability of vPMU functionality.
+ */
+#define LIBXL_HAVE_PHYSINFO_CAP_VPMU 1
+
 /*
  * libxl ABI compatibility
  *
diff --git a/tools/libs/light/libxl.c b/tools/libs/light/libxl.c
index 204eb0be2d..a032723fde 100644
--- a/tools/libs/light/libxl.c
+++ b/tools/libs/light/libxl.c
@@ -404,6 +404,7 @@ int libxl_get_physinfo(libxl_ctx *ctx, libxl_physinfo *physinfo)
         !!(xcphysinfo.capabilities & XEN_SYSCTL_PHYSCAP_iommu_hap_pt_share);
     physinfo->cap_vmtrace =
         !!(xcphysinfo.capabilities & XEN_SYSCTL_PHYSCAP_vmtrace);
+    physinfo->cap_vpmu = !!(xcphysinfo.capabilities & XEN_SYSCTL_PHYSCAP_vpmu);
 
     GC_FREE;
     return 0;
diff --git a/tools/libs/light/libxl_types.idl b/tools/libs/light/libxl_types.idl
index 3f9fff653a..993e83acca 100644
--- a/tools/libs/light/libxl_types.idl
+++ b/tools/libs/light/libxl_types.idl
@@ -1061,6 +1061,7 @@ libxl_physinfo = Struct("physinfo", [
     ("cap_shadow", bool),
     ("cap_iommu_hap_pt_share", bool),
     ("cap_vmtrace", bool),
+    ("cap_vpmu", bool),
     ], dir=DIR_OUT)
 
 libxl_connectorinfo = Struct("connectorinfo", [
diff --git a/tools/ocaml/libs/xc/xenctrl.ml b/tools/ocaml/libs/xc/xenctrl.ml
index 7ed1c00e47..7a4030a192 100644
--- a/tools/ocaml/libs/xc/xenctrl.ml
+++ b/tools/ocaml/libs/xc/xenctrl.ml
@@ -122,6 +122,7 @@ type physinfo_cap_flag =
 	| CAP_Shadow
 	| CAP_IOMMU_HAP_PT_SHARE
 	| CAP_Vmtrace
+	| CAP_Vpmu
 
 type physinfo =
 {
diff --git a/tools/ocaml/libs/xc/xenctrl.mli b/tools/ocaml/libs/xc/xenctrl.mli
index 391d4abdf8..6900513e7f 100644
--- a/tools/ocaml/libs/xc/xenctrl.mli
+++ b/tools/ocaml/libs/xc/xenctrl.mli
@@ -107,6 +107,7 @@ type physinfo_cap_flag =
   | CAP_Shadow
   | CAP_IOMMU_HAP_PT_SHARE
   | CAP_Vmtrace
+  | CAP_Vpmu
 
 type physinfo = {
   threads_per_core : int;
diff --git a/tools/xl/xl_info.c b/tools/xl/xl_info.c
index 8383e4a6df..2c86b317b7 100644
--- a/tools/xl/xl_info.c
+++ b/tools/xl/xl_info.c
@@ -210,7 +210,7 @@ static void output_physinfo(void)
          info.hw_cap[4], info.hw_cap[5], info.hw_cap[6], info.hw_cap[7]
         );
 
-    maybe_printf("virt_caps              :%s%s%s%s%s%s%s%s\n",
+    maybe_printf("virt_caps              :%s%s%s%s%s%s%s%s%s\n",
          info.cap_pv ? " pv" : "",
          info.cap_hvm ? " hvm" : "",
          info.cap_hvm && info.cap_hvm_directio ? " hvm_directio" : "",
@@ -218,7 +218,8 @@ static void output_physinfo(void)
          info.cap_hap ? " hap" : "",
          info.cap_shadow ? " shadow" : "",
          info.cap_iommu_hap_pt_share ? " iommu_hap_pt_share" : "",
-         info.cap_vmtrace ? " vmtrace" : ""
+         info.cap_vmtrace ? " vmtrace" : "",
+         info.cap_vpmu ? " vpmu" : ""
         );
 
     vinfo = libxl_get_version_info(ctx);
diff --git a/xen/common/domain.c b/xen/common/domain.c
index 40d67ec342..262b6c0c3c 100644
--- a/xen/common/domain.c
+++ b/xen/common/domain.c
@@ -84,6 +84,8 @@ vcpu_info_t dummy_vcpu_info;
 
 bool __read_mostly vmtrace_available;
 
+bool __read_mostly vpmu_is_available;
+
 static void __domain_finalise_shutdown(struct domain *d)
 {
     struct vcpu *v;
diff --git a/xen/common/sysctl.c b/xen/common/sysctl.c
index 3558641cd9..6e7189bb3c 100644
--- a/xen/common/sysctl.c
+++ b/xen/common/sysctl.c
@@ -280,6 +280,9 @@ long do_sysctl(XEN_GUEST_HANDLE_PARAM(xen_sysctl_t) u_sysctl)
         if ( vmtrace_available )
             pi->capabilities |= XEN_SYSCTL_PHYSCAP_vmtrace;
 
+        if ( vpmu_is_available )
+            pi->capabilities |= XEN_SYSCTL_PHYSCAP_vpmu;
+
         if ( copy_to_guest(u_sysctl, op, 1) )
             ret = -EFAULT;
     }
diff --git a/xen/include/public/sysctl.h b/xen/include/public/sysctl.h
index 039ccf885c..fead0e5b53 100644
--- a/xen/include/public/sysctl.h
+++ b/xen/include/public/sysctl.h
@@ -100,10 +100,12 @@ struct xen_sysctl_tbuf_op {
 #define _XEN_SYSCTL_PHYSCAP_iommu_hap_pt_share 5
 #define XEN_SYSCTL_PHYSCAP_iommu_hap_pt_share  \
     (1u << _XEN_SYSCTL_PHYSCAP_iommu_hap_pt_share)
-#define XEN_SYSCTL_PHYSCAP_vmtrace       (1 << 6)
+#define XEN_SYSCTL_PHYSCAP_vmtrace       (1u << 6)
+/* The platform supports vPMU. */
+#define XEN_SYSCTL_PHYSCAP_vpmu          (1u << 7)
 
 /* Max XEN_SYSCTL_PHYSCAP_* constant.  Used for ABI checking. */
-#define XEN_SYSCTL_PHYSCAP_MAX XEN_SYSCTL_PHYSCAP_vmtrace
+#define XEN_SYSCTL_PHYSCAP_MAX XEN_SYSCTL_PHYSCAP_vpmu
 
 struct xen_sysctl_physinfo {
     uint32_t threads_per_core;
diff --git a/xen/include/xen/domain.h b/xen/include/xen/domain.h
index 1708c36964..160c8dbdab 100644
--- a/xen/include/xen/domain.h
+++ b/xen/include/xen/domain.h
@@ -133,4 +133,6 @@ static inline void vnuma_destroy(struct vnuma_info *vnuma) { ASSERT(!vnuma); }
 
 extern bool vmtrace_available;
 
+extern bool vpmu_is_available;
+
 #endif /* __XEN_DOMAIN_H__ */
-- 
2.29.0



^ permalink raw reply related	[flat|nested] 11+ messages in thread
* [PATCH v4 2/3] xen/arm: Check for PMU platform support
  2021-10-11  9:00 [PATCH v4 0/3] Expose PMU to the guests Michal Orzel
  2021-10-11  9:00 ` [PATCH v4 1/3] xen+tools: Introduce XEN_SYSCTL_PHYSCAP_vpmu Michal Orzel
@ 2021-10-11  9:00 ` Michal Orzel
  2021-10-11 10:17   ` Julien Grall
  2021-10-11  9:00 ` [PATCH v4 3/3] xen: Expose the PMU to the guests Michal Orzel
  2 siblings, 1 reply; 11+ messages in thread
From: Michal Orzel @ 2021-10-11  9:00 UTC (permalink / raw)
  To: xen-devel
  Cc: Stefano Stabellini, Julien Grall, Volodymyr Babchuk, bertrand.marquis

ID_AA64DFR0_EL1/ID_DFR0_EL1 registers provide
information about PMU support. Replace structure
dbg64/dbg32 with a union and fill in all the
register fields according to document:
ARM Architecture Registers(DDI 0595, 2021-06).

Add macros boot_dbg_feature64/boot_dbg_feature32
to check for a debug feature. Add macro
cpu_has_pmu to check for PMU support.

Signed-off-by: Michal Orzel <michal.orzel@arm.com>
Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
Reviewed-by: Bertrand Marquis <bertrand.marquis@arm.com>
---
Changes since v3:
-none
Changes since v2:
-none
Changes since v1:
-new in v2
---
 xen/include/asm-arm/cpufeature.h | 49 ++++++++++++++++++++++++++++++--
 1 file changed, 47 insertions(+), 2 deletions(-)

diff --git a/xen/include/asm-arm/cpufeature.h b/xen/include/asm-arm/cpufeature.h
index 5ca09b0bff..4fce23844d 100644
--- a/xen/include/asm-arm/cpufeature.h
+++ b/xen/include/asm-arm/cpufeature.h
@@ -4,6 +4,7 @@
 #ifdef CONFIG_ARM_64
 #define cpu_feature64(c, feat)         ((c)->pfr64.feat)
 #define boot_cpu_feature64(feat)       (system_cpuinfo.pfr64.feat)
+#define boot_dbg_feature64(feat)       (system_cpuinfo.dbg64.feat)
 
 #define cpu_feature64_has_el0_32(c)    (cpu_feature64(c, el0) == 2)
 
@@ -22,6 +23,7 @@
 
 #define cpu_feature32(c, feat)         ((c)->pfr32.feat)
 #define boot_cpu_feature32(feat)       (system_cpuinfo.pfr32.feat)
+#define boot_dbg_feature32(feat)       (system_cpuinfo.dbg32.feat)
 
 #define cpu_has_arm       (boot_cpu_feature32(arm) == 1)
 #define cpu_has_thumb     (boot_cpu_feature32(thumb) >= 1)
@@ -32,8 +34,10 @@
 
 #ifdef CONFIG_ARM_32
 #define cpu_has_gentimer  (boot_cpu_feature32(gentimer) == 1)
+#define cpu_has_pmu       (boot_dbg_feature32(perfmon) >= 1)
 #else
 #define cpu_has_gentimer  (1)
+#define cpu_has_pmu       (boot_dbg_feature64(pmu_ver) >= 1)
 #endif
 #define cpu_has_security  (boot_cpu_feature32(security) > 0)
 
@@ -181,8 +185,28 @@ struct cpuinfo_arm {
         };
     } pfr64;
 
-    struct {
+    union {
         register_t bits[2];
+        struct {
+            /* DFR0 */
+            unsigned long debug_ver:4;
+            unsigned long trace_ver:4;
+            unsigned long pmu_ver:4;
+            unsigned long brps:4;
+            unsigned long __res0:4;
+            unsigned long wrps:4;
+            unsigned long __res1:4;
+            unsigned long ctx_cmps:4;
+            unsigned long pms_ver:4;
+            unsigned long double_lock:4;
+            unsigned long trace_filt:4;
+            unsigned long __res2:4;
+            unsigned long mtpmu:4;
+            unsigned long __res3:12;
+
+            /* DFR1 */
+            unsigned long __res4:64;
+        };
     } dbg64;
 
     struct {
@@ -321,8 +345,29 @@ struct cpuinfo_arm {
         };
     } pfr32;
 
-    struct {
+    union {
         register_t bits[2];
+        struct {
+            /* DFR0 */
+            unsigned long copdbg:4;
+            unsigned long copsdbg:4;
+            unsigned long mmapdbg:4;
+            unsigned long coptrc:4;
+            unsigned long mmaptrc:4;
+            unsigned long mprofdbg:4;
+            unsigned long perfmon:4;
+            unsigned long tracefilt:4;
+#ifdef CONFIG_ARM_64
+            unsigned long __res0:32;
+#endif
+
+            /* DFR1 */
+            unsigned long mtpmu:4;
+            unsigned long __res1:28;
+#ifdef CONFIG_ARM_64
+            unsigned long __res2:32;
+#endif
+        };
     } dbg32;
 
     struct {
-- 
2.29.0



^ permalink raw reply related	[flat|nested] 11+ messages in thread
* [PATCH v4 3/3] xen: Expose the PMU to the guests
  2021-10-11  9:00 [PATCH v4 0/3] Expose PMU to the guests Michal Orzel
  2021-10-11  9:00 ` [PATCH v4 1/3] xen+tools: Introduce XEN_SYSCTL_PHYSCAP_vpmu Michal Orzel
  2021-10-11  9:00 ` [PATCH v4 2/3] xen/arm: Check for PMU platform support Michal Orzel
@ 2021-10-11  9:00 ` Michal Orzel
  2021-10-11 10:21   ` Julien Grall
  2021-10-11 20:09   ` Stefano Stabellini
  2 siblings, 2 replies; 11+ messages in thread
From: Michal Orzel @ 2021-10-11  9:00 UTC (permalink / raw)
  To: xen-devel
  Cc: Ian Jackson, Wei Liu, Andrew Cooper, George Dunlap, Jan Beulich,
	Julien Grall, Stefano Stabellini, Nick Rosbrook, Anthony PERARD,
	Juergen Gross, Christian Lindig, David Scott, Volodymyr Babchuk,
	bertrand.marquis

Add parameter vpmu to xl domain configuration syntax
to enable the access to PMU registers by disabling
the PMU traps(currently only for ARM).

The current status is that the PMU registers are not
virtualized and the physical registers are directly
accessible when this parameter is enabled. There is no
interrupt support and Xen will not save/restore the
register values on context switches.

Please note that this feature is experimental.

Signed-off-by: Michal Orzel <michal.orzel@arm.com>
Signed-off-by: Julien Grall <julien@xen.org>
Reviewed-by: Bertrand Marquis <bertrand.marquis@arm.com>
---
Changes since v3:
-fail if vpmu is set but not supported
-rebase on top of latest staging
Changes since v2:
-remove redundant check from x86 code
-do not define bit position and mask separately
Changes since v1:
-modify vpmu parameter to be common rather than arch specific
---
 docs/man/xl.cfg.5.pod.in             | 17 +++++++++++++++++
 tools/golang/xenlight/helpers.gen.go |  6 ++++++
 tools/golang/xenlight/types.gen.go   |  1 +
 tools/include/libxl.h                |  6 ++++++
 tools/libs/light/libxl_create.c      | 10 ++++++++++
 tools/libs/light/libxl_types.idl     |  2 ++
 tools/ocaml/libs/xc/xenctrl.ml       |  1 +
 tools/ocaml/libs/xc/xenctrl.mli      |  1 +
 tools/xl/xl_parse.c                  |  2 ++
 xen/arch/arm/domain.c                | 12 +++++++++---
 xen/arch/arm/setup.c                 |  1 +
 xen/common/domain.c                  | 10 +++++++++-
 xen/include/asm-arm/domain.h         |  1 +
 xen/include/public/domctl.h          |  4 +++-
 14 files changed, 69 insertions(+), 5 deletions(-)

diff --git a/docs/man/xl.cfg.5.pod.in b/docs/man/xl.cfg.5.pod.in
index 4b1e3028d2..55c4881205 100644
--- a/docs/man/xl.cfg.5.pod.in
+++ b/docs/man/xl.cfg.5.pod.in
@@ -690,6 +690,23 @@ default.
 B<NOTE>: Acceptable values are platform specific.  For Intel Processor
 Trace, this value must be a power of 2 between 4k and 16M.
 
+=item B<vpmu=BOOLEAN>
+
+Currently ARM only.
+
+Specifies whether to enable the access to PMU registers by disabling
+the PMU traps.
+
+The PMU registers are not virtualized and the physical registers are directly
+accessible when this parameter is enabled. There is no interrupt support and
+Xen will not save/restore the register values on context switches.
+
+vPMU, by design and purpose, exposes system level performance
+information to the guest. Only to be used by sufficiently privileged
+domains. This feature is currently in experimental state.
+
+If this option is not specified then it will default to B<false>.
+
 =back
 
 =head2 Devices
diff --git a/tools/golang/xenlight/helpers.gen.go b/tools/golang/xenlight/helpers.gen.go
index c8669837d8..2449580bad 100644
--- a/tools/golang/xenlight/helpers.gen.go
+++ b/tools/golang/xenlight/helpers.gen.go
@@ -1119,6 +1119,9 @@ return fmt.Errorf("converting field ArchX86.MsrRelaxed: %v", err)
 }
 x.Altp2M = Altp2MMode(xc.altp2m)
 x.VmtraceBufKb = int(xc.vmtrace_buf_kb)
+if err := x.Vpmu.fromC(&xc.vpmu);err != nil {
+return fmt.Errorf("converting field Vpmu: %v", err)
+}
 
  return nil}
 
@@ -1600,6 +1603,9 @@ return fmt.Errorf("converting field ArchX86.MsrRelaxed: %v", err)
 }
 xc.altp2m = C.libxl_altp2m_mode(x.Altp2M)
 xc.vmtrace_buf_kb = C.int(x.VmtraceBufKb)
+if err := x.Vpmu.toC(&xc.vpmu); err != nil {
+return fmt.Errorf("converting field Vpmu: %v", err)
+}
 
  return nil
  }
diff --git a/tools/golang/xenlight/types.gen.go b/tools/golang/xenlight/types.gen.go
index 45f2cba3d2..b2e8bd1a85 100644
--- a/tools/golang/xenlight/types.gen.go
+++ b/tools/golang/xenlight/types.gen.go
@@ -521,6 +521,7 @@ MsrRelaxed Defbool
 }
 Altp2M Altp2MMode
 VmtraceBufKb int
+Vpmu Defbool
 }
 
 type DomainBuildInfoTypeUnion interface {
diff --git a/tools/include/libxl.h b/tools/include/libxl.h
index ec5e3badae..ee73eb06f1 100644
--- a/tools/include/libxl.h
+++ b/tools/include/libxl.h
@@ -508,6 +508,12 @@
  */
 #define LIBXL_HAVE_PHYSINFO_CAP_VPMU 1
 
+/*
+ * LIBXL_HAVE_VPMU indicates that libxl_domain_build_info has a vpmu parameter,
+ * which allows to enable the access to PMU registers.
+ */
+#define LIBXL_HAVE_VPMU 1
+
 /*
  * libxl ABI compatibility
  *
diff --git a/tools/libs/light/libxl_create.c b/tools/libs/light/libxl_create.c
index e356b2106d..2a0234ec16 100644
--- a/tools/libs/light/libxl_create.c
+++ b/tools/libs/light/libxl_create.c
@@ -91,6 +91,7 @@ int libxl__domain_build_info_setdefault(libxl__gc *gc,
     }
 
     libxl_defbool_setdefault(&b_info->device_model_stubdomain, false);
+    libxl_defbool_setdefault(&b_info->vpmu, false);
 
     if (libxl_defbool_val(b_info->device_model_stubdomain) &&
         !b_info->device_model_ssidref)
@@ -622,6 +623,9 @@ int libxl__domain_make(libxl__gc *gc, libxl_domain_config *d_config,
                 create.flags |= XEN_DOMCTL_CDF_nested_virt;
         }
 
+        if ( libxl_defbool_val(b_info->vpmu) )
+            create.flags |= XEN_DOMCTL_CDF_vpmu;
+
         assert(info->passthrough != LIBXL_PASSTHROUGH_DEFAULT);
         LOG(DETAIL, "passthrough: %s",
             libxl_passthrough_to_string(info->passthrough));
@@ -1199,6 +1203,12 @@ int libxl__domain_config_setdefault(libxl__gc *gc,
         goto error_out;
     }
 
+    if (libxl_defbool_val(d_config->b_info.vpmu) && !physinfo.cap_vpmu) {
+        ret = ERROR_INVAL;
+        LOGD(ERROR, domid, "vpmu not supported on this platform\n");
+        goto error_out;
+    }
+
     ret = 0;
  error_out:
     return ret;
diff --git a/tools/libs/light/libxl_types.idl b/tools/libs/light/libxl_types.idl
index 993e83acca..b96fb5c47e 100644
--- a/tools/libs/light/libxl_types.idl
+++ b/tools/libs/light/libxl_types.idl
@@ -655,6 +655,8 @@ libxl_domain_build_info = Struct("domain_build_info",[
     # Use zero value to disable this feature.
     ("vmtrace_buf_kb", integer),
 
+    ("vpmu", libxl_defbool),
+
     ], dir=DIR_IN,
        copy_deprecated_fn="libxl__domain_build_info_copy_deprecated",
 )
diff --git a/tools/ocaml/libs/xc/xenctrl.ml b/tools/ocaml/libs/xc/xenctrl.ml
index 7a4030a192..86758babb3 100644
--- a/tools/ocaml/libs/xc/xenctrl.ml
+++ b/tools/ocaml/libs/xc/xenctrl.ml
@@ -70,6 +70,7 @@ type domain_create_flag =
 	| CDF_IOMMU
 	| CDF_NESTED_VIRT
 	| CDF_VPCI
+	| CDF_VPMU
 
 type domain_create_iommu_opts =
 	| IOMMU_NO_SHAREPT
diff --git a/tools/ocaml/libs/xc/xenctrl.mli b/tools/ocaml/libs/xc/xenctrl.mli
index 6900513e7f..0fdb0cc169 100644
--- a/tools/ocaml/libs/xc/xenctrl.mli
+++ b/tools/ocaml/libs/xc/xenctrl.mli
@@ -63,6 +63,7 @@ type domain_create_flag =
   | CDF_IOMMU
   | CDF_NESTED_VIRT
   | CDF_VPCI
+  | CDF_VPMU
 
 type domain_create_iommu_opts =
   | IOMMU_NO_SHAREPT
diff --git a/tools/xl/xl_parse.c b/tools/xl/xl_parse.c
index 17dddb4cd5..c503b9be00 100644
--- a/tools/xl/xl_parse.c
+++ b/tools/xl/xl_parse.c
@@ -2750,6 +2750,8 @@ skip_usbdev:
                     "If it fixes an issue you are having please report to "
                     "xen-devel@lists.xenproject.org.\n");
 
+    xlu_cfg_get_defbool(config, "vpmu", &b_info->vpmu, 0);
+
     xlu_cfg_destroy(config);
 }
 
diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
index aae4472479..2f988c790e 100644
--- a/xen/arch/arm/domain.c
+++ b/xen/arch/arm/domain.c
@@ -276,6 +276,8 @@ static void ctxt_switch_to(struct vcpu *n)
      * timer. The interrupt needs to be injected into the guest. */
     WRITE_SYSREG(n->arch.cntkctl, CNTKCTL_EL1);
     virt_timer_restore(n);
+
+    WRITE_SYSREG(n->arch.mdcr_el2, MDCR_EL2);
 }
 
 /* Update per-VCPU guest runstate shared memory area (if registered). */
@@ -586,6 +588,10 @@ int arch_vcpu_create(struct vcpu *v)
 
     v->arch.hcr_el2 = get_default_hcr_flags();
 
+    v->arch.mdcr_el2 = HDCR_TDRA | HDCR_TDOSA | HDCR_TDA;
+    if ( !(v->domain->options & XEN_DOMCTL_CDF_vpmu) )
+        v->arch.mdcr_el2 |= HDCR_TPM | HDCR_TPMCR;
+
     if ( (rc = vcpu_vgic_init(v)) != 0 )
         goto fail;
 
@@ -622,9 +628,9 @@ int arch_sanitise_domain_config(struct xen_domctl_createdomain *config)
 {
     unsigned int max_vcpus;
 
-    /* HVM and HAP must be set. IOMMU and VPCI may or may not be */
-    if ( (config->flags & ~XEN_DOMCTL_CDF_iommu & ~XEN_DOMCTL_CDF_vpci) !=
-         (XEN_DOMCTL_CDF_hvm | XEN_DOMCTL_CDF_hap) )
+    /* HVM and HAP must be set. IOMMU, VPCI and VPMU may or may not be */
+    if ( (config->flags & ~(XEN_DOMCTL_CDF_iommu | XEN_DOMCTL_CDF_vpci |
+          XEN_DOMCTL_CDF_vpmu)) != (XEN_DOMCTL_CDF_hvm | XEN_DOMCTL_CDF_hap) )
     {
         dprintk(XENLOG_INFO, "Unsupported configuration %#x\n",
                 config->flags);
diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
index 49dc90d198..85386a765a 100644
--- a/xen/arch/arm/setup.c
+++ b/xen/arch/arm/setup.c
@@ -108,6 +108,7 @@ static void __init processor_id(void)
 
     identify_cpu(c);
     current_cpu_data = *c;
+    vpmu_is_available = cpu_has_pmu;
 
     if ( c->midr.implementer < ARRAY_SIZE(processor_implementers) &&
          processor_implementers[c->midr.implementer] )
diff --git a/xen/common/domain.c b/xen/common/domain.c
index 262b6c0c3c..8543fb54fd 100644
--- a/xen/common/domain.c
+++ b/xen/common/domain.c
@@ -480,12 +480,14 @@ static int sanitise_domain_config(struct xen_domctl_createdomain *config)
     bool hvm = config->flags & XEN_DOMCTL_CDF_hvm;
     bool hap = config->flags & XEN_DOMCTL_CDF_hap;
     bool iommu = config->flags & XEN_DOMCTL_CDF_iommu;
+    bool vpmu = config->flags & XEN_DOMCTL_CDF_vpmu;
 
     if ( config->flags &
          ~(XEN_DOMCTL_CDF_hvm | XEN_DOMCTL_CDF_hap |
            XEN_DOMCTL_CDF_s3_integrity | XEN_DOMCTL_CDF_oos_off |
            XEN_DOMCTL_CDF_xs_domain | XEN_DOMCTL_CDF_iommu |
-           XEN_DOMCTL_CDF_nested_virt | XEN_DOMCTL_CDF_vpci) )
+           XEN_DOMCTL_CDF_nested_virt | XEN_DOMCTL_CDF_vpci |
+           XEN_DOMCTL_CDF_vpmu) )
     {
         dprintk(XENLOG_INFO, "Unknown CDF flags %#x\n", config->flags);
         return -EINVAL;
@@ -534,6 +536,12 @@ static int sanitise_domain_config(struct xen_domctl_createdomain *config)
         return -EINVAL;
     }
 
+    if ( vpmu && !vpmu_is_available )
+    {
+        dprintk(XENLOG_INFO, "vpmu requested but cannot be enabled this way\n");
+        return -EINVAL;
+    }
+
     return arch_sanitise_domain_config(config);
 }
 
diff --git a/xen/include/asm-arm/domain.h b/xen/include/asm-arm/domain.h
index c9277b5c6d..14e575288f 100644
--- a/xen/include/asm-arm/domain.h
+++ b/xen/include/asm-arm/domain.h
@@ -166,6 +166,7 @@ struct arch_vcpu
 
     /* HYP configuration */
     register_t hcr_el2;
+    register_t mdcr_el2;
 
     uint32_t teecr, teehbr; /* ThumbEE, 32-bit guests only */
 #ifdef CONFIG_ARM_32
diff --git a/xen/include/public/domctl.h b/xen/include/public/domctl.h
index 4cb3f662c2..a53cbd16f4 100644
--- a/xen/include/public/domctl.h
+++ b/xen/include/public/domctl.h
@@ -71,9 +71,11 @@ struct xen_domctl_createdomain {
 #define _XEN_DOMCTL_CDF_nested_virt   6
 #define XEN_DOMCTL_CDF_nested_virt    (1U << _XEN_DOMCTL_CDF_nested_virt)
 #define XEN_DOMCTL_CDF_vpci           (1U << 7)
+/* Should we expose the vPMU to the guest? */
+#define XEN_DOMCTL_CDF_vpmu           (1U << 8)
 
 /* Max XEN_DOMCTL_CDF_* constant.  Used for ABI checking. */
-#define XEN_DOMCTL_CDF_MAX XEN_DOMCTL_CDF_vpci
+#define XEN_DOMCTL_CDF_MAX XEN_DOMCTL_CDF_vpmu
 
     uint32_t flags;
 
-- 
2.29.0



^ permalink raw reply related	[flat|nested] 11+ messages in thread
* Re: [PATCH v4 2/3] xen/arm: Check for PMU platform support
  2021-10-11  9:00 ` [PATCH v4 2/3] xen/arm: Check for PMU platform support Michal Orzel
@ 2021-10-11 10:17   ` Julien Grall
  2021-10-11 12:06     ` Michal Orzel
  0 siblings, 1 reply; 11+ messages in thread
From: Julien Grall @ 2021-10-11 10:17 UTC (permalink / raw)
  To: Michal Orzel, xen-devel
  Cc: Stefano Stabellini, Volodymyr Babchuk, bertrand.marquis

Hi Michal,

On 11/10/2021 10:00, Michal Orzel wrote:
> ID_AA64DFR0_EL1/ID_DFR0_EL1 registers provide
> information about PMU support. Replace structure
> dbg64/dbg32 with a union and fill in all the
> register fields according to document:
> ARM Architecture Registers(DDI 0595, 2021-06).
> 
> Add macros boot_dbg_feature64/boot_dbg_feature32
> to check for a debug feature. Add macro
> cpu_has_pmu to check for PMU support.
> 
> Signed-off-by: Michal Orzel <michal.orzel@arm.com>
> Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
> Reviewed-by: Bertrand Marquis <bertrand.marquis@arm.com>
> ---
> Changes since v3:
> -none
> Changes since v2:
> -none
> Changes since v1:
> -new in v2
> ---
>   xen/include/asm-arm/cpufeature.h | 49 ++++++++++++++++++++++++++++++--
>   1 file changed, 47 insertions(+), 2 deletions(-)
> 
> diff --git a/xen/include/asm-arm/cpufeature.h b/xen/include/asm-arm/cpufeature.h
> index 5ca09b0bff..4fce23844d 100644
> --- a/xen/include/asm-arm/cpufeature.h
> +++ b/xen/include/asm-arm/cpufeature.h
> @@ -4,6 +4,7 @@
>   #ifdef CONFIG_ARM_64
>   #define cpu_feature64(c, feat)         ((c)->pfr64.feat)
>   #define boot_cpu_feature64(feat)       (system_cpuinfo.pfr64.feat)
> +#define boot_dbg_feature64(feat)       (system_cpuinfo.dbg64.feat)
>   
>   #define cpu_feature64_has_el0_32(c)    (cpu_feature64(c, el0) == 2)
>   
> @@ -22,6 +23,7 @@
>   
>   #define cpu_feature32(c, feat)         ((c)->pfr32.feat)
>   #define boot_cpu_feature32(feat)       (system_cpuinfo.pfr32.feat)
> +#define boot_dbg_feature32(feat)       (system_cpuinfo.dbg32.feat)
>   
>   #define cpu_has_arm       (boot_cpu_feature32(arm) == 1)
>   #define cpu_has_thumb     (boot_cpu_feature32(thumb) >= 1)
> @@ -32,8 +34,10 @@
>   
>   #ifdef CONFIG_ARM_32
>   #define cpu_has_gentimer  (boot_cpu_feature32(gentimer) == 1)
> +#define cpu_has_pmu       (boot_dbg_feature32(perfmon) >= 1)

 From my understanding, on Armv7, perfmon == 0 only means PMUv2 is not 
present. IOW, it doesn't say whether PMUv1 is supported.

I think it is OK to treat as the PMU is not supported (ARMv8 treat it 
like that too), but I would like a comment in the code so it is clear 
this is a deliberate choice.

>   #else
>   #define cpu_has_gentimer  (1)
> +#define cpu_has_pmu       (boot_dbg_feature64(pmu_ver) >= 1)
>   #endif
>   #define cpu_has_security  (boot_cpu_feature32(security) > 0)
>   
> @@ -181,8 +185,28 @@ struct cpuinfo_arm {
>           };
>       } pfr64;
>   
> -    struct {
> +    union {
>           register_t bits[2];
> +        struct {
> +            /* DFR0 */
> +            unsigned long debug_ver:4;
> +            unsigned long trace_ver:4;
> +            unsigned long pmu_ver:4;
> +            unsigned long brps:4;
> +            unsigned long __res0:4;
> +            unsigned long wrps:4;
> +            unsigned long __res1:4;
> +            unsigned long ctx_cmps:4;
> +            unsigned long pms_ver:4;
> +            unsigned long double_lock:4;
> +            unsigned long trace_filt:4;
> +            unsigned long __res2:4;
> +            unsigned long mtpmu:4;
> +            unsigned long __res3:12;
> +
> +            /* DFR1 */
> +            unsigned long __res4:64;
> +        };
>       } dbg64;
>   
>       struct {
> @@ -321,8 +345,29 @@ struct cpuinfo_arm {
>           };
>       } pfr32;
>   
> -    struct {
> +    union {
>           register_t bits[2];
> +        struct {
> +            /* DFR0 */
> +            unsigned long copdbg:4;
> +            unsigned long copsdbg:4;
> +            unsigned long mmapdbg:4;
> +            unsigned long coptrc:4;
> +            unsigned long mmaptrc:4;
> +            unsigned long mprofdbg:4;
> +            unsigned long perfmon:4;
> +            unsigned long tracefilt:4;
> +#ifdef CONFIG_ARM_64
> +            unsigned long __res0:32;
> +#endif
> +
> +            /* DFR1 */
> +            unsigned long mtpmu:4;
> +            unsigned long __res1:28;
> +#ifdef CONFIG_ARM_64
> +            unsigned long __res2:32;
> +#endif
> +        };
>       } dbg32;
>   
>       struct {
> 

Cheers,

-- 
Julien Grall


^ permalink raw reply	[flat|nested] 11+ messages in thread
* Re: [PATCH v4 3/3] xen: Expose the PMU to the guests
  2021-10-11  9:00 ` [PATCH v4 3/3] xen: Expose the PMU to the guests Michal Orzel
@ 2021-10-11 10:21   ` Julien Grall
  2021-10-11 14:11     ` Bertrand Marquis
  2021-10-11 20:09   ` Stefano Stabellini
  1 sibling, 1 reply; 11+ messages in thread
From: Julien Grall @ 2021-10-11 10:21 UTC (permalink / raw)
  To: Michal Orzel, xen-devel
  Cc: Ian Jackson, Wei Liu, Andrew Cooper, George Dunlap, Jan Beulich,
	Stefano Stabellini, Nick Rosbrook, Anthony PERARD, Juergen Gross,
	Christian Lindig, David Scott, Volodymyr Babchuk,
	bertrand.marquis

Hi Michal,

On 11/10/2021 10:00, Michal Orzel wrote:
> Add parameter vpmu to xl domain configuration syntax
> to enable the access to PMU registers by disabling
> the PMU traps(currently only for ARM).
> 
> The current status is that the PMU registers are not
> virtualized and the physical registers are directly
> accessible when this parameter is enabled. There is no
> interrupt support and Xen will not save/restore the
> register values on context switches.
> 
> Please note that this feature is experimental.
> 
> Signed-off-by: Michal Orzel <michal.orzel@arm.com>
> Signed-off-by: Julien Grall <julien@xen.org>
> Reviewed-by: Bertrand Marquis <bertrand.marquis@arm.com>
> ---
> Changes since v3:
> -fail if vpmu is set but not supported
> -rebase on top of latest staging
> Changes since v2:
> -remove redundant check from x86 code
> -do not define bit position and mask separately
> Changes since v1:
> -modify vpmu parameter to be common rather than arch specific
> ---
>   docs/man/xl.cfg.5.pod.in             | 17 +++++++++++++++++
>   tools/golang/xenlight/helpers.gen.go |  6 ++++++
>   tools/golang/xenlight/types.gen.go   |  1 +
>   tools/include/libxl.h                |  6 ++++++
>   tools/libs/light/libxl_create.c      | 10 ++++++++++
>   tools/libs/light/libxl_types.idl     |  2 ++
>   tools/ocaml/libs/xc/xenctrl.ml       |  1 +
>   tools/ocaml/libs/xc/xenctrl.mli      |  1 +
>   tools/xl/xl_parse.c                  |  2 ++
>   xen/arch/arm/domain.c                | 12 +++++++++---
>   xen/arch/arm/setup.c                 |  1 +
>   xen/common/domain.c                  | 10 +++++++++-
>   xen/include/asm-arm/domain.h         |  1 +
>   xen/include/public/domctl.h          |  4 +++-
>   14 files changed, 69 insertions(+), 5 deletions(-)
> 
> diff --git a/docs/man/xl.cfg.5.pod.in b/docs/man/xl.cfg.5.pod.in
> index 4b1e3028d2..55c4881205 100644
> --- a/docs/man/xl.cfg.5.pod.in
> +++ b/docs/man/xl.cfg.5.pod.in
> @@ -690,6 +690,23 @@ default.
>   B<NOTE>: Acceptable values are platform specific.  For Intel Processor
>   Trace, this value must be a power of 2 between 4k and 16M.
>   
> +=item B<vpmu=BOOLEAN>
> +
> +Currently ARM only.
> +
> +Specifies whether to enable the access to PMU registers by disabling
> +the PMU traps.
> +
> +The PMU registers are not virtualized and the physical registers are directly
> +accessible when this parameter is enabled. There is no interrupt support and
> +Xen will not save/restore the register values on context switches.
> +
> +vPMU, by design and purpose, exposes system level performance
> +information to the guest. Only to be used by sufficiently privileged
> +domains. This feature is currently in experimental state.

Please update SUPPORT.MD to mention the support of this feature.

> +
> +If this option is not specified then it will default to B<false>.
> +
>   =back
>   
>   =head2 Devices
> diff --git a/tools/golang/xenlight/helpers.gen.go b/tools/golang/xenlight/helpers.gen.go
> index c8669837d8..2449580bad 100644
> --- a/tools/golang/xenlight/helpers.gen.go
> +++ b/tools/golang/xenlight/helpers.gen.go
> @@ -1119,6 +1119,9 @@ return fmt.Errorf("converting field ArchX86.MsrRelaxed: %v", err)
>   }
>   x.Altp2M = Altp2MMode(xc.altp2m)
>   x.VmtraceBufKb = int(xc.vmtrace_buf_kb)
> +if err := x.Vpmu.fromC(&xc.vpmu);err != nil {
> +return fmt.Errorf("converting field Vpmu: %v", err)
> +}
>   
>    return nil}
>   
> @@ -1600,6 +1603,9 @@ return fmt.Errorf("converting field ArchX86.MsrRelaxed: %v", err)
>   }
>   xc.altp2m = C.libxl_altp2m_mode(x.Altp2M)
>   xc.vmtrace_buf_kb = C.int(x.VmtraceBufKb)
> +if err := x.Vpmu.toC(&xc.vpmu); err != nil {
> +return fmt.Errorf("converting field Vpmu: %v", err)
> +}
>   
>    return nil
>    }
> diff --git a/tools/golang/xenlight/types.gen.go b/tools/golang/xenlight/types.gen.go
> index 45f2cba3d2..b2e8bd1a85 100644
> --- a/tools/golang/xenlight/types.gen.go
> +++ b/tools/golang/xenlight/types.gen.go
> @@ -521,6 +521,7 @@ MsrRelaxed Defbool
>   }
>   Altp2M Altp2MMode
>   VmtraceBufKb int
> +Vpmu Defbool
>   }
>   
>   type DomainBuildInfoTypeUnion interface {
> diff --git a/tools/include/libxl.h b/tools/include/libxl.h
> index ec5e3badae..ee73eb06f1 100644
> --- a/tools/include/libxl.h
> +++ b/tools/include/libxl.h
> @@ -508,6 +508,12 @@
>    */
>   #define LIBXL_HAVE_PHYSINFO_CAP_VPMU 1
>   
> +/*
> + * LIBXL_HAVE_VPMU indicates that libxl_domain_build_info has a vpmu parameter,
> + * which allows to enable the access to PMU registers.
> + */
> +#define LIBXL_HAVE_VPMU 1
> +
>   /*
>    * libxl ABI compatibility
>    *
> diff --git a/tools/libs/light/libxl_create.c b/tools/libs/light/libxl_create.c
> index e356b2106d..2a0234ec16 100644
> --- a/tools/libs/light/libxl_create.c
> +++ b/tools/libs/light/libxl_create.c
> @@ -91,6 +91,7 @@ int libxl__domain_build_info_setdefault(libxl__gc *gc,
>       }
>   
>       libxl_defbool_setdefault(&b_info->device_model_stubdomain, false);
> +    libxl_defbool_setdefault(&b_info->vpmu, false);
>   
>       if (libxl_defbool_val(b_info->device_model_stubdomain) &&
>           !b_info->device_model_ssidref)
> @@ -622,6 +623,9 @@ int libxl__domain_make(libxl__gc *gc, libxl_domain_config *d_config,
>                   create.flags |= XEN_DOMCTL_CDF_nested_virt;
>           }
>   
> +        if ( libxl_defbool_val(b_info->vpmu) )
> +            create.flags |= XEN_DOMCTL_CDF_vpmu;
> +
>           assert(info->passthrough != LIBXL_PASSTHROUGH_DEFAULT);
>           LOG(DETAIL, "passthrough: %s",
>               libxl_passthrough_to_string(info->passthrough));
> @@ -1199,6 +1203,12 @@ int libxl__domain_config_setdefault(libxl__gc *gc,
>           goto error_out;
>       }
>   
> +    if (libxl_defbool_val(d_config->b_info.vpmu) && !physinfo.cap_vpmu) {
> +        ret = ERROR_INVAL;
> +        LOGD(ERROR, domid, "vpmu not supported on this platform\n");
> +        goto error_out;
> +    }
> +
>       ret = 0;
>    error_out:
>       return ret;
> diff --git a/tools/libs/light/libxl_types.idl b/tools/libs/light/libxl_types.idl
> index 993e83acca..b96fb5c47e 100644
> --- a/tools/libs/light/libxl_types.idl
> +++ b/tools/libs/light/libxl_types.idl
> @@ -655,6 +655,8 @@ libxl_domain_build_info = Struct("domain_build_info",[
>       # Use zero value to disable this feature.
>       ("vmtrace_buf_kb", integer),
>   
> +    ("vpmu", libxl_defbool),
> +
>       ], dir=DIR_IN,
>          copy_deprecated_fn="libxl__domain_build_info_copy_deprecated",
>   )
> diff --git a/tools/ocaml/libs/xc/xenctrl.ml b/tools/ocaml/libs/xc/xenctrl.ml
> index 7a4030a192..86758babb3 100644
> --- a/tools/ocaml/libs/xc/xenctrl.ml
> +++ b/tools/ocaml/libs/xc/xenctrl.ml
> @@ -70,6 +70,7 @@ type domain_create_flag =
>   	| CDF_IOMMU
>   	| CDF_NESTED_VIRT
>   	| CDF_VPCI
> +	| CDF_VPMU
>   
>   type domain_create_iommu_opts =
>   	| IOMMU_NO_SHAREPT
> diff --git a/tools/ocaml/libs/xc/xenctrl.mli b/tools/ocaml/libs/xc/xenctrl.mli
> index 6900513e7f..0fdb0cc169 100644
> --- a/tools/ocaml/libs/xc/xenctrl.mli
> +++ b/tools/ocaml/libs/xc/xenctrl.mli
> @@ -63,6 +63,7 @@ type domain_create_flag =
>     | CDF_IOMMU
>     | CDF_NESTED_VIRT
>     | CDF_VPCI
> +  | CDF_VPMU
>   
>   type domain_create_iommu_opts =
>     | IOMMU_NO_SHAREPT
> diff --git a/tools/xl/xl_parse.c b/tools/xl/xl_parse.c
> index 17dddb4cd5..c503b9be00 100644
> --- a/tools/xl/xl_parse.c
> +++ b/tools/xl/xl_parse.c
> @@ -2750,6 +2750,8 @@ skip_usbdev:
>                       "If it fixes an issue you are having please report to "
>                       "xen-devel@lists.xenproject.org.\n");
>   
> +    xlu_cfg_get_defbool(config, "vpmu", &b_info->vpmu, 0);
> +
>       xlu_cfg_destroy(config);
>   }
>   
> diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
> index aae4472479..2f988c790e 100644
> --- a/xen/arch/arm/domain.c
> +++ b/xen/arch/arm/domain.c
> @@ -276,6 +276,8 @@ static void ctxt_switch_to(struct vcpu *n)
>        * timer. The interrupt needs to be injected into the guest. */
>       WRITE_SYSREG(n->arch.cntkctl, CNTKCTL_EL1);
>       virt_timer_restore(n);
> +
> +    WRITE_SYSREG(n->arch.mdcr_el2, MDCR_EL2);
>   }
>   
>   /* Update per-VCPU guest runstate shared memory area (if registered). */
> @@ -586,6 +588,10 @@ int arch_vcpu_create(struct vcpu *v)
>   
>       v->arch.hcr_el2 = get_default_hcr_flags();
>   
> +    v->arch.mdcr_el2 = HDCR_TDRA | HDCR_TDOSA | HDCR_TDA;
> +    if ( !(v->domain->options & XEN_DOMCTL_CDF_vpmu) )
> +        v->arch.mdcr_el2 |= HDCR_TPM | HDCR_TPMCR;
> +
>       if ( (rc = vcpu_vgic_init(v)) != 0 )
>           goto fail;
>   
> @@ -622,9 +628,9 @@ int arch_sanitise_domain_config(struct xen_domctl_createdomain *config)
>   {
>       unsigned int max_vcpus;
>   
> -    /* HVM and HAP must be set. IOMMU and VPCI may or may not be */
> -    if ( (config->flags & ~XEN_DOMCTL_CDF_iommu & ~XEN_DOMCTL_CDF_vpci) !=
> -         (XEN_DOMCTL_CDF_hvm | XEN_DOMCTL_CDF_hap) )
> +    /* HVM and HAP must be set. IOMMU, VPCI and VPMU may or may not be */
> +    if ( (config->flags & ~(XEN_DOMCTL_CDF_iommu | XEN_DOMCTL_CDF_vpci |
> +          XEN_DOMCTL_CDF_vpmu)) != (XEN_DOMCTL_CDF_hvm | XEN_DOMCTL_CDF_hap) )

The split here is not easy to read. Can you introduce two masks (one for 
the flags that must be set and the other optional) so we don't need to 
split the optional options over two lines.

>       {
>           dprintk(XENLOG_INFO, "Unsupported configuration %#x\n",
>                   config->flags);
> diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
> index 49dc90d198..85386a765a 100644
> --- a/xen/arch/arm/setup.c
> +++ b/xen/arch/arm/setup.c
> @@ -108,6 +108,7 @@ static void __init processor_id(void)
>   
>       identify_cpu(c);
>       current_cpu_data = *c;
> +    vpmu_is_available = cpu_has_pmu;

This wants to be set after the ID registers are sanitized (i.e. after 
the secondary CPUs have been brought).

Also, on Armv8, the features supported by 64-bit and 32-bit exception 
level are separate. I couldn't find anything in the Arm that suggest 
that if the PMU is implemented by the former, then the latter must be. 
Do you have the page in hand?

>   
>       if ( c->midr.implementer < ARRAY_SIZE(processor_implementers) &&
>            processor_implementers[c->midr.implementer] )
> diff --git a/xen/common/domain.c b/xen/common/domain.c
> index 262b6c0c3c..8543fb54fd 100644
> --- a/xen/common/domain.c
> +++ b/xen/common/domain.c
> @@ -480,12 +480,14 @@ static int sanitise_domain_config(struct xen_domctl_createdomain *config)
>       bool hvm = config->flags & XEN_DOMCTL_CDF_hvm;
>       bool hap = config->flags & XEN_DOMCTL_CDF_hap;
>       bool iommu = config->flags & XEN_DOMCTL_CDF_iommu;
> +    bool vpmu = config->flags & XEN_DOMCTL_CDF_vpmu;
>   
>       if ( config->flags &
>            ~(XEN_DOMCTL_CDF_hvm | XEN_DOMCTL_CDF_hap |
>              XEN_DOMCTL_CDF_s3_integrity | XEN_DOMCTL_CDF_oos_off |
>              XEN_DOMCTL_CDF_xs_domain | XEN_DOMCTL_CDF_iommu |
> -           XEN_DOMCTL_CDF_nested_virt | XEN_DOMCTL_CDF_vpci) )
> +           XEN_DOMCTL_CDF_nested_virt | XEN_DOMCTL_CDF_vpci |
> +           XEN_DOMCTL_CDF_vpmu) )
>       {
>           dprintk(XENLOG_INFO, "Unknown CDF flags %#x\n", config->flags);
>           return -EINVAL;
> @@ -534,6 +536,12 @@ static int sanitise_domain_config(struct xen_domctl_createdomain *config)
>           return -EINVAL;
>       }
>   
> +    if ( vpmu && !vpmu_is_available )
> +    {
> +        dprintk(XENLOG_INFO, "vpmu requested but cannot be enabled this way\n");
> +        return -EINVAL;
> +    }
> +
>       return arch_sanitise_domain_config(config);
>   }
>   
> diff --git a/xen/include/asm-arm/domain.h b/xen/include/asm-arm/domain.h
> index c9277b5c6d..14e575288f 100644
> --- a/xen/include/asm-arm/domain.h
> +++ b/xen/include/asm-arm/domain.h
> @@ -166,6 +166,7 @@ struct arch_vcpu
>   
>       /* HYP configuration */
>       register_t hcr_el2;
> +    register_t mdcr_el2;
>   
>       uint32_t teecr, teehbr; /* ThumbEE, 32-bit guests only */
>   #ifdef CONFIG_ARM_32
> diff --git a/xen/include/public/domctl.h b/xen/include/public/domctl.h
> index 4cb3f662c2..a53cbd16f4 100644
> --- a/xen/include/public/domctl.h
> +++ b/xen/include/public/domctl.h
> @@ -71,9 +71,11 @@ struct xen_domctl_createdomain {
>   #define _XEN_DOMCTL_CDF_nested_virt   6
>   #define XEN_DOMCTL_CDF_nested_virt    (1U << _XEN_DOMCTL_CDF_nested_virt)
>   #define XEN_DOMCTL_CDF_vpci           (1U << 7)
> +/* Should we expose the vPMU to the guest? */
> +#define XEN_DOMCTL_CDF_vpmu           (1U << 8)
>   
>   /* Max XEN_DOMCTL_CDF_* constant.  Used for ABI checking. */
> -#define XEN_DOMCTL_CDF_MAX XEN_DOMCTL_CDF_vpci
> +#define XEN_DOMCTL_CDF_MAX XEN_DOMCTL_CDF_vpmu
>   
>       uint32_t flags;
>   
> 

Cheers,

-- 
Julien Grall


^ permalink raw reply	[flat|nested] 11+ messages in thread
* Re: [PATCH v4 1/3] xen+tools: Introduce XEN_SYSCTL_PHYSCAP_vpmu
  2021-10-11  9:00 ` [PATCH v4 1/3] xen+tools: Introduce XEN_SYSCTL_PHYSCAP_vpmu Michal Orzel
@ 2021-10-11 11:02   ` Ian Jackson
  0 siblings, 0 replies; 11+ messages in thread
From: Ian Jackson @ 2021-10-11 11:02 UTC (permalink / raw)
  To: Michal Orzel
  Cc: xen-devel, George Dunlap, Nick Rosbrook, Wei Liu, Andrew Cooper,
	Jan Beulich, Julien Grall, Stefano Stabellini, Anthony PERARD,
	Juergen Gross, Christian Lindig, David Scott, bertrand.marquis

Michal Orzel writes ("[PATCH v4 1/3] xen+tools: Introduce XEN_SYSCTL_PHYSCAP_vpmu"):
> Introduce flag XEN_SYSCTL_PHYSCAP_vpmu which
> indicates whether the platform supports vPMU
> functionality. Modify Xen and tools accordingly.
> 
> Take the opportunity and fix XEN_SYSCTL_PHYSCAP_vmtrace
> definition in sysctl.h which wrongly use (1 << 6)
> instead of (1u << 6).
> 
> Signed-off-by: Michal Orzel <michal.orzel@arm.com>
> Reviewed-by: Bertrand Marquis <bertrand.marquis@arm.com>
> Acked-by: Nick Rosbrook <rosbrookn@ainfosec.com>
> Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>

Tools:

Acked-by: Ian Jackson <iwj@xenproject.org>


^ permalink raw reply	[flat|nested] 11+ messages in thread
* Re: [PATCH v4 2/3] xen/arm: Check for PMU platform support
  2021-10-11 10:17   ` Julien Grall
@ 2021-10-11 12:06     ` Michal Orzel
  0 siblings, 0 replies; 11+ messages in thread
From: Michal Orzel @ 2021-10-11 12:06 UTC (permalink / raw)
  To: Julien Grall, xen-devel
  Cc: Stefano Stabellini, Volodymyr Babchuk, bertrand.marquis

Hi Julien,

On 11.10.2021 12:17, Julien Grall wrote:
> Hi Michal,
> 
> On 11/10/2021 10:00, Michal Orzel wrote:
>> ID_AA64DFR0_EL1/ID_DFR0_EL1 registers provide
>> information about PMU support. Replace structure
>> dbg64/dbg32 with a union and fill in all the
>> register fields according to document:
>> ARM Architecture Registers(DDI 0595, 2021-06).
>>
>> Add macros boot_dbg_feature64/boot_dbg_feature32
>> to check for a debug feature. Add macro
>> cpu_has_pmu to check for PMU support.
>>
>> Signed-off-by: Michal Orzel <michal.orzel@arm.com>
>> Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
>> Reviewed-by: Bertrand Marquis <bertrand.marquis@arm.com>
>> ---
>> Changes since v3:
>> -none
>> Changes since v2:
>> -none
>> Changes since v1:
>> -new in v2
>> ---
>>   xen/include/asm-arm/cpufeature.h | 49 ++++++++++++++++++++++++++++++--
>>   1 file changed, 47 insertions(+), 2 deletions(-)
>>
>> diff --git a/xen/include/asm-arm/cpufeature.h b/xen/include/asm-arm/cpufeature.h
>> index 5ca09b0bff..4fce23844d 100644
>> --- a/xen/include/asm-arm/cpufeature.h
>> +++ b/xen/include/asm-arm/cpufeature.h
>> @@ -4,6 +4,7 @@
>>   #ifdef CONFIG_ARM_64
>>   #define cpu_feature64(c, feat)         ((c)->pfr64.feat)
>>   #define boot_cpu_feature64(feat)       (system_cpuinfo.pfr64.feat)
>> +#define boot_dbg_feature64(feat)       (system_cpuinfo.dbg64.feat)
>>     #define cpu_feature64_has_el0_32(c)    (cpu_feature64(c, el0) == 2)
>>   @@ -22,6 +23,7 @@
>>     #define cpu_feature32(c, feat)         ((c)->pfr32.feat)
>>   #define boot_cpu_feature32(feat)       (system_cpuinfo.pfr32.feat)
>> +#define boot_dbg_feature32(feat)       (system_cpuinfo.dbg32.feat)
>>     #define cpu_has_arm       (boot_cpu_feature32(arm) == 1)
>>   #define cpu_has_thumb     (boot_cpu_feature32(thumb) >= 1)
>> @@ -32,8 +34,10 @@
>>     #ifdef CONFIG_ARM_32
>>   #define cpu_has_gentimer  (boot_cpu_feature32(gentimer) == 1)
>> +#define cpu_has_pmu       (boot_dbg_feature32(perfmon) >= 1)
> 
> From my understanding, on Armv7, perfmon == 0 only means PMUv2 is not present. IOW, it doesn't say whether PMUv1 is supported.
> 
> I think it is OK to treat as the PMU is not supported (ARMv8 treat it like that too), but I would like a comment in the code so it is clear this is a deliberate choice.
> 
We are checking if any version of PMU is supported meaning perfmon is >=1.
On ARMv8 any value higher than 0 means that PMU is present. 0 means not supported.
On ARMv7 the above is not really true. Any value higher than 0 and lower than 15 means the PMU is supported.
So I think I should do:
#define cpu_has_pmu       ((boot_dbg_feature32(perfmon) >= 1) && \
                           (boot_dbg_feature32(perfmon) < 15))

Do you agree? 

>>   #else
>>   #define cpu_has_gentimer  (1)
>> +#define cpu_has_pmu       (boot_dbg_feature64(pmu_ver) >= 1)
>>   #endif
>>   #define cpu_has_security  (boot_cpu_feature32(security) > 0)
>>   @@ -181,8 +185,28 @@ struct cpuinfo_arm {
>>           };
>>       } pfr64;
>>   -    struct {
>> +    union {
>>           register_t bits[2];
>> +        struct {
>> +            /* DFR0 */
>> +            unsigned long debug_ver:4;
>> +            unsigned long trace_ver:4;
>> +            unsigned long pmu_ver:4;
>> +            unsigned long brps:4;
>> +            unsigned long __res0:4;
>> +            unsigned long wrps:4;
>> +            unsigned long __res1:4;
>> +            unsigned long ctx_cmps:4;
>> +            unsigned long pms_ver:4;
>> +            unsigned long double_lock:4;
>> +            unsigned long trace_filt:4;
>> +            unsigned long __res2:4;
>> +            unsigned long mtpmu:4;
>> +            unsigned long __res3:12;
>> +
>> +            /* DFR1 */
>> +            unsigned long __res4:64;
>> +        };
>>       } dbg64;
>>         struct {
>> @@ -321,8 +345,29 @@ struct cpuinfo_arm {
>>           };
>>       } pfr32;
>>   -    struct {
>> +    union {
>>           register_t bits[2];
>> +        struct {
>> +            /* DFR0 */
>> +            unsigned long copdbg:4;
>> +            unsigned long copsdbg:4;
>> +            unsigned long mmapdbg:4;
>> +            unsigned long coptrc:4;
>> +            unsigned long mmaptrc:4;
>> +            unsigned long mprofdbg:4;
>> +            unsigned long perfmon:4;
>> +            unsigned long tracefilt:4;
>> +#ifdef CONFIG_ARM_64
>> +            unsigned long __res0:32;
>> +#endif
>> +
>> +            /* DFR1 */
>> +            unsigned long mtpmu:4;
>> +            unsigned long __res1:28;
>> +#ifdef CONFIG_ARM_64
>> +            unsigned long __res2:32;
>> +#endif
>> +        };
>>       } dbg32;
>>         struct {
>>
> 
> Cheers,
> 

Cheers,
Michal


^ permalink raw reply	[flat|nested] 11+ messages in thread
* Re: [PATCH v4 3/3] xen: Expose the PMU to the guests
  2021-10-11 10:21   ` Julien Grall
@ 2021-10-11 14:11     ` Bertrand Marquis
  2021-10-12 15:29       ` Julien Grall
  0 siblings, 1 reply; 11+ messages in thread
From: Bertrand Marquis @ 2021-10-11 14:11 UTC (permalink / raw)
  To: Julien Grall
  Cc: Michal Orzel, Xen-devel, Ian Jackson, Wei Liu, Andrew Cooper,
	George Dunlap, Jan Beulich, Stefano Stabellini, Nick Rosbrook,
	Anthony PERARD, Juergen Gross, Christian Lindig, David Scott,
	Volodymyr Babchuk

Hi Julien,

> On 11 Oct 2021, at 11:21, Julien Grall <julien@xen.org> wrote:
> 
> Hi Michal,
> 
> On 11/10/2021 10:00, Michal Orzel wrote:
>> Add parameter vpmu to xl domain configuration syntax
>> to enable the access to PMU registers by disabling
>> the PMU traps(currently only for ARM).
>> The current status is that the PMU registers are not
>> virtualized and the physical registers are directly
>> accessible when this parameter is enabled. There is no
>> interrupt support and Xen will not save/restore the
>> register values on context switches.
>> Please note that this feature is experimental.
>> Signed-off-by: Michal Orzel <michal.orzel@arm.com>
>> Signed-off-by: Julien Grall <julien@xen.org>
>> Reviewed-by: Bertrand Marquis <bertrand.marquis@arm.com>
>> ---
>> Changes since v3:
>> -fail if vpmu is set but not supported
>> -rebase on top of latest staging
>> Changes since v2:
>> -remove redundant check from x86 code
>> -do not define bit position and mask separately
>> Changes since v1:
>> -modify vpmu parameter to be common rather than arch specific
>> ---
>>  docs/man/xl.cfg.5.pod.in             | 17 +++++++++++++++++
>>  tools/golang/xenlight/helpers.gen.go |  6 ++++++
>>  tools/golang/xenlight/types.gen.go   |  1 +
>>  tools/include/libxl.h                |  6 ++++++
>>  tools/libs/light/libxl_create.c      | 10 ++++++++++
>>  tools/libs/light/libxl_types.idl     |  2 ++
>>  tools/ocaml/libs/xc/xenctrl.ml       |  1 +
>>  tools/ocaml/libs/xc/xenctrl.mli      |  1 +
>>  tools/xl/xl_parse.c                  |  2 ++
>>  xen/arch/arm/domain.c                | 12 +++++++++---
>>  xen/arch/arm/setup.c                 |  1 +
>>  xen/common/domain.c                  | 10 +++++++++-
>>  xen/include/asm-arm/domain.h         |  1 +
>>  xen/include/public/domctl.h          |  4 +++-
>>  14 files changed, 69 insertions(+), 5 deletions(-)
>> diff --git a/docs/man/xl.cfg.5.pod.in b/docs/man/xl.cfg.5.pod.in
>> index 4b1e3028d2..55c4881205 100644
>> --- a/docs/man/xl.cfg.5.pod.in
>> +++ b/docs/man/xl.cfg.5.pod.in
>> @@ -690,6 +690,23 @@ default.
>>  B<NOTE>: Acceptable values are platform specific.  For Intel Processor
>>  Trace, this value must be a power of 2 between 4k and 16M.
>>  +=item B<vpmu=BOOLEAN>
>> +
>> +Currently ARM only.
>> +
>> +Specifies whether to enable the access to PMU registers by disabling
>> +the PMU traps.
>> +
>> +The PMU registers are not virtualized and the physical registers are directly
>> +accessible when this parameter is enabled. There is no interrupt support and
>> +Xen will not save/restore the register values on context switches.
>> +
>> +vPMU, by design and purpose, exposes system level performance
>> +information to the guest. Only to be used by sufficiently privileged
>> +domains. This feature is currently in experimental state.
> 
> Please update SUPPORT.MD to mention the support of this feature.
> 
>> +
>> +If this option is not specified then it will default to B<false>.
>> +
>>  =back
>>    =head2 Devices
>> diff --git a/tools/golang/xenlight/helpers.gen.go b/tools/golang/xenlight/helpers.gen.go
>> index c8669837d8..2449580bad 100644
>> --- a/tools/golang/xenlight/helpers.gen.go
>> +++ b/tools/golang/xenlight/helpers.gen.go
>> @@ -1119,6 +1119,9 @@ return fmt.Errorf("converting field ArchX86.MsrRelaxed: %v", err)
>>  }
>>  x.Altp2M = Altp2MMode(xc.altp2m)
>>  x.VmtraceBufKb = int(xc.vmtrace_buf_kb)
>> +if err := x.Vpmu.fromC(&xc.vpmu);err != nil {
>> +return fmt.Errorf("converting field Vpmu: %v", err)
>> +}
>>     return nil}
>>  @@ -1600,6 +1603,9 @@ return fmt.Errorf("converting field ArchX86.MsrRelaxed: %v", err)
>>  }
>>  xc.altp2m = C.libxl_altp2m_mode(x.Altp2M)
>>  xc.vmtrace_buf_kb = C.int(x.VmtraceBufKb)
>> +if err := x.Vpmu.toC(&xc.vpmu); err != nil {
>> +return fmt.Errorf("converting field Vpmu: %v", err)
>> +}
>>     return nil
>>   }
>> diff --git a/tools/golang/xenlight/types.gen.go b/tools/golang/xenlight/types.gen.go
>> index 45f2cba3d2..b2e8bd1a85 100644
>> --- a/tools/golang/xenlight/types.gen.go
>> +++ b/tools/golang/xenlight/types.gen.go
>> @@ -521,6 +521,7 @@ MsrRelaxed Defbool
>>  }
>>  Altp2M Altp2MMode
>>  VmtraceBufKb int
>> +Vpmu Defbool
>>  }
>>    type DomainBuildInfoTypeUnion interface {
>> diff --git a/tools/include/libxl.h b/tools/include/libxl.h
>> index ec5e3badae..ee73eb06f1 100644
>> --- a/tools/include/libxl.h
>> +++ b/tools/include/libxl.h
>> @@ -508,6 +508,12 @@
>>   */
>>  #define LIBXL_HAVE_PHYSINFO_CAP_VPMU 1
>>  +/*
>> + * LIBXL_HAVE_VPMU indicates that libxl_domain_build_info has a vpmu parameter,
>> + * which allows to enable the access to PMU registers.
>> + */
>> +#define LIBXL_HAVE_VPMU 1
>> +
>>  /*
>>   * libxl ABI compatibility
>>   *
>> diff --git a/tools/libs/light/libxl_create.c b/tools/libs/light/libxl_create.c
>> index e356b2106d..2a0234ec16 100644
>> --- a/tools/libs/light/libxl_create.c
>> +++ b/tools/libs/light/libxl_create.c
>> @@ -91,6 +91,7 @@ int libxl__domain_build_info_setdefault(libxl__gc *gc,
>>      }
>>        libxl_defbool_setdefault(&b_info->device_model_stubdomain, false);
>> +    libxl_defbool_setdefault(&b_info->vpmu, false);
>>        if (libxl_defbool_val(b_info->device_model_stubdomain) &&
>>          !b_info->device_model_ssidref)
>> @@ -622,6 +623,9 @@ int libxl__domain_make(libxl__gc *gc, libxl_domain_config *d_config,
>>                  create.flags |= XEN_DOMCTL_CDF_nested_virt;
>>          }
>>  +        if ( libxl_defbool_val(b_info->vpmu) )
>> +            create.flags |= XEN_DOMCTL_CDF_vpmu;
>> +
>>          assert(info->passthrough != LIBXL_PASSTHROUGH_DEFAULT);
>>          LOG(DETAIL, "passthrough: %s",
>>              libxl_passthrough_to_string(info->passthrough));
>> @@ -1199,6 +1203,12 @@ int libxl__domain_config_setdefault(libxl__gc *gc,
>>          goto error_out;
>>      }
>>  +    if (libxl_defbool_val(d_config->b_info.vpmu) && !physinfo.cap_vpmu) {
>> +        ret = ERROR_INVAL;
>> +        LOGD(ERROR, domid, "vpmu not supported on this platform\n");
>> +        goto error_out;
>> +    }
>> +
>>      ret = 0;
>>   error_out:
>>      return ret;
>> diff --git a/tools/libs/light/libxl_types.idl b/tools/libs/light/libxl_types.idl
>> index 993e83acca..b96fb5c47e 100644
>> --- a/tools/libs/light/libxl_types.idl
>> +++ b/tools/libs/light/libxl_types.idl
>> @@ -655,6 +655,8 @@ libxl_domain_build_info = Struct("domain_build_info",[
>>      # Use zero value to disable this feature.
>>      ("vmtrace_buf_kb", integer),
>>  +    ("vpmu", libxl_defbool),
>> +
>>      ], dir=DIR_IN,
>>         copy_deprecated_fn="libxl__domain_build_info_copy_deprecated",
>>  )
>> diff --git a/tools/ocaml/libs/xc/xenctrl.ml b/tools/ocaml/libs/xc/xenctrl.ml
>> index 7a4030a192..86758babb3 100644
>> --- a/tools/ocaml/libs/xc/xenctrl.ml
>> +++ b/tools/ocaml/libs/xc/xenctrl.ml
>> @@ -70,6 +70,7 @@ type domain_create_flag =
>>  	| CDF_IOMMU
>>  	| CDF_NESTED_VIRT
>>  	| CDF_VPCI
>> +	| CDF_VPMU
>>    type domain_create_iommu_opts =
>>  	| IOMMU_NO_SHAREPT
>> diff --git a/tools/ocaml/libs/xc/xenctrl.mli b/tools/ocaml/libs/xc/xenctrl.mli
>> index 6900513e7f..0fdb0cc169 100644
>> --- a/tools/ocaml/libs/xc/xenctrl.mli
>> +++ b/tools/ocaml/libs/xc/xenctrl.mli
>> @@ -63,6 +63,7 @@ type domain_create_flag =
>>    | CDF_IOMMU
>>    | CDF_NESTED_VIRT
>>    | CDF_VPCI
>> +  | CDF_VPMU
>>    type domain_create_iommu_opts =
>>    | IOMMU_NO_SHAREPT
>> diff --git a/tools/xl/xl_parse.c b/tools/xl/xl_parse.c
>> index 17dddb4cd5..c503b9be00 100644
>> --- a/tools/xl/xl_parse.c
>> +++ b/tools/xl/xl_parse.c
>> @@ -2750,6 +2750,8 @@ skip_usbdev:
>>                      "If it fixes an issue you are having please report to "
>>                      "xen-devel@lists.xenproject.org.\n");
>>  +    xlu_cfg_get_defbool(config, "vpmu", &b_info->vpmu, 0);
>> +
>>      xlu_cfg_destroy(config);
>>  }
>>  diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
>> index aae4472479..2f988c790e 100644
>> --- a/xen/arch/arm/domain.c
>> +++ b/xen/arch/arm/domain.c
>> @@ -276,6 +276,8 @@ static void ctxt_switch_to(struct vcpu *n)
>>       * timer. The interrupt needs to be injected into the guest. */
>>      WRITE_SYSREG(n->arch.cntkctl, CNTKCTL_EL1);
>>      virt_timer_restore(n);
>> +
>> +    WRITE_SYSREG(n->arch.mdcr_el2, MDCR_EL2);
>>  }
>>    /* Update per-VCPU guest runstate shared memory area (if registered). */
>> @@ -586,6 +588,10 @@ int arch_vcpu_create(struct vcpu *v)
>>        v->arch.hcr_el2 = get_default_hcr_flags();
>>  +    v->arch.mdcr_el2 = HDCR_TDRA | HDCR_TDOSA | HDCR_TDA;
>> +    if ( !(v->domain->options & XEN_DOMCTL_CDF_vpmu) )
>> +        v->arch.mdcr_el2 |= HDCR_TPM | HDCR_TPMCR;
>> +
>>      if ( (rc = vcpu_vgic_init(v)) != 0 )
>>          goto fail;
>>  @@ -622,9 +628,9 @@ int arch_sanitise_domain_config(struct xen_domctl_createdomain *config)
>>  {
>>      unsigned int max_vcpus;
>>  -    /* HVM and HAP must be set. IOMMU and VPCI may or may not be */
>> -    if ( (config->flags & ~XEN_DOMCTL_CDF_iommu & ~XEN_DOMCTL_CDF_vpci) !=
>> -         (XEN_DOMCTL_CDF_hvm | XEN_DOMCTL_CDF_hap) )
>> +    /* HVM and HAP must be set. IOMMU, VPCI and VPMU may or may not be */
>> +    if ( (config->flags & ~(XEN_DOMCTL_CDF_iommu | XEN_DOMCTL_CDF_vpci |
>> +          XEN_DOMCTL_CDF_vpmu)) != (XEN_DOMCTL_CDF_hvm | XEN_DOMCTL_CDF_hap) )
> 
> The split here is not easy to read. Can you introduce two masks (one for the flags that must be set and the other optional) so we don't need to split the optional options over two lines.
> 
>>      {
>>          dprintk(XENLOG_INFO, "Unsupported configuration %#x\n",
>>                  config->flags);
>> diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
>> index 49dc90d198..85386a765a 100644
>> --- a/xen/arch/arm/setup.c
>> +++ b/xen/arch/arm/setup.c
>> @@ -108,6 +108,7 @@ static void __init processor_id(void)
>>        identify_cpu(c);
>>      current_cpu_data = *c;
>> +    vpmu_is_available = cpu_has_pmu;
> 
> This wants to be set after the ID registers are sanitized (i.e. after the secondary CPUs have been brought).
> 
> Also, on Armv8, the features supported by 64-bit and 32-bit exception level are separate. I couldn't find anything in the Arm that suggest that if the PMU is implemented by the former, then the latter must be. Do you have the page in hand?

Chapter D7.1 at the end is saying this:

"The Performance Monitors Extension is common to AArch64 operation and AArch32 operation.”

Is this the justification you are looking for ?

Cheers
Bertrand

> 
>>        if ( c->midr.implementer < ARRAY_SIZE(processor_implementers) &&
>>           processor_implementers[c->midr.implementer] )
>> diff --git a/xen/common/domain.c b/xen/common/domain.c
>> index 262b6c0c3c..8543fb54fd 100644
>> --- a/xen/common/domain.c
>> +++ b/xen/common/domain.c
>> @@ -480,12 +480,14 @@ static int sanitise_domain_config(struct xen_domctl_createdomain *config)
>>      bool hvm = config->flags & XEN_DOMCTL_CDF_hvm;
>>      bool hap = config->flags & XEN_DOMCTL_CDF_hap;
>>      bool iommu = config->flags & XEN_DOMCTL_CDF_iommu;
>> +    bool vpmu = config->flags & XEN_DOMCTL_CDF_vpmu;
>>        if ( config->flags &
>>           ~(XEN_DOMCTL_CDF_hvm | XEN_DOMCTL_CDF_hap |
>>             XEN_DOMCTL_CDF_s3_integrity | XEN_DOMCTL_CDF_oos_off |
>>             XEN_DOMCTL_CDF_xs_domain | XEN_DOMCTL_CDF_iommu |
>> -           XEN_DOMCTL_CDF_nested_virt | XEN_DOMCTL_CDF_vpci) )
>> +           XEN_DOMCTL_CDF_nested_virt | XEN_DOMCTL_CDF_vpci |
>> +           XEN_DOMCTL_CDF_vpmu) )
>>      {
>>          dprintk(XENLOG_INFO, "Unknown CDF flags %#x\n", config->flags);
>>          return -EINVAL;
>> @@ -534,6 +536,12 @@ static int sanitise_domain_config(struct xen_domctl_createdomain *config)
>>          return -EINVAL;
>>      }
>>  +    if ( vpmu && !vpmu_is_available )
>> +    {
>> +        dprintk(XENLOG_INFO, "vpmu requested but cannot be enabled this way\n");
>> +        return -EINVAL;
>> +    }
>> +
>>      return arch_sanitise_domain_config(config);
>>  }
>>  diff --git a/xen/include/asm-arm/domain.h b/xen/include/asm-arm/domain.h
>> index c9277b5c6d..14e575288f 100644
>> --- a/xen/include/asm-arm/domain.h
>> +++ b/xen/include/asm-arm/domain.h
>> @@ -166,6 +166,7 @@ struct arch_vcpu
>>        /* HYP configuration */
>>      register_t hcr_el2;
>> +    register_t mdcr_el2;
>>        uint32_t teecr, teehbr; /* ThumbEE, 32-bit guests only */
>>  #ifdef CONFIG_ARM_32
>> diff --git a/xen/include/public/domctl.h b/xen/include/public/domctl.h
>> index 4cb3f662c2..a53cbd16f4 100644
>> --- a/xen/include/public/domctl.h
>> +++ b/xen/include/public/domctl.h
>> @@ -71,9 +71,11 @@ struct xen_domctl_createdomain {
>>  #define _XEN_DOMCTL_CDF_nested_virt   6
>>  #define XEN_DOMCTL_CDF_nested_virt    (1U << _XEN_DOMCTL_CDF_nested_virt)
>>  #define XEN_DOMCTL_CDF_vpci           (1U << 7)
>> +/* Should we expose the vPMU to the guest? */
>> +#define XEN_DOMCTL_CDF_vpmu           (1U << 8)
>>    /* Max XEN_DOMCTL_CDF_* constant.  Used for ABI checking. */
>> -#define XEN_DOMCTL_CDF_MAX XEN_DOMCTL_CDF_vpci
>> +#define XEN_DOMCTL_CDF_MAX XEN_DOMCTL_CDF_vpmu
>>        uint32_t flags;
>>  
> 
> Cheers,
> 
> -- 
> Julien Grall


^ permalink raw reply	[flat|nested] 11+ messages in thread
* Re: [PATCH v4 3/3] xen: Expose the PMU to the guests
  2021-10-11  9:00 ` [PATCH v4 3/3] xen: Expose the PMU to the guests Michal Orzel
  2021-10-11 10:21   ` Julien Grall
@ 2021-10-11 20:09   ` Stefano Stabellini
  1 sibling, 0 replies; 11+ messages in thread
From: Stefano Stabellini @ 2021-10-11 20:09 UTC (permalink / raw)
  To: Michal Orzel
  Cc: xen-devel, Ian Jackson, Wei Liu, Andrew Cooper, George Dunlap,
	Jan Beulich, Julien Grall, Stefano Stabellini, Nick Rosbrook,
	Anthony PERARD, Juergen Gross, Christian Lindig, David Scott,
	Volodymyr Babchuk, bertrand.marquis

On Mon, 11 Oct 2021, Michal Orzel wrote:
> Add parameter vpmu to xl domain configuration syntax
> to enable the access to PMU registers by disabling
> the PMU traps(currently only for ARM).
> 
> The current status is that the PMU registers are not
> virtualized and the physical registers are directly
> accessible when this parameter is enabled. There is no
> interrupt support and Xen will not save/restore the
> register values on context switches.
> 
> Please note that this feature is experimental.
> 
> Signed-off-by: Michal Orzel <michal.orzel@arm.com>
> Signed-off-by: Julien Grall <julien@xen.org>
> Reviewed-by: Bertrand Marquis <bertrand.marquis@arm.com>

I just wanted to say that I don't have any further comments on this
patch (I saw the ones from Julien which are fine)



^ permalink raw reply	[flat|nested] 11+ messages in thread
* Re: [PATCH v4 3/3] xen: Expose the PMU to the guests
  2021-10-11 14:11     ` Bertrand Marquis
@ 2021-10-12 15:29       ` Julien Grall
  0 siblings, 0 replies; 11+ messages in thread
From: Julien Grall @ 2021-10-12 15:29 UTC (permalink / raw)
  To: Bertrand Marquis
  Cc: Michal Orzel, Xen-devel, Ian Jackson, Wei Liu, Andrew Cooper,
	George Dunlap, Jan Beulich, Stefano Stabellini, Nick Rosbrook,
	Anthony PERARD, Juergen Gross, Christian Lindig, David Scott,
	Volodymyr Babchuk

Hi Bertrand,

On 11/10/2021 15:11, Bertrand Marquis wrote:
>> On 11/10/2021 10:00, Michal Orzel wrote:
>>> Add parameter vpmu to xl domain configuration syntax
>>> to enable the access to PMU registers by disabling
>>> the PMU traps(currently only for ARM).
>>> The current status is that the PMU registers are not
>>> virtualized and the physical registers are directly
>>> accessible when this parameter is enabled. There is no
>>> interrupt support and Xen will not save/restore the
>>> register values on context switches.
>>> Please note that this feature is experimental.
>>> Signed-off-by: Michal Orzel <michal.orzel@arm.com>
>>> Signed-off-by: Julien Grall <julien@xen.org>
>>> Reviewed-by: Bertrand Marquis <bertrand.marquis@arm.com>
>>> ---
>>> Changes since v3:
>>> -fail if vpmu is set but not supported
>>> -rebase on top of latest staging
>>> Changes since v2:
>>> -remove redundant check from x86 code
>>> -do not define bit position and mask separately
>>> Changes since v1:
>>> -modify vpmu parameter to be common rather than arch specific
>>> ---
>>>   docs/man/xl.cfg.5.pod.in             | 17 +++++++++++++++++
>>>   tools/golang/xenlight/helpers.gen.go |  6 ++++++
>>>   tools/golang/xenlight/types.gen.go   |  1 +
>>>   tools/include/libxl.h                |  6 ++++++
>>>   tools/libs/light/libxl_create.c      | 10 ++++++++++
>>>   tools/libs/light/libxl_types.idl     |  2 ++
>>>   tools/ocaml/libs/xc/xenctrl.ml       |  1 +
>>>   tools/ocaml/libs/xc/xenctrl.mli      |  1 +
>>>   tools/xl/xl_parse.c                  |  2 ++
>>>   xen/arch/arm/domain.c                | 12 +++++++++---
>>>   xen/arch/arm/setup.c                 |  1 +
>>>   xen/common/domain.c                  | 10 +++++++++-
>>>   xen/include/asm-arm/domain.h         |  1 +
>>>   xen/include/public/domctl.h          |  4 +++-
>>>   14 files changed, 69 insertions(+), 5 deletions(-)
>>> diff --git a/docs/man/xl.cfg.5.pod.in b/docs/man/xl.cfg.5.pod.in
>>> index 4b1e3028d2..55c4881205 100644
>>> --- a/docs/man/xl.cfg.5.pod.in
>>> +++ b/docs/man/xl.cfg.5.pod.in
>>> @@ -690,6 +690,23 @@ default.
>>>   B<NOTE>: Acceptable values are platform specific.  For Intel Processor
>>>   Trace, this value must be a power of 2 between 4k and 16M.
>>>   +=item B<vpmu=BOOLEAN>
>>> +
>>> +Currently ARM only.
>>> +
>>> +Specifies whether to enable the access to PMU registers by disabling
>>> +the PMU traps.
>>> +
>>> +The PMU registers are not virtualized and the physical registers are directly
>>> +accessible when this parameter is enabled. There is no interrupt support and
>>> +Xen will not save/restore the register values on context switches.
>>> +
>>> +vPMU, by design and purpose, exposes system level performance
>>> +information to the guest. Only to be used by sufficiently privileged
>>> +domains. This feature is currently in experimental state.
>>
>> Please update SUPPORT.MD to mention the support of this feature.
>>
>>> +
>>> +If this option is not specified then it will default to B<false>.
>>> +
>>>   =back
>>>     =head2 Devices
>>> diff --git a/tools/golang/xenlight/helpers.gen.go b/tools/golang/xenlight/helpers.gen.go
>>> index c8669837d8..2449580bad 100644
>>> --- a/tools/golang/xenlight/helpers.gen.go
>>> +++ b/tools/golang/xenlight/helpers.gen.go
>>> @@ -1119,6 +1119,9 @@ return fmt.Errorf("converting field ArchX86.MsrRelaxed: %v", err)
>>>   }
>>>   x.Altp2M = Altp2MMode(xc.altp2m)
>>>   x.VmtraceBufKb = int(xc.vmtrace_buf_kb)
>>> +if err := x.Vpmu.fromC(&xc.vpmu);err != nil {
>>> +return fmt.Errorf("converting field Vpmu: %v", err)
>>> +}
>>>      return nil}
>>>   @@ -1600,6 +1603,9 @@ return fmt.Errorf("converting field ArchX86.MsrRelaxed: %v", err)
>>>   }
>>>   xc.altp2m = C.libxl_altp2m_mode(x.Altp2M)
>>>   xc.vmtrace_buf_kb = C.int(x.VmtraceBufKb)
>>> +if err := x.Vpmu.toC(&xc.vpmu); err != nil {
>>> +return fmt.Errorf("converting field Vpmu: %v", err)
>>> +}
>>>      return nil
>>>    }
>>> diff --git a/tools/golang/xenlight/types.gen.go b/tools/golang/xenlight/types.gen.go
>>> index 45f2cba3d2..b2e8bd1a85 100644
>>> --- a/tools/golang/xenlight/types.gen.go
>>> +++ b/tools/golang/xenlight/types.gen.go
>>> @@ -521,6 +521,7 @@ MsrRelaxed Defbool
>>>   }
>>>   Altp2M Altp2MMode
>>>   VmtraceBufKb int
>>> +Vpmu Defbool
>>>   }
>>>     type DomainBuildInfoTypeUnion interface {
>>> diff --git a/tools/include/libxl.h b/tools/include/libxl.h
>>> index ec5e3badae..ee73eb06f1 100644
>>> --- a/tools/include/libxl.h
>>> +++ b/tools/include/libxl.h
>>> @@ -508,6 +508,12 @@
>>>    */
>>>   #define LIBXL_HAVE_PHYSINFO_CAP_VPMU 1
>>>   +/*
>>> + * LIBXL_HAVE_VPMU indicates that libxl_domain_build_info has a vpmu parameter,
>>> + * which allows to enable the access to PMU registers.
>>> + */
>>> +#define LIBXL_HAVE_VPMU 1
>>> +
>>>   /*
>>>    * libxl ABI compatibility
>>>    *
>>> diff --git a/tools/libs/light/libxl_create.c b/tools/libs/light/libxl_create.c
>>> index e356b2106d..2a0234ec16 100644
>>> --- a/tools/libs/light/libxl_create.c
>>> +++ b/tools/libs/light/libxl_create.c
>>> @@ -91,6 +91,7 @@ int libxl__domain_build_info_setdefault(libxl__gc *gc,
>>>       }
>>>         libxl_defbool_setdefault(&b_info->device_model_stubdomain, false);
>>> +    libxl_defbool_setdefault(&b_info->vpmu, false);
>>>         if (libxl_defbool_val(b_info->device_model_stubdomain) &&
>>>           !b_info->device_model_ssidref)
>>> @@ -622,6 +623,9 @@ int libxl__domain_make(libxl__gc *gc, libxl_domain_config *d_config,
>>>                   create.flags |= XEN_DOMCTL_CDF_nested_virt;
>>>           }
>>>   +        if ( libxl_defbool_val(b_info->vpmu) )
>>> +            create.flags |= XEN_DOMCTL_CDF_vpmu;
>>> +
>>>           assert(info->passthrough != LIBXL_PASSTHROUGH_DEFAULT);
>>>           LOG(DETAIL, "passthrough: %s",
>>>               libxl_passthrough_to_string(info->passthrough));
>>> @@ -1199,6 +1203,12 @@ int libxl__domain_config_setdefault(libxl__gc *gc,
>>>           goto error_out;
>>>       }
>>>   +    if (libxl_defbool_val(d_config->b_info.vpmu) && !physinfo.cap_vpmu) {
>>> +        ret = ERROR_INVAL;
>>> +        LOGD(ERROR, domid, "vpmu not supported on this platform\n");
>>> +        goto error_out;
>>> +    }
>>> +
>>>       ret = 0;
>>>    error_out:
>>>       return ret;
>>> diff --git a/tools/libs/light/libxl_types.idl b/tools/libs/light/libxl_types.idl
>>> index 993e83acca..b96fb5c47e 100644
>>> --- a/tools/libs/light/libxl_types.idl
>>> +++ b/tools/libs/light/libxl_types.idl
>>> @@ -655,6 +655,8 @@ libxl_domain_build_info = Struct("domain_build_info",[
>>>       # Use zero value to disable this feature.
>>>       ("vmtrace_buf_kb", integer),
>>>   +    ("vpmu", libxl_defbool),
>>> +
>>>       ], dir=DIR_IN,
>>>          copy_deprecated_fn="libxl__domain_build_info_copy_deprecated",
>>>   )
>>> diff --git a/tools/ocaml/libs/xc/xenctrl.ml b/tools/ocaml/libs/xc/xenctrl.ml
>>> index 7a4030a192..86758babb3 100644
>>> --- a/tools/ocaml/libs/xc/xenctrl.ml
>>> +++ b/tools/ocaml/libs/xc/xenctrl.ml
>>> @@ -70,6 +70,7 @@ type domain_create_flag =
>>>   	| CDF_IOMMU
>>>   	| CDF_NESTED_VIRT
>>>   	| CDF_VPCI
>>> +	| CDF_VPMU
>>>     type domain_create_iommu_opts =
>>>   	| IOMMU_NO_SHAREPT
>>> diff --git a/tools/ocaml/libs/xc/xenctrl.mli b/tools/ocaml/libs/xc/xenctrl.mli
>>> index 6900513e7f..0fdb0cc169 100644
>>> --- a/tools/ocaml/libs/xc/xenctrl.mli
>>> +++ b/tools/ocaml/libs/xc/xenctrl.mli
>>> @@ -63,6 +63,7 @@ type domain_create_flag =
>>>     | CDF_IOMMU
>>>     | CDF_NESTED_VIRT
>>>     | CDF_VPCI
>>> +  | CDF_VPMU
>>>     type domain_create_iommu_opts =
>>>     | IOMMU_NO_SHAREPT
>>> diff --git a/tools/xl/xl_parse.c b/tools/xl/xl_parse.c
>>> index 17dddb4cd5..c503b9be00 100644
>>> --- a/tools/xl/xl_parse.c
>>> +++ b/tools/xl/xl_parse.c
>>> @@ -2750,6 +2750,8 @@ skip_usbdev:
>>>                       "If it fixes an issue you are having please report to "
>>>                       "xen-devel@lists.xenproject.org.\n");
>>>   +    xlu_cfg_get_defbool(config, "vpmu", &b_info->vpmu, 0);
>>> +
>>>       xlu_cfg_destroy(config);
>>>   }
>>>   diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
>>> index aae4472479..2f988c790e 100644
>>> --- a/xen/arch/arm/domain.c
>>> +++ b/xen/arch/arm/domain.c
>>> @@ -276,6 +276,8 @@ static void ctxt_switch_to(struct vcpu *n)
>>>        * timer. The interrupt needs to be injected into the guest. */
>>>       WRITE_SYSREG(n->arch.cntkctl, CNTKCTL_EL1);
>>>       virt_timer_restore(n);
>>> +
>>> +    WRITE_SYSREG(n->arch.mdcr_el2, MDCR_EL2);
>>>   }
>>>     /* Update per-VCPU guest runstate shared memory area (if registered). */
>>> @@ -586,6 +588,10 @@ int arch_vcpu_create(struct vcpu *v)
>>>         v->arch.hcr_el2 = get_default_hcr_flags();
>>>   +    v->arch.mdcr_el2 = HDCR_TDRA | HDCR_TDOSA | HDCR_TDA;
>>> +    if ( !(v->domain->options & XEN_DOMCTL_CDF_vpmu) )
>>> +        v->arch.mdcr_el2 |= HDCR_TPM | HDCR_TPMCR;
>>> +
>>>       if ( (rc = vcpu_vgic_init(v)) != 0 )
>>>           goto fail;
>>>   @@ -622,9 +628,9 @@ int arch_sanitise_domain_config(struct xen_domctl_createdomain *config)
>>>   {
>>>       unsigned int max_vcpus;
>>>   -    /* HVM and HAP must be set. IOMMU and VPCI may or may not be */
>>> -    if ( (config->flags & ~XEN_DOMCTL_CDF_iommu & ~XEN_DOMCTL_CDF_vpci) !=
>>> -         (XEN_DOMCTL_CDF_hvm | XEN_DOMCTL_CDF_hap) )
>>> +    /* HVM and HAP must be set. IOMMU, VPCI and VPMU may or may not be */
>>> +    if ( (config->flags & ~(XEN_DOMCTL_CDF_iommu | XEN_DOMCTL_CDF_vpci |
>>> +          XEN_DOMCTL_CDF_vpmu)) != (XEN_DOMCTL_CDF_hvm | XEN_DOMCTL_CDF_hap) )
>>
>> The split here is not easy to read. Can you introduce two masks (one for the flags that must be set and the other optional) so we don't need to split the optional options over two lines.
>>
>>>       {
>>>           dprintk(XENLOG_INFO, "Unsupported configuration %#x\n",
>>>                   config->flags);
>>> diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
>>> index 49dc90d198..85386a765a 100644
>>> --- a/xen/arch/arm/setup.c
>>> +++ b/xen/arch/arm/setup.c
>>> @@ -108,6 +108,7 @@ static void __init processor_id(void)
>>>         identify_cpu(c);
>>>       current_cpu_data = *c;
>>> +    vpmu_is_available = cpu_has_pmu;
>>
>> This wants to be set after the ID registers are sanitized (i.e. after the secondary CPUs have been brought).
>>
>> Also, on Armv8, the features supported by 64-bit and 32-bit exception level are separate. I couldn't find anything in the Arm that suggest that if the PMU is implemented by the former, then the latter must be. Do you have the page in hand?
> 
> Chapter D7.1 at the end is saying this:
> 
> "The Performance Monitors Extension is common to AArch64 operation and AArch32 operation.”
> 
> Is this the justification you are looking for ?

Yes. Thank you!

Cheers,

-- 
Julien Grall


^ permalink raw reply	[flat|nested] 11+ messages in thread
end of thread, other threads:[~2021-10-12 15:29 UTC | newest]

Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-10-11  9:00 [PATCH v4 0/3] Expose PMU to the guests Michal Orzel
2021-10-11  9:00 ` [PATCH v4 1/3] xen+tools: Introduce XEN_SYSCTL_PHYSCAP_vpmu Michal Orzel
2021-10-11 11:02   ` Ian Jackson
2021-10-11  9:00 ` [PATCH v4 2/3] xen/arm: Check for PMU platform support Michal Orzel
2021-10-11 10:17   ` Julien Grall
2021-10-11 12:06     ` Michal Orzel
2021-10-11  9:00 ` [PATCH v4 3/3] xen: Expose the PMU to the guests Michal Orzel
2021-10-11 10:21   ` Julien Grall
2021-10-11 14:11     ` Bertrand Marquis
2021-10-12 15:29       ` Julien Grall
2021-10-11 20:09   ` Stefano Stabellini

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.