Re: [PATCH v6 07/25] KVM: arm64: iommu: Introduce IOMMU driver infrastructure

[Mostafa Saleh <smostafa@google.com>]

On Fri, Jul 10, 2026 at 05:17:32PM +0100, Vincent Donnefort wrote:
> On Fri, May 01, 2026 at 11:19:09AM +0000, Mostafa Saleh wrote:
> > To establish DMA isolation, KVM needs an IOMMU driver which provides
> > ops implemented at EL2.
> > 
> > Only one driver can be used and is registered with
> > kvm_iommu_register_driver() by passing pointer to the ops.
> > 
> > This must be called before module_init() which is the point KVM
> > initializes.
> 
> Everywhere s/KVM/pKVM/ as this can be registered only if pKVM is enabled.

Will do.

> 
> > 
> > Signed-off-by: Jean-Philippe Brucker <jean-philippe@linaro.org>
> > Signed-off-by: Mostafa Saleh <smostafa@google.com>
> > ---
> >  arch/arm64/include/asm/kvm_host.h       |  5 +++++
> >  arch/arm64/kvm/Makefile                 |  2 +-
> >  arch/arm64/kvm/hyp/include/nvhe/iommu.h | 13 +++++++++++++
> >  arch/arm64/kvm/hyp/nvhe/Makefile        |  3 ++-
> >  arch/arm64/kvm/hyp/nvhe/iommu/iommu.c   | 20 +++++++++++++++++++
> >  arch/arm64/kvm/hyp/nvhe/setup.c         |  5 +++++
> >  arch/arm64/kvm/iommu.c                  | 26 +++++++++++++++++++++++++
> >  7 files changed, 72 insertions(+), 2 deletions(-)
> >  create mode 100644 arch/arm64/kvm/hyp/include/nvhe/iommu.h
> >  create mode 100644 arch/arm64/kvm/hyp/nvhe/iommu/iommu.c
> >  create mode 100644 arch/arm64/kvm/iommu.c
> > 
> > diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
> > index 851f6171751c..52898d2a3ec6 100644
> > --- a/arch/arm64/include/asm/kvm_host.h
> > +++ b/arch/arm64/include/asm/kvm_host.h
> > @@ -1733,4 +1733,9 @@ static __always_inline enum fgt_group_id __fgt_reg_to_group_id(enum vcpu_sysreg
> >  
> >  long kvm_get_cap_for_kvm_ioctl(unsigned int ioctl, long *ext);
> >  
> > +#ifndef __KVM_NVHE_HYPERVISOR__
> 
> I think it's not necessary, a lot of things aren't guarded for
> __KVM_NVHE_HYPERVISOR__.

That makes it clear that it is hypervisor code, but no strong opinion.

> 
> > +struct kvm_iommu_ops;
> 
> Perhaps we can keep this private to hyp code by just using (void *) here?

That does not leak the struct, it is just a forward declaration, I
beleive it is more clear and readable this way.

> 
> > +int kvm_iommu_register_driver(struct kvm_iommu_ops *hyp_ops);
> > +#endif
> > +
> >  #endif /* __ARM64_KVM_HOST_H__ */
> > diff --git a/arch/arm64/kvm/Makefile b/arch/arm64/kvm/Makefile
> > index 59612d2f277c..0ddef54f7434 100644
> > --- a/arch/arm64/kvm/Makefile
> > +++ b/arch/arm64/kvm/Makefile
> > @@ -24,7 +24,7 @@ kvm-y += arm.o mmu.o mmio.o psci.o hypercalls.o pvtime.o \
> >  	 vgic/vgic-mmio.o vgic/vgic-mmio-v2.o \
> >  	 vgic/vgic-mmio-v3.o vgic/vgic-kvm-device.o \
> >  	 vgic/vgic-its.o vgic/vgic-debug.o vgic/vgic-v3-nested.o \
> > -	 vgic/vgic-v5.o
> > +	 vgic/vgic-v5.o iommu.o
> 
> alphabetical order.

That does not seem to be already sorted.

> 
> >  
> >  kvm-$(CONFIG_HW_PERF_EVENTS)  += pmu-emul.o pmu.o
> >  kvm-$(CONFIG_ARM64_PTR_AUTH)  += pauth.o
> > diff --git a/arch/arm64/kvm/hyp/include/nvhe/iommu.h b/arch/arm64/kvm/hyp/include/nvhe/iommu.h
> > new file mode 100644
> > index 000000000000..1ac70cc28a9e
> > --- /dev/null
> > +++ b/arch/arm64/kvm/hyp/include/nvhe/iommu.h
> > @@ -0,0 +1,13 @@
> > +/* SPDX-License-Identifier: GPL-2.0 */
> > +#ifndef __ARM64_KVM_NVHE_IOMMU_H__
> > +#define __ARM64_KVM_NVHE_IOMMU_H__
> > +
> > +#include <asm/kvm_host.h>
> > +
> > +struct kvm_iommu_ops {
> > +	int (*init)(void);
> > +};
> > +
> > +int kvm_iommu_init(void);
> > +
> > +#endif /* __ARM64_KVM_NVHE_IOMMU_H__ */
> > diff --git a/arch/arm64/kvm/hyp/nvhe/Makefile b/arch/arm64/kvm/hyp/nvhe/Makefile
> > index 89d0533921f9..606c0e1b7bd0 100644
> > --- a/arch/arm64/kvm/hyp/nvhe/Makefile
> > +++ b/arch/arm64/kvm/hyp/nvhe/Makefile
> > @@ -24,7 +24,8 @@ CFLAGS_switch.nvhe.o += -Wno-override-init
> >  
> >  hyp-obj-y := timer-sr.o sysreg-sr.o debug-sr.o switch.o tlb.o hyp-init.o host.o \
> >  	 hyp-main.o hyp-smp.o psci-relay.o early_alloc.o page_alloc.o \
> > -	 cache.o setup.o mm.o mem_protect.o sys_regs.o pkvm.o stacktrace.o ffa.o
> > +	 cache.o setup.o mm.o mem_protect.o sys_regs.o pkvm.o stacktrace.o ffa.o \
> > +	 iommu/iommu.o
> >  hyp-obj-y += ../vgic-v3-sr.o ../aarch32.o ../vgic-v2-cpuif-proxy.o ../entry.o \
> >  	 ../fpsimd.o ../hyp-entry.o ../exception.o ../pgtable.o ../vgic-v5-sr.o clock.o
> >  hyp-obj-y += ../../../kernel/smccc-call.o
> > diff --git a/arch/arm64/kvm/hyp/nvhe/iommu/iommu.c b/arch/arm64/kvm/hyp/nvhe/iommu/iommu.c
> > new file mode 100644
> > index 000000000000..406c8fb9b3b9
> > --- /dev/null
> > +++ b/arch/arm64/kvm/hyp/nvhe/iommu/iommu.c
> > @@ -0,0 +1,20 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * IOMMU operations for pKVM
> > + *
> > + * Copyright (C) 2022 Linaro Ltd.
> > + */
> > +#include <nvhe/iommu.h>
> > +
> > +/* Only one set of ops supported */
> > +struct kvm_iommu_ops *kvm_iommu_ops;
> > +
> > +
> 
> Empty line.
> 

Will remove it.

> > +int kvm_iommu_init(void)
> > +{
> > +	/* Keep DMA isolation optional. */
> > +	if (!kvm_iommu_ops || !kvm_iommu_ops->init)
> > +		return 0;
> 
> The absence of the SME workaround on affected systems breaks pKVM init. So
> perhaps the IOMMU should do the same? (see pkvm_check_sme_dvmsync_fw_call())
> 

I initially done something similar, but I was not sure if that is too
limiting as now pKVM will not run on many hardware, I think the
best option is to make this cmdline configurable wether DMA isolation
is required or not.
But for this series we can keep it optional.

> > +
> > +	return kvm_iommu_ops->init();
> > +}
> > diff --git a/arch/arm64/kvm/hyp/nvhe/setup.c b/arch/arm64/kvm/hyp/nvhe/setup.c
> > index 8041f6e80cd1..1f6b221db9a0 100644
> > --- a/arch/arm64/kvm/hyp/nvhe/setup.c
> > +++ b/arch/arm64/kvm/hyp/nvhe/setup.c
> > @@ -14,6 +14,7 @@
> >  #include <nvhe/early_alloc.h>
> >  #include <nvhe/ffa.h>
> >  #include <nvhe/gfp.h>
> > +#include <nvhe/iommu.h>
> >  #include <nvhe/memory.h>
> >  #include <nvhe/mem_protect.h>
> >  #include <nvhe/mm.h>
> > @@ -329,6 +330,10 @@ void __noreturn __pkvm_init_finalise(void)
> >  	if (ret)
> >  		goto out;
> >  
> > +	ret = kvm_iommu_init();
> > +	if (ret)
> > +		goto out;
> > +
> >  	ret = hyp_ffa_init(ffa_proxy_pages);
> >  	if (ret)
> >  		goto out;
> > diff --git a/arch/arm64/kvm/iommu.c b/arch/arm64/kvm/iommu.c
> > new file mode 100644
> > index 000000000000..f247384fa193
> > --- /dev/null
> > +++ b/arch/arm64/kvm/iommu.c
> > @@ -0,0 +1,26 @@
> > +// SPDX-License-Identifier: GPL-2.0-only
> > +/*
> > + * Copyright (C) 2025 Google LLC
> > + * Author: Mostafa Saleh <smostafa@google.com>
> > + */
> > +
> > +#include <linux/kvm_host.h>
> > +
> > +extern struct kvm_iommu_ops *kvm_nvhe_sym(kvm_iommu_ops);
> > +
> > +static DEFINE_MUTEX(kvm_iommu_reg_lock);
> 
> could be moved directly into the only user: kvm_iommu_register_driver

No strong opinion, it seemed it can be easily re-used this way.

> 
> > +
> > +int kvm_iommu_register_driver(struct kvm_iommu_ops *hyp_ops)
> 
> __init ?

Will do.

> 
> > +{
> > +	guard(mutex)(&kvm_iommu_reg_lock);
> > +
> > +	/* Only protected KVM before de-privilege. */
> 
> Don't think that comment brings much information, looking at the line below.

I will remove it.

> 
> > +	if (!is_protected_kvm_enabled() || is_kvm_arm_initialised())
> > +		return -EINVAL;
> 
> -EPERM?

I can see it both ways, my thought that it is invalid to register a
driver after a certain point, but -EPERM makes sense also.

Thanks,
Mostafa