| Message ID | 20220420164836.1181-1-shameerali.kolothum.thodi@huawei.com |
|---|---|
| Headers | show |
| Series | ACPI/IORT: Support for IORT RMR node | expand |
Looks good:
Reviewed-by: Christoph Hellwig <hch@lst.de>
On Wed, Apr 20, 2022 at 05:48:28PM +0100, Shameer Kolothum wrote: > @@ -141,6 +149,11 @@ struct iommu_resv_region { > size_t length; > int prot; > enum iommu_resv_type type; > + union { > + struct iommu_iort_rmr_data rmr; > + } fw_data; > + void (*resv_region_free_fw_data)(struct device *dev, > + struct iommu_resv_region *region); I'd shorten the name to just free. Also any reason the call to this method isn't also added in this patch as it logically belongs here?
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. 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. 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. It's up to the maintainers as to whether they see this as a problem or not. Thanks, Steve > Thanks, > Shameer > [1] https://lore.kernel.org/all/44610361.fMDQidcC6G@kreacher/ > > From old: > We have faced issues with 3408iMR RAID controller cards which > fail to boot when SMMU is enabled. This is because these > controllers make use of host memory for various caching related > purposes and when SMMU is enabled the iMR firmware fails to > access these memory regions as there is no mapping for them. > IORT RMR provides a way for UEFI to describe and report these > memory regions so that the kernel can make a unity mapping for > these in SMMU. > > Change History: > > v8 --> v9 > - Adressed comments from Robin on interfaces. > - Addressed comments from Lorenzo. > > v7 --> v8 > - Patch #1 has temp definitions for RMR related changes till > the ACPICA header changes are part of kernel. > - No early parsing of RMR node info and is only parsed at the > time of use. > - Changes to the RMR get/put API format compared to the > previous version. > - Support for RMR descriptor shared by multiple stream IDs. > > v6 --> v7 > -fix pointed out by Steve to the SMMUv2 SMR bypass install in patch #8. > > v5 --> v6 > - Addressed comments from Robin & Lorenzo. > : Moved iort_parse_rmr() to acpi_iort_init() from > iort_init_platform_devices(). > : Removed use of struct iort_rmr_entry during the initial > parse. Using struct iommu_resv_region instead. > : Report RMR address alignment and overlap errors, but continue. > : Reworked arm_smmu_init_bypass_stes() (patch # 6). > - Updated SMMUv2 bypass SMR code. Thanks to Jon N (patch #8). > - Set IOMMU protection flags(IOMMU_CACHE, IOMMU_MMIO) based > on Type of RMR region. Suggested by Jon N. > > v4 --> v5 > -Added a fw_data union to struct iommu_resv_region and removed > struct iommu_rmr (Based on comments from Joerg/Robin). > -Added iommu_put_rmrs() to release mem. > -Thanks to Steve for verifying on SMMUv2, but not added the Tested-by > yet because of the above changes. > > v3 -->v4 > -Included the SMMUv2 SMR bypass install changes suggested by > Steve(patch #7) > -As per Robin's comments, RMR reserve implementation is now > more generic (patch #8) and dropped v3 patches 8 and 10. > -Rebase to 5.13-rc1 > > RFC v2 --> v3 > -Dropped RFC tag as the ACPICA header changes are now ready to be > part of 5.13[0]. But this series still has a dependency on that patch. > -Added IORT E.b related changes(node flags, _DSM function 5 checks for > PCIe). > -Changed RMR to stream id mapping from M:N to M:1 as per the spec and > discussion here[1]. > -Last two patches add support for SMMUv2(Thanks to Jon Nettleton!) > > Jon Nettleton (1): > iommu/arm-smmu: Get associated RMR info and install bypass SMR > > Shameer Kolothum (8): > iommu: Introduce a union to struct iommu_resv_region > ACPI/IORT: Make iort_iommu_msi_get_resv_regions() return void > ACPI/IORT: Provide a generic helper to retrieve reserve regions > ACPI/IORT: Add support to retrieve IORT RMR reserved regions > ACPI/IORT: Add a helper to retrieve RMR info directly > iommu/arm-smmu-v3: Introduce strtab init helper > iommu/arm-smmu-v3: Refactor arm_smmu_init_bypass_stes() to force > bypass > iommu/arm-smmu-v3: Get associated RMR info and install bypass STE > > drivers/acpi/arm64/iort.c | 335 ++++++++++++++++++-- > drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 78 ++++- > drivers/iommu/arm/arm-smmu/arm-smmu.c | 52 +++ > drivers/iommu/dma-iommu.c | 2 +- > drivers/iommu/iommu.c | 12 +- > include/linux/acpi_iort.h | 14 +- > include/linux/iommu.h | 13 + > 7 files changed, 461 insertions(+), 45 deletions(-) >
> -----Original Message----- > From: Steven Price [mailto:steven.price@arm.com] > Sent: 21 April 2022 13:59 > To: Shameerali Kolothum Thodi <shameerali.kolothum.thodi@huawei.com>; > linux-arm-kernel@lists.infradead.org; linux-acpi@vger.kernel.org; > iommu@lists.linux-foundation.org > Cc: Linuxarm <linuxarm@huawei.com>; lorenzo.pieralisi@arm.com; > joro@8bytes.org; robin.murphy@arm.com; will@kernel.org; wanghuiqiang > <wanghuiqiang@huawei.com>; Guohanjun (Hanjun Guo) > <guohanjun@huawei.com>; Sami.Mujawar@arm.com; jon@solid-run.com; > eric.auger@redhat.com; laurentiu.tudor@nxp.com; hch@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. > > 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? Thanks, Shameer
On 2022-04-21 15:43, Shameerali Kolothum Thodi wrote: > > >> -----Original Message----- >> From: Steven Price [mailto:steven.price@arm.com] >> Sent: 21 April 2022 13:59 >> To: Shameerali Kolothum Thodi <shameerali.kolothum.thodi@huawei.com>; >> linux-arm-kernel@lists.infradead.org; linux-acpi@vger.kernel.org; >> iommu@lists.linux-foundation.org >> Cc: Linuxarm <linuxarm@huawei.com>; lorenzo.pieralisi@arm.com; >> joro@8bytes.org; robin.murphy@arm.com; will@kernel.org; wanghuiqiang >> <wanghuiqiang@huawei.com>; Guohanjun (Hanjun Guo) >> <guohanjun@huawei.com>; Sami.Mujawar@arm.com; jon@solid-run.com; >> eric.auger@redhat.com; laurentiu.tudor@nxp.com; hch@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.
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. Thanks, Shameer [1] https://lore.kernel.org/all/44610361.fMDQidcC6G@kreacher/