Re: [PATCH] iommu/arm: Expose ARM SMMUv3 related registers via sysfs
On 3/25/22 18:03, Robin Murphy wrote: > On 2022-03-23 15:09, Miguel Luis wrote: >>> On 23 Mar 2022, at 12:40, Robin Murphy 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. > > OK, so what do end-users of Intel and AMD systems do with that > understanding then? > It's features probing and informational purposes really -- that's all one can do with these registers regardless of IOMMU implementation. There some others which print the version of the iommu that also appear as RO sysfs entries. Those two registers on those two implementations have proved useful the last couple of years when I got new SDPs/machines. At the end of the day some tool (or script) pretty prints what's supported.. on what usually is depicted in very lenghty manuals. And that's fed into a toolbox that prints all hardware and software capabilities alongside diagnostics / etc (for troubleshooting). You know, similar to `cpuid` on the x86 cpuid side (albeit you could argue that it's behind a 'special' instruction). And that 'decodes' to an human-friendly on what goes in those lenghty SDMs. `/proc/cpuinfo` translates that into a set of keywords (which may be software-specific or hardware features). And more recently `kcpuid` for raw-supported features. I am sure there's more examples. >> 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. > > Indeed the driver already prints the supported features at boot, largely > because it's useful for debugging. But again, what's the advantage of > knowing what you might theoretically be able to do with a machine if you > don't have the software support to actually do it? Are there users out > there who aren't going to update their OS *unless* they can cling to the > hope that a new OS might see the opportunity to use foreign-endianness > pagetables and finally take it? > > I appreciate the natural human instinct to want to poke around at and > evaluate a new shiny thing, but a sufficiently interested user can > already do that with /dev/mem or any number of other ways. We don't need > the burden of maintaining an upstream sysfs ABI for curiosity. > /dev/mem is an interesting alternative -- I wasn't quite sure you could use the register addresses here arbitrarily on ARM. Anyways, I suppose it's an worthwhile alternative in case it works. > The other fact of the matter is that a great deal of systems with SMMUv3 > will be using one of Arm Ltd's implementations, and as soon as one knows > that one can readily look up all the details in Arm's documentation. > Especially when one already has that site open to find the SMMUv3 > architecture spec to make sense of cryptic register dumps, right? > Heh, the idea was to avoid going to those specs all the time, but I understand your earlier point(s). Also, IOMMUs are getting bigger in terms of featureset -- so it would be nice to better introspect what iommus can do (in the whatever mix of implementations) in a less chaotic manner. Maybe implementation-specific sysfs entries are not it, but hopefully something else in a more generic-way, in the advent of a more direct interface to iommus. ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH] iommu/arm: Expose ARM SMMUv3 related registers via sysfs
On 2022-03-23 15:09, Miguel Luis wrote: On 23 Mar 2022, at 12:40, Robin Murphy 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. OK, so what do end-users of Intel and AMD systems do with that understanding then? 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. Indeed the driver already prints the supported features at boot, largely because it's useful for debugging. But again, what's the advantage of knowing what you might theoretically be able to do with a machine if you don't have the software support to actually do it? Are there users out there who aren't going to update their OS *unless* they can cling to the hope that a new OS might see the opportunity to use foreign-endianness pagetables and finally take it? I appreciate the natural human instinct to want to poke around at and evaluate a new shiny thing, but a sufficiently interested user can already do that with /dev/mem or any number of other ways. We don't need the burden of maintaining an upstream sysfs ABI for curiosity. The other fact of the matter is that a great deal of systems with SMMUv3 will be using one of Arm Ltd's implementations, and as soon as one knows that one can readily look up all the details in Arm's documentation. Especially when one already has that site open to find the SMMUv3 architecture spec to make sense of cryptic register dumps, right? 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? Yes, Let's take some features of Arm's MMU-600 implementation as examples: https://developer.arm.com/documentation/100310/0202/Functional-description/Constraints-and-limitations-of-use/SMMUv3-support - MMU-600 supports ATS and PRI. Whether or not a given MMU-600 can ever actually recieve any ATS or PRI requests depends on whether it's wired up to a PCIe root complex capable of sending them. - MMU-600 supports 20-bit SubstreamIDs (PASIDs), since that represents the maximum size of context table it can index. How many of those bits are actually capable of being driven by endpoints and/or carried across interconnects depends on those respective components, and isn't MMU-600's concern. - MMU-600 supports stalling faults. Whether having a stalled transaction sat across the interconnect might lead to a deadlock with transactions attempting to load data to resolve the fault is the interconnect's problem. - If the integrator correctly wires up the "evento" signal and sets the "sup_sev" configuration tie-off, MMU-600 supports the SEV feature to send wake-up events to waiting CPUs. Except early revisions have an erratum where sometimes they'll miss sending an event, so software had better knmow to cross-check SMMU_IIDR before trusting it. - Similarly, it's not unheard of for integrators to set configuration tie-offs incorrectly, or find issues after the fact which mean that features like COHACC and BTM are broken. Therefore in practice those are effectively ignored in favour of firmware overrides. Note that on AMD at least, most of the reported features are actually firmware overrides as well. However the main thing is that Intel and AMD design and implement their IOMMU alongside their PCIe implementation and everything else and only sell you the whole combination, so features there can be inherently more joined-up than the building-block nature of typical Arm-based systems allows. Chasing apparent parity for the sake of it where a sufficient level of parity just doesn't exist is a waste of time (see also: endless /proc/cpuinfo nonsense). You can paint a baseball orange but it's still never going to be nice to eat. Thanks, Robin. 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/905.smmuv3/iommu/smmu3.0x0905/arm-iommu Nit: my main comment above notwithstanding, is this level of hierarchcy meaningful or
Re: [PATCH] iommu/arm: Expose ARM SMMUv3 related registers via sysfs
> On 23 Mar 2022, at 12:40, Robin Murphy 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/905.smmuv3/iommu/smmu3.0x0905/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/905.smmuv3/iommu/smmu3.0x0905/arm-iommu/smmu_idr5
>> /sys/devices/platform/905.smmuv3/iommu/smmu3.0x0905/arm-iommu/smmu_idr3
>> /sys/devices/platform/905.smmuv3/iommu/smmu3.0x0905/arm-iommu/smmu_idr1
>> /sys/devices/platform/905.smmuv3/iommu/smmu3.0x0905/arm-iommu/smmu_aidr
>> /sys/devices/platform/905.smmuv3/iommu/smmu3.0x0905/arm-iommu/smmu_idr6
>> /sys/devices/platform/905.smmuv3/iommu/smmu3.0x0905/arm-iommu/smmu_idr4
>> /sys/devices/platform/905.smmuv3/iommu/smmu3.0x0905/arm-iommu/smmu_iidr
>> /sys/devices/platform/905.smmuv3/iommu/smmu3.0x0905/arm-iommu/smmu_idr2
>> /sys/devices/platform/905.smmuv3/iommu/smmu3.0x0905/arm-iommu/smmu_idr0
>> Signed-off-by: Miguel Luis
>> ---
>> 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 |=
Re: [PATCH] iommu/arm: Expose ARM SMMUv3 related registers via sysfs
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.
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.
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/905.smmuv3/iommu/smmu3.0x0905/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.
Robin.
/sys/devices/platform/905.smmuv3/iommu/smmu3.0x0905/arm-iommu/smmu_idr5
/sys/devices/platform/905.smmuv3/iommu/smmu3.0x0905/arm-iommu/smmu_idr3
/sys/devices/platform/905.smmuv3/iommu/smmu3.0x0905/arm-iommu/smmu_idr1
/sys/devices/platform/905.smmuv3/iommu/smmu3.0x0905/arm-iommu/smmu_aidr
/sys/devices/platform/905.smmuv3/iommu/smmu3.0x0905/arm-iommu/smmu_idr6
/sys/devices/platform/905.smmuv3/iommu/smmu3.0x0905/arm-iommu/smmu_idr4
/sys/devices/platform/905.smmuv3/iommu/smmu3.0x0905/arm-iommu/smmu_iidr
/sys/devices/platform/905.smmuv3/iommu/smmu3.0x0905/arm-iommu/smmu_idr2
/sys/devices/platform/905.smmuv3/iommu/smmu3.0x0905/arm-iommu/smmu_idr0
Signed-off-by: Miguel Luis
---
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
