> On 01-Jul-2020, at 4:41 PM, Paul Mackerras <pau...@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-...@vger.kernel.org and k...@vger.kernel.org > on KVM patches. Sure, will include KVM mailing list in the next version Thanks Athira > > Paul.
