Re: [Xen-devel] [PATCH v19 07/14] x86/VPMU: Initialize PMU for PV(H) guests

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 17.03.15 at 15:54, <boris.ostrovsky@xxxxxxxxxx> wrote:
> @@ -263,14 +264,14 @@ void vpmu_initialise(struct vcpu *v)
>      BUILD_BUG_ON(sizeof(struct xen_pmu_intel_ctxt) > XENPMU_CTXT_PAD_SZ);
>      BUILD_BUG_ON(sizeof(struct xen_pmu_amd_ctxt) > XENPMU_CTXT_PAD_SZ);
>  
> -    if ( is_pvh_vcpu(v) )
> -        return;
> -
>      ASSERT(!vpmu->flags && !vpmu->context);
>  
> -    spin_lock(&vpmu_lock);
> -    vpmu_count++; /* Prevent vpmu_mode from changing until we are done */
> -    spin_unlock(&vpmu_lock);
> +    if ( v->domain != hardware_domain )
> +    {
> +        spin_lock(&vpmu_lock);
> +        vpmu_count++; /* Prevent vpmu_mode from changing until we are done */
> +        spin_unlock(&vpmu_lock);
> +    }

So why is this being made conditional here? A patch this size would
certainly benefit from a slightly more verbose description...

And then if the conditional is indeed needed - why don't you use
is_hardware_domain()?

> +static int pvpmu_init(struct domain *d, xen_pmu_params_t *params)
> +{
> +    struct vcpu *v;
> +    struct vpmu_struct *vpmu;
> +    struct page_info *page;
> +    uint64_t gfn = params->val;
> +
> +    if ( vpmu_mode == XENPMU_MODE_OFF )
> +        return -EINVAL;
> +
> +    if ( (params->vcpu >= d->max_vcpus) || (d->vcpu == NULL) ||
> +         (d->vcpu[params->vcpu] == NULL) )
> +        return -EINVAL;
> +
> +    page = get_page_from_gfn(d, gfn, NULL, P2M_ALLOC);
> +    if ( !page )
> +        return -EINVAL;
> +
> +    if ( !get_page_type(page, PGT_writable_page) )
> +    {
> +        put_page(page);
> +        return -EINVAL;
> +    }
> +
> +    v = d->vcpu[params->vcpu];
> +    vpmu = vcpu_vpmu(v);
> +
> +    spin_lock(&vpmu->vpmu_lock);
> +
> +    if ( v->arch.vpmu.xenpmu_data )
> +    {
> +        put_page_and_type(page);
> +        spin_unlock(&vpmu->vpmu_lock);

These two lines should be switched.

> +        return -EEXIST;
> +    }
> +
> +    v->arch.vpmu.xenpmu_data = __map_domain_page_global(page);
> +    if ( !v->arch.vpmu.xenpmu_data )
> +    {
> +        put_page_and_type(page);
> +        spin_unlock(&vpmu->vpmu_lock);

Same here.

> +static void pvpmu_finish(struct domain *d, xen_pmu_params_t *params)
> +{
> +    struct vcpu *v;
> +    struct vpmu_struct *vpmu;
> +    uint64_t mfn;
> +
> +    if ( (params->vcpu >= d->max_vcpus) || (d->vcpu == NULL) ||
> +         (d->vcpu[params->vcpu] == NULL) )
> +        return;
> +
> +    v = d->vcpu[params->vcpu];
> +    if ( v != current )
> +        vcpu_pause(v);
> +
> +    vpmu = vcpu_vpmu(v);
> +    spin_lock(&vpmu->vpmu_lock);
> +
> +    vpmu_destroy(v);
> +
> +    if ( v->arch.vpmu.xenpmu_data )
> +    {
> +        mfn = domain_page_map_to_mfn(v->arch.vpmu.xenpmu_data);
> +        ASSERT(mfn != 0);
> +        unmap_domain_page_global(v->arch.vpmu.xenpmu_data);
> +        put_page_and_type(mfn_to_page(mfn));
> +        v->arch.vpmu.xenpmu_data = NULL;
> +    }
> +
> +    spin_unlock(&vpmu->vpmu_lock);

Similarly here the put should be moved out of the locked region.

Jan


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