Re: [PATCH] xen/arm: Introduce pmu_access parameter

[Julien Grall <julien@xxxxxxx>]

On 02/09/2021 08:57, Bertrand Marquis wrote:
> Hi Julien,
Hi Bertrand,

> If I understand it right, you want a per guest parameter to be able to allow 
> PMU accesses.
> This would require:
> - to save/restore MDCR on context switch
> - introduce a new config parameter for guests (might or might not need a tool 
> change)
> - have a xen command line parameter to have a solution to Allow PMU for dom0 
> (or maybe a DTB one)
Yes.

> But this would NOT include:
> - interrupt support (only needed to get informed of overflow)
> - provide PMU virtualization (not even sure something like that could make much 
> sense)
I am guessing the following is also included here:

- provide a PMU node in the DTB for the domain.

Without those 3, I feel we are exposing an half backed PMU to the guest. 
But this would still be a good first step, so I would be OK if they are 
not implemented in the first shot.
> I am not saying that we will do that now but as I need to unblock this I could 
> evaluate the effort and see if it could be possible to do this in the future.
Well... Below the patch I wrote during my breakfast this morning. This 
has not been tested and miss some documentation.
From 690a92cffed82451dcbd8b966e8dee31c1dce5fc Mon Sep 17 00:00:00 2001
From: Julien Grall <jgrall@xxxxxxxxxx>
Date: Thu, 2 Sep 2021 08:46:12 +0000
Subject: [PATCH] xen/arm: Expose the PMU to the guest

There are requests to expose the PMU (even in a hackish/non-secure way)
to the guests. This is taking the first steps by adding a per-domain
flag to disable the PMU traps.

Long term, we will want to at least expose the PMU interrupts, device-tree
binding. For more use cases, we will also need to save/restore the
PMU context.

Signed-off-by: Julien Grall <jgrall@xxxxxxxxxx>
---
 tools/include/libxl.h            |  2 ++
 tools/libs/light/libxl_arm.c     |  3 +++
 tools/libs/light/libxl_types.idl |  2 ++
 tools/xl/xl_parse.c              |  3 +++
 xen/arch/arm/domain.c            | 10 ++++++++--
 xen/include/asm-arm/domain.h     |  1 +
 xen/include/public/domctl.h      |  4 ++++
 7 files changed, 23 insertions(+), 2 deletions(-)

diff --git a/tools/include/libxl.h b/tools/include/libxl.h
index b9ba16d69869..d3e795a38670 100644
--- a/tools/include/libxl.h
+++ b/tools/include/libxl.h
@@ -502,6 +502,8 @@
  */
 #define LIBXL_HAVE_X86_MSR_RELAXED 1

+#define LIBXL_HAVE_ARM_VPMU 1
+
 /*
  * libxl ABI compatibility
  *
diff --git a/tools/libs/light/libxl_arm.c b/tools/libs/light/libxl_arm.c
index e3140a6e0039..89865a90dd3e 100644
--- a/tools/libs/light/libxl_arm.c
+++ b/tools/libs/light/libxl_arm.c
@@ -29,6 +29,9 @@ int libxl__arch_domain_prepare_config(libxl__gc *gc,
     uint32_t vuart_irq;
     bool vuart_enabled = false;

+    if (d_config->b_info.arch.vpmu)
+        config->flags |= XEN_DOMCTL_CDF_PMU;
+
     /*
      * If pl011 vuart is enabled then increment the nr_spis to allow 
allocation
      * of SPI VIRQ for pl011.
diff --git a/tools/libs/light/libxl_types.idl 
b/tools/libs/light/libxl_types.idl
index 3f9fff653a4a..daf768cba568 100644
--- a/tools/libs/light/libxl_types.idl
+++ b/tools/libs/light/libxl_types.idl
@@ -644,6 +644,8 @@ libxl_domain_build_info = Struct("domain_build_info",[

     ("arch_arm", Struct(None, [("gic_version", libxl_gic_version),
                                ("vuart", libxl_vuart_type),
+                              # XXX: Can this be common?
+                               ("vpmu", boolean)
                               ])),
     ("arch_x86", Struct(None, [("msr_relaxed", libxl_defbool),
                               ])),
diff --git a/tools/xl/xl_parse.c b/tools/xl/xl_parse.c
index 17dddb4cd534..6e497cc0b67e 100644
--- a/tools/xl/xl_parse.c
+++ b/tools/xl/xl_parse.c
@@ -2729,6 +2729,9 @@ skip_usbdev:
         }
     }

+    /* XXX: This probably want to be common or #ifdef-ed */
+    xlu_cfg_get_defbool(config, "vpmu", &b_info->arch_arm.vpmu, 0);
+
     if (!xlu_cfg_get_string (config, "tee", &buf, 1)) {
         e = libxl_tee_type_from_string(buf, &b_info->tee);
         if (e) {
diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
index 19c756ac3d46..a0e2321008ab 100644
--- a/xen/arch/arm/domain.c
+++ b/xen/arch/arm/domain.c
@@ -276,6 +276,9 @@ 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);
+
+    /* XXX: Check the position and synchronization requirement */
+    WRITE_SYSREG(n->arch.mdcr_el2, MDCR_EL2);
 }

 /* Update per-VCPU guest runstate shared memory area (if registered). */
@@ -585,6 +588,9 @@ int arch_vcpu_create(struct vcpu *v)
     v->arch.vmpidr = MPIDR_SMP | vcpuid_to_vaffinity(v->vcpu_id);

     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_PMU) )
+        v->arch.mdcr_el2 |= HDCR_TPM|HDCR_TPMCR;

     if ( (rc = vcpu_vgic_init(v)) != 0 )
         goto fail;
@@ -622,8 +628,8 @@ int arch_sanitise_domain_config(struct 
xen_domctl_createdomain *config)
 {
     unsigned int max_vcpus;

-    /* HVM and HAP must be set. IOMMU may or may not be */
-    if ( (config->flags & ~XEN_DOMCTL_CDF_iommu) !=
+    /* HVM and HAP must be set. IOMMU and PMU may or may not be */
+    if ( (config->flags & ~(XEN_DOMCTL_CDF_iommu | XEN_DOMCTL_CDF_pmu)) !=
          (XEN_DOMCTL_CDF_hvm | XEN_DOMCTL_CDF_hap) )
     {
         dprintk(XENLOG_INFO, "Unsupported configuration %#x\n",
diff --git a/xen/include/asm-arm/domain.h b/xen/include/asm-arm/domain.h
index c9277b5c6d94..14e575288f77 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 96696e3842da..db9539ddd579 100644
--- a/xen/include/public/domctl.h
+++ b/xen/include/public/domctl.h
@@ -71,6 +71,10 @@ struct xen_domctl_createdomain {
 #define _XEN_DOMCTL_CDF_nested_virt   6
 #define XEN_DOMCTL_CDF_nested_virt    (1U << _XEN_DOMCTL_CDF_nested_virt)

+/* Should we expose the vPMU to the guest? */
+#define _XEN_DOMCTL_CDF_pmu           6
+#define XEN_DOMCTL_CDF_pmu            (1U<<_XEN_DOMCTL_CDF_pmu)
+
 /* Max XEN_DOMCTL_CDF_* constant.  Used for ABI checking. */
 #define XEN_DOMCTL_CDF_MAX XEN_DOMCTL_CDF_nested_virt

> In the meantime we will start maintaining an internal branch with patches 
> refused upstream as this is blocking us.
For the future, please consider a per-domain option from the beginning. 
This is not much extra effort (see the patch above) and would make the 
acceptance of a patch more likely.
Cheers,

--
Julien Grall