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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 01.07.14 at 16:37, <boris.ostrovsky@xxxxxxxxxx> wrote:
> @@ -187,34 +194,68 @@ int vpmu_do_interrupt(struct cpu_user_regs *regs)
>          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(curr_vcpu->domain) )
> +        if ( !is_hvm_domain(current->domain) )

"current"? Not "curr_vcpu"?

>          {
> -            /*
> -             * 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 *)&vpmu->xenpmu_data->pmu.r.regs;
> -            XLAT_cpu_user_regs(cmp, gregs);
> +            /* Store appropriate registers in xenpmu_data */
> +            if ( is_pv_32bit_domain(curr_vcpu->domain) )

I.e. I wonder how the qualifiers above and here can depend on
different domains.

> +            {
> +                /*
> +                 * 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 *)&vpmu->xenpmu_data->pmu.r.regs;
> +                XLAT_cpu_user_regs(cmp, gregs);
> +
> +                /* Adjust RPL for kernel mode */
> +                if ( (cmp->cs & 3) == 1 )
> +                    cmp->cs &= ~3;
> +            }
> +            else if ( !is_hardware_domain(curr_vcpu->domain) &&
> +                      !is_idle_vcpu(curr_vcpu) )
> +            {
> +                /* 64-bit unprivileged 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));
> +
> +            if ( !is_pvh_domain(current->domain) )
> +            {
> +                if ( current->arch.flags & TF_kernel_mode )
> +                    v->arch.vpmu.xenpmu_data->pmu.r.regs.cs &= ~3;
> +            }
> +            else
> +            {
> +                struct segment_register seg_cs;
> +
> +                hvm_get_segment_register(current, x86_seg_cs, &seg_cs);
> +                v->arch.vpmu.xenpmu_data->pmu.r.regs.cs = seg_cs.sel;
> +            }

There's an ugly mixture of "v" (iirc a latched copy of "current") and
"current" in the whole if/else above.

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

And again here.

> --- a/xen/include/public/pmu.h
> +++ b/xen/include/public/pmu.h
> @@ -48,11 +48,14 @@ DEFINE_XEN_GUEST_HANDLE(xen_pmu_params_t);
>  
>  /* PMU modes:
>   * - XENPMU_MODE_OFF:   No PMU virtualization
> - * - XENPMU_MODE_ON:    Guests can profile themselves, dom0 profiles
> + * - XENPMU_MODE_SELF:  Guests can profile themselves, dom0 profiles
>   *                      itself and Xen
> + * - XENPMU_MODE_ALL:   Only dom0 has access to VPMU and it profiles
> + *                      everyone: itself, the hypervisor and the guests.
>   */
>  #define XENPMU_MODE_OFF           0
> -#define XENPMU_MODE_ON            (1<<0)
> +#define XENPMU_MODE_SELF          (1<<0)
> +#define XENPMU_MODE_ALL           (1<<1)

While I assume the whole series will go in at once, you should still
avoid incompatible public interface changes like this just in case it
won't. Plus, using the final name from the beginning would eliminate
a couple of hunks from this patch.

Jan

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