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 =3D get_reg_val(id, vcpu->arch.sdar);
= = = break;
case KVM_REG_PPC_SIER:
- = = *val =3D get_reg_val(id, vcpu->arch.sier);
+ i =3D id = - KVM_REG_PPC_SIER;
+ *val =3D 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 =3D = 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 =3D set_reg_val(id, *val);
= = = break;
case KVM_REG_PPC_SIER:
- = = vcpu->arch.sier =3D set_reg_val(id, *val);
+ i =3D id = - KVM_REG_PPC_SIER;
+ vcpu->arch.sier[i] =3D = 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