| Message ID | 1593595262-1433-3-git-send-email-atrajeev@linux.vnet.ibm.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | powerpc/perf: Add support for power10 PMU Hardware | expand |
On Wed, Jul 01, 2020 at 05:20:54AM -0400, Athira Rajeev wrote: > PowerISA v3.1 has added new performance monitoring unit (PMU) > special purpose registers (SPRs). They are > > Monitor Mode Control Register 3 (MMCR3) > Sampled Instruction Event Register A (SIER2) > Sampled Instruction Event Register B (SIER3) > > Patch addes support to save/restore these new > SPRs while entering/exiting guest. This mostly looks reasonable, at a quick glance at least, but I am puzzled by two of the changes you are making. See below. > diff --git a/arch/powerpc/kvm/book3s_hv.c b/arch/powerpc/kvm/book3s_hv.c > index 6bf66649..c265800 100644 > --- a/arch/powerpc/kvm/book3s_hv.c > +++ b/arch/powerpc/kvm/book3s_hv.c > @@ -1698,7 +1698,8 @@ static int kvmppc_get_one_reg_hv(struct kvm_vcpu *vcpu, u64 id, > *val = get_reg_val(id, vcpu->arch.sdar); > break; > case KVM_REG_PPC_SIER: > - *val = get_reg_val(id, vcpu->arch.sier); > + i = id - KVM_REG_PPC_SIER; > + *val = get_reg_val(id, vcpu->arch.sier[i]); This is inside a switch (id) statement, so here we know that id is KVM_REG_PPC_SIER. In other words i will always be zero, so what is the point of doing the subtraction? > break; > case KVM_REG_PPC_IAMR: > *val = get_reg_val(id, vcpu->arch.iamr); > @@ -1919,7 +1920,8 @@ static int kvmppc_set_one_reg_hv(struct kvm_vcpu *vcpu, u64 id, > vcpu->arch.sdar = set_reg_val(id, *val); > break; > case KVM_REG_PPC_SIER: > - vcpu->arch.sier = set_reg_val(id, *val); > + i = id - KVM_REG_PPC_SIER; > + vcpu->arch.sier[i] = set_reg_val(id, *val); Same comment here. I think that new defines for the new registers will need to be added to arch/powerpc/include/uapi/asm/kvm.h and Documentation/virt/kvm/api.rst, and then new cases will need to be added to these switch statements. By the way, please cc kvm-ppc@vger.kernel.org and kvm@vger.kernel.org on KVM patches. Paul.
> On 01-Jul-2020, at 4:41 PM, Paul Mackerras <paulus@ozlabs.org> wrote: > > On Wed, Jul 01, 2020 at 05:20:54AM -0400, Athira Rajeev wrote: >> PowerISA v3.1 has added new performance monitoring unit (PMU) >> special purpose registers (SPRs). They are >> >> Monitor Mode Control Register 3 (MMCR3) >> Sampled Instruction Event Register A (SIER2) >> Sampled Instruction Event Register B (SIER3) >> >> Patch addes support to save/restore these new >> SPRs while entering/exiting guest. > > This mostly looks reasonable, at a quick glance at least, but I am > puzzled by two of the changes you are making. See below. > >> diff --git a/arch/powerpc/kvm/book3s_hv.c b/arch/powerpc/kvm/book3s_hv.c >> index 6bf66649..c265800 100644 >> --- a/arch/powerpc/kvm/book3s_hv.c >> +++ b/arch/powerpc/kvm/book3s_hv.c >> @@ -1698,7 +1698,8 @@ static int kvmppc_get_one_reg_hv(struct kvm_vcpu *vcpu, u64 id, >> *val = get_reg_val(id, vcpu->arch.sdar); >> break; >> case KVM_REG_PPC_SIER: >> - *val = get_reg_val(id, vcpu->arch.sier); >> + i = id - KVM_REG_PPC_SIER; >> + *val = get_reg_val(id, vcpu->arch.sier[i]); > > This is inside a switch (id) statement, so here we know that id is > KVM_REG_PPC_SIER. In other words i will always be zero, so what is > the point of doing the subtraction? > >> break; >> case KVM_REG_PPC_IAMR: >> *val = get_reg_val(id, vcpu->arch.iamr); >> @@ -1919,7 +1920,8 @@ static int kvmppc_set_one_reg_hv(struct kvm_vcpu *vcpu, u64 id, >> vcpu->arch.sdar = set_reg_val(id, *val); >> break; >> case KVM_REG_PPC_SIER: >> - vcpu->arch.sier = set_reg_val(id, *val); >> + i = id - KVM_REG_PPC_SIER; >> + vcpu->arch.sier[i] = set_reg_val(id, *val); > > Same comment here. Hi Paul, Thanks for reviewing the patch. Yes, true that currently `id` will be zero since it is only KVM_REG_PPC_SIER. I have kept the subtraction here considering that there will be addition of new registers to switch case. ex: case KVM_REG_PPC_SIER..KVM_REG_PPC_SIER3 > > I think that new defines for the new registers will need to be added > to arch/powerpc/include/uapi/asm/kvm.h and > Documentation/virt/kvm/api.rst, and then new cases will need to be > added to these switch statements. Yes, New registers are not yet added to kvm.h I will address these comments and include changes for arch/powerpc/include/uapi/asm/kvm.h and Documentation/virt/kvm/api.rst in the next version. > > By the way, please cc kvm-ppc@vger.kernel.org and kvm@vger.kernel.org > on KVM patches. Sure, will include KVM mailing list in the next version Thanks Athira > > Paul.
@@ -637,12 +637,12 @@ struct kvm_vcpu_arch {
> u32 ccr1;
> u32 dbsr;
>
> - u64 mmcr[5];
> + u64 mmcr[6];
> u32 pmc[8];
> u32 spmc[2];
> u64 siar;
> + mfspr r5, SPRN_MMCR3
> + mfspr r6, SPRN_SIER2
> + mfspr r7, SPRN_SIER3
> + std r5, VCPU_MMCR + 40(r9)
> + std r6, VCPU_SIER + 8(r9)
> + std r7, VCPU_SIER + 16(r9)
This is looking pretty fragile now. vcpu mmcr[6] stores (in this strict order):
mmcr0, mmcr1, mmcra, mmcr2, mmcrs, mmmcr3.
Can we clean that up? Give mmcra and mmcrs their own entries in vcpu and then
have a flat array for mmcr0-3.
Mikey
diff --git a/arch/powerpc/include/asm/kvm_book3s_asm.h b/arch/powerpc/include/asm/kvm_book3s_asm.h index 45704f2..078f464 100644 --- a/arch/powerpc/include/asm/kvm_book3s_asm.h +++ b/arch/powerpc/include/asm/kvm_book3s_asm.h @@ -119,7 +119,7 @@ struct kvmppc_host_state { void __iomem *xive_tima_virt; u32 saved_xirr; u64 dabr; - u64 host_mmcr[7]; /* MMCR 0,1,A, SIAR, SDAR, MMCR2, SIER */ + u64 host_mmcr[10]; /* MMCR 0,1,A, SIAR, SDAR, MMCR2, SIER, MMCR3, SIER2/3 */ u32 host_pmc[8]; u64 host_purr; u64 host_spurr; diff --git a/arch/powerpc/include/asm/kvm_host.h b/arch/powerpc/include/asm/kvm_host.h index 7e2d061..d718061 100644 --- a/arch/powerpc/include/asm/kvm_host.h +++ b/arch/powerpc/include/asm/kvm_host.h @@ -637,12 +637,12 @@ struct kvm_vcpu_arch { u32 ccr1; u32 dbsr; - u64 mmcr[5]; + u64 mmcr[6]; u32 pmc[8]; u32 spmc[2]; u64 siar; u64 sdar; - u64 sier; + u64 sier[3]; #ifdef CONFIG_PPC_TRANSACTIONAL_MEM u64 tfhar; u64 texasr; diff --git a/arch/powerpc/kernel/asm-offsets.c b/arch/powerpc/kernel/asm-offsets.c index 6657dc6..20a8b1e 100644 --- a/arch/powerpc/kernel/asm-offsets.c +++ b/arch/powerpc/kernel/asm-offsets.c @@ -696,6 +696,9 @@ int main(void) HSTATE_FIELD(HSTATE_SDAR, host_mmcr[4]); HSTATE_FIELD(HSTATE_MMCR2, host_mmcr[5]); HSTATE_FIELD(HSTATE_SIER, host_mmcr[6]); + HSTATE_FIELD(HSTATE_MMCR3, host_mmcr[7]); + HSTATE_FIELD(HSTATE_SIER2, host_mmcr[8]); + HSTATE_FIELD(HSTATE_SIER3, host_mmcr[9]); HSTATE_FIELD(HSTATE_PMC1, host_pmc[0]); HSTATE_FIELD(HSTATE_PMC2, host_pmc[1]); HSTATE_FIELD(HSTATE_PMC3, host_pmc[2]); diff --git a/arch/powerpc/kvm/book3s_hv.c b/arch/powerpc/kvm/book3s_hv.c index 6bf66649..c265800 100644 --- a/arch/powerpc/kvm/book3s_hv.c +++ b/arch/powerpc/kvm/book3s_hv.c @@ -1698,7 +1698,8 @@ static int kvmppc_get_one_reg_hv(struct kvm_vcpu *vcpu, u64 id, *val = get_reg_val(id, vcpu->arch.sdar); break; case KVM_REG_PPC_SIER: - *val = get_reg_val(id, vcpu->arch.sier); + i = id - KVM_REG_PPC_SIER; + *val = get_reg_val(id, vcpu->arch.sier[i]); break; case KVM_REG_PPC_IAMR: *val = get_reg_val(id, vcpu->arch.iamr); @@ -1919,7 +1920,8 @@ static int kvmppc_set_one_reg_hv(struct kvm_vcpu *vcpu, u64 id, vcpu->arch.sdar = set_reg_val(id, *val); break; case KVM_REG_PPC_SIER: - vcpu->arch.sier = set_reg_val(id, *val); + i = id - KVM_REG_PPC_SIER; + vcpu->arch.sier[i] = set_reg_val(id, *val); break; case KVM_REG_PPC_IAMR: vcpu->arch.iamr = set_reg_val(id, *val); diff --git a/arch/powerpc/kvm/book3s_hv_interrupts.S b/arch/powerpc/kvm/book3s_hv_interrupts.S index 63fd81f..59822cb 100644 --- a/arch/powerpc/kvm/book3s_hv_interrupts.S +++ b/arch/powerpc/kvm/book3s_hv_interrupts.S @@ -140,6 +140,14 @@ BEGIN_FTR_SECTION std r8, HSTATE_MMCR2(r13) std r9, HSTATE_SIER(r13) END_FTR_SECTION_IFSET(CPU_FTR_ARCH_207S) +BEGIN_FTR_SECTION + mfspr r5, SPRN_MMCR3 + mfspr r6, SPRN_SIER2 + mfspr r7, SPRN_SIER3 + std r5, HSTATE_MMCR3(r13) + std r6, HSTATE_SIER2(r13) + std r7, HSTATE_SIER3(r13) +END_FTR_SECTION_IFSET(CPU_FTR_ARCH_31) mfspr r3, SPRN_PMC1 mfspr r5, SPRN_PMC2 mfspr r6, SPRN_PMC3 diff --git a/arch/powerpc/kvm/book3s_hv_rmhandlers.S b/arch/powerpc/kvm/book3s_hv_rmhandlers.S index 7194389..57b6c14 100644 --- a/arch/powerpc/kvm/book3s_hv_rmhandlers.S +++ b/arch/powerpc/kvm/book3s_hv_rmhandlers.S @@ -3436,6 +3436,14 @@ END_FTR_SECTION_IFSET(CPU_FTR_PMAO_BUG) mtspr SPRN_SIAR, r7 mtspr SPRN_SDAR, r8 BEGIN_FTR_SECTION + ld r5, VCPU_MMCR + 40(r4) + ld r6, VCPU_SIER + 8(r4) + ld r7, VCPU_SIER + 16(r4) + mtspr SPRN_MMCR3, r5 + mtspr SPRN_SIER2, r6 + mtspr SPRN_SIER3, r7 +END_FTR_SECTION_IFSET(CPU_FTR_ARCH_31) +BEGIN_FTR_SECTION ld r5, VCPU_MMCR + 24(r4) ld r6, VCPU_SIER(r4) mtspr SPRN_MMCR2, r5 @@ -3496,6 +3504,14 @@ BEGIN_FTR_SECTION mtspr SPRN_MMCR2, r8 mtspr SPRN_SIER, r9 END_FTR_SECTION_IFSET(CPU_FTR_ARCH_207S) +BEGIN_FTR_SECTION + ld r5, HSTATE_MMCR3(r13) + ld r6, HSTATE_SIER2(r13) + ld r7, HSTATE_SIER3(r13) + mtspr SPRN_MMCR3, r5 + mtspr SPRN_SIER2, r6 + mtspr SPRN_SIER3, r7 +END_FTR_SECTION_IFSET(CPU_FTR_ARCH_31) mtspr SPRN_MMCR0, r3 isync mtlr r0 @@ -3555,6 +3571,14 @@ END_FTR_SECTION_IFSET(CPU_FTR_ARCH_207S) BEGIN_FTR_SECTION std r10, VCPU_MMCR + 24(r9) END_FTR_SECTION_IFSET(CPU_FTR_ARCH_207S) +BEGIN_FTR_SECTION + mfspr r5, SPRN_MMCR3 + mfspr r6, SPRN_SIER2 + mfspr r7, SPRN_SIER3 + std r5, VCPU_MMCR + 40(r9) + std r6, VCPU_SIER + 8(r9) + std r7, VCPU_SIER + 16(r9) +END_FTR_SECTION_IFSET(CPU_FTR_ARCH_31) std r7, VCPU_SIAR(r9) std r8, VCPU_SDAR(r9) mfspr r3, SPRN_PMC1
PowerISA v3.1 has added new performance monitoring unit (PMU) special purpose registers (SPRs). They are Monitor Mode Control Register 3 (MMCR3) Sampled Instruction Event Register A (SIER2) Sampled Instruction Event Register B (SIER3) Patch addes support to save/restore these new SPRs while entering/exiting guest. Signed-off-by: Athira Rajeev <atrajeev@linux.vnet.ibm.com> --- arch/powerpc/include/asm/kvm_book3s_asm.h | 2 +- arch/powerpc/include/asm/kvm_host.h | 4 ++-- arch/powerpc/kernel/asm-offsets.c | 3 +++ arch/powerpc/kvm/book3s_hv.c | 6 ++++-- arch/powerpc/kvm/book3s_hv_interrupts.S | 8 ++++++++ arch/powerpc/kvm/book3s_hv_rmhandlers.S | 24 ++++++++++++++++++++++++ 6 files changed, 42 insertions(+), 5 deletions(-)