Re: [Xen-devel] [PATCH v8 14/19] x86/VPMU: Handle PMU interrupts for PV guests

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 01.07.14 at 16:37, <boris.ostrovsky@xxxxxxxxxx> wrote:
> --- a/xen/arch/x86/hvm/vpmu.c
> +++ b/xen/arch/x86/hvm/vpmu.c
> @@ -80,40 +80,169 @@ static void __init parse_vpmu_param(char *s)
>  
>  void vpmu_lvtpc_update(uint32_t val)
>  {
> -    struct vpmu_struct *vpmu = vcpu_vpmu(current);
> +    struct vcpu *v = current;

"curr" please (here and wherever else).

>  int vpmu_do_wrmsr(unsigned int msr, uint64_t msr_content)
>  {
> -    struct vpmu_struct *vpmu = vcpu_vpmu(current);
> +    struct vcpu *v = current;
> +    struct vpmu_struct *vpmu = vcpu_vpmu(v);
>  
>      if ( !(vpmu_mode & XENPMU_MODE_ON) )
>          return 0;
>  
>      if ( vpmu->arch_vpmu_ops && vpmu->arch_vpmu_ops->do_wrmsr )
> -        return vpmu->arch_vpmu_ops->do_wrmsr(msr, msr_content);
> +    {
> +        int ret = vpmu->arch_vpmu_ops->do_wrmsr(msr, msr_content);
> +
> +        /*
> +         * We may have received a PMU interrupt during WRMSR handling
> +         * and since do_wrmsr may load VPMU context we should save
> +         * (and unload) it again.
> +         */
> +        if ( !is_hvm_domain(v->domain) &&
> +             (vpmu->xenpmu_data &&
> +              (vpmu->xenpmu_data->pmu_flags & PMU_CACHED)) )

Chains of && don't need parentheses to separate one from the other.

>  int vpmu_do_interrupt(struct cpu_user_regs *regs)
>  {
> -    struct vcpu *v = current;
> -    struct vpmu_struct *vpmu = vcpu_vpmu(v);
> +    struct vcpu *curr_vcpu, *v = current;

Here the naming is particularly relevant: "curr_vcpu" really doesn't
appear to mean anything "current", it's just the subject vCPU. I.e.
that one would be "v", and what currently is "v" ought to be "curr".

> +        else if ( !is_hardware_domain(curr_vcpu->domain) &&
> +                  !is_idle_vcpu(curr_vcpu) )
> +        {
> +            /* PV(H) guest */
> +            gregs = guest_cpu_user_regs();
> +            memcpy(&vpmu->xenpmu_data->pmu.r.regs,
> +                   gregs, sizeof(struct cpu_user_regs));
> +        }
> +        else
> +            memcpy(&vpmu->xenpmu_data->pmu.r.regs,
> +                   regs, sizeof(struct cpu_user_regs));

Can neither of these two memcpy()s be (type safe) assignments
instead?

> @@ -523,6 +654,12 @@ long do_xenpmu_op(int op, 
> XEN_GUEST_HANDLE_PARAM(xen_pmu_params_t) arg)
>          vpmu_lvtpc_update(current->arch.vpmu.xenpmu_data->pmu.l.lapic_lvtpc);
>          ret = 0;
>          break;
> +    case XENPMU_flush:
> +        current->arch.vpmu.xenpmu_data->pmu_flags &= ~PMU_CACHED;
> +        vpmu_lvtpc_update(current->arch.vpmu.xenpmu_data->pmu.l.lapic_lvtpc);
> +        vpmu_load(current);

Worth having a "curr" local variable in this function?

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel