Re: [PATCH 08/18] KVM: arm64: nv: Handle HCR_EL2.NV system register traps

From: Marc Zyngier <maz@kernel.org>
To: Oliver Upton <oliver.upton@linux.dev>
Cc: Joey Gouly <joey.gouly@arm.com>,
	kvmarm@lists.linux.dev, kvm@vger.kernel.org,
	linux-arm-kernel@lists.infradead.org,
	Alexandru Elisei <alexandru.elisei@arm.com>,
	Andre Przywara <andre.przywara@arm.com>,
	Catalin Marinas <catalin.marinas@arm.com>,
	Christoffer Dall <christoffer.dall@arm.com>,
	Ganapatrao Kulkarni <gankulkarni@os.amperecomputing.com>,
	Russell King <rmk+kernel@armlinux.org.uk>,
	James Morse <james.morse@arm.com>,
	Suzuki K Poulose <suzuki.poulose@arm.com>,
	Zenghui Yu <yuzenghui@huawei.com>,
	nd@arm.com
Subject: Re: [PATCH 08/18] KVM: arm64: nv: Handle HCR_EL2.NV system register traps
Date: Fri, 24 Feb 2023 19:03:21 +0000	[thread overview]
Message-ID: <86v8jqx4w6.wl-maz@kernel.org> (raw)
In-Reply-To: <Y/kDp9nYrAmYuh6y@linux.dev>

On Fri, 24 Feb 2023 18:36:23 +0000,
Oliver Upton <oliver.upton@linux.dev> wrote:
> 
> Hi Joey,
> 
> On Fri, Feb 24, 2023 at 05:39:15PM +0000, Joey Gouly wrote:
> > I'm having an issue with this commit where a VCPU is getting a CNTVOFF_EL2 set
> > to 0, so it sees the same time as the host system, and the other VCPU has the
> > correct offset.
> 
> Yikes!
> 
> > The flow of execution looks like this:
> > 	KVM_CREATE_VCPU 0 (VMM) ->
> > 		kvm_timer_vcpu_init ->
> > 			update_vtimer_cntvoff (VCPU0.CNTVOFF_EL2=kvm_phys_timer_read)
> > 	KVM_ARM_VCPU_INIT (VMM) ->
> > 		kvm_arch_vcpu_ioctl_vcpu_init ->
> > 			kvm_vcpu_set_target ->
> > 				kvm_reset_vcpu ->
> > 					kvm_reset_sys_regs (VCPU0.CNTVOFF_EL2=0)
> > 
> > 	KVM_CREATE_VCPU 1 (VMM) ->
> > 		kvm_timer_vcpu_init ->
> > 			update_vtimer_cntvoff (VCPU0.CNTVOFF_EL2=VCPU1.CNTVOFF_EL2=kvm_phys_timer_read)
> > 	KVM_ARM_VCPU_INIT (VMM) ->
> > 		kvm_arch_vcpu_ioctl_vcpu_init ->
> > 			kvm_vcpu_set_target ->
> > 				kvm_reset_vcpu ->
> > 					kvm_reset_sys_regs (VCPU1.CNTVOFF_EL2=0)
> > 
> > 	At this point VCPU0 has CNTVOFF_EL2 == kvm_phys_timer_read, and VCPU1
> > 	has CNTVOFF_EL2 == 0.
> > 
> > The VCPUs having different CNTVOFF_EL2 valuess is just a symptom of the fact that
> > CNTVOFF_EL2 is now reset in kvm_reset_sys_regs.
> 
> Right, and the fundamental problem at hand is that we used CNTVOFF_EL2
> to stash the _host's_ offset even though we are trying to change the
> meaning of it to be part of the virtual EL2's context.

What is driving me nuts is that I have never managed to reproduce the
problem so far. I guess I have too much crap in my tree to spot it...

> 
> > The following patch gets it booting again, but I'm sure it's not the right fix.
> 
> I'd rather we just break the host away from using the shadow reg
> altogether and separately track the host offset. As it so happens Marc
> has a patch that does exactly that [*].

Yup, might as well fix that *and* kill the lock inversion issue in one
go...

> Marc, do you want to resend that patch in isolation addressing the
> feedback and link to this bug report?
> 
> [*] https://lore.kernel.org/kvmarm/20230216142123.2638675-6-maz@kernel.org/

Joey, could you please give that patch a go, just to be on the safe
side?

Thanks,

	M.

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