Re: [PATCH v8 04/25] iommu/arm-smmu-v3: Move IDR parsing to common functions

[Nicolin Chen]

On Tue, Sep 22, 2026 at 01:12:37PM +0000, Mostafa Saleh wrote:
> Move parsing of IDRs to functions so that it can be re-used
> from the hypervisor.
> 
> As the new functions operate on structs from both the hypervisor
> and the kernel which would be different, we rely on the compilation
> unit to having ARM_SMMU_OBJ point to the correct struct; some

s/to having/to have

> best-effort static asserts were added .

s/added \./added\.

> +#ifndef __ARM_SMMU_V3_COMMON_LIB_H
> +#define __ARM_SMMU_V3_COMMON_LIB_H
> +
> +#include <linux/build_bug.h>
> +#include <linux/compiler_types.h>
> +#include <linux/kernel.h>
> +
> +/*
> + * The IDR probe functions are used by the kernel and the
> + * hypervisor drivers where ARM_SMMU_OBJ might be defined
> + * differently.
> + * Ensure fields used by them are defined and has the correct
> + * types.

s/has/have

We have 80 cols per line to write comments :)

> + */
> +#ifndef __KVM_NVHE_HYPERVISOR__
> +typedef struct arm_smmu_device ARM_SMMU_OBJ;
> +#endif

It's probably safer to include arm-smmu-v3.h so everything would
be self-defined.

Also, Jason's suggestion in v7 was hyp_arm_smmu_v3_device, which
looks nicer than ARM_SMMU_OBJ...

> +static_assert(__same_type(typeof_member(ARM_SMMU_OBJ, features), u32));
> +static_assert(__same_type(typeof_member(ARM_SMMU_OBJ, options), u32));
> +static_assert(__same_type(typeof_member(ARM_SMMU_OBJ, oas), unsigned long));
> +static_assert(__same_type(typeof_member(ARM_SMMU_OBJ, pgsize_bitmap), unsigned long));
> +static_assert(__same_type(typeof_member(ARM_SMMU_OBJ, base), void __iomem *));
> +
> +void arm_smmu_device_iidr_probe(ARM_SMMU_OBJ *smmu);
> +u32 arm_smmu_idr0_probe(ARM_SMMU_OBJ *smmu);
> +void arm_smmu_idr3_probe(ARM_SMMU_OBJ *smmu);
> +u32 arm_smmu_idr5_probe(ARM_SMMU_OBJ *smmu);

Can we use "arm_smmu_device_xyz_probe" matching with the existing
arm_smmu_device_iidr_probe?

> +	if (coherent && !disable_msipolling &&
> +	    smmu->features & ARM_SMMU_FEAT_MSI)
> +		smmu->options |= ARM_SMMU_OPT_MSIPOLL;

Will pKVM ever use MSIPOLL?

> +	if (smmu->features & ARM_SMMU_FEAT_HYP &&
> +	    cpus_have_cap(ARM64_HAS_VIRT_HOST_EXTN))
> +		smmu->features |= ARM_SMMU_FEAT_E2H;

Why is ARM64_HAS_VIRT_HOST_EXTN left behind?

> -	if (!(reg & (IDR0_S1P | IDR0_S2P))) {
> +	if (!(smmu->features & (ARM_SMMU_FEAT_TRANS_S1 | ARM_SMMU_FEAT_TRANS_S2))) {
>  		dev_err(smmu->dev, "no translation support!\n");
>  		return -ENXIO;

This change seems unnecessary. The code above and below this line
still uses "reg" returned by idr0_probe(). So, the original code
should have read well:

        if (!!(reg & IDR0_COHACC) != coherent)
                dev_warn(smmu->dev, "IDR0.COHACC overridden by FW configuration (%s)\n",
                         str_true_false(coherent));

	if (!(reg & (IDR0_S1P | IDR0_S2P))) {
                dev_err(smmu->dev, "no translation support!\n");
                return -ENXIO;
        }

        /* We only support the AArch64 table format at present */
        if (!(FIELD_GET(IDR0_TTF, reg) & IDR0_TTF_AARCH64)) {
                dev_err(smmu->dev, "AArch64 table format not supported!\n");
                return -ENXIO;
        }

Nicolin