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

[Jordan Niethe]

On Wed, Jun 7, 2023 at 5:53 PM Nicholas Piggin <npiggin at gmail.com> wrote:
[snip]
>
> 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?

Yeah the names are not great.
I didn't call it VCPU_WRAPPER because I wanted to keep separate
BOOK3S_WRAPPER for book3s registers
HV_WRAPPER for hv specific registers
I will change it to something like you suggested.

[snip]
>
> Stray hunk I think.

Yep.

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

That changes once the guest state buffer IDs are included in a later
patch.

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

Yes, I will do something like that, for the BOOK3S_WRAPPER and
HV_WRAPPER
too.

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

True.

Thanks,
Jordan

>
> Thanks,
> Nick
>