Re: [RFC PATCH v2 2/6] KVM: PPC: Add fpr getters and setters

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

On Mon Jun 5, 2023 at 4:48 PM AEST, Jordan Niethe wrote:
> Add wrappers for fpr registers to prepare for supporting PAPR nested
> guests.
>
> Signed-off-by: Jordan Niethe <jpn@linux.vnet.ibm.com>
> ---
>  arch/powerpc/include/asm/kvm_book3s.h | 31 +++++++++++++++++++++++++++
>  arch/powerpc/include/asm/kvm_booke.h  | 10 +++++++++
>  arch/powerpc/kvm/book3s.c             | 16 +++++++-------
>  arch/powerpc/kvm/emulate_loadstore.c  |  2 +-
>  arch/powerpc/kvm/powerpc.c            | 22 +++++++++----------
>  5 files changed, 61 insertions(+), 20 deletions(-)
>
> diff --git a/arch/powerpc/include/asm/kvm_book3s.h b/arch/powerpc/include/asm/kvm_book3s.h
> index 4e91f54a3f9f..a632e79639f0 100644
> --- a/arch/powerpc/include/asm/kvm_book3s.h
> +++ b/arch/powerpc/include/asm/kvm_book3s.h
> @@ -413,6 +413,37 @@ static inline ulong kvmppc_get_fault_dar(struct kvm_vcpu *vcpu)
>  	return vcpu->arch.fault_dar;
>  }
>  
> +static inline u64 kvmppc_get_fpr(struct kvm_vcpu *vcpu, int i)
> +{
> +	return vcpu->arch.fp.fpr[i][TS_FPROFFSET];
> +}
> +
> +static inline void kvmppc_set_fpr(struct kvm_vcpu *vcpu, int i, u64 val)
> +{
> +	vcpu->arch.fp.fpr[i][TS_FPROFFSET] = val;
> +}
> +
> +static inline u64 kvmppc_get_fpscr(struct kvm_vcpu *vcpu)
> +{
> +	return vcpu->arch.fp.fpscr;
> +}
> +
> +static inline void kvmppc_set_fpscr(struct kvm_vcpu *vcpu, u64 val)
> +{
> +	vcpu->arch.fp.fpscr = val;
> +}
> +
> +
> +static inline u64 kvmppc_get_vsx_fpr(struct kvm_vcpu *vcpu, int i, int j)
> +{
> +	return vcpu->arch.fp.fpr[i][j];
> +}
> +
> +static inline void kvmppc_set_vsx_fpr(struct kvm_vcpu *vcpu, int i, int j, u64 val)
> +{
> +	vcpu->arch.fp.fpr[i][j] = val;
> +}
> +
>  #define BOOK3S_WRAPPER_SET(reg, size)					\
>  static inline void kvmppc_set_##reg(struct kvm_vcpu *vcpu, u##size val)	\
>  {									\
> diff --git a/arch/powerpc/include/asm/kvm_booke.h b/arch/powerpc/include/asm/kvm_booke.h
> index 0c3401b2e19e..7c3291aa8922 100644
> --- a/arch/powerpc/include/asm/kvm_booke.h
> +++ b/arch/powerpc/include/asm/kvm_booke.h
> @@ -89,6 +89,16 @@ static inline ulong kvmppc_get_pc(struct kvm_vcpu *vcpu)
>  	return vcpu->arch.regs.nip;
>  }
>  
> +static inline void kvmppc_set_fpr(struct kvm_vcpu *vcpu, int i, u64 val)
> +{
> +	vcpu->arch.fp.fpr[i][TS_FPROFFSET] = val;
> +}
> +
> +static inline u64 kvmppc_get_fpr(struct kvm_vcpu *vcpu, int i)
> +{
> +	return vcpu->arch.fp.fpr[i][TS_FPROFFSET];
> +}
> +
>  #ifdef CONFIG_BOOKE
>  static inline ulong kvmppc_get_fault_dar(struct kvm_vcpu *vcpu)
>  {
> diff --git a/arch/powerpc/kvm/book3s.c b/arch/powerpc/kvm/book3s.c
> index 2fe31b518886..6cd20ab9e94e 100644
> --- a/arch/powerpc/kvm/book3s.c
> +++ b/arch/powerpc/kvm/book3s.c
> @@ -636,17 +636,17 @@ int kvmppc_get_one_reg(struct kvm_vcpu *vcpu, u64 id,
>  			break;
>  		case KVM_REG_PPC_FPR0 ... KVM_REG_PPC_FPR31:
>  			i = id - KVM_REG_PPC_FPR0;
> -			*val = get_reg_val(id, VCPU_FPR(vcpu, i));
> +			*val = get_reg_val(id, kvmppc_get_fpr(vcpu, i));
>  			break;
>  		case KVM_REG_PPC_FPSCR:
> -			*val = get_reg_val(id, vcpu->arch.fp.fpscr);
> +			*val = get_reg_val(id, kvmppc_get_fpscr(vcpu));
>  			break;
>  #ifdef CONFIG_VSX
>  		case KVM_REG_PPC_VSR0 ... KVM_REG_PPC_VSR31:
>  			if (cpu_has_feature(CPU_FTR_VSX)) {
>  				i = id - KVM_REG_PPC_VSR0;
> -				val->vsxval[0] = vcpu->arch.fp.fpr[i][0];
> -				val->vsxval[1] = vcpu->arch.fp.fpr[i][1];
> +				val->vsxval[0] = kvmppc_get_vsx_fpr(vcpu, i, 0);
> +				val->vsxval[1] = kvmppc_get_vsx_fpr(vcpu, i, 1);
>  			} else {
>  				r = -ENXIO;
>  			}
> @@ -724,7 +724,7 @@ int kvmppc_set_one_reg(struct kvm_vcpu *vcpu, u64 id,
>  			break;
>  		case KVM_REG_PPC_FPR0 ... KVM_REG_PPC_FPR31:
>  			i = id - KVM_REG_PPC_FPR0;
> -			VCPU_FPR(vcpu, i) = set_reg_val(id, *val);
> +			kvmppc_set_fpr(vcpu, i, set_reg_val(id, *val));
>  			break;
>  		case KVM_REG_PPC_FPSCR:
>  			vcpu->arch.fp.fpscr = set_reg_val(id, *val);
> @@ -733,8 +733,8 @@ int kvmppc_set_one_reg(struct kvm_vcpu *vcpu, u64 id,
>  		case KVM_REG_PPC_VSR0 ... KVM_REG_PPC_VSR31:
>  			if (cpu_has_feature(CPU_FTR_VSX)) {
>  				i = id - KVM_REG_PPC_VSR0;
> -				vcpu->arch.fp.fpr[i][0] = val->vsxval[0];
> -				vcpu->arch.fp.fpr[i][1] = val->vsxval[1];
> +				kvmppc_set_vsx_fpr(vcpu, i, 0, val->vsxval[0]);
> +				kvmppc_set_vsx_fpr(vcpu, i, 1, val->vsxval[1]);
>  			} else {
>  				r = -ENXIO;
>  			}
> @@ -765,7 +765,7 @@ int kvmppc_set_one_reg(struct kvm_vcpu *vcpu, u64 id,
>  			break;
>  #endif /* CONFIG_KVM_XIVE */
>  		case KVM_REG_PPC_FSCR:
> -			vcpu->arch.fscr = set_reg_val(id, *val);
> +			kvmppc_set_fpscr(vcpu, set_reg_val(id, *val));
>  			break;
>  		case KVM_REG_PPC_TAR:
>  			kvmppc_set_tar(vcpu, set_reg_val(id, *val));
> diff --git a/arch/powerpc/kvm/emulate_loadstore.c b/arch/powerpc/kvm/emulate_loadstore.c
> index 059c08ae0340..e6e66c3792f8 100644
> --- a/arch/powerpc/kvm/emulate_loadstore.c
> +++ b/arch/powerpc/kvm/emulate_loadstore.c
> @@ -250,7 +250,7 @@ int kvmppc_emulate_loadstore(struct kvm_vcpu *vcpu)
>  				vcpu->arch.mmio_sp64_extend = 1;
>  
>  			emulated = kvmppc_handle_store(vcpu,
> -					VCPU_FPR(vcpu, op.reg), size, 1);
> +					kvmppc_get_fpr(vcpu, op.reg), size, 1);
>  
>  			if ((op.type & UPDATE) && (emulated != EMULATE_FAIL))
>  				kvmppc_set_gpr(vcpu, op.update_reg, op.ea);
> diff --git a/arch/powerpc/kvm/powerpc.c b/arch/powerpc/kvm/powerpc.c
> index ca9793c3d437..7f913e68342a 100644
> --- a/arch/powerpc/kvm/powerpc.c
> +++ b/arch/powerpc/kvm/powerpc.c
> @@ -938,7 +938,7 @@ static inline void kvmppc_set_vsr_dword(struct kvm_vcpu *vcpu,
>  		val.vsxval[offset] = gpr;
>  		VCPU_VSX_VR(vcpu, index - 32) = val.vval;
>  	} else {
> -		VCPU_VSX_FPR(vcpu, index, offset) = gpr;
> +		kvmppc_set_vsx_fpr(vcpu, index, offset, gpr);
>  	}
>  }
>  

Is there a particular reason some reg sets are broken into their own
patches? Looking at this hunk you think the VR one got missed, but it's
in its own patch.

Not really a big deal but I wouldn't mind them all in one patch. Or at
least the FP/VR/VSR ine one since they're quite regular and similar.

Thanks,
Nick