Re: [PATCH] KVM:Enable Perfmon Support for Guest in PowerPC

Re: [PATCH] KVM:Enable Perfmon Support for Guest in PowerPC

[Scott Wood]

On Tue, 2014-09-16 at 07:41 -0500, Tomar Amit-B51888 wrote:
> Below set of patches would enable Performance Monitor for kvm guest.
> 
>  

Please read Documentation/SubmittingPatches, look at patches other
people have submitted, and (As Bharat pointed out) use git to generate
and send the patch.  As is, the patch is whitespace-mangled, lacks a
Signed-off-by: line, lacks a good patch description, and has extraneous
lines (the "#######" stuff) between files of the diff.

You should also mention limitations, such as not handling perfmon
interrupts at all.


> +/* PMRN is in the same location as SPRN  PMR(11:20)=SPR(11:20) */
> 
> +static inline unsigned int get_pmrn(u32 inst)
> 
> +{
> 
> +        return ((inst >> 16) & 0x1f) | ((inst >> 6) & 0x3e0);
> 
> +}

Why not just reuse get_sprn()?

>  static inline unsigned int get_dcrn(u32 inst)
> 
> {
> 
>  
> 
>  
> 
> ###########################################################################################
> 
> --- a/arch/powerpc/include/uapi/asm/kvm_para.h
> 
> +++ b/arch/powerpc/include/uapi/asm/kvm_para.h
> 
> @@ -56,7 +56,14 @@ struct kvm_vcpu_arch_shared {
> 
>         __u32 mas6;
> 
>        __u32 esr;
> 
>         __u32 pir;
> 
> -
> 
> +        __u32 pmc0;
> 
> +        __u32 pmc1;
> 
> +        __u32 pmc2;
> 
> +        __u32 pmc3;
> 
> +        __u32 upmc0;
> 
> +        __u32 upmc1;
> 
> +        __u32 pmgc0;

Why are you putting this in the shared page?

Plus, this is userspace ABI so you can't go adding things in the middle
of the struct, or without advertising the feature.

> diff --git a/arch/powerpc/kvm/emulate.c b/arch/powerpc/kvm/emulate.c
> 
> index e96b50d..81c22bc 100644
> 
> --- a/arch/powerpc/kvm/emulate.c
> 
> +++ b/arch/powerpc/kvm/emulate.c
> 
> @@ -31,6 +31,8 @@
> 
> #include <asm/kvm_ppc.h>
> 
> #include <asm/disassemble.h>
> 
> #include <asm/ppc-opcode.h>
> 
> +#include <asm/reg_fsl_emb.h>
> 
> +#include <asm/reg_booke.h>
> 
> #include "timing.h"
> 
> #include "trace.h"
> 
> @@ -90,6 +92,88 @@ u32 kvmppc_get_dec(struct kvm_vcpu *vcpu, u64 tb)
> 
>                return vcpu->arch.dec - jd;
> 
> }
> 
> +static int kvmppc_emulate_mtpmr(struct kvm_vcpu *vcpu, int pmrn, int
> rs)
> 
> +{
> 
> +
> 
> +       enum emulation_result emulated = EMULATE_DONE;
> 
> +       ulong pmr_val = kvmppc_get_gpr(vcpu, rs);
> 
> +
> 
> +       switch(pmrn)  {
> 
> +       case PMRN_PMC0:
> 
> +               vcpu->arch.shared->pmc0 = pmr_val;
> 
> +               break;
> 
> +       case PMRN_PMC1:
> 
> +               vcpu->arch.shared->pmc1 = pmr_val;
> 
> +               break;
> 
> +       case PMRN_PMC2:
> 
> +               vcpu->arch.shared->pmc2 = pmr_val;
> 
> +               break;
> 
> +       case PMRN_PMC3:
> 
> +               vcpu->arch.shared->pmc3 = pmr_val;
> 
> +               break;
> 
> +       case PMRN_UPMC0:
> 
> +               vcpu->arch.shared->upmc0 = pmr_val;
> 
> +               break;
> 
> +      case PMRN_UPMC1:
> 
> +               vcpu->arch.shared->upmc1 = pmr_val;
> 
> +               break;
> 
> +      case PMRN_PMGC0:
> 
> +                if (mfspr(SPRN_MSRP) & MSRP_PMMP) {
> 
> +
> 
> +                if (!(pmr_val & PMGC0_PMIE))
> 
> +                                mtspr(SPRN_MSRP, mfspr(SPRN_MSRP) &
> ~MSRP_PMMP);

Use kvm_guest_protect_msr() to manipulate MSRP (as is, the code would
break on e500v2).

Explain in a comment why you're clearing MSRP_PMMP here.  How would
MSRP_PMMP ever get set again, if the guest later sets PMIE?  What
happens to the host if the guest enables PMIE and an interrupt happens?

-Scott