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

[Jan Beulich <jbeulich@xxxxxxxx>]

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.

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

> +    /*
> +     * 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.

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

Jan