Re: [PATCH v8 12/25] iommu/arm-smmu-v3-kvm: Probe SMMU HW

[Mostafa Saleh <smostafa@google.com>]

On Tue, Sep 22, 2026 at 06:43:44PM -0700, Nicolin Chen wrote:
> On Tue, Sep 22, 2026 at 01:12:45PM +0000, Mostafa Saleh wrote:
> > +++ b/drivers/iommu/arm/arm-smmu-v3/pkvm/arm-smmu-v3-hyp.h
> > @@ -11,6 +11,9 @@
> >   *
> >   * Other members are filled and used at runtime by the SMMU driver.
> >   * @base		Virtual address of SMMU registers
> > + * @oas			PA size
> > + * @pgsize_bitmap	Supported page sizes
> > + * @sid_bits		Max number of SID bits supported
> >   */
> 
> Some of them belongs to one of the prior patches.
> 
> And, if we are going to list them all, could we do in kdoc style?

I will fix it.

> 
> > +++ b/drivers/iommu/arm/arm-smmu-v3/pkvm/arm-smmu-v3.c
> > @@ -29,6 +29,47 @@ static void smmu_deinit_device(struct hyp_arm_smmu_v3_device *smmu)
> >  	smmu->base = NULL;
> >  }
> >  
> > +/*
> > + * Mini-probe and validation for the hypervisor.
> > + */
> > +static int smmu_probe(struct hyp_arm_smmu_v3_device *smmu)
> > +{
> > +	u32 reg;
> > +
> > +	if (!(smmu->features & ARM_SMMU_FEAT_COHERENCY))
> > +		return -EINVAL;
> 
> Does this need to check IDR0.COHACC? If not, an inline note that
> explains why?
> 

This is similar to the kernel driver logic, where FW will override
coherency and in case of mismatch the driver will just WARN and trust
the FW.
pKVM can't print a warning as WARN is fatal in EL2. So it just picks
what the FW says. I am not sure what systems rely on this, but I can
add more strict checks and fail probe in case of mismatch with pKVM.

> > +
> > +	reg = arm_smmu_idr0_probe(smmu);
> > +
> > +	if (!(FIELD_GET(IDR0_TTF, reg) & IDR0_TTF_AARCH64))
> > +		return -ENXIO;
> > +
> > +	if (!(smmu->features & (ARM_SMMU_FEAT_TT_LE | ARM_SMMU_FEAT_TT_BE)))
> > +		return -ENXIO;
> > +
> > +	reg = readl_relaxed(smmu->base + ARM_SMMU_IDR1);
> > +	if (reg & (IDR1_TABLES_PRESET | IDR1_QUEUES_PRESET | IDR1_REL))
> > +		return -EINVAL;
> > +
> > +	smmu->sid_bits = FIELD_GET(IDR1_SIDSIZE, reg);
> > +	/* Follows the kernel logic */
> > +	if (smmu->sid_bits <= STRTAB_SPLIT)
> > +		smmu->features &= ~ARM_SMMU_FEAT_2_LVL_STRTAB;
> 
> The IDR1 reg guard and sid_bits can be shared with the main driver.
> 
> Maybe add an arm_smmu_device_idr1_probe()?
> 

I thought about that, but this will be only used for sid_bits, the
hypervisor struct doesn't have ssid_bits as it doesn't use it, it
only shadows the STE and not the CD as the CD is translated by stage-2.

Other fields also not used by the hypervisor (PRIQS, EVTQS...)

As sashiko pointed, I will need to add ARM_SMMU_FEAT_ATTR_TYPES_OVR
check in the hypervisor, so maybe arm_smmu_device_idr1_probe() can be added
to partially probe the register and return the value so it can be probed the
main driver.

> > +
> > +	arm_smmu_idr3_probe(smmu);
> > +
> > +	arm_smmu_idr5_probe(smmu);
> > +	if (!smmu->oas)
> > +		smmu->oas = 48;
> 
> The truncation can be shared with the main driver too?

The main driver will print an error, if arm_smmu_idr5_probe() fix it
silently we will lose that.

> 
> > @@ -43,8 +84,14 @@ static int smmu_init_device(struct hyp_arm_smmu_v3_device *smmu)
> >  		return ret;
> >  
> >  	smmu->base = (void __iomem *)haddr;
> > +	ret = smmu_probe(smmu);
> > +	if (ret)
> > +		goto out_ret;
> >  
> >  	return 0;
> > +out_ret:
> > +	smmu_deinit_device(smmu);
> 
> Nit: out_deinit

Sure.

Thanks,
Mostafa

> 
> Nicolin