Re: [PATCH] iommu/arm: Expose ARM SMMUv3 related registers via sysfs

From: Miguel Luis
Date: Wed Mar 23 2022 - 11:09:50 EST

Next message: Matthew Wilcox: "Re: [PATCH v6 27/39] kasan, mm: only define ___GFP_SKIP_KASAN_POISON with HW_TAGS"
Previous message: Philipp Zabel: "Re: [PATCH 0/2] add fwnode support to reset subsystem"
In reply to: Robin Murphy: "Re: [PATCH] iommu/arm: Expose ARM SMMUv3 related registers via sysfs"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

> On 23 Mar 2022, at 12:40, Robin Murphy <robin.murphy@xxxxxxx> wrote:
>
> On 2022-03-23 12:54, Miguel Luis wrote:
>> Allows userspace to check for SMMUv3 features.
>
> What will userspace do with that information?
>
> It hardly matters what fancy new features might be present, if the kernel and/or the abstracted interfaces available to userspace aren't using them. Any functionality which is supported by a usable interface should ideally be represented via that interface itself.
>

The inspiration was the same that Intel (cap/ecap) and AMD (cap/features) took
exposing it's iommu feature registers on sysfs. It's an easy way to understand
which features are supported by the hardware regardless of what the kernel
supports.

For example I could print the smmu->features and that would cover kernel
supported features but wouldn't help when new hardware arrives to know which
features are supported by the hardware.

> Furthermore many of the raw SMMU features depend on other system components and/or firmware, so the ID registers alone don't tell the full story anyway.
>

Would you mind to elaborate a bit more on that please? Would that mean that if a
feature bit is set it doesn’t really tell that the feature is supported?

>> Expose the following RO registers related to ARM SMMUv3 via sysfs:
>> SMMU_IDR0
>> SMMU_IDR1
>> SMMU_IDR2
>> SMMU_IDR3
>> SMMU_IDR4
>> SMMU_IDR5
>> SMMU_IDR6
>> SMMU_IIDR
>> SMMU_AIDR
>> # find /sys | grep arm-iommu
>> /sys/devices/platform/9050000.smmuv3/iommu/smmu3.0x0000000009050000/arm-iommu
>
> Nit: my main comment above notwithstanding, is this level of hierarchcy meaningful or useful? "arm-iommu" isn't an established name for anything as far as I'm aware.
>

I've followed the existent convention in other IOMMUs but I'm totally open
to alternatives/suggestions.

Miguel

> Robin.
>
>> /sys/devices/platform/9050000.smmuv3/iommu/smmu3.0x0000000009050000/arm-iommu/smmu_idr5
>> /sys/devices/platform/9050000.smmuv3/iommu/smmu3.0x0000000009050000/arm-iommu/smmu_idr3
>> /sys/devices/platform/9050000.smmuv3/iommu/smmu3.0x0000000009050000/arm-iommu/smmu_idr1
>> /sys/devices/platform/9050000.smmuv3/iommu/smmu3.0x0000000009050000/arm-iommu/smmu_aidr
>> /sys/devices/platform/9050000.smmuv3/iommu/smmu3.0x0000000009050000/arm-iommu/smmu_idr6
>> /sys/devices/platform/9050000.smmuv3/iommu/smmu3.0x0000000009050000/arm-iommu/smmu_idr4
>> /sys/devices/platform/9050000.smmuv3/iommu/smmu3.0x0000000009050000/arm-iommu/smmu_iidr
>> /sys/devices/platform/9050000.smmuv3/iommu/smmu3.0x0000000009050000/arm-iommu/smmu_idr2
>> /sys/devices/platform/9050000.smmuv3/iommu/smmu3.0x0000000009050000/arm-iommu/smmu_idr0
>> Signed-off-by: Miguel Luis <miguel.luis@xxxxxxxxxx>
>> ---
>> drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 199 ++++++++++++++++----
>> drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h | 27 +++
>> 2 files changed, 191 insertions(+), 35 deletions(-)
>> diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
>> index 6dc6d8b6b368..7f779d3f88f2 100644
>> --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
>> +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
>> @@ -3424,17 +3424,16 @@ static int arm_smmu_device_reset(struct arm_smmu_device *smmu, bool bypass)
>> static int arm_smmu_device_hw_probe(struct arm_smmu_device *smmu)
>> {
>> - u32 reg;
>> bool coherent = smmu->features & ARM_SMMU_FEAT_COHERENCY;
>> /* IDR0 */
>> - reg = readl_relaxed(smmu->base + ARM_SMMU_IDR0);
>> + smmu->idr0 = readl_relaxed(smmu->base + ARM_SMMU_IDR0);
>> /* 2-level structures */
>> - if (FIELD_GET(IDR0_ST_LVL, reg) == IDR0_ST_LVL_2LVL)
>> + if (FIELD_GET(IDR0_ST_LVL, smmu->idr0) == IDR0_ST_LVL_2LVL)
>> smmu->features |= ARM_SMMU_FEAT_2_LVL_STRTAB;
>> - if (reg & IDR0_CD2L)
>> + if (smmu->idr0 & IDR0_CD2L)
>> smmu->features |= ARM_SMMU_FEAT_2_LVL_CDTAB;
>> /*
>> @@ -3442,7 +3441,7 @@ static int arm_smmu_device_hw_probe(struct arm_smmu_device *smmu)
>> * We currently require the same endianness as the CPU, but this
>> * could be changed later by adding a new IO_PGTABLE_QUIRK.
>> */
>> - switch (FIELD_GET(IDR0_TTENDIAN, reg)) {
>> + switch (FIELD_GET(IDR0_TTENDIAN, smmu->idr0)) {
>> case IDR0_TTENDIAN_MIXED:
>> smmu->features |= ARM_SMMU_FEAT_TT_LE | ARM_SMMU_FEAT_TT_BE;
>> break;
>> @@ -3461,22 +3460,22 @@ static int arm_smmu_device_hw_probe(struct arm_smmu_device *smmu)
>> }
>> /* Boolean feature flags */
>> - if (IS_ENABLED(CONFIG_PCI_PRI) && reg & IDR0_PRI)
>> + if (IS_ENABLED(CONFIG_PCI_PRI) && smmu->idr0 & IDR0_PRI)
>> smmu->features |= ARM_SMMU_FEAT_PRI;
>> - if (IS_ENABLED(CONFIG_PCI_ATS) && reg & IDR0_ATS)
>> + if (IS_ENABLED(CONFIG_PCI_ATS) && smmu->idr0 & IDR0_ATS)
>> smmu->features |= ARM_SMMU_FEAT_ATS;
>> - if (reg & IDR0_SEV)
>> + if (smmu->idr0 & IDR0_SEV)
>> smmu->features |= ARM_SMMU_FEAT_SEV;
>> - if (reg & IDR0_MSI) {
>> + if (smmu->idr0 & IDR0_MSI) {
>> smmu->features |= ARM_SMMU_FEAT_MSI;
>> if (coherent && !disable_msipolling)
>> smmu->options |= ARM_SMMU_OPT_MSIPOLL;
>> }
>> - if (reg & IDR0_HYP) {
>> + if (smmu->idr0 & IDR0_HYP) {
>> smmu->features |= ARM_SMMU_FEAT_HYP;
>> if (cpus_have_cap(ARM64_HAS_VIRT_HOST_EXTN))
>> smmu->features |= ARM_SMMU_FEAT_E2H;
>> @@ -3486,11 +3485,11 @@ static int arm_smmu_device_hw_probe(struct arm_smmu_device *smmu)
>> * The coherency feature as set by FW is used in preference to the ID
>> * register, but warn on mismatch.
>> */
>> - if (!!(reg & IDR0_COHACC) != coherent)
>> + if (!!(smmu->idr0 & IDR0_COHACC) != coherent)
>> dev_warn(smmu->dev, "IDR0.COHACC overridden by FW configuration (%s)\n",
>> coherent ? "true" : "false");
>> - switch (FIELD_GET(IDR0_STALL_MODEL, reg)) {
>> + switch (FIELD_GET(IDR0_STALL_MODEL, smmu->idr0)) {
>> case IDR0_STALL_MODEL_FORCE:
>> smmu->features |= ARM_SMMU_FEAT_STALL_FORCE;
>> fallthrough;
>> @@ -3498,19 +3497,19 @@ static int arm_smmu_device_hw_probe(struct arm_smmu_device *smmu)
>> smmu->features |= ARM_SMMU_FEAT_STALLS;
>> }
>> - if (reg & IDR0_S1P)
>> + if (smmu->idr0 & IDR0_S1P)
>> smmu->features |= ARM_SMMU_FEAT_TRANS_S1;
>> - if (reg & IDR0_S2P)
>> + if (smmu->idr0 & IDR0_S2P)
>> smmu->features |= ARM_SMMU_FEAT_TRANS_S2;
>> - if (!(reg & (IDR0_S1P | IDR0_S2P))) {
>> + if (!(smmu->idr0 & (IDR0_S1P | IDR0_S2P))) {
>> dev_err(smmu->dev, "no translation support!\n");
>> return -ENXIO;
>> }
>> /* We only support the AArch64 table format at present */
>> - switch (FIELD_GET(IDR0_TTF, reg)) {
>> + switch (FIELD_GET(IDR0_TTF, smmu->idr0)) {
>> case IDR0_TTF_AARCH32_64:
>> smmu->ias = 40;
>> fallthrough;
>> @@ -3522,19 +3521,20 @@ static int arm_smmu_device_hw_probe(struct arm_smmu_device *smmu)
>> }
>> /* ASID/VMID sizes */
>> - smmu->asid_bits = reg & IDR0_ASID16 ? 16 : 8;
>> - smmu->vmid_bits = reg & IDR0_VMID16 ? 16 : 8;
>> + smmu->asid_bits = smmu->idr0 & IDR0_ASID16 ? 16 : 8;
>> + smmu->vmid_bits = smmu->idr0 & IDR0_VMID16 ? 16 : 8;
>> /* IDR1 */
>> - reg = readl_relaxed(smmu->base + ARM_SMMU_IDR1);
>> - if (reg & (IDR1_TABLES_PRESET | IDR1_QUEUES_PRESET | IDR1_REL)) {
>> + smmu->idr1 = readl_relaxed(smmu->base + ARM_SMMU_IDR1);
>> +
>> + if (smmu->idr1 & (IDR1_TABLES_PRESET | IDR1_QUEUES_PRESET | IDR1_REL)) {
>> dev_err(smmu->dev, "embedded implementation not supported\n");
>> return -ENXIO;
>> }
>> /* Queue sizes, capped to ensure natural alignment */
>> smmu->cmdq.q.llq.max_n_shift = min_t(u32, CMDQ_MAX_SZ_SHIFT,
>> - FIELD_GET(IDR1_CMDQS, reg));
>> + FIELD_GET(IDR1_CMDQS, smmu->idr1));
>> if (smmu->cmdq.q.llq.max_n_shift <= ilog2(CMDQ_BATCH_ENTRIES)) {
>> /*
>> * We don't support splitting up batches, so one batch of
>> @@ -3548,13 +3548,13 @@ static int arm_smmu_device_hw_probe(struct arm_smmu_device *smmu)
>> }
>> smmu->evtq.q.llq.max_n_shift = min_t(u32, EVTQ_MAX_SZ_SHIFT,
>> - FIELD_GET(IDR1_EVTQS, reg));
>> + FIELD_GET(IDR1_EVTQS, smmu->idr1));
>> smmu->priq.q.llq.max_n_shift = min_t(u32, PRIQ_MAX_SZ_SHIFT,
>> - FIELD_GET(IDR1_PRIQS, reg));
>> + FIELD_GET(IDR1_PRIQS, smmu->idr1));
>> /* SID/SSID sizes */
>> - smmu->ssid_bits = FIELD_GET(IDR1_SSIDSIZE, reg);
>> - smmu->sid_bits = FIELD_GET(IDR1_SIDSIZE, reg);
>> + smmu->ssid_bits = FIELD_GET(IDR1_SSIDSIZE, smmu->idr1);
>> + smmu->sid_bits = FIELD_GET(IDR1_SIDSIZE, smmu->idr1);
>> /*
>> * If the SMMU supports fewer bits than would fill a single L2 stream
>> @@ -3563,31 +3563,37 @@ static int arm_smmu_device_hw_probe(struct arm_smmu_device *smmu)
>> if (smmu->sid_bits <= STRTAB_SPLIT)
>> smmu->features &= ~ARM_SMMU_FEAT_2_LVL_STRTAB;
>> + /* IDR2 */
>> + smmu->idr2 = readl_relaxed(smmu->base + ARM_SMMU_IDR2);
>> +
>> /* IDR3 */
>> - reg = readl_relaxed(smmu->base + ARM_SMMU_IDR3);
>> - if (FIELD_GET(IDR3_RIL, reg))
>> + smmu->idr3 = readl_relaxed(smmu->base + ARM_SMMU_IDR3);
>> + if (FIELD_GET(IDR3_RIL, smmu->idr3))
>> smmu->features |= ARM_SMMU_FEAT_RANGE_INV;
>> + /* IDR4 */
>> + smmu->idr4 = readl_relaxed(smmu->base + ARM_SMMU_IDR4);
>> +
>> /* IDR5 */
>> - reg = readl_relaxed(smmu->base + ARM_SMMU_IDR5);
>> + smmu->idr5 = readl_relaxed(smmu->base + ARM_SMMU_IDR5);
>> /* Maximum number of outstanding stalls */
>> - smmu->evtq.max_stalls = FIELD_GET(IDR5_STALL_MAX, reg);
>> + smmu->evtq.max_stalls = FIELD_GET(IDR5_STALL_MAX, smmu->idr5);
>> /* Page sizes */
>> - if (reg & IDR5_GRAN64K)
>> + if (smmu->idr5 & IDR5_GRAN64K)
>> smmu->pgsize_bitmap |= SZ_64K | SZ_512M;
>> - if (reg & IDR5_GRAN16K)
>> + if (smmu->idr5 & IDR5_GRAN16K)
>> smmu->pgsize_bitmap |= SZ_16K | SZ_32M;
>> - if (reg & IDR5_GRAN4K)
>> + if (smmu->idr5 & IDR5_GRAN4K)
>> smmu->pgsize_bitmap |= SZ_4K | SZ_2M | SZ_1G;
>> /* Input address size */
>> - if (FIELD_GET(IDR5_VAX, reg) == IDR5_VAX_52_BIT)
>> + if (FIELD_GET(IDR5_VAX, smmu->idr5) == IDR5_VAX_52_BIT)
>> smmu->features |= ARM_SMMU_FEAT_VAX;
>> /* Output address size */
>> - switch (FIELD_GET(IDR5_OAS, reg)) {
>> + switch (FIELD_GET(IDR5_OAS, smmu->idr5)) {
>> case IDR5_OAS_32_BIT:
>> smmu->oas = 32;
>> break;
>> @@ -3632,6 +3638,16 @@ static int arm_smmu_device_hw_probe(struct arm_smmu_device *smmu)
>> dev_info(smmu->dev, "ias %lu-bit, oas %lu-bit (features 0x%08x)\n",
>> smmu->ias, smmu->oas, smmu->features);
>> +
>> + /* IDR6 */
>> + smmu->idr6 = readl_relaxed(smmu->base + ARM_SMMU_IDR6);
>> +
>> + /* IIDR */
>> + smmu->iidr = readl_relaxed(smmu->base + ARM_SMMU_IIDR);
>> +
>> + /* AIDR */
>> + smmu->aidr = readl_relaxed(smmu->base + ARM_SMMU_AIDR);
>> +
>> return 0;
>> }
>> @@ -3752,6 +3768,119 @@ static void __iomem *arm_smmu_ioremap(struct device *dev, resource_size_t start,
>> return devm_ioremap_resource(dev, &res);
>> }
>> +static ssize_t smmu_idr0_show(struct device *dev,
>> + struct device_attribute *attr,
>> + char *buf)
>> +{
>> + struct arm_smmu_device *smmu = dev_to_arm_smmu_device(dev);
>> +
>> + return sprintf(buf, "%x\n", smmu->idr0);
>> +}
>> +static DEVICE_ATTR_RO(smmu_idr0);
>> +
>> +static ssize_t smmu_idr1_show(struct device *dev,
>> + struct device_attribute *attr,
>> + char *buf)
>> +{
>> + struct arm_smmu_device *smmu = dev_to_arm_smmu_device(dev);
>> +
>> + return sprintf(buf, "%x\n", smmu->idr1);
>> +}
>> +static DEVICE_ATTR_RO(smmu_idr1);
>> +
>> +static ssize_t smmu_idr2_show(struct device *dev,
>> + struct device_attribute *attr,
>> + char *buf)
>> +{
>> + struct arm_smmu_device *smmu = dev_to_arm_smmu_device(dev);
>> +
>> + return sprintf(buf, "%x\n", smmu->idr2);
>> +}
>> +static DEVICE_ATTR_RO(smmu_idr2);
>> +
>> +static ssize_t smmu_idr3_show(struct device *dev,
>> + struct device_attribute *attr,
>> + char *buf)
>> +{
>> + struct arm_smmu_device *smmu = dev_to_arm_smmu_device(dev);
>> +
>> + return sprintf(buf, "%x\n", smmu->idr3);
>> +}
>> +static DEVICE_ATTR_RO(smmu_idr3);
>> +
>> +static ssize_t smmu_idr4_show(struct device *dev,
>> + struct device_attribute *attr,
>> + char *buf)
>> +{
>> + struct arm_smmu_device *smmu = dev_to_arm_smmu_device(dev);
>> +
>> + return sprintf(buf, "%x\n", smmu->idr4);
>> +}
>> +static DEVICE_ATTR_RO(smmu_idr4);
>> +
>> +static ssize_t smmu_idr5_show(struct device *dev,
>> + struct device_attribute *attr,
>> + char *buf)
>> +{
>> + struct arm_smmu_device *smmu = dev_to_arm_smmu_device(dev);
>> +
>> + return sprintf(buf, "%x\n", smmu->idr5);
>> +}
>> +static DEVICE_ATTR_RO(smmu_idr5);
>> +
>> +static ssize_t smmu_idr6_show(struct device *dev,
>> + struct device_attribute *attr,
>> + char *buf)
>> +{
>> + struct arm_smmu_device *smmu = dev_to_arm_smmu_device(dev);
>> +
>> + return sprintf(buf, "%x\n", smmu->idr6);
>> +}
>> +static DEVICE_ATTR_RO(smmu_idr6);
>> +
>> +static ssize_t smmu_iidr_show(struct device *dev,
>> + struct device_attribute *attr,
>> + char *buf)
>> +{
>> + struct arm_smmu_device *smmu = dev_to_arm_smmu_device(dev);
>> +
>> + return sprintf(buf, "%x\n", smmu->iidr);
>> +}
>> +static DEVICE_ATTR_RO(smmu_iidr);
>> +
>> +static ssize_t smmu_aidr_show(struct device *dev,
>> + struct device_attribute *attr,
>> + char *buf)
>> +{
>> + struct arm_smmu_device *smmu = dev_to_arm_smmu_device(dev);
>> +
>> + return sprintf(buf, "%x\n", smmu->aidr);
>> +}
>> +static DEVICE_ATTR_RO(smmu_aidr);
>> +
>> +static struct attribute *arm_iommu_attrs[] = {
>> + &dev_attr_smmu_idr0.attr,
>> + &dev_attr_smmu_idr1.attr,
>> + &dev_attr_smmu_idr2.attr,
>> + &dev_attr_smmu_idr3.attr,
>> + &dev_attr_smmu_idr4.attr,
>> + &dev_attr_smmu_idr5.attr,
>> + &dev_attr_smmu_idr6.attr,
>> + &dev_attr_smmu_iidr.attr,
>> + &dev_attr_smmu_aidr.attr,
>> + NULL,
>> +};
>> +
>> +static struct attribute_group arm_iommu_group = {
>> + .name = "arm-iommu",
>> + .attrs = arm_iommu_attrs,
>> +};
>> +
>> +static const struct attribute_group *arm_iommu_groups[] = {
>> + &arm_iommu_group,
>> + NULL,
>> +};
>> +
>> static int arm_smmu_device_probe(struct platform_device *pdev)
>> {
>> int irq, ret;
>> @@ -3839,7 +3968,7 @@ static int arm_smmu_device_probe(struct platform_device *pdev)
>> return ret;
>> /* And we're up. Go go go! */
>> - ret = iommu_device_sysfs_add(&smmu->iommu, dev, NULL,
>> + ret = iommu_device_sysfs_add(&smmu->iommu, dev, arm_iommu_groups,
>> "smmu3.%pa", &ioaddr);
>> if (ret)
>> return ret;
>> diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
>> index cd48590ada30..ffa1123bf37c 100644
>> --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
>> +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
>> @@ -50,9 +50,13 @@
>> #define IDR1_SSIDSIZE GENMASK(10, 6)
>> #define IDR1_SIDSIZE GENMASK(5, 0)
>> +#define ARM_SMMU_IDR2 0x8
>> +
>> #define ARM_SMMU_IDR3 0xc
>> #define IDR3_RIL (1 << 10)
>> +#define ARM_SMMU_IDR4 0x10
>> +
>> #define ARM_SMMU_IDR5 0x14
>> #define IDR5_STALL_MAX GENMASK(31, 16)
>> #define IDR5_GRAN64K (1 << 6)
>> @@ -69,6 +73,9 @@
>> #define IDR5_VAX GENMASK(11, 10)
>> #define IDR5_VAX_52_BIT 1
>> +#define ARM_SMMU_IIDR 0x18
>> +#define ARM_SMMU_AIDR 0x1C
>> +
>> #define ARM_SMMU_CR0 0x20
>> #define CR0_ATSCHK (1 << 4)
>> #define CR0_CMDQEN (1 << 3)
>> @@ -154,6 +161,8 @@
>> #define ARM_SMMU_REG_SZ 0xe00
>> +#define ARM_SMMU_IDR6 0x0190
>> +
>> /* Common MSI config fields */
>> #define MSI_CFG0_ADDR_MASK GENMASK_ULL(51, 2)
>> #define MSI_CFG2_SH GENMASK(5, 4)
>> @@ -669,6 +678,17 @@ struct arm_smmu_device {
>> struct arm_smmu_strtab_cfg strtab_cfg;
>> + /* SMMU feature registers */
>> + u32 idr0;
>> + u32 idr1;
>> + u32 idr2;
>> + u32 idr3;
>> + u32 idr4;
>> + u32 idr5;
>> + u32 idr6;
>> + u32 iidr;
>> + u32 aidr;
>> +
>> /* IOMMU core code handle */
>> struct iommu_device iommu;
>> @@ -676,6 +696,13 @@ struct arm_smmu_device {
>> struct mutex streams_mutex;
>> };
>> +static inline struct arm_smmu_device *dev_to_arm_smmu_device(struct device *dev)
>> +{
>> + struct iommu_device *iommu = dev_to_iommu_device(dev);
>> +
>> + return container_of(iommu, struct arm_smmu_device, iommu);
>> +}
>> +
>> struct arm_smmu_stream {
>> u32 id;
>> struct arm_smmu_master *master;

Next message: Matthew Wilcox: "Re: [PATCH v6 27/39] kasan, mm: only define ___GFP_SKIP_KASAN_POISON with HW_TAGS"
Previous message: Philipp Zabel: "Re: [PATCH 0/2] add fwnode support to reset subsystem"
In reply to: Robin Murphy: "Re: [PATCH] iommu/arm: Expose ARM SMMUv3 related registers via sysfs"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]