<html><head><meta http-equiv="Content-Type" content="text/html; charset=us-ascii"></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; line-break: after-white-space;" class=""><br class=""><div><br class=""><blockquote type="cite" class=""><div class="">On 01-Jul-2020, at 4:41 PM, Paul Mackerras <<a href="mailto:paulus@ozlabs.org" class="">paulus@ozlabs.org</a>> wrote:</div><br class="Apple-interchange-newline"><div class=""><div class="">On Wed, Jul 01, 2020 at 05:20:54AM -0400, Athira Rajeev wrote:<br class=""><blockquote type="cite" class="">PowerISA v3.1 has added new performance monitoring unit (PMU)<br class="">special purpose registers (SPRs). They are<br class=""><br class="">Monitor Mode Control Register 3 (MMCR3)<br class="">Sampled Instruction Event Register A (SIER2)<br class="">Sampled Instruction Event Register B (SIER3)<br class=""><br class="">Patch addes support to save/restore these new<br class="">SPRs while entering/exiting guest.<br class=""></blockquote><br class="">This mostly looks reasonable, at a quick glance at least, but I am<br class="">puzzled by two of the changes you are making. See below.<br class=""><br class=""><blockquote type="cite" class="">diff --git a/arch/powerpc/kvm/book3s_hv.c b/arch/powerpc/kvm/book3s_hv.c<br class="">index 6bf66649..c265800 100644<br class="">--- a/arch/powerpc/kvm/book3s_hv.c<br class="">+++ b/arch/powerpc/kvm/book3s_hv.c<br class="">@@ -1698,7 +1698,8 @@ static int kvmppc_get_one_reg_hv(struct kvm_vcpu *vcpu, u64 id,<br class=""> <span class="Apple-tab-span" style="white-space:pre"> </span><span class="Apple-tab-span" style="white-space:pre"> </span>*val = get_reg_val(id, vcpu->arch.sdar);<br class=""> <span class="Apple-tab-span" style="white-space:pre"> </span><span class="Apple-tab-span" style="white-space:pre"> </span>break;<br class=""> <span class="Apple-tab-span" style="white-space:pre"> </span>case KVM_REG_PPC_SIER:<br class="">-<span class="Apple-tab-span" style="white-space:pre"> </span><span class="Apple-tab-span" style="white-space:pre"> </span>*val = get_reg_val(id, vcpu->arch.sier);<br class="">+<span class="Apple-tab-span" style="white-space:pre"> </span><span class="Apple-tab-span" style="white-space:pre"> </span>i = id - KVM_REG_PPC_SIER;<br class="">+<span class="Apple-tab-span" style="white-space:pre"> </span><span class="Apple-tab-span" style="white-space:pre"> </span>*val = get_reg_val(id, vcpu->arch.sier[i]);<br class=""></blockquote><br class="">This is inside a switch (id) statement, so here we know that id is<br class="">KVM_REG_PPC_SIER. In other words i will always be zero, so what is<br class="">the point of doing the subtraction?<br class=""><br class=""><blockquote type="cite" class=""> <span class="Apple-tab-span" style="white-space:pre"> </span><span class="Apple-tab-span" style="white-space:pre"> </span>break;<br class=""> <span class="Apple-tab-span" style="white-space:pre"> </span>case KVM_REG_PPC_IAMR:<br class=""> <span class="Apple-tab-span" style="white-space:pre"> </span><span class="Apple-tab-span" style="white-space:pre"> </span>*val = get_reg_val(id, vcpu->arch.iamr);<br class="">@@ -1919,7 +1920,8 @@ static int kvmppc_set_one_reg_hv(struct kvm_vcpu *vcpu, u64 id,<br class=""> <span class="Apple-tab-span" style="white-space:pre"> </span><span class="Apple-tab-span" style="white-space:pre"> </span>vcpu->arch.sdar = set_reg_val(id, *val);<br class=""> <span class="Apple-tab-span" style="white-space:pre"> </span><span class="Apple-tab-span" style="white-space:pre"> </span>break;<br class=""> <span class="Apple-tab-span" style="white-space:pre"> </span>case KVM_REG_PPC_SIER:<br class="">-<span class="Apple-tab-span" style="white-space:pre"> </span><span class="Apple-tab-span" style="white-space:pre"> </span>vcpu->arch.sier = set_reg_val(id, *val);<br class="">+<span class="Apple-tab-span" style="white-space:pre"> </span><span class="Apple-tab-span" style="white-space:pre"> </span>i = id - KVM_REG_PPC_SIER;<br class="">+<span class="Apple-tab-span" style="white-space:pre"> </span><span class="Apple-tab-span" style="white-space:pre"> </span>vcpu->arch.sier[i] = set_reg_val(id, *val);<br class=""></blockquote><br class="">Same comment here.<br class=""></div></div></blockquote><div><br class=""></div><div style="margin: 0px; font-stretch: normal; line-height: normal; font-family: "Helvetica Neue";" class="">Hi Paul,</div><div style="margin: 0px; font-stretch: normal; line-height: normal; font-family: "Helvetica Neue"; min-height: 14px;" class=""><br class=""></div><div style="margin: 0px; font-stretch: normal; line-height: normal; font-family: "Helvetica Neue";" class="">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. </div><div style="margin: 0px; font-stretch: normal; line-height: normal; font-family: "Helvetica Neue";" class="">ex: case KVM_REG_PPC_SIER..KVM_REG_PPC_SIER3</div><div style="margin: 0px; font-stretch: normal; line-height: normal; font-family: "Helvetica Neue";" class=""><br class=""></div><blockquote type="cite" class=""><div class=""><div class=""><br class="">I think that new defines for the new registers will need to be added<br class="">to arch/powerpc/include/uapi/asm/kvm.h and<br class="">Documentation/virt/kvm/api.rst, and then new cases will need to be<br class="">added to these switch statements.<br class=""></div></div></blockquote><div><br class=""></div><div style="margin: 0px; font-stretch: normal; line-height: normal; font-family: "Helvetica Neue";" class="">Yes, New registers are not yet added to kvm.h </div><div style="margin: 0px; font-stretch: normal; line-height: normal; font-family: "Helvetica Neue";" class="">I will address these comments and include changes for arch/powerpc/include/uapi/asm/kvm.h and Documentation/virt/kvm/api.rst in the</div><div style="margin: 0px; font-stretch: normal; line-height: normal; font-family: "Helvetica Neue";" class="">next version.</div><div style="margin: 0px; font-stretch: normal; line-height: normal; font-family: "Helvetica Neue";" class=""><br class=""></div><blockquote type="cite" class=""><div class=""><div class=""><br class="">By the way, please cc <a href="mailto:kvm-ppc@vger.kernel.org" class="">kvm-ppc@vger.kernel.org</a> and <a href="mailto:kvm@vger.kernel.org" class="">kvm@vger.kernel.org</a><br class="">on KVM patches.<br class=""></div></div></blockquote><div><br class=""></div><span style="caret-color: rgb(0, 0, 0); color: rgb(0, 0, 0); font-family: "Helvetica Neue";" class="">Sure, will include KVM mailing list in the next version</span></div><div><font color="#000000" face="Helvetica Neue" class=""><span style="caret-color: rgb(0, 0, 0);" class=""><br class=""></span></font></div><div><font color="#000000" face="Helvetica Neue" class=""><span style="caret-color: rgb(0, 0, 0);" class="">Thanks</span></font></div><div><font color="#000000" face="Helvetica Neue" class=""><span style="caret-color: rgb(0, 0, 0);" class="">Athira <br class=""></span></font><blockquote type="cite" class=""><div class=""><div class=""><br class="">Paul.<br class=""></div></div></blockquote></div><br class=""></body></html>