From mboxrd@z Thu Jan  1 00:00:00 1970
From: "Jan Beulich" <JBeulich@suse.com>
Subject: Re: [PATCH v18 12/16] x86/VPMU: Merge vpmu_rdmsr and
 vpmu_wrmsr
Date: Fri, 20 Feb 2015 14:49:01 +0000
Message-ID: <54E7576D020000780006216F@mail.emea.novell.com>
References: <1424125619-10851-1-git-send-email-boris.ostrovsky@oracle.com>
	<1424125619-10851-13-git-send-email-boris.ostrovsky@oracle.com>
Mime-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
Return-path: <xen-devel-bounces@lists.xen.org>
In-Reply-To: <1424125619-10851-13-git-send-email-boris.ostrovsky@oracle.com>
Content-Disposition: inline
List-Unsubscribe: <http://lists.xen.org/cgi-bin/mailman/options/xen-devel>,
	<mailto:xen-devel-request@lists.xen.org?subject=unsubscribe>
List-Post: <mailto:xen-devel@lists.xen.org>
List-Help: <mailto:xen-devel-request@lists.xen.org?subject=help>
List-Subscribe: <http://lists.xen.org/cgi-bin/mailman/listinfo/xen-devel>,
	<mailto:xen-devel-request@lists.xen.org?subject=subscribe>
Sender: xen-devel-bounces@lists.xen.org
Errors-To: xen-devel-bounces@lists.xen.org
To: Boris Ostrovsky <boris.ostrovsky@oracle.com>
Cc: kevin.tian@intel.com, suravee.suthikulpanit@amd.com, andrew.cooper3@citrix.com, tim@xen.org, dietmar.hahn@ts.fujitsu.com, xen-devel@lists.xen.org, Aravind.Gopalakrishnan@amd.com, jun.nakajima@intel.com, dgdegra@tycho.nsa.gov
List-Id: xen-devel@lists.xenproject.org

>>> On 16.02.15 at 23:26, <boris.ostrovsky@oracle.com> wrote:
> --- a/xen/arch/x86/hvm/vpmu.c
> +++ b/xen/arch/x86/hvm/vpmu.c
> @@ -100,65 +100,48 @@ void vpmu_lvtpc_update(uint32_t val)
>          apic_write(APIC_LVTPC, vpmu->hw_lapic_lvtpc);
>  }
>  
> -int vpmu_do_wrmsr(unsigned int msr, uint64_t msr_content, uint64_t supported)
> +int vpmu_do_msr(unsigned int msr, uint64_t *msr_content,
> +                uint64_t supported, bool_t is_write)
>  {
> -    struct vcpu *curr = current;
> +    struct vcpu *curr;
>      struct vpmu_struct *vpmu;
> +    struct arch_vpmu_ops *ops;

const

> +    int ret = 0;
>  
>      if ( !(vpmu_mode & (XENPMU_MODE_SELF | XENPMU_MODE_HV)) )
> -        return 0;
> +        goto nop;
>  
> +    curr = current;

This should remain the initializer of the variable.

>      vpmu = vcpu_vpmu(curr);
> -    if ( vpmu->arch_vpmu_ops && vpmu->arch_vpmu_ops->do_wrmsr )
> -    {
> -        int ret = vpmu->arch_vpmu_ops->do_wrmsr(msr, msr_content, supported);
> -
> -        /*
> -         * We may have received a PMU interrupt during WRMSR handling
> -         * and since do_wrmsr may load VPMU context we should save
> -         * (and unload) it again.
> -         */
> -        if ( !is_hvm_vcpu(curr) && vpmu->xenpmu_data &&
> -             (vpmu->xenpmu_data->pmu.pmu_flags & PMU_CACHED) )
> -        {
> -            vpmu_set(vpmu, VPMU_CONTEXT_SAVE);
> -            vpmu->arch_vpmu_ops->arch_vpmu_save(vpmu);
> -            vpmu_reset(vpmu, VPMU_CONTEXT_SAVE | VPMU_CONTEXT_LOADED);
> -        }
> -        return ret;
> -    }
> -
> -    return 0;
> -}
> -
> -int vpmu_do_rdmsr(unsigned int msr, uint64_t *msr_content)
> -{
> -    struct vcpu *curr = current;
> -    struct vpmu_struct *vpmu;
> +    ops = vpmu->arch_vpmu_ops;
> +    if ( !ops )
> +        goto nop;
> +
> +    if ( is_write && ops->do_wrmsr )
> +        ret = ops->do_wrmsr(msr, *msr_content, supported);
> +    else if ( !is_write && ops->do_rdmsr )
> +        ret = ops->do_rdmsr(msr, msr_content);
> +    else
> +        goto nop;
>  
> -    if ( !(vpmu_mode & (XENPMU_MODE_SELF | XENPMU_MODE_HV)) )
> +    /*
> +     * We may have received a PMU interrupt while handling MSR access
> +     * and since do_wr/rdmsr may load VPMU context we should save
> +     * (and unload) it again.
> +     */
> +    if ( !is_hvm_vcpu(curr) &&
> +         vpmu->xenpmu_data && (vpmu->xenpmu_data->pmu.pmu_flags & PMU_CACHED) )
>      {
> -        *msr_content = 0;
> -        return 0;
> +        vpmu_set(vpmu, VPMU_CONTEXT_SAVE);
> +        ops->arch_vpmu_save(vpmu);
> +        vpmu_reset(vpmu, VPMU_CONTEXT_SAVE | VPMU_CONTEXT_LOADED);
>      }
>  
> -    vpmu = vcpu_vpmu(curr);
> -    if ( vpmu->arch_vpmu_ops && vpmu->arch_vpmu_ops->do_rdmsr )
> -    {
> -        int ret = vpmu->arch_vpmu_ops->do_rdmsr(msr, msr_content);
> +    return ret;
>  
> -        if ( !is_hvm_vcpu(curr) && vpmu->xenpmu_data &&
> -             (vpmu->xenpmu_data->pmu.pmu_flags & PMU_CACHED) )
> -        {
> -            vpmu_set(vpmu, VPMU_CONTEXT_SAVE);
> -            vpmu->arch_vpmu_ops->arch_vpmu_save(vpmu);
> -            vpmu_reset(vpmu, VPMU_CONTEXT_SAVE | VPMU_CONTEXT_LOADED);
> -        }
> -        return ret;
> -    }
> -    else
> + nop:
> +    if ( !is_write )
>          *msr_content = 0;

This could easily be done at the beginning of the function, avoiding
the need for the (kind of odd) "nop" label.

> -
>      return 0;

Please don't delete blank lines before return statements.

With those changes
Acked-by: Jan Beulich <jbeulich@suse.com>

Jan