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

[Andrew Cooper <Andrew.Cooper3@xxxxxxxxxx>]

On 19/07/2022 18:18, Tamas K Lengyel wrote:
> diff --git a/xen/arch/x86/cpu/vpmu.c b/xen/arch/x86/cpu/vpmu.c
> index d2c03a1104..2b5d64a60d 100644
> --- a/xen/arch/x86/cpu/vpmu.c
> +++ b/xen/arch/x86/cpu/vpmu.c
> @@ -529,6 +529,16 @@ void vpmu_initialise(struct vcpu *v)
>          put_vpmu(v);
>  }
>  
> +void vpmu_allocate_context(struct vcpu *v)
> +{
> +    struct vpmu_struct *vpmu = vcpu_vpmu(v);
> +
> +    if ( vpmu_is_set(vpmu, VPMU_CONTEXT_ALLOCATED) )
> +        return;
> +
> +    alternative_call(vpmu_ops.allocate_context, v);

You need to fill in an AMD pointer, or make this conditional.

All alternatives have NULL pointers turned into UDs.

Should be a two-liner on the AMD side.

> diff --git a/xen/arch/x86/cpu/vpmu_intel.c b/xen/arch/x86/cpu/vpmu_intel.c
> index 8612f46973..31dc0ee14b 100644
> --- a/xen/arch/x86/cpu/vpmu_intel.c
> +++ b/xen/arch/x86/cpu/vpmu_intel.c
>  static int core2_vpmu_verify(struct vcpu *v)
> @@ -474,7 +485,11 @@ static int core2_vpmu_alloc_resource(struct vcpu *v)
>                                      sizeof(uint64_t) * fixed_pmc_cnt;
>  
>      vpmu->context = core2_vpmu_cxt;
> +    vpmu->context_size = sizeof(struct xen_pmu_intel_ctxt) +
> +                         fixed_pmc_cnt * sizeof(uint64_t) +
> +                         arch_pmc_cnt * sizeof(struct xen_pmu_cntr_pair);

This wants deduplicating with the earlier calculation, surely?

>      vpmu->priv_context = p;
> +    vpmu->priv_context_size = sizeof(uint64_t);
>  
>      if ( !has_vlapic(v->domain) )
>      {
> @@ -882,6 +897,7 @@ static int cf_check vmx_vpmu_initialise(struct vcpu *v)
>  
>  static const struct arch_vpmu_ops __initconst_cf_clobber core2_vpmu_ops = {
>      .initialise = vmx_vpmu_initialise,
> +    .allocate_context = core2_vpmu_alloc_resource,

core2_vpmu_alloc_resource() needs to gain a cf_check to not explode on
TGL/SPR.

>      .do_wrmsr = core2_vpmu_do_wrmsr,
>      .do_rdmsr = core2_vpmu_do_rdmsr,
>      .do_interrupt = core2_vpmu_do_interrupt,
> diff --git a/xen/arch/x86/include/asm/vpmu.h b/xen/arch/x86/include/asm/vpmu.h
> index e5709bd44a..14d0939247 100644
> --- a/xen/arch/x86/include/asm/vpmu.h
> +++ b/xen/arch/x86/include/asm/vpmu.h
> @@ -106,8 +109,10 @@ void vpmu_lvtpc_update(uint32_t val);
>  int vpmu_do_msr(unsigned int msr, uint64_t *msr_content, bool is_write);
>  void vpmu_do_interrupt(struct cpu_user_regs *regs);
>  void vpmu_initialise(struct vcpu *v);
> +void vpmu_allocate_context(struct vcpu *v);
>  void vpmu_destroy(struct vcpu *v);
>  void vpmu_save(struct vcpu *v);
> +void vpmu_save_force(void *arg);

Needs the cf_check to compile.

>  int vpmu_load(struct vcpu *v, bool_t from_guest);
>  void vpmu_dump(struct vcpu *v);
>  
> diff --git a/xen/arch/x86/mm/mem_sharing.c b/xen/arch/x86/mm/mem_sharing.c
> index 8f9d9ed9a9..39cd03abf7 100644
> --- a/xen/arch/x86/mm/mem_sharing.c
> +++ b/xen/arch/x86/mm/mem_sharing.c
> @@ -1653,6 +1653,50 @@ static void copy_vcpu_nonreg_state(struct vcpu 
> *d_vcpu, struct vcpu *cd_vcpu)
>      hvm_set_nonreg_state(cd_vcpu, &nrs);
>  }
>  
> +static int copy_vpmu(struct vcpu *d_vcpu, struct vcpu *cd_vcpu)
> +{
> +    struct vpmu_struct *d_vpmu = vcpu_vpmu(d_vcpu);
> +    struct vpmu_struct *cd_vpmu = vcpu_vpmu(cd_vcpu);
> +
> +    if ( !vpmu_are_all_set(d_vpmu, VPMU_INITIALIZED | 
> VPMU_CONTEXT_ALLOCATED) )
> +        return 0;
> +    if ( !vpmu_is_set(cd_vpmu, VPMU_CONTEXT_ALLOCATED) )
> +    {
> +        vpmu_allocate_context(cd_vcpu);
> +        if ( !vpmu_is_set(cd_vpmu, VPMU_CONTEXT_ALLOCATED) )
> +            return -ENOMEM;

vpmu_allocate_context() already checks VPMU_CONTEXT_ALLOCATED.  But
isn't the double check here redundant?

The subsequent check looks like you want to pass the hook's return value
up through vpmu_allocate_context().

(And if you feel like turning it from bool-as-int to something more
sane, say -errno, that would also be great.)

Otherwise, looks plausible.

~Andrew