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
