[PATCH 0/2] KVM: arm64: pKVM fixes

[PATCH 0/2] KVM: arm64: pKVM fixes

[Alexandru Elisei]

Similar to [1], when trying to run an unprotected VM on FVP with S1PIE enabled
and kvm-arm.mode=protected, writes to PIRE0_EL1 made by the guest in
__cpu_setup() are trapped by KVM and the BUG_ON(!r->access) is hit. That's
because HFGWTR_EL2.nPIRE0_EL1 is an inverse polarity trap and the FGT values for
the unprotected pKVM VCPU weren't being propagated from kvm_arch_vcpu_load().

Couldn't figure out how to run a protected pKVM VM, so that's untested.

Patch #2 is just a minor fixup for something I found by code inspection when I
trying to figure out how unprotected pKVM works.

[1] https://lore.kernel.org/all/20251112102853.47759-1-alexandru.elisei@arm.com/

Alexandru Elisei (2):
  KVM: arm64: Copy FGT traps to unprotected pKVM VCPU on VCPU load
  KVM: arm64: Remove extra argument for
    __pvkm_host_{share,unshare}_hyp()

 arch/arm64/kvm/hyp/nvhe/hyp-main.c | 12 ++++++++++--
 arch/arm64/kvm/hyp/nvhe/pkvm.c     |  1 -
 arch/arm64/kvm/mmu.c               |  2 +-
 3 files changed, 11 insertions(+), 4 deletions(-)


base-commit: 7d0a66e4bb9081d75c82ec4957c50034cb0ea449
-- 
2.52.0

[PATCH 1/2] KVM: arm64: Copy FGT traps to unprotected pKVM VCPU on VCPU load

[Alexandru Elisei]

Commit fb10ddf35c1c ("KVM: arm64: Compute per-vCPU FGTs at vcpu_load()")
introduced per-VCPU FGT traps. For an unprotected pKVM VCPU, the untrusted
host FGT configuration is copied in pkvm_vcpu_init_traps(), which is called
from __pkvm_init_vcpu(). __pkvm_init_vcpu() is called once per VCPU (when
the VCPU is first run) which means that the uninitialized, zero, values for
the FGT registers end up being used for the entire lifetime of the VCPU.
This causes both unwanted traps (for the inverse polarity trap bits) and
the guest being allowed to access registers it shouldn't.

Fix it by copying the FGT traps for unprotected pKVM VCPUs when the
untrusted host loads the VCPU.

Fixes: fb10ddf35c1c ("KVM: arm64: Compute per-vCPU FGTs at vcpu_load()")
Signed-off-by: Alexandru Elisei <alexandru.elisei@arm.com>
---
 arch/arm64/kvm/hyp/nvhe/hyp-main.c | 12 ++++++++++--
 arch/arm64/kvm/hyp/nvhe/pkvm.c     |  1 -
 2 files changed, 10 insertions(+), 3 deletions(-)

diff --git a/arch/arm64/kvm/hyp/nvhe/hyp-main.c b/arch/arm64/kvm/hyp/nvhe/hyp-main.c
index 29430c031095..ee0f1343100c 100644
--- a/arch/arm64/kvm/hyp/nvhe/hyp-main.c
+++ b/arch/arm64/kvm/hyp/nvhe/hyp-main.c
@@ -167,6 +167,7 @@ static void handle___pkvm_vcpu_load(struct kvm_cpu_context *host_ctxt)
 	DECLARE_REG(unsigned int, vcpu_idx, host_ctxt, 2);
 	DECLARE_REG(u64, hcr_el2, host_ctxt, 3);
 	struct pkvm_hyp_vcpu *hyp_vcpu;
+	struct kvm_vcpu *hyp_kvm_vcpu;
 
 	if (!is_protected_kvm_enabled())
 		return;
@@ -175,10 +176,17 @@ static void handle___pkvm_vcpu_load(struct kvm_cpu_context *host_ctxt)
 	if (!hyp_vcpu)
 		return;
 
+	hyp_kvm_vcpu = &hyp_vcpu->vcpu;
+
 	if (pkvm_hyp_vcpu_is_protected(hyp_vcpu)) {
 		/* Propagate WFx trapping flags */
-		hyp_vcpu->vcpu.arch.hcr_el2 &= ~(HCR_TWE | HCR_TWI);
-		hyp_vcpu->vcpu.arch.hcr_el2 |= hcr_el2 & (HCR_TWE | HCR_TWI);
+		hyp_kvm_vcpu->arch.hcr_el2 &= ~(HCR_TWE | HCR_TWI);
+		hyp_kvm_vcpu->arch.hcr_el2 |= hcr_el2 & (HCR_TWE | HCR_TWI);
+	} else {
+		struct kvm_vcpu *host_vcpu = hyp_vcpu->host_vcpu;
+
+		memcpy(&hyp_kvm_vcpu->arch.fgt, host_vcpu->arch.fgt,
+		       sizeof(hyp_kvm_vcpu->arch.fgt));
 	}
 }
 
diff --git a/arch/arm64/kvm/hyp/nvhe/pkvm.c b/arch/arm64/kvm/hyp/nvhe/pkvm.c
index 43bde061b65d..05774aed09cb 100644
--- a/arch/arm64/kvm/hyp/nvhe/pkvm.c
+++ b/arch/arm64/kvm/hyp/nvhe/pkvm.c
@@ -172,7 +172,6 @@ static int pkvm_vcpu_init_traps(struct pkvm_hyp_vcpu *hyp_vcpu)
 
 		/* Trust the host for non-protected vcpu features. */
 		vcpu->arch.hcrx_el2 = host_vcpu->arch.hcrx_el2;
-		memcpy(vcpu->arch.fgt, host_vcpu->arch.fgt, sizeof(vcpu->arch.fgt));
 		return 0;
 	}
 
-- 
2.52.0

Re: [PATCH 1/2] KVM: arm64: Copy FGT traps to unprotected pKVM VCPU on VCPU load

[Will Deacon]

Hey Alexandru,

[+Fuad as we ran into this last week]

On Wed, Dec 10, 2025 at 01:21:01PM +0000, Alexandru Elisei wrote:
> Commit fb10ddf35c1c ("KVM: arm64: Compute per-vCPU FGTs at vcpu_load()")
> introduced per-VCPU FGT traps. For an unprotected pKVM VCPU, the untrusted
> host FGT configuration is copied in pkvm_vcpu_init_traps(), which is called
> from __pkvm_init_vcpu(). __pkvm_init_vcpu() is called once per VCPU (when
> the VCPU is first run) which means that the uninitialized, zero, values for
> the FGT registers end up being used for the entire lifetime of the VCPU.
> This causes both unwanted traps (for the inverse polarity trap bits) and
> the guest being allowed to access registers it shouldn't.
> 
> Fix it by copying the FGT traps for unprotected pKVM VCPUs when the
> untrusted host loads the VCPU.
> 
> Fixes: fb10ddf35c1c ("KVM: arm64: Compute per-vCPU FGTs at vcpu_load()")
> Signed-off-by: Alexandru Elisei <alexandru.elisei@arm.com>
> ---
>  arch/arm64/kvm/hyp/nvhe/hyp-main.c | 12 ++++++++++--
>  arch/arm64/kvm/hyp/nvhe/pkvm.c     |  1 -
>  2 files changed, 10 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/arm64/kvm/hyp/nvhe/hyp-main.c b/arch/arm64/kvm/hyp/nvhe/hyp-main.c
> index 29430c031095..ee0f1343100c 100644
> --- a/arch/arm64/kvm/hyp/nvhe/hyp-main.c
> +++ b/arch/arm64/kvm/hyp/nvhe/hyp-main.c
> @@ -167,6 +167,7 @@ static void handle___pkvm_vcpu_load(struct kvm_cpu_context *host_ctxt)
>  	DECLARE_REG(unsigned int, vcpu_idx, host_ctxt, 2);
>  	DECLARE_REG(u64, hcr_el2, host_ctxt, 3);
>  	struct pkvm_hyp_vcpu *hyp_vcpu;
> +	struct kvm_vcpu *hyp_kvm_vcpu;
>  
>  	if (!is_protected_kvm_enabled())
>  		return;
> @@ -175,10 +176,17 @@ static void handle___pkvm_vcpu_load(struct kvm_cpu_context *host_ctxt)
>  	if (!hyp_vcpu)
>  		return;
>  
> +	hyp_kvm_vcpu = &hyp_vcpu->vcpu;
> +
>  	if (pkvm_hyp_vcpu_is_protected(hyp_vcpu)) {
>  		/* Propagate WFx trapping flags */
> -		hyp_vcpu->vcpu.arch.hcr_el2 &= ~(HCR_TWE | HCR_TWI);
> -		hyp_vcpu->vcpu.arch.hcr_el2 |= hcr_el2 & (HCR_TWE | HCR_TWI);
> +		hyp_kvm_vcpu->arch.hcr_el2 &= ~(HCR_TWE | HCR_TWI);
> +		hyp_kvm_vcpu->arch.hcr_el2 |= hcr_el2 & (HCR_TWE | HCR_TWI);

I don't think the 'hyp_kvm_vcpu' really makes this much clearer and it's
the line only ends up being a single character shorter. Just continue to
use 'hyp_vcpu->vcpu.'?

> +	} else {
> +		struct kvm_vcpu *host_vcpu = hyp_vcpu->host_vcpu;
> +
> +		memcpy(&hyp_kvm_vcpu->arch.fgt, host_vcpu->arch.fgt,
> +		       sizeof(hyp_kvm_vcpu->arch.fgt));
>  	}
>  }
>  
> diff --git a/arch/arm64/kvm/hyp/nvhe/pkvm.c b/arch/arm64/kvm/hyp/nvhe/pkvm.c
> index 43bde061b65d..05774aed09cb 100644
> --- a/arch/arm64/kvm/hyp/nvhe/pkvm.c
> +++ b/arch/arm64/kvm/hyp/nvhe/pkvm.c
> @@ -172,7 +172,6 @@ static int pkvm_vcpu_init_traps(struct pkvm_hyp_vcpu *hyp_vcpu)
>  
>  		/* Trust the host for non-protected vcpu features. */
>  		vcpu->arch.hcrx_el2 = host_vcpu->arch.hcrx_el2;
> -		memcpy(vcpu->arch.fgt, host_vcpu->arch.fgt, sizeof(vcpu->arch.fgt));
>  		return 0;
>  	}

Otherwise, looks good to me:

Acked-by: Will Deacon <will@kernel.org>

Will

Re: [PATCH 1/2] KVM: arm64: Copy FGT traps to unprotected pKVM VCPU on VCPU load

[Alexandru Elisei]

Hi Will,

On Fri, Dec 12, 2025 at 05:36:01AM +0900, Will Deacon wrote:
> Hey Alexandru,
> 
> [+Fuad as we ran into this last week]
> 
> On Wed, Dec 10, 2025 at 01:21:01PM +0000, Alexandru Elisei wrote:
> > Commit fb10ddf35c1c ("KVM: arm64: Compute per-vCPU FGTs at vcpu_load()")
> > introduced per-VCPU FGT traps. For an unprotected pKVM VCPU, the untrusted
> > host FGT configuration is copied in pkvm_vcpu_init_traps(), which is called
> > from __pkvm_init_vcpu(). __pkvm_init_vcpu() is called once per VCPU (when
> > the VCPU is first run) which means that the uninitialized, zero, values for
> > the FGT registers end up being used for the entire lifetime of the VCPU.
> > This causes both unwanted traps (for the inverse polarity trap bits) and
> > the guest being allowed to access registers it shouldn't.
> > 
> > Fix it by copying the FGT traps for unprotected pKVM VCPUs when the
> > untrusted host loads the VCPU.
> > 
> > Fixes: fb10ddf35c1c ("KVM: arm64: Compute per-vCPU FGTs at vcpu_load()")
> > Signed-off-by: Alexandru Elisei <alexandru.elisei@arm.com>
> > ---
> >  arch/arm64/kvm/hyp/nvhe/hyp-main.c | 12 ++++++++++--
> >  arch/arm64/kvm/hyp/nvhe/pkvm.c     |  1 -
> >  2 files changed, 10 insertions(+), 3 deletions(-)
> > 
> > diff --git a/arch/arm64/kvm/hyp/nvhe/hyp-main.c b/arch/arm64/kvm/hyp/nvhe/hyp-main.c
> > index 29430c031095..ee0f1343100c 100644
> > --- a/arch/arm64/kvm/hyp/nvhe/hyp-main.c
> > +++ b/arch/arm64/kvm/hyp/nvhe/hyp-main.c
> > @@ -167,6 +167,7 @@ static void handle___pkvm_vcpu_load(struct kvm_cpu_context *host_ctxt)
> >  	DECLARE_REG(unsigned int, vcpu_idx, host_ctxt, 2);
> >  	DECLARE_REG(u64, hcr_el2, host_ctxt, 3);
> >  	struct pkvm_hyp_vcpu *hyp_vcpu;
> > +	struct kvm_vcpu *hyp_kvm_vcpu;
> >  
> >  	if (!is_protected_kvm_enabled())
> >  		return;
> > @@ -175,10 +176,17 @@ static void handle___pkvm_vcpu_load(struct kvm_cpu_context *host_ctxt)
> >  	if (!hyp_vcpu)
> >  		return;
> >  
> > +	hyp_kvm_vcpu = &hyp_vcpu->vcpu;
> > +
> >  	if (pkvm_hyp_vcpu_is_protected(hyp_vcpu)) {
> >  		/* Propagate WFx trapping flags */
> > -		hyp_vcpu->vcpu.arch.hcr_el2 &= ~(HCR_TWE | HCR_TWI);
> > -		hyp_vcpu->vcpu.arch.hcr_el2 |= hcr_el2 & (HCR_TWE | HCR_TWI);
> > +		hyp_kvm_vcpu->arch.hcr_el2 &= ~(HCR_TWE | HCR_TWI);
> > +		hyp_kvm_vcpu->arch.hcr_el2 |= hcr_el2 & (HCR_TWE | HCR_TWI);
> 
> I don't think the 'hyp_kvm_vcpu' really makes this much clearer and it's
> the line only ends up being a single character shorter. Just continue to
> use 'hyp_vcpu->vcpu.'?

That makes sense, I'll respin after rc1.

> 
> > +	} else {
> > +		struct kvm_vcpu *host_vcpu = hyp_vcpu->host_vcpu;
> > +
> > +		memcpy(&hyp_kvm_vcpu->arch.fgt, host_vcpu->arch.fgt,
> > +		       sizeof(hyp_kvm_vcpu->arch.fgt));
> >  	}
> >  }
> >  
> > diff --git a/arch/arm64/kvm/hyp/nvhe/pkvm.c b/arch/arm64/kvm/hyp/nvhe/pkvm.c
> > index 43bde061b65d..05774aed09cb 100644
> > --- a/arch/arm64/kvm/hyp/nvhe/pkvm.c
> > +++ b/arch/arm64/kvm/hyp/nvhe/pkvm.c
> > @@ -172,7 +172,6 @@ static int pkvm_vcpu_init_traps(struct pkvm_hyp_vcpu *hyp_vcpu)
> >  
> >  		/* Trust the host for non-protected vcpu features. */
> >  		vcpu->arch.hcrx_el2 = host_vcpu->arch.hcrx_el2;
> > -		memcpy(vcpu->arch.fgt, host_vcpu->arch.fgt, sizeof(vcpu->arch.fgt));
> >  		return 0;
> >  	}
> 
> Otherwise, looks good to me:
> 
> Acked-by: Will Deacon <will@kernel.org>

Thanks,
Alex

Re: [PATCH 1/2] KVM: arm64: Copy FGT traps to unprotected pKVM VCPU on VCPU load

[Fuad Tabba]

Hi Alexandru,

On Wed, 10 Dec 2025 at 13:21, Alexandru Elisei <alexandru.elisei@arm.com> wrote:
>
> Commit fb10ddf35c1c ("KVM: arm64: Compute per-vCPU FGTs at vcpu_load()")
> introduced per-VCPU FGT traps. For an unprotected pKVM VCPU, the untrusted
> host FGT configuration is copied in pkvm_vcpu_init_traps(), which is called
> from __pkvm_init_vcpu(). __pkvm_init_vcpu() is called once per VCPU (when
> the VCPU is first run) which means that the uninitialized, zero, values for
> the FGT registers end up being used for the entire lifetime of the VCPU.
> This causes both unwanted traps (for the inverse polarity trap bits) and
> the guest being allowed to access registers it shouldn't.
>
> Fix it by copying the FGT traps for unprotected pKVM VCPUs when the
> untrusted host loads the VCPU.
>
> Fixes: fb10ddf35c1c ("KVM: arm64: Compute per-vCPU FGTs at vcpu_load()")
> Signed-off-by: Alexandru Elisei <alexandru.elisei@arm.com>
> ---
>  arch/arm64/kvm/hyp/nvhe/hyp-main.c | 12 ++++++++++--
>  arch/arm64/kvm/hyp/nvhe/pkvm.c     |  1 -
>  2 files changed, 10 insertions(+), 3 deletions(-)
>
> diff --git a/arch/arm64/kvm/hyp/nvhe/hyp-main.c b/arch/arm64/kvm/hyp/nvhe/hyp-main.c
> index 29430c031095..ee0f1343100c 100644
> --- a/arch/arm64/kvm/hyp/nvhe/hyp-main.c
> +++ b/arch/arm64/kvm/hyp/nvhe/hyp-main.c
> @@ -167,6 +167,7 @@ static void handle___pkvm_vcpu_load(struct kvm_cpu_context *host_ctxt)
>         DECLARE_REG(unsigned int, vcpu_idx, host_ctxt, 2);
>         DECLARE_REG(u64, hcr_el2, host_ctxt, 3);
>         struct pkvm_hyp_vcpu *hyp_vcpu;
> +       struct kvm_vcpu *hyp_kvm_vcpu;
>
>         if (!is_protected_kvm_enabled())
>                 return;
> @@ -175,10 +176,17 @@ static void handle___pkvm_vcpu_load(struct kvm_cpu_context *host_ctxt)
>         if (!hyp_vcpu)
>                 return;
>
> +       hyp_kvm_vcpu = &hyp_vcpu->vcpu;
> +
>         if (pkvm_hyp_vcpu_is_protected(hyp_vcpu)) {
>                 /* Propagate WFx trapping flags */
> -               hyp_vcpu->vcpu.arch.hcr_el2 &= ~(HCR_TWE | HCR_TWI);
> -               hyp_vcpu->vcpu.arch.hcr_el2 |= hcr_el2 & (HCR_TWE | HCR_TWI);
> +               hyp_kvm_vcpu->arch.hcr_el2 &= ~(HCR_TWE | HCR_TWI);
> +               hyp_kvm_vcpu->arch.hcr_el2 |= hcr_el2 & (HCR_TWE | HCR_TWI);
> +       } else {
> +               struct kvm_vcpu *host_vcpu = hyp_vcpu->host_vcpu;
> +
> +               memcpy(&hyp_kvm_vcpu->arch.fgt, host_vcpu->arch.fgt,
> +                      sizeof(hyp_kvm_vcpu->arch.fgt));
>         }
>  }

With the fix Will mentioned (hyp-main.c never uses the variable name
hyp_kvm_vcpu, it's vcpu for the hyp, or host_vcpu for the host's).

Tested-by: Fuad Tabba <tabba@google.com>
Reviewed-by: Fuad Tabba <tabba@google.com>

Cheers,
/fuad
>
> diff --git a/arch/arm64/kvm/hyp/nvhe/pkvm.c b/arch/arm64/kvm/hyp/nvhe/pkvm.c
> index 43bde061b65d..05774aed09cb 100644
> --- a/arch/arm64/kvm/hyp/nvhe/pkvm.c
> +++ b/arch/arm64/kvm/hyp/nvhe/pkvm.c
> @@ -172,7 +172,6 @@ static int pkvm_vcpu_init_traps(struct pkvm_hyp_vcpu *hyp_vcpu)
>
>                 /* Trust the host for non-protected vcpu features. */
>                 vcpu->arch.hcrx_el2 = host_vcpu->arch.hcrx_el2;
> -               memcpy(vcpu->arch.fgt, host_vcpu->arch.fgt, sizeof(vcpu->arch.fgt));
>                 return 0;
>         }
>
> --
> 2.52.0
>
>

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

[Alexandru Elisei]

__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);
 
-- 
2.52.0

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

[Marc Zyngier]

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?

Thanks,

	M.

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

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

[Alexandru Elisei]

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.

Thanks,
Alex

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

[Marc Zyngier]

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.