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

[Bertrand Marquis <Bertrand.Marquis@xxxxxxx>]

Hi Michal,

> On 8 Oct 2021, at 09:19, Michal Orzel <Michal.Orzel@xxxxxxx> 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>

Cheers
Bertrand

> ---
> 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      |  4 ++++
> 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                | 10 ++++++++--
> xen/arch/arm/setup.c                 |  1 +
> xen/common/domain.c                  |  9 ++++++++-
> xen/include/asm-arm/domain.h         |  1 +
> xen/include/public/domctl.h          |  4 +++-
> 14 files changed, 61 insertions(+), 4 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..d123c2f703 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));
> 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 6da3ed3c6f..addcf4cc59 100644
> --- a/tools/ocaml/libs/xc/xenctrl.ml
> +++ b/tools/ocaml/libs/xc/xenctrl.ml
> @@ -69,6 +69,7 @@ type domain_create_flag =
>       | CDF_XS_DOMAIN
>       | CDF_IOMMU
>       | CDF_NESTED_VIRT
> +     | 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 b8faff6721..0a5ce529e9 100644
> --- a/tools/ocaml/libs/xc/xenctrl.mli
> +++ b/tools/ocaml/libs/xc/xenctrl.mli
> @@ -62,6 +62,7 @@ type domain_create_flag =
>   | CDF_XS_DOMAIN
>   | CDF_IOMMU
>   | CDF_NESTED_VIRT
> +  | 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 19c756ac3d..2dd474da09 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,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_vpmu)) !=
>          (XEN_DOMCTL_CDF_hvm | XEN_DOMCTL_CDF_hap) )
>     {
>         dprintk(XENLOG_INFO, "Unsupported configuration %#x\n",
> 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 4d0e909eec..8b53c49d1e 100644
> --- a/xen/common/domain.c
> +++ b/xen/common/domain.c
> @@ -480,12 +480,13 @@ 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_nested_virt | XEN_DOMCTL_CDF_vpmu) )
>     {
>         dprintk(XENLOG_INFO, "Unknown CDF flags %#x\n", config->flags);
>         return -EINVAL;
> @@ -534,6 +535,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 96696e3842..238384b5ae 100644
> --- a/xen/include/public/domctl.h
> +++ b/xen/include/public/domctl.h
> @@ -70,9 +70,11 @@ struct xen_domctl_createdomain {
> #define XEN_DOMCTL_CDF_iommu          (1U<<_XEN_DOMCTL_CDF_iommu)
> #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_vpmu           (1U << 7)
> 
> /* Max XEN_DOMCTL_CDF_* constant.  Used for ABI checking. */
> -#define XEN_DOMCTL_CDF_MAX XEN_DOMCTL_CDF_nested_virt
> +#define XEN_DOMCTL_CDF_MAX XEN_DOMCTL_CDF_vpmu
> 
>     uint32_t flags;
> 
> -- 
> 2.29.0
> 
>