Re: [Xen-devel] [PATCH v3 12/16] x86/VPMU: Handle PMU interrupts for PV guests

["Jan Beulich" <JBeulich@xxxxxxxx>]

> @@ -82,7 +87,23 @@ int vpmu_do_wrmsr(unsigned int msr, uint64_t msr_content)
>      struct vpmu_struct *vpmu = vcpu_vpmu(current);
>  
>      if ( vpmu->arch_vpmu_ops && vpmu->arch_vpmu_ops->do_wrmsr )
> -        return vpmu->arch_vpmu_ops->do_wrmsr(msr, msr_content);
> +    {
> +        int val = vpmu->arch_vpmu_ops->do_wrmsr(msr, msr_content);

"val" is a misleading name here.

> +
> +        /*
> +         * 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(current->domain) &&
> +             current->arch.vpmu.xenpmu_data->pmu_flags & PMU_CACHED )

I'd suggest parenthesizing the operands of &.

> +        {
> +            vpmu_set(vpmu, VPMU_CONTEXT_SAVE);
> +            (void)vpmu->arch_vpmu_ops->arch_vpmu_save(current);

What's this cast good for?

> +            vpmu_reset(vpmu, VPMU_CONTEXT_SAVE | VPMU_CONTEXT_LOADED);
> +        }
> +        return val;
> +    }
>      return 0;
>  }
>  
> @@ -91,16 +112,87 @@ int vpmu_do_rdmsr(unsigned int msr, uint64_t 
> *msr_content)
>      struct vpmu_struct *vpmu = vcpu_vpmu(current);
>  
>      if ( vpmu->arch_vpmu_ops && vpmu->arch_vpmu_ops->do_rdmsr )
> -        return vpmu->arch_vpmu_ops->do_rdmsr(msr, msr_content);
> +    {
> +        int val = vpmu->arch_vpmu_ops->do_rdmsr(msr, msr_content);
> +
> +        if ( !is_hvm_domain(current->domain) &&
> +             current->arch.vpmu.xenpmu_data->pmu_flags)

Coding style (here and elsewhere).

> +    /* dom0 will handle this interrupt */
> +    if ( v->domain->domain_id >= DOMID_FIRST_RESERVED )
> +    {
> +        if ( smp_processor_id() >= dom0->max_vcpus )
> +            return 0;
> +        v = dom0->vcpu[smp_processor_id()];
> +    }

Ugly new uses of "dom0". And the correlation between
smp_processor_id() and dom0->max_vcpus doesn't look sane
either.

> +
> +    vpmu = vcpu_vpmu(v);
> +    if ( !is_hvm_domain(v->domain) )
> +    {
> +        /* PV guest or dom0 is doing system profiling */
> +        void *p;
> +        struct cpu_user_regs *gregs;

const.

> +        int err;
> +
> +        if (v->arch.vpmu.xenpmu_data->pmu_flags & PMU_CACHED)
> +            return 1;
> +
> +        /* PV guest will be reading PMU MSRs from xenpmu_data */
> +        vpmu_set(vpmu, VPMU_CONTEXT_SAVE | VPMU_CONTEXT_LOADED);
> +        err = vpmu->arch_vpmu_ops->arch_vpmu_save(v);
> +        vpmu_reset(vpmu, VPMU_CONTEXT_SAVE | VPMU_CONTEXT_LOADED);
> +
> +        /* Store appropriate registers in xenpmu_data */
> +        p = &v->arch.vpmu.xenpmu_data->pmu.regs;

This and the code below should be done in a type safe manner if
at all possible. I.e. p should not be void *, but a union of pointers
or a pointer to a union.

> +        if ( is_pv_32bit_domain(current->domain) )
> +        {
> +            /*
> +             * 32-bit dom0 cannot process Xen's addresses (which are 64 bit)
> +             * and therefore we treat it the same way as a non-priviledged
> +             * PV 32-bit domain.
> +             */
> +            struct compat_cpu_user_regs cmp;
> +
> +            gregs = guest_cpu_user_regs();
> +            XLAT_cpu_user_regs(&cmp, gregs);
> +            memcpy(p, &cmp, sizeof(struct compat_cpu_user_regs));

And then there would be no point in using an intermediate variable
here.

> +        }
> +        else if ( (current->domain != dom0) && !is_idle_vcpu(current) )

Did you perhaps mean !is_control_domain() or
!is_hardware_domain()?

Jan


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