From mboxrd@z Thu Jan  1 00:00:00 1970
From: Radim =?utf-8?B?S3LEjW3DocWZ?= <rkrcmar@redhat.com>
Subject: Re: [RFC] vPMU support for AMD system
Date: Thu, 2 Oct 2014 15:50:59 +0200
Message-ID: <20141002135058.GA28737@potion.redhat.com>
References: <542B61DA.3070307@redhat.com>
Mime-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Cc: gleb@kernel.org, pbonzini@redhat.com, KVM <kvm@vger.kernel.org>
To: Wei Huang <wei@redhat.com>
Return-path: <kvm-owner@vger.kernel.org>
Received: from mx1.redhat.com ([209.132.183.28]:21959 "EHLO mx1.redhat.com"
	rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP
	id S1752602AbaJBNvD (ORCPT <rfc822;kvm@vger.kernel.org>);
	Thu, 2 Oct 2014 09:51:03 -0400
Content-Disposition: inline
In-Reply-To: <542B61DA.3070307@redhat.com>
Sender: kvm-owner@vger.kernel.org
List-ID: <kvm.vger.kernel.org>

2014-09-30 21:07-0500, Wei Huang:
> Hi Paolo and Gleb,
> 2. CURRENT DESIGN
> The design is similar to existing vPMU support. Many data structures such as
> "struct kvm_pmc" and "struct kvm_pmu" are re-used for this design.
> 
> Original pmu.c code was designed with Intel PMU in mind. To avoid the
> confusion, the current design converts existing pmu.c into intel-pmu.c and
> create a similar code, named amd-pmu.c. PMU function calls in x86.c are then
> dispatched via function pointers (i.e. struct kvm_pmu_ops). The initial
> setup is done in kvm_pmu_arch_init().
> 
> I have other ideas listed below. Please review and give some suggestion:
> 
> (a) Merge intel-pmu.c and amd-pmu.c into pmu.c
> * PMU will probe the architecture first before first usage;
> * It will still use function pointers to dispatch global functions (e.g.
> kvm_pmu_reset);
> * For static functions that are the same (e.g. pmc_is_gp), we will use the
> same copy;
> * For static functions that are slightly different (e.g. global_idx_to_pmc),
> we will use a shared version with if-else statement, depending on Intel or
> AMD.

(if-else forest is daunting.)

> I think this approach is quite acceptable, except that there will be many
> if-else in the code. Not clean enough.
> 
> (b) Convert intel-pmu.c => vmx.c and convert amd-pmu.c => svm.c
> * PMU function pointers will be created in kvm_x86_ops;
> * The entry functions will be created inside vmx.c and svm.c respectively;

I would be nicer to keep them in separate files and link to vmx/svm.

> * There might be common functions defined in pmu.c.
> 
> This design is viable too. But to be honest, it is a bit messy compared with
> (a).

This makes sense as we will use only vmx+intel_pmu and svm+amd_pmu, so
we'd have less code loaded in both cases.

I consider design (c) strictly better than the current one (2):
  (c) keep {intel,amd}-pmu.c and introduce pmu.[hc] that joins duplicate
      functions and wraps kvm_pmu_ops

And if we decide to move VMX/SVM related code into their respective
modules, we won't have to change callers.