Re: [PATCH] iommu: arm-smmu-impl: add NXP hook to preserve bootmappings

[Laurentiu Tudor <laurentiu.tudor@nxp.com>]

Hi Robin,

Sorry for the late reply, we had a few days of over here. Comments inline.

On 11/25/2020 8:10 PM, Robin Murphy wrote:
> On 2020-11-25 15:50, laurentiu.tudor@nxp.com wrote:
>> From: Laurentiu Tudor <laurentiu.tudor@nxp.com>
>>
>> Add a NXP specific hook to preserve SMMU mappings present at
>> boot time (created by the boot loader). These are needed for
>> MC firmware present on some NXP chips to continue working
>> across kernel boot and SMMU initialization.
>>
>> Signed-off-by: Laurentiu Tudor <laurentiu.tudor@nxp.com>
>> ---
>>   drivers/iommu/arm/arm-smmu/arm-smmu-impl.c | 33 ++++++++++++++++++++++
>>   1 file changed, 33 insertions(+)
>>
>> diff --git a/drivers/iommu/arm/arm-smmu/arm-smmu-impl.c
>> b/drivers/iommu/arm/arm-smmu/arm-smmu-impl.c
>> index 7fed89c9d18a..ca07d9d4be69 100644
>> --- a/drivers/iommu/arm/arm-smmu/arm-smmu-impl.c
>> +++ b/drivers/iommu/arm/arm-smmu/arm-smmu-impl.c
>> @@ -187,6 +187,36 @@ static const struct arm_smmu_impl
>> mrvl_mmu500_impl = {
>>       .reset = arm_mmu500_reset,
>>   };
>>   +static int nxp_cfg_probe(struct arm_smmu_device *smmu)
>> +{
>> +    int i, cnt = 0;
>> +    u32 smr;
>> +
>> +    for (i = 0; i < smmu->num_mapping_groups; i++) {
>> +        smr = arm_smmu_gr0_read(smmu, ARM_SMMU_GR0_SMR(i));
>> +
>> +        if (FIELD_GET(ARM_SMMU_SMR_VALID, smr)) {
> 
> I bet this is fun over kexec...

Right. I haven't even considered kexec.

> Note that the Qualcomm special case got a bit of a free pass since it
> involves working around a totally broken hypervisor, plus gets to play
> the "nobody sane will run an enterprise distro on their phone" card to
> an extent; I don't think the likes of Layerscape kit get it quite so
> easy ;)

I agree that this is not ideal, but the plan here was to have something
to boot vanilla kernel OOB on our chips, which is something on my mind
for quite a while now. I do realize that we won't get away with it
in the long run.

>> +            smmu->smrs[i].id = FIELD_GET(ARM_SMMU_SMR_ID, smr);
>> +            smmu->smrs[i].mask = FIELD_GET(ARM_SMMU_SMR_MASK, smr);
>> +            smmu->smrs[i].valid = true;
>> +
>> +            smmu->s2crs[i].type = S2CR_TYPE_BYPASS;
>> +            smmu->s2crs[i].privcfg = S2CR_PRIVCFG_DEFAULT;
>> +            smmu->s2crs[i].cbndx = 0xff;
>> +
>> +            cnt++;
>> +        }
>> +    }
>> +
>> +    dev_notice(smmu->dev, "\tpreserved %d boot mapping%s\n", cnt,
>> +           cnt == 1 ? "" : "s");
> 
> That gets you around the initial SMMU reset, but what happens for the
> arbitrarily long period of time between the MC device getting attached
> to a default domain and the MC driver actually probing and (presumably)
> being able to map and reinitialise its firmware?

Perhaps I'm missing something, but won't the MC firmware live based on
this bypass mapping created by the bootloader and that gets preserved?

>> +
>> +    return 0;
>> +}
>> +
>> +static const struct arm_smmu_impl nxp_impl = {
>> +    .cfg_probe = nxp_cfg_probe,
>> +};
> 
> I believe you're mostly using MMU-500, so you probably don't want to
> simply throw out the relevant errata workarounds.
> 
>>   struct arm_smmu_device *arm_smmu_impl_init(struct arm_smmu_device
>> *smmu)
>>   {
>> @@ -226,5 +256,8 @@ struct arm_smmu_device *arm_smmu_impl_init(struct
>> arm_smmu_device *smmu)
>>       if (of_device_is_compatible(np, "marvell,ap806-smmu-500"))
>>           smmu->impl = &mrvl_mmu500_impl;
>>   +    if (of_property_read_bool(np, "nxp,keep-boot-mappings"))
>> +        smmu->impl = &nxp_impl;
> 
> Normally you'd get a "what about ACPI?" here, but given the number of
> calls and email threads we've had specifically about trying to make ACPI
> support for these platforms work, that gets upgraded to at least a "WHAT
> ABOUT ACPI!?" :P
I do have ACPI in mind, but for now I just wanted to have a
first impression on the approach. One idea I was pondering on was to
have this property in the MC node (quick reminder: MC is exposed as a NC
in ACPI, should be able to replicate the property in there too). In the
mean time, we are in collaboration with our partners on using RMRR by
adding support for it in the arm-smmu-v2 driver.

> But seriously, the case of device firmware in memory being active before
> handover to Linux is *literally* the original reason behind IORT RMRs.
> We already know we need a way to specify the equivalent thing for DT
> systems, such that both can be handled commonly. I really don't want to
> have to support a vendor-specific mechanism for not-even-fully-solving a
> completely generic issue, sorry.
> 

I remember that some months ago there was a proposal from nvidia [1] to
map per-device reserved memory into SMMU. Would it make sense to revive
it as it seemed a viable solution for our case too?

[1]
https://patchwork.kernel.org/project/linux-arm-kernel/list/?series=213701&state=%2A&archive=both

---
Best Regards, Laurentiu