Re: [PATCH 2/2] KVM: arm64: Remove extra argument for __pvkm_host_{share,unshare}_hyp()

[Marc Zyngier <maz@kernel.org>]

On Thu, 11 Dec 2025 09:33:40 +0000,
Alexandru Elisei <alexandru.elisei@arm.com> wrote:
> 
> Hi Marc,
> 
> On Thu, Dec 11, 2025 at 08:15:28AM +0000, Marc Zyngier wrote:
> > On Wed, 10 Dec 2025 13:21:02 +0000,
> > Alexandru Elisei <alexandru.elisei@arm.com> wrote:
> > > 
> > > __pvkm_host_share_hyp() and __pkvm_host_unshare_hyp() both have one
> > > parameter, the pfn, not two. Even though correctness isn't impacted because
> > > the SMCCC handlers pass the first argument and ignore the second one, let's
> > > call the functions with the proper number of arguments.
> > > 
> > > Signed-off-by: Alexandru Elisei <alexandru.elisei@arm.com>
> > > ---
> > >  arch/arm64/kvm/mmu.c | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > 
> > > diff --git a/arch/arm64/kvm/mmu.c b/arch/arm64/kvm/mmu.c
> > > index 7cc964af8d30..6c6abcd8e89e 100644
> > > --- a/arch/arm64/kvm/mmu.c
> > > +++ b/arch/arm64/kvm/mmu.c
> > > @@ -497,7 +497,7 @@ static int share_pfn_hyp(u64 pfn)
> > >  	this->count = 1;
> > >  	rb_link_node(&this->node, parent, node);
> > >  	rb_insert_color(&this->node, &hyp_shared_pfns);
> > > -	ret = kvm_call_hyp_nvhe(__pkvm_host_share_hyp, pfn, 1);
> > > +	ret = kvm_call_hyp_nvhe(__pkvm_host_share_hyp, pfn);
> > >  unlock:
> > >  	mutex_unlock(&hyp_shared_pfns_lock);
> > >  
> > 
> > Yeah, we lost all form of type-checking when everything was hastily
> > converted to SMCCC to avoid function pointers. Somehow, I feel that
> > the cure was worse than the disease.
> > 
> > I wish we'd reintroduce some form of compile-time checks, maybe by
> > having generated stubs?
> 
> I also think compile-time checks would be useful, do you have something
> particular in mind?
> 
> If not, right now I can't think of a way to generate the stubs automagically,
> but I can give it a (probably bad) try and take it from there.

On first approximation, all the hypercalls are implemented like this
(taking __pkvm_init() as an example):


We have the body of the function, with its forward declaration:

int __pkvm_init(phys_addr_t phys, unsigned long size, unsigned long nr_cpus,
		unsigned long *per_cpu_base, u32 hyp_va_bits);

int __pkvm_init(phys_addr_t phys, unsigned long size, unsigned long nr_cpus,
		unsigned long *per_cpu_base, u32 hyp_va_bits)
{
	[...]
}

We also have the callee stub that does the argument un-marshalling:

static void handle___pkvm_init(struct kvm_cpu_context *host_ctxt)
{
	DECLARE_REG(phys_addr_t, phys, host_ctxt, 1);
	DECLARE_REG(unsigned long, size, host_ctxt, 2);
	DECLARE_REG(unsigned long, nr_cpus, host_ctxt, 3);
	DECLARE_REG(unsigned long *, per_cpu_base, host_ctxt, 4);
	DECLARE_REG(u32, hyp_va_bits, host_ctxt, 5);

	/*
	 * __pkvm_init() will return only if an error occurred, otherwise it
	 * will tail-call in __pkvm_init_finalise() which will have to deal
	 * with the host context directly.
	 */
	cpu_reg(host_ctxt, 1) = __pkvm_init(phys, size, nr_cpus, per_cpu_base,
					    hyp_va_bits);
}

and then the call site:

	ret = kvm_call_hyp_nvhe(__pkvm_init, hyp_mem_base, hyp_mem_size,
				num_possible_cpus(), kern_hyp_va(per_cpu_base),
				hyp_va_bits);

The problem is that kvm_call_hyp_nvhe() ignores the type conveyed by
"__pkvm_init", as it is immediately tokenized as an SMCCC function
number.

One idea would be to add something like this:

diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
index b552a1e03848c..fdf09beeae380 100644
--- a/arch/arm64/include/asm/kvm_host.h
+++ b/arch/arm64/include/asm/kvm_host.h
@@ -1208,10 +1208,11 @@ void kvm_arm_resume_guest(struct kvm *kvm);
 #define vcpu_has_run_once(vcpu)	(!!READ_ONCE((vcpu)->pid))
 
 #ifndef __KVM_NVHE_HYPERVISOR__
-#define kvm_call_hyp_nvhe(f, ...)						\
+#define kvm_call_hyp_nvhe(f, ...)					\
 	({								\
 		struct arm_smccc_res res;				\
 									\
+		(void)f(##__VA_ARGS__);					\
 		arm_smccc_1_1_hvc(KVM_HOST_SMCCC_FUNC(f),		\
 				  ##__VA_ARGS__, &res);			\
 		WARN_ON(res.a0 != SMCCC_RET_SUCCESS);			\

and generate stub functions that don't do anything, can be eliminated
by the compiler, but not before type-checking.

Another possible idea would be:

diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
index b552a1e03848c..4f53a3240a511 100644
--- a/arch/arm64/include/asm/kvm_host.h
+++ b/arch/arm64/include/asm/kvm_host.h
@@ -1208,12 +1208,11 @@ void kvm_arm_resume_guest(struct kvm *kvm);
 #define vcpu_has_run_once(vcpu)	(!!READ_ONCE((vcpu)->pid))
 
 #ifndef __KVM_NVHE_HYPERVISOR__
-#define kvm_call_hyp_nvhe(f, ...)						\
+#define kvm_call_hyp_nvhe(f, ...)					\
 	({								\
 		struct arm_smccc_res res;				\
 									\
-		arm_smccc_1_1_hvc(KVM_HOST_SMCCC_FUNC(f),		\
-				  ##__VA_ARGS__, &res);			\
+		nvhe_hvc_ ## f(##__VA_ARGS__, &res);			\
 		WARN_ON(res.a0 != SMCCC_RET_SUCCESS);			\
 									\
 		res.a1;							\

where the stub function would actually do the type checking. I tend to
prefer this.

In any case, you would need have some form of declarative IDL, and get
both the caller and callee stubs to be totally generated. It shouldn't
be too complicated to do it in awk or some other stuff.

Thoughts?

	M.

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