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

2023-08-06 Thread Jordan Niethe 
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(-)

diff --git a/arch/powerpc/include/asm/kvm_book3s.h 
b/arch/powerpc/include/asm/kvm_book3s.h
index bbf5e2c5fe09..1a7e837ea2d5 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,121 @@ 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;
+}
+
+#ifdef CONFIG_VSX
+static inline void kvmppc_get_vsx_vr(struct kvm_vcpu *vcpu, int i, vector128 
*v)
+{
+   *v =  vcpu->arch.vr.vr[i];
+}
+
+static inline void kvmppc_set_vsx_vr(struct kvm_vcpu *vcpu, int i,
+vector128 *val)
+{
+   vcpu->arch.vr.vr[i] = *val;
+}
+
+static inline u32 kvmppc_get_vscr(struct kvm_vcpu *vcpu)
+{
+   return vcpu->arch.vr.vscr.u[3];
+}
+
+static inline void kvmppc_set_vscr(struct kvm_vcpu *vcpu, u32 val)
+{
+   vcpu->arch.vr.vscr.u[3] = val;
+}
+#endif
+
+#define KVMPPC_BOOK3S_VCPU_ACCESSOR_SET(reg, size) \
+static inline void kvmppc_set_##reg(struct kvm_vcpu *vcpu, u##size val)
\
+{  \
+   \
+   vcpu->arch.reg = val;   \
+}
+
+#define KVMPPC_BOOK3S_VCPU_ACCESSOR_GET(reg, size) \
+static inline u##size kvmppc_get_##reg(struct kvm_vcpu *vcpu)  \
+{  \
+   return vcpu->arch.reg;  \
+}
+
+#define KVMPPC_BOOK3S_VCPU_ACCESSOR(reg, size) \
+   KVMPPC_BOOK3S_VCPU_ACCESSOR_SET(reg, size)  \
+   KVMPPC_BOOK3S_VCPU_ACCESSOR_GET(reg, size)  \
+
+KVMPPC_BOOK3S_VCPU_ACCESSOR(tar, 64)
+KVMPPC_BOOK3S_VCPU_ACCESSOR(ebbhr, 64)
+KVMPPC_BOOK3S_VCPU_ACCESSOR(ebbrr, 64)
+KVMPPC_BOOK3S_VCPU_ACCESSOR(bescr, 64)
+KVMPPC_BOOK3S_VCPU_ACCESSOR(ic, 64)
+K

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

2023-08-14 Thread Nicholas Piggin 
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.

> 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

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

2023-08-15 Thread Jordan Niethe 




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

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

2023-08-16 Thread Michael Ellerman 
Jordan Niethe  writes:
> 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.
>

...
> diff --git a/arch/powerpc/include/asm/kvm_book3s.h 
> b/arch/powerpc/include/asm/kvm_book3s.h
> index bbf5e2c5fe09..1a7e837ea2d5 100644
> --- a/arch/powerpc/include/asm/kvm_book3s.h
> +++ b/arch/powerpc/include/asm/kvm_book3s.h
> @@ -403,10 +413,121 @@ static inline ulong kvmppc_get_fault_dar(struct 
> kvm_vcpu *vcpu)
...
> +
> +#ifdef CONFIG_VSX
> +static inline void kvmppc_get_vsx_vr(struct kvm_vcpu *vcpu, int i, vector128 
> *v)
> +{
> + *v =  vcpu->arch.vr.vr[i];
> +}

This is causing build errors if VSX is disabled.

I'm using g5_defconfig plus:

  CONFIG_VIRTUALIZATION=y
  CONFIG_KVM_BOOK3S_64=y
  CONFIG_KVM_BOOK3S_64_PR=y

Which gives me:

  ../arch/powerpc/kvm/powerpc.c: In function ‘kvmppc_set_vmx_dword’:
  ../arch/powerpc/kvm/powerpc.c:1061:9: error: implicit declaration of function 
‘kvmppc_get_vsx_vr’; did you mean ‘kvmppc_get_vsx_fpr’? 
[-Werror=implicit-function-declaration]
   1061 | kvmppc_get_vsx_vr(vcpu, index, &val.vval);
| ^
| kvmppc_get_vsx_fpr
  ../arch/powerpc/kvm/powerpc.c:1063:9: error: implicit declaration of function 
‘kvmppc_set_vsx_vr’; did you mean ‘kvmppc_set_vsx_fpr’? 
[-Werror=implicit-function-declaration]
   1063 | kvmppc_set_vsx_vr(vcpu, index, &val.vval);
| ^
| kvmppc_set_vsx_fpr
  In file included from ../arch/powerpc/kvm/powerpc.c:25:
  ../arch/powerpc/kvm/powerpc.c: In function ‘kvm_vcpu_ioctl_get_one_reg’:
  ../arch/powerpc/kvm/powerpc.c:1729:52: error: implicit declaration of 
function ‘kvmppc_get_vscr’; did you mean ‘kvmppc_get_sr’? 
[-Werror=implicit-function-declaration]
   1729 | val = get_reg_val(reg->id, 
kvmppc_get_vscr(vcpu));
|^~~
  ../arch/powerpc/include/asm/kvm_ppc.h:412:29: note: in definition of macro 
‘get_reg_val’
412 | case 4: __u.wval = (reg); break;\
| ^~~
  ../arch/powerpc/kvm/powerpc.c: In function ‘kvm_vcpu_ioctl_set_one_reg’:
  ../arch/powerpc/kvm/powerpc.c:1780:25: error: implicit declaration of 
function ‘kvmppc_set_vscr’; did you mean ‘kvmppc_set_fscr’? 
[-Werror=implicit-function-declaration]
   1780 | kvmppc_set_vscr(vcpu, set_reg_val(reg->id, 
val));
| ^~~
| kvmppc_set_fscr


Looking at kvm_vcpu_arch, the thread_vr_state and members are guarded by
CONFIG_ALTIVEC, not CONFIG_VSX.

Switching to that fixes the build.

Whether it makes sense to be getting/setting those registers when VSX=n
is not immediately clear, but is a separate problem.

cheers