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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 13.05.14 at 17:53, <boris.ostrovsky@xxxxxxxxxx> wrote:
> --- a/xen/arch/x86/hvm/vpmu.c
> +++ b/xen/arch/x86/hvm/vpmu.c
> @@ -76,7 +76,12 @@ void vpmu_lvtpc_update(uint32_t val)
>      struct vpmu_struct *vpmu = vcpu_vpmu(current);
>  
>      vpmu->hw_lapic_lvtpc = PMU_APIC_VECTOR | (val & APIC_LVT_MASKED);
> -    apic_write(APIC_LVTPC, vpmu->hw_lapic_lvtpc);
> +
> +    /* Postpone APIC updates for PV guests if PMU interrupt is pending */
> +    if ( !is_pv_domain(current->domain) ||
> +         !(current->arch.vpmu.xenpmu_data &&
> +           current->arch.vpmu.xenpmu_data->pmu_flags & PMU_CACHED) )

Please be consistent with parenthesization - compare this and ...

> @@ -87,7 +92,23 @@ int vpmu_do_wrmsr(unsigned int msr, uint64_t msr_content)
>          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(current->domain) &&
> +            (current->arch.vpmu.xenpmu_data->pmu_flags & PMU_CACHED) )

... this.

> @@ -99,14 +120,87 @@ int vpmu_do_rdmsr(unsigned int msr, uint64_t 
> *msr_content)
>          return 0;
>  
>      if ( vpmu->arch_vpmu_ops && vpmu->arch_vpmu_ops->do_rdmsr )
> -        return vpmu->arch_vpmu_ops->do_rdmsr(msr, msr_content);
> +    {
> +        int ret = vpmu->arch_vpmu_ops->do_rdmsr(msr, msr_content);
> +
> +        if ( !is_hvm_domain(current->domain) &&
> +            (current->arch.vpmu.xenpmu_data->pmu_flags & PMU_CACHED) )

Wouldn't the same comment as in WRMSR handling apply here too?
If so, either replicate it or add a brief comment referring to the
other one.

>  int vpmu_do_interrupt(struct cpu_user_regs *regs)
>  {
>      struct vcpu *v = current;
> -    struct vpmu_struct *vpmu = vcpu_vpmu(v);
> +    struct vpmu_struct *vpmu;
> +
> +    /* dom0 will handle interrupt for special domains (e.g. idle domain) */
> +    if ( v->domain->domain_id >= DOMID_FIRST_RESERVED )
> +        v = hardware_domain->vcpu[smp_processor_id() %
> +            hardware_domain->max_vcpus];

Please don't assume that all fields in the array are populated - there
are plenty of examples where pointers read from this array get
checked against NULL.

> +        /* Store appropriate registers in xenpmu_data */
> +        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();
> +
> +            cmp = (void *)&v->arch.vpmu.xenpmu_data->pmu.r.regs;
> +            XLAT_cpu_user_regs(cmp, gregs);
> +            memcpy(&v->arch.vpmu.xenpmu_data->pmu.r.regs,
> +                   &cmp, sizeof(struct compat_cpu_user_regs));

Afaict this memcpy() does nothing (src == dst).

Jan


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