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

[Bertrand Marquis <Bertrand.Marquis@xxxxxxx>]

Hi,

> On 2 Sep 2021, at 09:59, Julien Grall <julien@xxxxxxx> wrote:
> 
> 
> 
> 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.

Impressive but be careful not to put jam on your keyboard :-)

We are clearly not at your level of expertise and this would have taken us a 
lot more time, even if we tried without eating in parallel.

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

We wanted to share something we did internally which we thought could be useful 
for others.
We will be more careful in the future.

Cheers
Bertrand

> 
> Cheers,
> 
> -- 
> Julien Grall