Re: [PATCH 2/3] x86/vPMU: invoke <vendor>_vpmu_initialise() through a hook as well

[Andrew Cooper <amc96@xxxxxxxx>]

On 29/11/2021 09:10, Jan Beulich wrote:
> I see little point in having an open-coded switch() statement to achieve
> the same; like other vendor-specific operations the function can be
> supplied in the respective ops structure instances.
>
> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>

Yeah, that logic was just bizarre.

Now that the ARM folks have added XEN_DOMCTL_CDF_vpmu, there is a huge
quantity of simplification which can be done around
vpmu_available()/etc, but that's definitely future work.

I think it would probably be good to get a position where we can enable
hwdom vpmu by default.  There is a spec on how to share perfcounters
with firmware, which we ought to investigate to let the watchdog
coexists beside vPMU.

> --- a/xen/arch/x86/cpu/vpmu.c
> +++ b/xen/arch/x86/cpu/vpmu.c
> @@ -480,12 +470,17 @@ static int vpmu_arch_initialise(struct v
>          return -EINVAL;
>      }
>  
> -    vpmu->hw_lapic_lvtpc = PMU_APIC_VECTOR | APIC_LVT_MASKED;
> -
> +    ret = alternative_call(vpmu_ops.initialise, v);
>      if ( ret )
> +    {
>          printk(XENLOG_G_WARNING "VPMU: Initialization failed for %pv\n", v);
> +        return ret;
> +    }
> +
> +    vpmu->hw_lapic_lvtpc = PMU_APIC_VECTOR | APIC_LVT_MASKED;
> +    vpmu_set(vpmu, VPMU_INITIALIZED);

It occurs to me that if, in previous patch, you do:

    if ( ret )
        printk(XENLOG_G_WARNING "VPMU: Initialization failed for %pv\n", v);
+    else
+        vpmu_set(vpmu, VPMU_INITIALIZED);

then you don't need to undo the adjustments in
{svm,vmx}_vpmu_initialise() in this patch.

>  
> -    return ret;
> +    return 0;
>  }
>  
>  static void get_vpmu(struct vcpu *v)
> --- a/xen/arch/x86/cpu/vpmu_amd.c
> +++ b/xen/arch/x86/cpu/vpmu_amd.c
> @@ -529,11 +516,22 @@ int svm_vpmu_initialise(struct vcpu *v)
>                 offsetof(struct xen_pmu_amd_ctxt, regs));
>      }
>  
> -    vpmu_set(vpmu, VPMU_INITIALIZED | VPMU_CONTEXT_ALLOCATED);
> +    vpmu_set(vpmu, VPMU_CONTEXT_ALLOCATED);
>  
>      return 0;
>  }
>  
> +static const struct arch_vpmu_ops __initconstrel amd_vpmu_ops = {
> +    .initialise = svm_vpmu_initialise,
> +    .do_wrmsr = amd_vpmu_do_wrmsr,
> +    .do_rdmsr = amd_vpmu_do_rdmsr,
> +    .do_interrupt = amd_vpmu_do_interrupt,
> +    .arch_vpmu_destroy = amd_vpmu_destroy,
> +    .arch_vpmu_save = amd_vpmu_save,
> +    .arch_vpmu_load = amd_vpmu_load,
> +    .arch_vpmu_dump = amd_vpmu_dump

As you're moving everything, trailing comma please.

> --- a/xen/arch/x86/cpu/vpmu_intel.c
> +++ b/xen/arch/x86/cpu/vpmu_intel.c
> @@ -893,11 +880,20 @@ int vmx_vpmu_initialise(struct vcpu *v)
>      if ( is_pv_vcpu(v) && !core2_vpmu_alloc_resource(v) )
>          return -EIO;
>  
> -    vpmu_set(vpmu, VPMU_INITIALIZED);
> -
>      return 0;
>  }
>  
> +static const struct arch_vpmu_ops __initconstrel core2_vpmu_ops = {
> +    .initialise = vmx_vpmu_initialise,
> +    .do_wrmsr = core2_vpmu_do_wrmsr,
> +    .do_rdmsr = core2_vpmu_do_rdmsr,
> +    .do_interrupt = core2_vpmu_do_interrupt,
> +    .arch_vpmu_destroy = core2_vpmu_destroy,
> +    .arch_vpmu_save = core2_vpmu_save,
> +    .arch_vpmu_load = core2_vpmu_load,
> +    .arch_vpmu_dump = core2_vpmu_dump

And here.

Reviewed-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>, although
preferably with the VPMU_INITIALIZED adjustment.

~Andrew