[PATCH v10 0/9] ACPI/IORT: Support for IORT RMR node

[Robin Murphy]

On 2022-04-21 15:43, Shameerali Kolothum Thodi wrote:
> 
> 
>> -----Original Message-----
>> From: Steven Price [mailto:steven.price at arm.com]
>> Sent: 21 April 2022 13:59
>> To: Shameerali Kolothum Thodi <shameerali.kolothum.thodi at huawei.com>;
>> linux-arm-kernel at lists.infradead.org; linux-acpi at vger.kernel.org;
>> iommu at lists.linux-foundation.org
>> Cc: Linuxarm <linuxarm at huawei.com>; lorenzo.pieralisi at arm.com;
>> joro at 8bytes.org; robin.murphy at arm.com; will at kernel.org; wanghuiqiang
>> <wanghuiqiang at huawei.com>; Guohanjun (Hanjun Guo)
>> <guohanjun at huawei.com>; Sami.Mujawar at arm.com; jon at solid-run.com;
>> eric.auger at redhat.com; laurentiu.tudor at nxp.com; hch at infradead.org
>> Subject: Re: [PATCH v10 0/9] ACPI/IORT: Support for IORT RMR node
>>
>> On 20/04/2022 17:48, Shameer Kolothum wrote:
>>> Hi
>>>
>>> v9 --> v10
>>>   - Dropped patch #1 ("Add temporary RMR node flag definitions") since
>>>     the ACPICA header updates patch is now in the mailing list[1]
>>>   - Based on the suggestion from Christoph, introduced a
>>>     resv_region_free_fw_data() callback in struct iommu_resv_region and
>>>     used that to free RMR specific memory allocations.
>>>
>>> Though there is a small change from v9 with respect to how we free up
>>> the FW specific data, I have taken the liberty to pick up the R-by and
>>> T-by tags from Lorenzo, Steve and Laurentiu. But please do take a look
>>> again and let me know.
>>
>> I've given this a go and it works fine on my Juno setup. So do keep my
>> T-by tag.
> 
> Many thanks for that.
> 
>> Sami has been kind enough to give me an updated firmware which also
>> fixes the RMR node in the IORT. Although as mentioned before the details
>> of the RMR node are currently being ignored so this doesn't change the
>> functionality but silences the warning.

Strictly they're not ignored, you just won't be getting past the point 
where they're not entirely not ignored. It'll appear to work because 
arm_smmu_rmr_install_bypass_smr() just bypasses the whole stream until 
the actual device turns up to join up to the StreamID and the "real" 
processing of RMRs happens via iommu_create_device_direct_mappings() - 
if there's no actual HDLCD device described in the DSDT, that will never 
happen, and even if there is, chances are that things will currently 
happen in the wrong order such we'd end up waiting to replay 
iommu_probe_device() from acpi_iommu_configure_id() once a driver binds, 
and *that* definitely can't happen without teaching the HDLCD driver 
about ACPI.

>> My concern is that with the RMR region effectively ignored we may see
>> more broken firmware, and while a length of zero produces a warning, an
>> otherwise incorrect length will currently "silently work" but mean that
>> any future tightening would cause problems. For example if the SMMU
>> driver were to recreate the mappings to only cover the region specified
>> in the RMR it may not be large enough if the RMR base/length are not
>> correct.
> 
> Not sure how we can further validate the RMR if the firmware provides an
> incorrect one. I see your point of future tightening causing problems
> with broken firmware. But then it is indeed a "broken firmware"...
> 
>   It's up to the maintainers as to whether they see this as a
>> problem or not.
> 
> Hi Robin,
> 
> Any thoughts on this?

In general we can't second-guess firmware. Even a zero-length RMR should 
have ample opportunity to blow up outside this one corner case where 
Linux never gets to associate the StreamID with a corresponding device.

Robin.