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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 08.08.14 at 18:55, <boris.ostrovsky@xxxxxxxxxx> wrote:
> --- a/xen/arch/x86/hvm/svm/svm.c
> +++ b/xen/arch/x86/hvm/svm/svm.c
> @@ -1158,7 +1158,9 @@ static int svm_vcpu_initialise(struct vcpu *v)
>          return rc;
>      }
>  
> -    vpmu_initialise(v);
> +    /* PVH's VPMU is initialized via hypercall */
> +    if ( is_hvm_domain(v->domain) )
> +        vpmu_initialise(v);
>  
>      svm_guest_osvw_init(v);
>  
> @@ -1167,7 +1169,9 @@ static int svm_vcpu_initialise(struct vcpu *v)
>  
>  static void svm_vcpu_destroy(struct vcpu *v)
>  {
> -    vpmu_destroy(v);
> +    /* PVH's VPMU destroyed via hypercall */
> +    if ( is_hvm_domain(v->domain) )
> +        vpmu_destroy(v);

While I can see that being the case for init, is this really correct for
destroy even for a misbehaving guest?

> --- a/xen/arch/x86/hvm/svm/vpmu.c
> +++ b/xen/arch/x86/hvm/svm/vpmu.c
> @@ -386,14 +386,21 @@ 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 )
> +    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) +
> +                             2 * sizeof(uint64_t) * AMD_MAX_COUNTERS);
> +        if ( !ctxt )
> +        {
> +            gdprintk(XENLOG_WARNING, "Insufficient memory for PMU, "
> +                " PMU feature is unavailable on domain %d vcpu %d.\n",
> +                v->vcpu_id, v->domain->domain_id);

If you already have to touch this, please make it sane: gdprintk()
already prints a (domain,vcpu) pair, and two of them are unlikely
to be useful here. Also this should make use of %pv.

> +            return -ENOMEM;
> +        }
> +    }
> +    else
> +    {
> +        ctxt = &v->arch.vpmu.xenpmu_data->pmu.c.amd;
>      }

Pointless braces.

> @@ -387,7 +399,7 @@ static int core2_vpmu_alloc_resource(struct vcpu *v)
>  
>      return 1;
>  
> -out_err:
> +out_err_hvm:
>      vmx_rm_msr(MSR_CORE_PERF_GLOBAL_CTRL, VMX_HOST_MSR);
>      vmx_rm_msr(MSR_CORE_PERF_GLOBAL_CTRL, VMX_GUEST_MSR);
>      release_pmu_ownship(PMU_OWNER_HVM);
> @@ -395,6 +407,7 @@ out_err:
>      xfree(core2_vpmu_cxt);
>      xfree(p);
>  
> +out_err:

I think I said this before: Please indent labels by at least one space.

> --- a/xen/arch/x86/hvm/vpmu.c
> +++ b/xen/arch/x86/hvm/vpmu.c
> @@ -22,10 +22,13 @@
>  #include <xen/sched.h>
>  #include <xen/xenoprof.h>
>  #include <xen/event.h>
> +#include <xen/softirq.h>
> +#include <xen/hypercall.h>

Leftover additions from an earlier version?

Jan


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