Re: [PATCH v8 10/25] iommu/arm-smmu-v3-kvm: Add SMMUv3 driver

[Mostafa Saleh <smostafa@google.com>]

On Tue, Sep 22, 2026 at 03:39:04PM -0700, Nicolin Chen wrote:
> On Tue, Sep 22, 2026 at 01:12:43PM +0000, Mostafa Saleh wrote:
> > From: Jean-Philippe Brucker <jean-philippe@linaro.org>
> > 
> > Add the skeleton for an Arm SMMUv3 driver at EL2.
> > 
> > The driver rely on an array of SMMUv3s on the system, where at
> 
> s/rely/relies

Will do.

> 
> > +++ b/drivers/iommu/arm/Kconfig
> > @@ -141,3 +141,15 @@ config QCOM_IOMMU
> >  	select ARM_DMA_USE_IOMMU
> >  	help
> >  	  Support for IOMMU on certain Qualcomm SoCs.
> > +
> > +config ARM_SMMU_V3_PKVM
> > +	bool "ARM SMMUv3 support for protected Virtual Machines"
> > +	depends on KVM && ARM_SMMU_V3=y
> 
> Should it depend on OF?

Makes sense, I will add it.

> 
> > +	help
> > +	  Enable a SMMUv3 driver in the KVM hypervisor, to protect VMs against
> 
> s/a SMMUv3/an SMMUv3

Will do.

> 
> > +++ b/drivers/iommu/arm/arm-smmu-v3/pkvm/arm-smmu-v3-hyp.h
> > @@ -0,0 +1,31 @@
> > +/* SPDX-License-Identifier: GPL-2.0 */
> > +#ifndef __KVM_ARM_SMMU_V3_HYP_H
> > +#define __KVM_ARM_SMMU_V3_HYP_H
> > +
> > +#include <asm/kvm_asm.h>
> > +
> > +/*
> > + * Parameters from the trusted host:
> > + * @mmio_addr		base address of the SMMU registers
> > + * @mmio_size		size of the registers resource
> 
> Are these still in the host's physical address space or guest's?
> 
> If it's still "host" (though trusted), what's different from the
> ioaddr in the main driver?

This is the physical address of the SMMUv3 as read from the device
tree.

The "base" pointer is for the hypervisor virtual address which is
created in the private mapping range (similar to ioremap())

The hypervisor doesn't keep the host VA anywhere. Also note that
there is no guest support at the moment, only the host.

> 
> > +size_t __ro_after_init kvm_hyp_arm_smmu_v3_count;
> > +struct hyp_arm_smmu_v3_device *kvm_hyp_arm_smmu_v3_smmus;
> 
> Should kvm_hyp_arm_smmu_v3_smmus be __ro_after_init as well?

That won't work at the moment as the hypervisor writes this pointer
after __ro_after_init to convert the kernel VA to hypervisor VA
unlike the count which is set once at boot.
It might be possible to make the kernel do this conversion early, I
will need to double check.

It's worth noting that __ro_after_init is just for the hypervisor
hardening and not for protection as those are protected by stage-2
MMU.

> 
> > +
> > +#define for_each_smmu(smmu) \
> > +	for ((smmu) = kvm_hyp_arm_smmu_v3_smmus; \
> > +	     (smmu) != &kvm_hyp_arm_smmu_v3_smmus[kvm_hyp_arm_smmu_v3_count]; \
> > +	     (smmu)++)
> 
> "smmu" sounds too generic. Maybe for_each_pkvm_smmu?

This macro is private to this driver, so I guess that's enough, also
smmu is used everywhere else, but no strong opinion.

> 
> > +/* Called while is the host is still trusted. */
> > +static int smmu_init(void)
> 
> s/while is/while

Will do.

> 
> > +/* Shared with the kernel driver in EL1 */
> > +struct pkvm_iommu_ops smmu_ops = {
> > +	.init				= smmu_init,
> > +	.host_stage2_idmap		= smmu_host_stage2_idmap,
> 
> Can we add a "pvkm_arm_smmu_" prefix for the ops and functions here?

Similar to above, these symbols are private to this file and the
hypervisor symbols gets prefixed with "__kvm_nvhe_" anyway, so they
never clash with the kernel. But no strong opinon.

Thanks,
Mostafa

> 
> Nicolin