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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 





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 <gautam@xxxxxxxxxxxxx>
Signed-off-by: Jordan Niethe <jniethe5@xxxxxxxxx>
---
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




[Index of Archives]     [KVM ARM]     [KVM ia64]     [KVM ppc]     [Virtualization Tools]     [Spice Development]     [Libvirt]     [Libvirt Users]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite Questions]     [Linux Kernel]     [Linux SCSI]     [XFree86]

  Powered by Linux