Re: [PATCH v2 02/10] KVM: PPC: Book3S HV: Save/restore new PMU registers
From: Paul Mackerras <hidden>
Date: 2020-07-01 11:14:02
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.
quoted hunk ↗ jump to hunk
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?
quoted hunk ↗ jump to hunk
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.