Re: [Xen-devel] [PATCH v24 12/15] x86/VPMU: Handle PMU interrupts for PV(H) guests

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 10.06.15 at 17:04, <boris.ostrovsky@xxxxxxxxxx> wrote:
> @@ -211,27 +214,65 @@ static inline void context_load(struct vcpu *v)
>      }
>  }
>  
> -static void amd_vpmu_load(struct vcpu *v)
> +static int amd_vpmu_load(struct vcpu *v, bool_t from_guest)
>  {
>      struct vpmu_struct *vpmu = vcpu_vpmu(v);
> -    struct xen_pmu_amd_ctxt *ctxt = vpmu->context;
> -    uint64_t *ctrl_regs = vpmu_reg_pointer(ctxt, ctrls);
> +    struct xen_pmu_amd_ctxt *ctxt;
> +    uint64_t *ctrl_regs;
> +    unsigned int i;
>  
>      vpmu_reset(vpmu, VPMU_FROZEN);
>  
> -    if ( vpmu_is_set(vpmu, VPMU_CONTEXT_LOADED) )
> +    if ( !from_guest && vpmu_is_set(vpmu, VPMU_CONTEXT_LOADED) )
>      {
> -        unsigned int i;
> +        ctxt = vpmu->context;
> +        ctrl_regs = vpmu_reg_pointer(ctxt, ctrls);
>  
>          for ( i = 0; i < num_counters; i++ )
>              wrmsrl(ctrls[i], ctrl_regs[i]);
>  
> -        return;
> +        return 0;
> +    }
> +
> +    if ( from_guest )

Generally I dislike such redundancy (
    if ( cond1 && cond2 )
        return;
    if ( !cond1 )
        ...
which can be written without checking cond1 twice) - do you really
think it is beneficial for overall readability to have it that way?

> +    {
> +        unsigned int num_enabled = 0;
> +        struct xen_pmu_amd_ctxt *guest_ctxt = &vpmu->xenpmu_data->pmu.c.amd;
> +
> +        ASSERT(!is_hvm_vcpu(v));
> +
> +        ctxt = vpmu->context;
> +        ctrl_regs = vpmu_reg_pointer(ctxt, ctrls);
> +
> +        memcpy(&ctxt->regs[0], &guest_ctxt->regs[0], regs_sz);

So that's the live structure, not any staging area iiuc. What state
is the guest going to be in when validation fails (and you can't
restore the original state)? And what guarantees that nothing
elsewhere in the hypervisor uses the data _before_ the
validation below succeeds?

> +        for ( i = 0; i < num_counters; i++ )
> +        {
> +            if ( (ctrl_regs[i] & CTRL_RSVD_MASK) != ctrl_rsvd[i] )
> +            {
> +                /*
> +                 * Not necessary to re-init context since we should never 
> load
> +                 * it until guest provides valid values. But just to be safe.
> +                 */
> +                amd_vpmu_init_regs(ctxt);
> +                return -EINVAL;
> +            }
> +
> +            if ( is_pmu_enabled(ctrl_regs[i]) )
> +                num_enabled++;
> +        }
> +
> +        if ( num_enabled )

Looks like a boolean flag would do - the exact count doesn't seem to
be of interest here or in later patches?

> @@ -246,22 +287,17 @@ static inline void context_save(struct vcpu *v)
>          rdmsrl(counters[i], counter_regs[i]);
>  }
>  
> -static int amd_vpmu_save(struct vcpu *v)
> +static int amd_vpmu_save(struct vcpu *v,  bool_t to_guest)
>  {
>      struct vpmu_struct *vpmu = vcpu_vpmu(v);
>      unsigned int i;
>  
> -    /*
> -     * Stop the counters. If we came here via vpmu_save_force (i.e.
> -     * when VPMU_CONTEXT_SAVE is set) counters are already stopped.
> -     */
> +    for ( i = 0; i < num_counters; i++ )
> +        wrmsrl(ctrls[i], 0);

Wouldn't it make sense to retain the first sentence of the comment?

> @@ -478,6 +523,13 @@ int svm_vpmu_initialise(struct vcpu *v)
>      vpmu->context = ctxt;
>      vpmu->priv_context = NULL;
>  
> +    if ( !is_hvm_vcpu(v) )
> +    {
> +        /* Copy register offsets to shared area */
> +        ASSERT(vpmu->xenpmu_data);
> +        memcpy(&vpmu->xenpmu_data->pmu.c.amd, ctxt, sizeof(*ctxt));

At the first glance the comment looks as if it wasn't in line with
the sizeof() used - offsetof() would be more obvious here (or a
file scope constant like you use on the Intel side).

> @@ -552,6 +738,30 @@ long do_xenpmu_op(unsigned int op, 
> XEN_GUEST_HANDLE_PARAM(xen_pmu_params_t) arg)
>          pvpmu_finish(current->domain, &pmu_params);
>          break;
>  
> +    case XENPMU_lvtpc_set:
> +        xenpmu_data = current->arch.vpmu.xenpmu_data;
> +        if ( xenpmu_data == NULL )
> +            return -EINVAL;
> +        vpmu_lvtpc_update(xenpmu_data->pmu.l.lapic_lvtpc);
> +        break;
> +
> +    case XENPMU_flush:
> +        curr = current;
> +        vpmu = vcpu_vpmu(curr);
> +        xenpmu_data = curr->arch.vpmu.xenpmu_data;
> +        if ( xenpmu_data == NULL )
> +            return -EINVAL;
> +        xenpmu_data->pmu.pmu_flags &= ~PMU_CACHED;
> +        vpmu_reset(vpmu, VPMU_CACHED);
> +        vpmu_lvtpc_update(xenpmu_data->pmu.l.lapic_lvtpc);
> +        if ( vpmu_load(curr, 1) )
> +        {
> +            xenpmu_data->pmu.pmu_flags |= PMU_CACHED;
> +            vpmu_set(vpmu, VPMU_CACHED);
> +            return -EIO;
> +        }
> +        break ;
> +
>      default:
>          ret = -EINVAL;

Considering how the default case gets handled, can't at least the 1st
and 3rd return-s above become "ret = -E...", avoiding needlessly
many return points in the function?

Jan

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