Re: [PATCH 1/5] KVM: arm64: Add accessor for per-CPU state

From: Marc Zyngier <maz@kernel.org>
To: Suzuki K Poulose <suzuki.poulose@arm.com>
Cc: kvmarm@lists.linux.dev, linux-arm-kernel@lists.infradead.org,
	kvm@vger.kernel.org, James Morse <james.morse@arm.com>,
	Oliver Upton <oliver.upton@linux.dev>,
	Zenghui Yu <yuzenghui@huawei.com>,
	James Clark <james.clark@arm.com>,
	Anshuman Khandual <anshuman.khandual@arm.com>,
	Mark Brown <broonie@kernel.org>
Subject: Re: [PATCH 1/5] KVM: arm64: Add accessor for per-CPU state
Date: Sat, 09 Mar 2024 13:00:18 +0000	[thread overview]
Message-ID: <87y1arpnkt.wl-maz@kernel.org> (raw)
In-Reply-To: <4eba8428-fe6c-4c42-b135-ab831e5cf436@arm.com>

On Mon, 04 Mar 2024 12:05:20 +0000,
Suzuki K Poulose <suzuki.poulose@arm.com> 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@kernel.org>
> > ---
> >   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.