Re: [PATCH v2 06/17] arm64: Allow EL1 physical timer access when running VHE

From: Marc Zyngier <maz@kernel.org>
To: Oliver Upton <oliver.upton@linux.dev>
Cc: kvmarm@lists.linux.dev, kvm@vger.kernel.org,
	linux-arm-kernel@lists.infradead.org,
	James Morse <james.morse@arm.com>,
	Suzuki K Poulose <suzuki.poulose@arm.com>,
	Zenghui Yu <yuzenghui@huawei.com>,
	Quentin Perret <qperret@google.com>,
	Will Deacon <will@kernel.org>, Fuad Tabba <tabba@google.com>
Subject: Re: [PATCH v2 06/17] arm64: Allow EL1 physical timer access when running VHE
Date: Thu, 01 Jun 2023 12:59:24 +0100	[thread overview]
Message-ID: <87edmvpf4z.wl-maz@kernel.org> (raw)
In-Reply-To: <ZHg76wizp5RSXTLG@linux.dev>

On Thu, 01 Jun 2023 07:34:19 +0100,
Oliver Upton <oliver.upton@linux.dev> wrote:
> 
> Hey Marc,
> 
> On Fri, May 26, 2023 at 03:33:37PM +0100, Marc Zyngier wrote:
> > To initialise the timer access from EL2 when HCR_EL2.E2H is set,
> > we must make use the CNTHCTL_EL2 formap used is appropriate.
> > 
> > This amounts to shifting the timer/counter enable bits by 10
> > to the left.
> > 
> > Signed-off-by: Marc Zyngier <maz@kernel.org>
> > ---
> >  arch/arm64/include/asm/el2_setup.h | 5 +++++
> >  arch/arm64/kvm/hyp/nvhe/hyp-init.S | 9 +++++++++
> >  2 files changed, 14 insertions(+)
> > 
> > diff --git a/arch/arm64/include/asm/el2_setup.h b/arch/arm64/include/asm/el2_setup.h
> > index 037724b19c5c..225bf1f2514d 100644
> > --- a/arch/arm64/include/asm/el2_setup.h
> > +++ b/arch/arm64/include/asm/el2_setup.h
> > @@ -34,6 +34,11 @@
> >   */
> >  .macro __init_el2_timers
> >  	mov	x0, #3				// Enable EL1 physical timers
> > +	mrs	x1, hcr_el2
> > +	and	x1, x1, #HCR_E2H
> > +	cbz	x1, .LnVHE_\@
> > +	lsl	x0, x0, #10
> > +.LnVHE_\@:
> >  	msr	cnthctl_el2, x0
> >  	msr	cntvoff_el2, xzr		// Clear virtual offset
> >  .endm
> > diff --git a/arch/arm64/kvm/hyp/nvhe/hyp-init.S b/arch/arm64/kvm/hyp/nvhe/hyp-init.S
> > index a6d67c2bb5ae..f9ee10e29497 100644
> > --- a/arch/arm64/kvm/hyp/nvhe/hyp-init.S
> > +++ b/arch/arm64/kvm/hyp/nvhe/hyp-init.S
> > @@ -95,6 +95,15 @@ SYM_CODE_START_LOCAL(___kvm_hyp_init)
> >  	ldr	x1, [x0, #NVHE_INIT_HCR_EL2]
> >  	msr	hcr_el2, x1
> >  
> > +	mov	x2, #HCR_E2H
> > +	and	x2, x1, x2
> > +	cbz	x2, 1f
> > +
> > +	mrs	x1, cnthctl_el2
> > +	and	x1, x1, #~(BIT(0) | BIT(1))
> > +	orr	x1, x1, #(BIT(10) | BIT(11))
> > +	msr	cnthctl_el2, x1
> > +1:
> 
> Can this be done with an alternative block keyed on ARM64_KVM_HVHE?
> I get that __init_el2_timers needs to run before the cpu caps have been
> evaluated, but I don't think the same applies for ___kvm_hyp_init

I don't mind either way. My reasoning was that this code is hardly
ever executed, so the performance benefit of alternatives don't really
apply here (and the resulting object is smaller by 28 bytes... ;-).

Happy to change it if you think this makes sense.

Thanks,

	M.

-- 
Without deviation from the norm, progress is not possible.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel