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-...@vger.kernel.org and k...@vger.kernel.org on KVM patches. Paul.
