Re: [PATCH 13/17] KVM: monolithic: x86: drop the kvm_pmu_ops structure

[Paolo Bonzini]

On 24/09/19 02:51, Andrea Arcangeli wrote:
> This was covered in the commit header of patch 2:

Oops, sorry.

> Lot more patches are needed to get rid of kvm_x86_ops entirely because
> there are lots of places checking the actual value of the method
> before making the indirect call. I tried to start that, but then it
> got into potentially heavily rejecting territory, so I thought it was
> simpler to start with what I had, considering from a performance
> standpoint it's optimal already as far as retpolines are concerned.

The performance may be good enough, but the maintainability is bad.
Let's make a list of function pointers that are checked, and function
pointers that are written at init time.

For the former, it should be possible to make them __weak symbols so
that they are NULL if undeclared.  For the latter, module parameters can
be made extern and then you can have checks like kvm_x86_has_...() in
inline functions in a header file.

Paolo

Re: [PATCH 13/17] KVM: monolithic: x86: drop the kvm_pmu_ops structure

[Andrea Arcangeli]

On Mon, Sep 23, 2019 at 12:21:43PM +0200, Paolo Bonzini wrote:
> On 20/09/19 23:25, Andrea Arcangeli wrote:
> > Cleanup after this was finally left fully unused.
> > 
> > Signed-off-by: Andrea Arcangeli 
> > ---
> >  arch/x86/include/asm/kvm_host.h |  3 ---
> >  arch/x86/kvm/pmu.h  | 19 ---
> >  arch/x86/kvm/pmu_amd.c  | 15 ---
> >  arch/x86/kvm/svm.c  |  1 -
> >  arch/x86/kvm/vmx/pmu_intel.c| 15 ---
> >  arch/x86/kvm/vmx/vmx.c  |  2 --
> >  6 files changed, 55 deletions(-)
> 
> Is there any reason not to do the same for kvm_x86_ops?

This was covered in the commit header of patch 2:

To reduce the rejecting parts while tracking upstream, this doesn't
attempt to entirely remove the kvm_x86_ops structure yet, that is
meant for a later cleanup. The pmu ops have been already cleaned up in
this patchset because it was left completely unused right after the
conversion from pointer to functions to external functions.

Lot more patches are needed to get rid of kvm_x86_ops entirely because
there are lots of places checking the actual value of the method
before making the indirect call. I tried to start that, but then it
got into potentially heavily rejecting territory, so I thought it was
simpler to start with what I had, considering from a performance
standpoint it's optimal already as far as retpolines are concerned.

> (As an aside, patch 2 is not copying over the comments in the struct
> kvm_x86_ops declarations.  Granted there aren't many, but we should not
> lose the few that exist).

Yes sorry, this was actually unintentional and the comment need to be
retained in the header declaration.

Thanks,
Andrea

Re: [PATCH 13/17] KVM: monolithic: x86: drop the kvm_pmu_ops structure

[Paolo Bonzini]

On 20/09/19 23:25, Andrea Arcangeli wrote:
> Cleanup after this was finally left fully unused.
> 
> Signed-off-by: Andrea Arcangeli 
> ---
>  arch/x86/include/asm/kvm_host.h |  3 ---
>  arch/x86/kvm/pmu.h  | 19 ---
>  arch/x86/kvm/pmu_amd.c  | 15 ---
>  arch/x86/kvm/svm.c  |  1 -
>  arch/x86/kvm/vmx/pmu_intel.c| 15 ---
>  arch/x86/kvm/vmx/vmx.c  |  2 --
>  6 files changed, 55 deletions(-)

Is there any reason not to do the same for kvm_x86_ops?

(As an aside, patch 2 is not copying over the comments in the struct
kvm_x86_ops declarations.  Granted there aren't many, but we should not
lose the few that exist).

Paolo

[PATCH 13/17] KVM: monolithic: x86: drop the kvm_pmu_ops structure

[Andrea Arcangeli]

Cleanup after this was finally left fully unused.

Signed-off-by: Andrea Arcangeli 
---
 arch/x86/include/asm/kvm_host.h |  3 ---
 arch/x86/kvm/pmu.h  | 19 ---
 arch/x86/kvm/pmu_amd.c  | 15 ---
 arch/x86/kvm/svm.c  |  1 -
 arch/x86/kvm/vmx/pmu_intel.c| 15 ---
 arch/x86/kvm/vmx/vmx.c  |  2 --
 6 files changed, 55 deletions(-)

diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
index bd5f4f900288..000d4e5a5664 100644
--- a/arch/x86/include/asm/kvm_host.h
+++ b/arch/x86/include/asm/kvm_host.h
@@ -1160,9 +1160,6 @@ struct kvm_x86_ops {
   gfn_t offset, unsigned long mask);
int (*write_log_dirty)(struct kvm_vcpu *vcpu);
 
-   /* pmu operations of sub-arch */
-   const struct kvm_pmu_ops *pmu_ops;
-
/*
 * Architecture specific hooks for vCPU blocking due to
 * HLT instruction.
diff --git a/arch/x86/kvm/pmu.h b/arch/x86/kvm/pmu.h
index 09e80e8ee21a..513366c8b794 100644
--- a/arch/x86/kvm/pmu.h
+++ b/arch/x86/kvm/pmu.h
@@ -21,23 +21,6 @@ struct kvm_event_hw_type_mapping {
 
 #include "pmu_ops.h"
 
-struct kvm_pmu_ops {
-   unsigned (*find_arch_event)(struct kvm_pmu *pmu, u8 event_select,
-   u8 unit_mask);
-   unsigned (*find_fixed_event)(int idx);
-   bool (*pmc_is_enabled)(struct kvm_pmc *pmc);
-   struct kvm_pmc *(*pmc_idx_to_pmc)(struct kvm_pmu *pmu, int pmc_idx);
-   struct kvm_pmc *(*msr_idx_to_pmc)(struct kvm_vcpu *vcpu, unsigned idx,
- u64 *mask);
-   int (*is_valid_msr_idx)(struct kvm_vcpu *vcpu, unsigned idx);
-   bool (*is_valid_msr)(struct kvm_vcpu *vcpu, u32 msr);
-   int (*get_msr)(struct kvm_vcpu *vcpu, u32 msr, u64 *data);
-   int (*set_msr)(struct kvm_vcpu *vcpu, struct msr_data *msr_info);
-   void (*refresh)(struct kvm_vcpu *vcpu);
-   void (*init)(struct kvm_vcpu *vcpu);
-   void (*reset)(struct kvm_vcpu *vcpu);
-};
-
 static inline u64 pmc_bitmask(struct kvm_pmc *pmc)
 {
struct kvm_pmu *pmu = pmc_to_pmu(pmc);
@@ -124,6 +107,4 @@ int kvm_vm_ioctl_set_pmu_event_filter(struct kvm *kvm, void 
__user *argp);
 
 bool is_vmware_backdoor_pmc(u32 pmc_idx);
 
-extern struct kvm_pmu_ops intel_pmu_ops;
-extern struct kvm_pmu_ops amd_pmu_ops;
 #endif /* __KVM_X86_PMU_H */
diff --git a/arch/x86/kvm/pmu_amd.c b/arch/x86/kvm/pmu_amd.c
index 12d1fa3ba35a..0e01b48b3b0b 100644
--- a/arch/x86/kvm/pmu_amd.c
+++ b/arch/x86/kvm/pmu_amd.c
@@ -302,18 +302,3 @@ static void amd_pmu_reset(struct kvm_vcpu *vcpu)
 }
 
 #include "pmu_amd_ops.c"
-
-struct kvm_pmu_ops amd_pmu_ops = {
-   .find_arch_event = amd_find_arch_event,
-   .find_fixed_event = amd_find_fixed_event,
-   .pmc_is_enabled = amd_pmc_is_enabled,
-   .pmc_idx_to_pmc = amd_pmc_idx_to_pmc,
-   .msr_idx_to_pmc = amd_msr_idx_to_pmc,
-   .is_valid_msr_idx = amd_is_valid_msr_idx,
-   .is_valid_msr = amd_is_valid_msr,
-   .get_msr = amd_pmu_get_msr,
-   .set_msr = amd_pmu_set_msr,
-   .refresh = amd_pmu_refresh,
-   .init = amd_pmu_init,
-   .reset = amd_pmu_reset,
-};
diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
index 5a48beb58083..ca9fd1762519 100644
--- a/arch/x86/kvm/svm.c
+++ b/arch/x86/kvm/svm.c
@@ -7298,7 +7298,6 @@ static struct kvm_x86_ops svm_x86_ops __ro_after_init = {
 
.sched_in = svm_sched_in,
 
-   .pmu_ops = _pmu_ops,
.deliver_posted_interrupt = svm_deliver_avic_intr,
.dy_apicv_has_pending_interrupt = svm_dy_apicv_has_pending_interrupt,
.update_pi_irte = svm_update_pi_irte,
diff --git a/arch/x86/kvm/vmx/pmu_intel.c b/arch/x86/kvm/vmx/pmu_intel.c
index bfa765842772..e7c1253f47a2 100644
--- a/arch/x86/kvm/vmx/pmu_intel.c
+++ b/arch/x86/kvm/vmx/pmu_intel.c
@@ -359,18 +359,3 @@ static void intel_pmu_reset(struct kvm_vcpu *vcpu)
 }
 
 #include "pmu_intel_ops.c"
-
-struct kvm_pmu_ops intel_pmu_ops = {
-   .find_arch_event = intel_find_arch_event,
-   .find_fixed_event = intel_find_fixed_event,
-   .pmc_is_enabled = intel_pmc_is_enabled,
-   .pmc_idx_to_pmc = intel_pmc_idx_to_pmc,
-   .msr_idx_to_pmc = intel_msr_idx_to_pmc,
-   .is_valid_msr_idx = intel_is_valid_msr_idx,
-   .is_valid_msr = intel_is_valid_msr,
-   .get_msr = intel_pmu_get_msr,
-   .set_msr = intel_pmu_set_msr,
-   .refresh = intel_pmu_refresh,
-   .init = intel_pmu_init,
-   .reset = intel_pmu_reset,
-};
diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
index 09e6a477e06f..ff46008dc514 100644
--- a/arch/x86/kvm/vmx/vmx.c
+++ b/arch/x86/kvm/vmx/vmx.c
@@ -7779,8 +7779,6 @@ static struct kvm_x86_ops vmx_x86_ops __ro_after_init = {
.pre_block = vmx_pre_block,
.post_block = vmx_post_block,
 
-   .pmu_ops = _pmu_ops,
-
.update_pi_irte = vmx_update_pi_irte,
 
 #ifdef CONFIG_X86_64