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

[Tamas K Lengyel <tamas@xxxxxxxxxxxxx>]

On Thu, Jul 21, 2022 at 6:19 AM Jan Beulich <jbeulich@xxxxxxxx> wrote:
>
> On 20.07.2022 20:47, Tamas K Lengyel wrote:
> > --- a/xen/arch/x86/mm/mem_sharing.c
> > +++ b/xen/arch/x86/mm/mem_sharing.c
> > @@ -1653,6 +1653,46 @@ 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);
>
> I would hope two of the four pointers could actually be constified.

I don't think so, we do modify both the vpmu and vcpu state as-needed
on both the parent and the child.

> > +    if ( !vpmu_are_all_set(d_vpmu, VPMU_INITIALIZED | 
> > VPMU_CONTEXT_ALLOCATED) )
> > +        return 0;
> > +    if ( vpmu_allocate_context(cd_vcpu) )
> > +        return -ENOMEM;
>
> The function supplies an error code - please use it rather than
> assuming it's always going to be -ENOMEM. Alternatively make the
> function return bool. (Ideally the hook functions themselves would
> be well-formed in this regard, but I realize that the Intel one is
> pre-existing in its present undesirable shape.)

Sure.

> > +    /*
> > +     * The VPMU subsystem only saves the context when the CPU does a 
> > context
> > +     * switch. Otherwise, the relevant MSRs are not saved on vmexit.
> > +     * We force a save here in case the parent CPU context is still loaded.
> > +     */
> > +    if ( vpmu_is_set(d_vpmu, VPMU_CONTEXT_LOADED) )
> > +    {
> > +        int pcpu = smp_processor_id();
>
> unsigned int please.
>
> > +        if ( d_vpmu->last_pcpu != pcpu )
> > +        {
> > +            on_selected_cpus(cpumask_of(d_vpmu->last_pcpu),
> > +                             vpmu_save_force, (void *)d_vcpu, 1);
>
> No need for the cast afaict.
>
> > +            vpmu_reset(d_vpmu, VPMU_CONTEXT_LOADED);
> > +        } else
>
> Nit: Style.

Sure, these were all pretty much copy-pasted but will fix them.

> > +            vpmu_save(d_vcpu);
> > +    }
> > +
> > +    if ( vpmu_is_set(d_vpmu, VPMU_RUNNING) )
> > +        vpmu_set(cd_vpmu, VPMU_RUNNING);
> > +
> > +    /* Make sure context gets (re-)loaded when scheduled next */
> > +    vpmu_reset(cd_vpmu, VPMU_CONTEXT_LOADED);
> > +
> > +    memcpy(cd_vpmu->context, d_vpmu->context, d_vpmu->context_size);
> > +    memcpy(cd_vpmu->priv_context, d_vpmu->priv_context, 
> > d_vpmu->priv_context_size);
>
> Nit: Long line.

Ack.

Thanks,
Tamas