Re: [Xen-devel] [PATCH v12 for-xen-4.5 12/20] x86/VPMU: Initialize PMU for PV(H) guests

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 25.09.14 at 21:28, <boris.ostrovsky@xxxxxxxxxx> wrote:
> @@ -389,14 +390,26 @@ static int amd_vpmu_initialise(struct vcpu *v)
>        }
>      }
>  
> -    ctxt = xzalloc_bytes(sizeof(struct xen_pmu_amd_ctxt) +
> -                         2 * sizeof(uint64_t) * AMD_MAX_COUNTERS);
> -    if ( !ctxt )
> +    regs_size = 2 * sizeof(uint64_t) * AMD_MAX_COUNTERS;
> +    if ( is_hvm_domain(v->domain) )
>      {
> -        gdprintk(XENLOG_WARNING, "Insufficient memory for PMU, "
> -            " PMU feature is unavailable on domain %d vcpu %d.\n",
> -            v->vcpu_id, v->domain->domain_id);
> -        return -ENOMEM;
> +        ctxt = xzalloc_bytes(sizeof(struct xen_pmu_amd_ctxt) + regs_size);
> +        if ( !ctxt )
> +        {
> +            gdprintk(XENLOG_WARNING, "Insufficient memory for PMU, "
> +                "PMU feature is unavailable\n");
> +            return -ENOMEM;
> +        }
> +    }
> +    else
> +    {
> +        if ( sizeof(struct xen_pmu_data) + regs_size > PAGE_SIZE )

This is a compile time constant condition - no reason to issue a
message and return failure at runtime, just BUILD_BUG_ON() instead.

> --- a/xen/arch/x86/hvm/vmx/vpmu_core2.c
> +++ b/xen/arch/x86/hvm/vmx/vpmu_core2.c
> @@ -356,25 +356,45 @@ static int core2_vpmu_alloc_resource(struct vcpu *v)
>      struct vpmu_struct *vpmu = vcpu_vpmu(v);
>      struct xen_pmu_intel_ctxt *core2_vpmu_cxt = NULL;
>      uint64_t *p = NULL;
> +    unsigned int regs_size;
>  
> -    if ( !acquire_pmu_ownership(PMU_OWNER_HVM) )
> -        return 0;
> -
> -    wrmsrl(MSR_CORE_PERF_GLOBAL_CTRL, 0);
> -    if ( vmx_add_host_load_msr(MSR_CORE_PERF_GLOBAL_CTRL) )
> +    p = xzalloc_bytes(sizeof(uint64_t));
> +    if ( !p )
>          goto out_err;
>  
> -    if ( vmx_add_guest_msr(MSR_CORE_PERF_GLOBAL_CTRL) )
> -        goto out_err;
> -    vmx_write_guest_msr(MSR_CORE_PERF_GLOBAL_CTRL, 0);
> -
> -    core2_vpmu_cxt = xzalloc_bytes(sizeof(struct xen_pmu_intel_ctxt) +
> -                                   sizeof(uint64_t) * fixed_pmc_cnt +
> -                                   sizeof(struct xen_pmu_cntr_pair) *
> -                                   arch_pmc_cnt);
> -    p = xzalloc(uint64_t);
> -    if ( !core2_vpmu_cxt || !p )
> -        goto out_err;
> +    if ( has_hvm_container_domain(v->domain) )
> +    {
> +        if ( is_hvm_domain(v->domain) && 
> !acquire_pmu_ownership(PMU_OWNER_HVM) )
> +            goto out_err;
> +
> +        wrmsrl(MSR_CORE_PERF_GLOBAL_CTRL, 0);
> +        if ( vmx_add_host_load_msr(MSR_CORE_PERF_GLOBAL_CTRL) )
> +            goto out_err_hvm;
> +        if ( vmx_add_guest_msr(MSR_CORE_PERF_GLOBAL_CTRL) )
> +            goto out_err_hvm;
> +        vmx_write_guest_msr(MSR_CORE_PERF_GLOBAL_CTRL, 0);
> +    }
> +
> +    regs_size = sizeof(uint64_t) * fixed_pmc_cnt +
> +                sizeof(struct xen_pmu_cntr_pair) * arch_pmc_cnt;
> +    if ( is_hvm_domain(v->domain) )
> +    {
> +        core2_vpmu_cxt = xzalloc_bytes(sizeof(struct xen_pmu_intel_ctxt) +
> +                                       regs_size);
> +        if ( !core2_vpmu_cxt )
> +            goto out_err_hvm;
> +    }
> +    else
> +    {
> +        if ( sizeof(struct xen_pmu_data) + regs_size > PAGE_SIZE )
> +        {
> +            printk(XENLOG_WARNING

XENLOG_G_WARNING

Also, with the constituents of regs_size not changing at runtime,
issuing the message just once (and then perhaps in some __init
function) would seem the better approach.

> +static int pvpmu_init(struct domain *d, xen_pmu_params_t *params)
> +{
> +    struct vcpu *v;
> +    struct page_info *page;
> +    uint64_t gfn = params->val;
> +
> +    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];
> +    v->arch.vpmu.xenpmu_data = __map_domain_page_global(page);
> +    if ( !v->arch.vpmu.xenpmu_data )
> +    {
> +        put_page_and_type(page);
> +        return -EINVAL;
> +    }
> +
> +    vpmu_initialise(v);
> +
> +    return 0;
> +}
> +
> +static void pvpmu_finish(struct domain *d, xen_pmu_params_t *params)
> +{
> +    struct vcpu *v;
> +    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);
> +
> +    if ( v->arch.vpmu.xenpmu_data )
> +    {
> +        mfn = domain_page_map_to_mfn(v->arch.vpmu.xenpmu_data);
> +        if ( mfn_valid(mfn) )

Isn't this a must knowing that v->arch.vpmu.xenpmu_data is not
NULL? I.e. ASSERT()?

Jan

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