Xen project Mailing List

Re: [PATCH v4 3/3] xen: Expose the PMU to the guests

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@xxxxxxx>
Signed-off-by: Julien Grall <julien@xxxxxxx>
Reviewed-by: Bertrand Marquis <bertrand.marquis@xxxxxxx>
---
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@xxxxxxxxxxxxxxxxxxxx.\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

©2013 Xen Project, A Linux Foundation Collaborative Project. All Rights Reserved.
Linux Foundation is a registered trademark of The Linux Foundation.
Xen Project is a trademark of The Linux Foundation.