On Mon, 04 Mar 2024 12:05:20 +0000,
Suzuki K Poulose <suzuki.poulose@xxxxxxx> wrote:
>
> Hi Marc
>
> On 02/03/2024 11:19, Marc Zyngier wrote:
> > In order to facilitate the introduction of new per-CPU state,
> > add a new host_data_ptr() helped that hides some of the per-CPU
> > verbosity, and make it easier to move that state around in the
> > future.
> >
>
> This series looks like a good cleanup to make the whole host
> data handling cleaner. One comment below.
>
> > Signed-off-by: Marc Zyngier <maz@xxxxxxxxxx>
> > ---
> > arch/arm64/include/asm/kvm_host.h | 13 +++++++++++++
> > arch/arm64/kvm/arm.c | 2 +-
> > arch/arm64/kvm/hyp/include/hyp/debug-sr.h | 4 ++--
> > arch/arm64/kvm/hyp/include/hyp/switch.h | 11 +++++------
> > arch/arm64/kvm/hyp/nvhe/psci-relay.c | 2 +-
> > arch/arm64/kvm/hyp/nvhe/setup.c | 3 +--
> > arch/arm64/kvm/hyp/nvhe/switch.c | 4 ++--
> > arch/arm64/kvm/hyp/vhe/switch.c | 4 ++--
> > arch/arm64/kvm/hyp/vhe/sysreg-sr.c | 4 ++--
> > arch/arm64/kvm/pmu.c | 2 +-
> > 10 files changed, 30 insertions(+), 19 deletions(-)
> >
> > diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
> > index 21c57b812569..3ca2a9444f21 100644
> > --- a/arch/arm64/include/asm/kvm_host.h
> > +++ b/arch/arm64/include/asm/kvm_host.h
> > @@ -492,6 +492,17 @@ struct kvm_cpu_context {
> > u64 *vncr_array;
> > };
> > +/*
> > + * This structure is instanciated on a per-CPU basis, and contains
> > + * data that is:
> > + *
> > + * - tied to a single physical CPU, and
> > + * - either have a lifetime that does not extend past vcpu_put()
> > + * - or is an invariant for the lifetime of the system
> > + *
> > + * Use host_data_ptr(field) as a way to access a pointer to such a
> > + * field.
> > + */
> > struct kvm_host_data {
> > struct kvm_cpu_context host_ctxt;
> > };
> > @@ -1114,6 +1125,8 @@ struct kvm_vcpu *kvm_mpidr_to_vcpu(struct kvm *kvm, unsigned long mpidr);
> > DECLARE_KVM_HYP_PER_CPU(struct kvm_host_data, kvm_host_data);
> > +#define host_data_ptr(f) (&this_cpu_ptr(&kvm_host_data)->f)
> > +
> > static inline void kvm_init_host_cpu_context(struct kvm_cpu_context *cpu_ctxt)
> > {
> > /* The host's MPIDR is immutable, so let's set it up at boot time */
> > diff --git a/arch/arm64/kvm/arm.c b/arch/arm64/kvm/arm.c
> > index a25265aca432..d3d14cc41cb5 100644
> > --- a/arch/arm64/kvm/arm.c
> > +++ b/arch/arm64/kvm/arm.c
> > @@ -1960,7 +1960,7 @@ static void cpu_set_hyp_vector(void)
> > static void cpu_hyp_init_context(void)
> > {
> > - kvm_init_host_cpu_context(&this_cpu_ptr_hyp_sym(kvm_host_data)->host_ctxt);
> > + kvm_init_host_cpu_context(host_data_ptr(host_ctxt));
>
> This silently changes the "this_cpu_ptr_hyp_sym" to "this_cpu_ptr()"
> and thus we could be using the VHE host_data even in nVHE ?
> Rest looks fine to me.
Huh, well spotted. I'll switch the definition of host_data_ptr()
outside of the hypervisor code to use this_cpu_ptr_hyp_sym() instead,
like we have for some of the other helpers.
Thanks again,
M.
--
Without deviation from the norm, progress is not possible.
