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

[Tamas K Lengyel <tamas@xxxxxxxxxxxxx>]

On Tue, Jul 19, 2022 at 2:23 PM Andrew Cooper <Andrew.Cooper3@xxxxxxxxxx> wrote:
>
> 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.

There is no AMD caller to this so I'll just make it conditional to
ensure it's non-NULL.

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

Sure.

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

True, I could drop the top level check here.

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

Yea, I wanted to avoid having to rework the currently backwards
meaning of the returned int values of vpmu functions. So that's why I
double check the allocation worked instead. If I do what you recommend
it would be the only vpmu function that doesn't return void and the
only callback that returns error codes instead of boolean
success/failure. I rather keep the code self-consistent in vpmu and
just live with this arguably kinda odd looking logic here.

Tamas