Re: [Xen-devel] [PATCH v6 15/19] x86/VPMU: Add privileged PMU mode

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 13.05.14 at 17:53, <boris.ostrovsky@xxxxxxxxxx> wrote:
> @@ -128,16 +130,23 @@ int vpmu_do_interrupt(struct cpu_user_regs *regs)
>      struct vcpu *v = current;
>      struct vpmu_struct *vpmu;
>  
> -    /* dom0 will handle interrupt for special domains (e.g. idle domain) */
> -    if ( v->domain->domain_id >= DOMID_FIRST_RESERVED )
> +    /*
> +     * dom0 will handle interrupt for special domains (e.g. idle domain) or,
> +     * in XENPMU_MODE_PRIV, for everyone.
> +     */
> +    if ( (vpmu_mode & XENPMU_MODE_PRIV) ||
> +         (v->domain->domain_id >= DOMID_FIRST_RESERVED) )
>          v = hardware_domain->vcpu[smp_processor_id() %
>              hardware_domain->max_vcpus];
>  
>      vpmu = vcpu_vpmu(v);
> -    if ( !is_hvm_domain(v->domain) )
> +    if ( !vpmu_is_set(vpmu, VPMU_CONTEXT_ALLOCATED) )
> +        return 0;
> +
> +    if ( !is_hvm_domain(v->domain)  || (vpmu_mode & XENPMU_MODE_PRIV) )
>      {
>          /* PV guest or dom0 is doing system profiling */
> -        const struct cpu_user_regs *gregs;
> +        struct cpu_user_regs *gregs;

Stray/unintended change?

> @@ -148,34 +157,62 @@ int vpmu_do_interrupt(struct cpu_user_regs *regs)
>          err = vpmu->arch_vpmu_ops->arch_vpmu_save(v);
>          vpmu_reset(vpmu, VPMU_CONTEXT_SAVE | VPMU_CONTEXT_LOADED);
>  
> -        /* Store appropriate registers in xenpmu_data */
> -        if ( is_pv_32bit_domain(current->domain) )
> +        if ( !is_hvm_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();
> +            /* Store appropriate registers in xenpmu_data */
> +            if ( is_pv_32bit_domain(current->domain) )
> +            {
> +                gregs = guest_cpu_user_regs();
> +
> +                if ( (vpmu_mode & XENPMU_MODE_PRIV) &&
> +                     !is_pv_32bit_domain(v->domain) )
> +                    memcpy(&v->arch.vpmu.xenpmu_data->pmu.r.regs,
> +                           gregs, sizeof(struct cpu_user_regs));
> +                else
> +                {
> +                    /*
> +                     * 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;
> +
> +                    cmp = (struct compat_cpu_user_regs *)
> +                        &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));
> +                }
> +            }
> +            else if ( !is_hardware_domain(current->domain) &&
> +                      !is_idle_vcpu(current) )
> +            {
> +                /* PV guest */

/* 64-bit PV guest */?

> +                gregs = guest_cpu_user_regs();
> +                memcpy(&v->arch.vpmu.xenpmu_data->pmu.r.regs,
> +                       gregs, sizeof(struct cpu_user_regs));
> +            }
> +            else
> +                memcpy(&v->arch.vpmu.xenpmu_data->pmu.r.regs,
> +                       regs, sizeof(struct 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));
> +            gregs = &v->arch.vpmu.xenpmu_data->pmu.r.regs;
> +            gregs->cs = (current->arch.flags & TF_kernel_mode) ? 0 : 0x3;

Ah, no - you want to modify the structure here. But you could do this
directly on the ->pmu.r.regs field rather than first latching the pointer.

And as said before, it doesn't really look correct to simply set ->cs to
just the RPL, especially without any comment explaining why this is
(a) being done and (b) correct.


>          }
> -        else if ( !is_hardware_domain(current->domain) &&
> -                 !is_idle_vcpu(current) )
> +        else
>          {
> -            /* PV guest */
> +            /* HVM guest */
> +            struct segment_register cs;
> +
>              gregs = guest_cpu_user_regs();
>              memcpy(&v->arch.vpmu.xenpmu_data->pmu.r.regs,
>                     gregs, sizeof(struct cpu_user_regs));
> +
> +            hvm_get_segment_register(current, x86_seg_cs, &cs);
> +            gregs = &v->arch.vpmu.xenpmu_data->pmu.r.regs;
> +            gregs->cs = cs.attr.fields.dpl;

Same here then obviously.

Jan

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