Re: [PATCH v8 04/25] iommu/arm-smmu-v3: Move IDR parsing to common functions
flat view
From: Nicolin Chen <hidden>
Date: 2026-09-22 19:46:01
Also in:
kvmarm, linux-iommu, lkml
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\.
quoted hunk ↗ jump to hunk
+#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 :)
quoted hunk ↗ jump to hunk
+ */ +#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...
quoted hunk ↗ jump to hunk
+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?
quoted hunk ↗ jump to hunk
+ if (coherent && !disable_msipolling && + smmu->features & ARM_SMMU_FEAT_MSI) + smmu->options |= ARM_SMMU_OPT_MSIPOLL;
Will pKVM ever use MSIPOLL?
quoted hunk ↗ jump to hunk
+ 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?
quoted hunk ↗ jump to hunk
- 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