From mboxrd@z Thu Jan 1 00:00:00 1970 From: Jordan Niethe Date: Wed, 16 Aug 2023 03:11:00 +0000 Subject: Re: [PATCH v3 1/6] KVM: PPC: Use getters and setters for vcpu register state Message-Id: <71e14e67-3ba4-4ddd-921d-38181f3c0159@gmail.com> List-Id: References: <20230807014553.1168699-1-jniethe5@gmail.com> <20230807014553.1168699-2-jniethe5@gmail.com> In-Reply-To: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: Nicholas Piggin , linuxppc-dev@lists.ozlabs.org Cc: kvm@vger.kernel.org, kvm-ppc@vger.kernel.org, mikey@neuling.org, paulus@ozlabs.org, vaibhav@linux.ibm.com, sbhat@linux.ibm.com, gautam@linux.ibm.com, kconsul@linux.vnet.ibm.com, amachhiw@linux.vnet.ibm.com On 14/8/23 6:08 pm, Nicholas Piggin wrote: > On Mon Aug 7, 2023 at 11:45 AM AEST, Jordan Niethe wrote: >> There are already some getter and setter functions used for accessing >> vcpu register state, e.g. kvmppc_get_pc(). There are also more >> complicated examples that are generated by macros like >> kvmppc_get_sprg0() which are generated by the SHARED_SPRNG_WRAPPER() >> macro. >> >> In the new PAPR "Nestedv2" API for nested guest partitions the L1 is >> required to communicate with the L0 to modify and read nested guest >> state. >> >> Prepare to support this by replacing direct accesses to vcpu register >> state with wrapper functions. Follow the existing pattern of using >> macros to generate individual wrappers. These wrappers will >> be augmented for supporting Nestedv2 guests later. >> >> Signed-off-by: Gautam Menghani >> Signed-off-by: Jordan Niethe >> --- >> v3: >> - Do not add a helper for pvr >> - Use an expression when declaring variable in case >> - Squash in all getters and setters >> - Guatam: Pass vector registers by reference >> --- >> arch/powerpc/include/asm/kvm_book3s.h | 123 +++++++++++++- >> arch/powerpc/include/asm/kvm_booke.h | 10 ++ >> arch/powerpc/kvm/book3s.c | 38 ++--- >> arch/powerpc/kvm/book3s_64_mmu_hv.c | 4 +- >> arch/powerpc/kvm/book3s_64_mmu_radix.c | 9 +- >> arch/powerpc/kvm/book3s_64_vio.c | 4 +- >> arch/powerpc/kvm/book3s_hv.c | 220 +++++++++++++------------ >> arch/powerpc/kvm/book3s_hv.h | 58 +++++++ >> arch/powerpc/kvm/book3s_hv_builtin.c | 10 +- >> arch/powerpc/kvm/book3s_hv_p9_entry.c | 4 +- >> arch/powerpc/kvm/book3s_hv_ras.c | 5 +- >> arch/powerpc/kvm/book3s_hv_rm_mmu.c | 8 +- >> arch/powerpc/kvm/book3s_hv_rm_xics.c | 4 +- >> arch/powerpc/kvm/book3s_xive.c | 9 +- >> arch/powerpc/kvm/emulate_loadstore.c | 2 +- >> arch/powerpc/kvm/powerpc.c | 76 ++++----- >> 16 files changed, 395 insertions(+), 189 deletions(-) >> > > [snip] > >> + >> /* Expiry time of vcpu DEC relative to host TB */ >> static inline u64 kvmppc_dec_expires_host_tb(struct kvm_vcpu *vcpu) >> { >> - return vcpu->arch.dec_expires - vcpu->arch.vcore->tb_offset; >> + return kvmppc_get_dec_expires(vcpu) - kvmppc_get_tb_offset_hv(vcpu); >> } > > I don't see kvmppc_get_tb_offset_hv in this patch. It should be generated by: KVMPPC_BOOK3S_VCORE_ACCESSOR(tb_offset, 64) > >> diff --git a/arch/powerpc/kvm/book3s_64_mmu_hv.c b/arch/powerpc/kvm/book3s_64_mmu_hv.c >> index 7f765d5ad436..738f2ecbe9b9 100644 >> --- a/arch/powerpc/kvm/book3s_64_mmu_hv.c >> +++ b/arch/powerpc/kvm/book3s_64_mmu_hv.c >> @@ -347,7 +347,7 @@ static int kvmppc_mmu_book3s_64_hv_xlate(struct kvm_vcpu *vcpu, gva_t eaddr, >> unsigned long v, orig_v, gr; >> __be64 *hptep; >> long int index; >> - int virtmode = vcpu->arch.shregs.msr & (data ? MSR_DR : MSR_IR); >> + int virtmode = kvmppc_get_msr(vcpu) & (data ? MSR_DR : MSR_IR); >> >> if (kvm_is_radix(vcpu->kvm)) >> return kvmppc_mmu_radix_xlate(vcpu, eaddr, gpte, data, iswrite); > > So this isn't _only_ adding new accessors. This should be functionally a > noop, but I think it introduces a branch if PR is defined. That being checking kvmppc_shared_big_endian()? > > Shared page is a slight annoyance for HV, I'd like to get rid of it... > but that's another story. I think the pattern here would be to add a > kvmppc_get_msr_hv() accessor. Yes, that will work. > > And as a nitpick, for anywhere employing existing access functions, gprs > and such, could that be split into its own patch? Sure will do. One other thing I could do is make the existing functions use the macros if they don't already. Do you think that is worth doing? > > Looks pretty good aside from those little things. Thanks. > > Thanks, > Nick >