Re: [PATCH] iommu/arm-smmu: Report smmu type in dmesg

[Robin Murphy <robin.murphy@arm.com>]

On 09/03/17 12:02, Robert Richter wrote:
> On 07.03.17 18:41:33, Robin Murphy wrote:
>> On 07/03/17 14:06, Robert Richter wrote:
>>> On 06.03.17 18:22:08, Robin Murphy wrote:
>>>> On 06/03/17 13:58, Robert Richter wrote:
>>>>> The ARM SMMU detection especially depends from system firmware. For
>>>>> better diagnostic, log the detected type in dmesg.
>>>>
>>>> This paragraph especially depends from grammar. I think.
>>>
>>> Thanks for the mail on you. :)
>>>
>>>>
>>>>> The smmu type's name is now stored in struct arm_smmu_type and ACPI
>>>>> code is modified to use that struct too. Rename ARM_SMMU_MATCH_DATA()
>>>>> macro to ARM_SMMU_TYPE() for better readability.
>>>>>
>>>>> Signed-off-by: Robert Richter <rrichter@cavium.com>
>>>>> ---
>>>>>  drivers/iommu/arm-smmu.c | 61 ++++++++++++++++++++++++------------------------
>>>>>  1 file changed, 30 insertions(+), 31 deletions(-)
>>>>
>>>> That seems a relatively invasive diffstat for the sake of printing a
>>>> string once at boot time to what I can only assume is a small audience
>>>> of firmware developers who find "cat
>>>> /sys/firmware/devicetree/base/iommu*/compatible" (or the ACPI
>>>> equivalent) too hard ;)
>>>
>>> Reading firmware data is not really a solution as you don't know what
>>> the driver is doing with it. The actual background of this patch is to
>>> be sure a certain workaround was enabled in the kernel. ARM's cpu
>>> errata framework does this nicely. In case of smmus we just have the
>>> internal model implementation type which is not visible in the logs.
>>> Right now, there is no way to figure that out without knowing fw
>>> specifics and kernel sources.
>>
>> Ah, now it starts to become clear. In that case, if we want to confirm
>> the presence of specific workarounds, we should actually _confirm the
>> presence of specific workarounds_. I'd have no complaint with e.g. this:
>>
>> -----8<-----
>> diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
>> index f7411109670f..9e50a092632c 100644
>> --- a/drivers/iommu/arm-smmu.c
>> +++ b/drivers/iommu/arm-smmu.c
>> @@ -1934,6 +1934,8 @@ static int arm_smmu_device_cfg_probe(struct
>> arm_smmu_device *smmu)
>>                         atomic_add_return(smmu->num_context_banks,
>>                                           &cavium_smmu_context_count);
>>                 smmu->cavium_id_base -= smmu->num_context_banks;
>> +               dev_notice(smmu->dev, "\tusing ASID/VMID offset %u\n",
>> +                          smmu->cavium_id_base);
>>         }
>>
>>         /* ID2 */
>> ----->8-----
>>
>> and the equivalent for other things, if need be. If you just print "hey,
>> this is SMMU-x", the user is in fact no better off, since they would
>> then still have to go and look at the source for whatever kernel they're
>> running to find out which particular workarounds for SMMU-x bugs that
>> particular kernel implements.
> 
> I don't understand why you don't want to expose in some way the smmu
> type. There are a lot of things that can go wrong, esp. with firmware,
> to detect the proper smmu type.

Because there is only one reason for which detecting the "proper SMMU
type" matters - implementation-specific workarounds for areas in which a
given hardware implementation deviates from the architecture assumed by
the driver.

OK, let's print the model name. Now, if I give you this:

[    0.475009] arm-smmu 2b500000.iommu: probing hardware configuration...
[    0.481650] arm-smmu 2b500000.iommu: ARM MMU-401 r0p0 with:
[    0.486436] arm-smmu 2b500000.iommu: 	stage 2 translation
[    0.491925] arm-smmu 2b500000.iommu: 	coherent table walk
[    0.497420] arm-smmu 2b500000.iommu: 	stream matching with 32
register groups
[    0.504678] arm-smmu 2b500000.iommu: 	4 context banks (4 stage-2 only)
[    0.511312] arm-smmu 2b500000.iommu: 	Supported page sizes: 0x60211000
[    0.517943] arm-smmu 2b500000.iommu: 	Stage-2: 40-bit IPA -> 40-bit PA

please tell me which hardware problems this system has kernel
workarounds in place for, and which it doesn't. If you want another
hint, the version is 4.11.0-rc1+. Note the "+".

Robin.

> The above change is not a general solution too for reporting the
> enablement of smmu errata workarounds. The check could be done
> multiple times and in the fast path. For the particular problem the
> above would work, but still some message on the type detected would be
> fine. I could rework my patch in a way that .name is not permanently
> stored in struct arm_smmu_device.
> 
> -Robert
>