Re: [PATCH 06/16] KVM: arm64: timers: Use CNTPOFF_EL2 to offset the physical timer

From: Marc Zyngier <maz@kernel.org>
To: Colton Lewis <coltonlewis@google.com>
Cc: kvmarm@lists.linux.dev, kvm@vger.kernel.org,
	linux-arm-kernel@lists.infradead.org, james.morse@arm.com,
	suzuki.poulose@arm.com, oliver.upton@linux.dev,
	yuzenghui@huawei.com, ricarkol@google.com, sveith@amazon.de,
	dwmw2@infradead.org
Subject: Re: [PATCH 06/16] KVM: arm64: timers: Use CNTPOFF_EL2 to offset the physical timer
Date: Fri, 24 Feb 2023 08:59:19 +0000	[thread overview]
Message-ID: <864jrbxwuw.wl-maz@kernel.org> (raw)
In-Reply-To: <gsntzg948104.fsf@coltonlewis-kvm.c.googlers.com>

On Thu, 23 Feb 2023 22:34:19 +0000,
Colton Lewis <coltonlewis@google.com> wrote:
> 
> 
> CNTPOFF_EL2 should probably be added to the vcpu_sysreg enum in
> kvm_host.h for this patch. This seemed like the most relevant commit.
> 
> Marc Zyngier <maz@kernel.org> writes:
> 
> > +static bool has_cntpoff(void)
> > +{
> > +	return (has_vhe() && cpus_have_final_cap(ARM64_HAS_ECV_CNTPOFF));
> > +}
> > +
> 
> This being used to guard the register in set_cntpoff seems to say that
> we can only write CNTPOFF in VHE mode. Since it's possible the
> underlying hardware could still support CNTPOFF even if KVM is running
> in nVHE mode, should there be a way to set CNTPOFF in nVHE mode? Is that
> possible?

It would have to happen at EL2 (the register is called CNTPOFF_EL2,
which should give you a clue). This code execute at EL1 when running
nVHE, so any CNTPOFF_EL2 access would UNDEF.

> 
> This caused some confusion for me on my implementation as some teammates
> thought so but I could not get an implementation working for nVHE mode.

Using CNTPOFF for nVHE is utterly pointless unless you start
offloading the host's physical timer to the EL2 physical timer on
entry. Otherwise, you completely screw the host timer and everybody
dies.

> > @@ -84,7 +89,7 @@ u64 timer_get_cval(struct arch_timer_context *ctxt)
> 
> >   static u64 timer_get_offset(struct arch_timer_context *ctxt)
> >   {
> > -	if (ctxt->offset.vm_offset)
> > +	if (ctxt && ctxt->offset.vm_offset)
> >   		return *ctxt->offset.vm_offset;
> 
> >   	return 0;
> 
> nit: this change should be in the previous commit

No. I don't even think this is required anymore, due to the way the
offset is now handled on the VHE-only path.

> 
> > @@ -480,6 +491,7 @@ static void timer_save_state(struct
> > arch_timer_context *ctx)
> >   		write_sysreg_el0(0, SYS_CNTP_CTL);
> >   		isb();
> 
> > +		set_cntpoff(0);
> >   		break;
> >   	case NR_KVM_TIMERS:
> >   		BUG();
> 
> This seems to say CNTPOFF will be reset to 0 every time a vcpu
> switches out. What if the host originally had some value other than 0?
> Is KVM responsible for that context?

The host never has any value other than zero, just like it doesn't
have a value other than 0 for CNTVOFF_EL2. See the comment a few lines
above that explain the rationale for CNTVOFF, which mostly applies to
CNTPOFF as well.

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