Re: [PATCH v2] x86/mem_sharing: support forks with active vPMU state

[Andrew Cooper <Andrew.Cooper3@xxxxxxxxxx>]

On 20/07/2022 19:47, Tamas K Lengyel wrote:
> diff --git a/xen/arch/x86/cpu/vpmu_amd.c b/xen/arch/x86/cpu/vpmu_amd.c
> index 9bacc02ec1..4c76e24551 100644
> --- a/xen/arch/x86/cpu/vpmu_amd.c
> +++ b/xen/arch/x86/cpu/vpmu_amd.c
> @@ -518,6 +518,14 @@ static int cf_check svm_vpmu_initialise(struct vcpu *v)
>      return 0;
>  }
>  
> +#ifdef CONFIG_MEM_SHARING
> +static int cf_check amd_allocate_context(struct vcpu *v)
> +{
> +    ASSERT_UNREACHABLE();

What makes this unreachable?

I know none of this is tested on AMD, but it is in principle reachable I
think.

I'd just leave this as return 0.  It will be slightly less rude to
whomever adds forking support on AMD.

> +    return 0;
> +}
> +#endif
> +
>  static const struct arch_vpmu_ops __initconst_cf_clobber amd_vpmu_ops = {
>      .initialise = svm_vpmu_initialise,
>      .do_wrmsr = amd_vpmu_do_wrmsr,
> @@ -527,6 +535,10 @@ static const struct arch_vpmu_ops __initconst_cf_clobber 
> amd_vpmu_ops = {
>      .arch_vpmu_save = amd_vpmu_save,
>      .arch_vpmu_load = amd_vpmu_load,
>      .arch_vpmu_dump = amd_vpmu_dump,
> +
> +#ifdef CONFIG_MEM_SHARING
> +    .allocate_context = amd_allocate_context

Trailing comma please, and in the Intel structure.

> +#endif
>  };
>  
>  static const struct arch_vpmu_ops *__init common_init(void)
> diff --git a/xen/arch/x86/cpu/vpmu_intel.c b/xen/arch/x86/cpu/vpmu_intel.c
> index 8612f46973..01d4296485 100644
> --- a/xen/arch/x86/cpu/vpmu_intel.c
> +++ b/xen/arch/x86/cpu/vpmu_intel.c
> @@ -282,10 +282,17 @@ static inline void __core2_vpmu_save(struct vcpu *v)
>      for ( i = 0; i < fixed_pmc_cnt; i++ )
>          rdmsrl(MSR_CORE_PERF_FIXED_CTR0 + i, fixed_counters[i]);
>      for ( i = 0; i < arch_pmc_cnt; i++ )
> +    {
>          rdmsrl(MSR_IA32_PERFCTR0 + i, xen_pmu_cntr_pair[i].counter);
> +        rdmsrl(MSR_P6_EVNTSEL(i), xen_pmu_cntr_pair[i].control);
> +    }
>  
>      if ( !is_hvm_vcpu(v) )
>          rdmsrl(MSR_CORE_PERF_GLOBAL_STATUS, core2_vpmu_cxt->global_status);
> +    /* Save MSR to private context to make it fork-friendly */
> +    else if ( mem_sharing_enabled(v->domain) )
> +        vmx_read_guest_msr(v, MSR_CORE_PERF_GLOBAL_CTRL,
> +                           &core2_vpmu_cxt->global_ctrl);

/sigh.  So we're also not using the VMCS perf controls either.

That wants fixing too, but isn't a task for now.

Everything else LGTM.

~Andrew