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

From: "Nicholas Piggin" <npiggin@gmail.com>
To: "Jordan Niethe" <jpn@linux.vnet.ibm.com>,
	<linuxppc-dev@lists.ozlabs.org>
Cc: mikey@neuling.org, kautuk.consul.1980@gmail.com,
	kvm@vger.kernel.org, kvm-ppc@vger.kernel.org,
	sbhat@linux.ibm.com, vaibhav@linux.ibm.com
Subject: Re: [RFC PATCH v2 1/6] KVM: PPC: Use getters and setters for vcpu register state
Date: Wed, 07 Jun 2023 17:51:59 +1000	[thread overview]
Message-ID: <CT696R4C69N1.1OZKTR1A9D3X1@wheely> (raw)
In-Reply-To: <20230605064848.12319-2-jpn@linux.vnet.ibm.com>

On Mon Jun 5, 2023 at 4:48 PM 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 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 PAPR nested guests later.
>
> Signed-off-by: Jordan Niethe <jpn@linux.vnet.ibm.com>
> ---
>  arch/powerpc/include/asm/kvm_book3s.h  |  68 +++++++-
>  arch/powerpc/include/asm/kvm_ppc.h     |  48 ++++--
>  arch/powerpc/kvm/book3s.c              |  22 +--
>  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           | 222 +++++++++++++------------
>  arch/powerpc/kvm/book3s_hv.h           |  59 +++++++
>  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/powerpc.c             |   4 +-
>  15 files changed, 322 insertions(+), 158 deletions(-)
>
> diff --git a/arch/powerpc/include/asm/kvm_book3s.h b/arch/powerpc/include/asm/kvm_book3s.h
> index bbf5e2c5fe09..4e91f54a3f9f 100644
> --- a/arch/powerpc/include/asm/kvm_book3s.h
> +++ b/arch/powerpc/include/asm/kvm_book3s.h
> @@ -392,6 +392,16 @@ static inline ulong kvmppc_get_pc(struct kvm_vcpu *vcpu)
>  	return vcpu->arch.regs.nip;
>  }
>  
> +static inline void kvmppc_set_pid(struct kvm_vcpu *vcpu, u32 val)
> +{
> +	vcpu->arch.pid = val;
> +}
> +
> +static inline u32 kvmppc_get_pid(struct kvm_vcpu *vcpu)
> +{
> +	return vcpu->arch.pid;
> +}
> +
>  static inline u64 kvmppc_get_msr(struct kvm_vcpu *vcpu);
>  static inline bool kvmppc_need_byteswap(struct kvm_vcpu *vcpu)
>  {
> @@ -403,10 +413,66 @@ static inline ulong kvmppc_get_fault_dar(struct kvm_vcpu *vcpu)
>  	return vcpu->arch.fault_dar;
>  }
>  
> +#define BOOK3S_WRAPPER_SET(reg, size)					\
> +static inline void kvmppc_set_##reg(struct kvm_vcpu *vcpu, u##size val)	\
> +{									\
> +									\
> +	vcpu->arch.reg = val;						\
> +}
> +
> +#define BOOK3S_WRAPPER_GET(reg, size)					\
> +static inline u##size kvmppc_get_##reg(struct kvm_vcpu *vcpu)		\
> +{									\
> +	return vcpu->arch.reg;						\
> +}
> +
> +#define BOOK3S_WRAPPER(reg, size)					\
> +	BOOK3S_WRAPPER_SET(reg, size)					\
> +	BOOK3S_WRAPPER_GET(reg, size)					\
> +
> +BOOK3S_WRAPPER(tar, 64)
> +BOOK3S_WRAPPER(ebbhr, 64)
> +BOOK3S_WRAPPER(ebbrr, 64)
> +BOOK3S_WRAPPER(bescr, 64)
> +BOOK3S_WRAPPER(ic, 64)
> +BOOK3S_WRAPPER(vrsave, 64)
> +
> +
> +#define VCORE_WRAPPER_SET(reg, size)					\
> +static inline void kvmppc_set_##reg ##_hv(struct kvm_vcpu *vcpu, u##size val)	\
> +{									\
> +	vcpu->arch.vcore->reg = val;					\
> +}
> +
> +#define VCORE_WRAPPER_GET(reg, size)					\
> +static inline u##size kvmppc_get_##reg ##_hv(struct kvm_vcpu *vcpu)	\
> +{									\
> +	return vcpu->arch.vcore->reg;					\
> +}
> +
> +#define VCORE_WRAPPER(reg, size)					\
> +	VCORE_WRAPPER_SET(reg, size)					\
> +	VCORE_WRAPPER_GET(reg, size)					\
> +
> +
> +VCORE_WRAPPER(vtb, 64)
> +VCORE_WRAPPER(tb_offset, 64)
> +VCORE_WRAPPER(lpcr, 64)

The general idea is fine, some of the names could use a bit of
improvement. What's a BOOK3S_WRAPPER for example, is it not a
VCPU_WRAPPER, or alternatively why isn't a VCORE_WRAPPER Book3S
as well?

> +
> +static inline u64 kvmppc_get_dec_expires(struct kvm_vcpu *vcpu)
> +{
> +	return vcpu->arch.dec_expires;
> +}
> +
> +static inline void kvmppc_set_dec_expires(struct kvm_vcpu *vcpu, u64 val)
> +{
> +	vcpu->arch.dec_expires = val;
> +}
> +
>  /* 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);
>  }
>  
>  static inline bool is_kvmppc_resume_guest(int r)
> diff --git a/arch/powerpc/include/asm/kvm_ppc.h b/arch/powerpc/include/asm/kvm_ppc.h
> index 79a9c0bb8bba..fbac353ac46b 100644
> --- a/arch/powerpc/include/asm/kvm_ppc.h
> +++ b/arch/powerpc/include/asm/kvm_ppc.h
> @@ -936,7 +936,7 @@ static inline ulong kvmppc_get_##reg(struct kvm_vcpu *vcpu)		\
>  #define SPRNG_WRAPPER_SET(reg, bookehv_spr)				\
>  static inline void kvmppc_set_##reg(struct kvm_vcpu *vcpu, ulong val)	\
>  {									\
> -	mtspr(bookehv_spr, val);						\
> +	mtspr(bookehv_spr, val);					\
>  }									\
>  
>  #define SHARED_WRAPPER_GET(reg, size)					\

Stray hunk I think.

> @@ -957,10 +957,32 @@ static inline void kvmppc_set_##reg(struct kvm_vcpu *vcpu, u##size val)	\
>  	       vcpu->arch.shared->reg = cpu_to_le##size(val);		\
>  }									\
>  
> +#define SHARED_CACHE_WRAPPER_GET(reg, size)				\
> +static inline u##size kvmppc_get_##reg(struct kvm_vcpu *vcpu)		\
> +{									\
> +	if (kvmppc_shared_big_endian(vcpu))				\
> +	       return be##size##_to_cpu(vcpu->arch.shared->reg);	\
> +	else								\
> +	       return le##size##_to_cpu(vcpu->arch.shared->reg);	\
> +}									\
> +
> +#define SHARED_CACHE_WRAPPER_SET(reg, size)				\
> +static inline void kvmppc_set_##reg(struct kvm_vcpu *vcpu, u##size val)	\
> +{									\
> +	if (kvmppc_shared_big_endian(vcpu))				\
> +	       vcpu->arch.shared->reg = cpu_to_be##size(val);		\
> +	else								\
> +	       vcpu->arch.shared->reg = cpu_to_le##size(val);		\
> +}									\
> +
>  #define SHARED_WRAPPER(reg, size)					\
>  	SHARED_WRAPPER_GET(reg, size)					\
>  	SHARED_WRAPPER_SET(reg, size)					\
>  
> +#define SHARED_CACHE_WRAPPER(reg, size)					\
> +	SHARED_CACHE_WRAPPER_GET(reg, size)				\
> +	SHARED_CACHE_WRAPPER_SET(reg, size)				\

SHARED_CACHE_WRAPPER that does the same thing as SHARED_WRAPPER.

I know some of the names are a but crufty but it's probably a good time
to rethink them a bit.

KVMPPC_VCPU_SHARED_REG_ACCESSOR or something like that. A few
more keystrokes could help imensely.

> diff --git a/arch/powerpc/kvm/book3s_hv_p9_entry.c b/arch/powerpc/kvm/book3s_hv_p9_entry.c
> index 34f1db212824..34bc0a8a1288 100644
> --- a/arch/powerpc/kvm/book3s_hv_p9_entry.c
> +++ b/arch/powerpc/kvm/book3s_hv_p9_entry.c
> @@ -305,7 +305,7 @@ static void switch_mmu_to_guest_radix(struct kvm *kvm, struct kvm_vcpu *vcpu, u6
>  	u32 pid;
>  
>  	lpid = nested ? nested->shadow_lpid : kvm->arch.lpid;
> -	pid = vcpu->arch.pid;
> +	pid = kvmppc_get_pid(vcpu);
>  
>  	/*
>  	 * Prior memory accesses to host PID Q3 must be completed before we

Could add some accessors for get_lpid / get_guest_id which check for the
correct KVM mode maybe.

Thanks,
Nick