Re: [PATCH 6.12 8/8] KVM: arm64: Eagerly switch ZCR_EL{1,2}

[Mark Rutland <mark.rutland@arm.com>]

On Wed, Mar 19, 2025 at 09:15:54AM +0000, Marc Zyngier wrote:
> On Wed, 19 Mar 2025 00:26:14 +0000,
> Gavin Shan <gshan@redhat.com> wrote:
> > On 3/14/25 10:35 AM, Mark Brown wrote:

> > > diff --git a/arch/arm64/kvm/hyp/nvhe/hyp-main.c b/arch/arm64/kvm/hyp/nvhe/hyp-main.c
> > > index 4e757a77322c9efc59cdff501745f7c80d452358..1c8e2ad32e8c396fc4b11d5fec2e86728f2829d9 100644
> > > --- a/arch/arm64/kvm/hyp/nvhe/hyp-main.c
> > > +++ b/arch/arm64/kvm/hyp/nvhe/hyp-main.c
> > > @@ -5,6 +5,7 @@
> > >    */
> > >     #include <hyp/adjust_pc.h>
> > > +#include <hyp/switch.h>
> > >     #include <asm/pgtable-types.h>
> > >   #include <asm/kvm_asm.h>
> > > @@ -176,8 +177,12 @@ static void handle___kvm_vcpu_run(struct kvm_cpu_context *host_ctxt)
> > >   		sync_hyp_vcpu(hyp_vcpu);
> > >   		pkvm_put_hyp_vcpu(hyp_vcpu);
> > >   	} else {
> > > +		struct kvm_vcpu *vcpu = kern_hyp_va(host_vcpu);
> > > +
> > >   		/* The host is fully trusted, run its vCPU directly. */
> > > -		ret = __kvm_vcpu_run(host_vcpu);
> > > +		fpsimd_lazy_switch_to_guest(vcpu);
> > > +		ret = __kvm_vcpu_run(vcpu);
> > > +		fpsimd_lazy_switch_to_host(vcpu);
> > >   	}
> > >   
> > 
> > @host_vcpu should have been hypervisor's linear mapping address in v6.12. It looks
> > incorrect to assume it's a kernel's linear mapping address and convert it (@host_vcpu)
> > to the hypervisor's linear address agin, if I don't miss anything.
> 
> host_vcpu is passed as a parameter to the hypercall, and is definitely
> a kernel address.
> 
> However, at this stage, we have *already* converted it to a HYP VA:
> 
> https://web.git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/tree/arch/arm64/kvm/hyp/nvhe/hyp-main.c?h=linux-6.12.y#n147

That's also the case in v6.13; the earlier conversion was removed in
v6.14-rc1 in commit:

  f7d03fcbf1f48206 ("KVM: arm64: Introduce __pkvm_vcpu_{load,put}()")

... where the code in the 'else' block changed from:

|	ret = __kvm_vcpu_run(host_vcpu);

... to:

	ret = __kvm_vcpu_run(kern_hyp_va(host_vcpu));
|

In the upstream version of this patch, the code here changed from

|	/* The host is fully trusted, run its vCPU directly. */
|	ret = __kvm_vcpu_run(kern_hyp_va(host_vcpu));

... to:

|	struct kvm_vcpu *vcpu = kern_hyp_va(host_vcpu);
|
|	/* The host is fully trusted, run its vCPU directly. */
|	fpsimd_lazy_switch_to_guest(vcpu);
|	ret = __kvm_vcpu_run(vcpu);
|	fpsimd_lazy_switch_to_host(vcpu);

> The result is that this change is turning a perfectly valid HYP VA
> into... something. Odds are that the masking/patching will not mess up
> the address, but this is completely buggy anyway. In general,
> kern_hyp_va() is not an idempotent operation.

IIUC today it *happens* to be idempotent, but as you say that is not
guaranteed to remain the case, and this is definitely a logical bug.

> Thanks for noticing that something was wrong.
> 
> Broonie, can you please look into this?
> 
> Greg, it may be more prudent to unstage this series from 6.12-stable
> until we know for sure this is the only problem.

As above, likewise with the v6.13 version.

I'll go reply there linking to this thread.

Mark.