Re: [PATCH v3 1/6] KVM: PPC: Use getters and setters for vcpu register state

["Nicholas Piggin" <npiggin@gmail.com>]

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 <gautam@linux.ibm.com>
> Signed-off-by: Jordan Niethe <jniethe5@gmail.com>
> ---
> 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.

> 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.

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.

And as a nitpick, for anywhere employing existing access functions, gprs
and such, could that be split into its own patch?

Looks pretty good aside from those little things.

Thanks,
Nick