Re: [PATCH 2/2] thunderbolt: Use pre-boot DMA protection on AMD systems

From: Robin Murphy <robin.murphy@arm.com>
To: "Limonciello, Mario" <Mario.Limonciello@amd.com>,
	Christoph Hellwig <hch@infradead.org>
Cc: Michael Jamet <michael.jamet@intel.com>,
	Mika Westerberg <mika.westerberg@linux.intel.com>,
	"open list:THUNDERBOLT DRIVER" <linux-usb@vger.kernel.org>,
	open list <linux-kernel@vger.kernel.org>,
	Yehezkel Bernat <YehezkelShB@gmail.com>,
	"open list:AMD IOMMU (AMD-VI)" <iommu@lists.linux-foundation.org>,
	Andreas Noever <andreas.noever@gmail.com>,
	Will Deacon <will@kernel.org>
Subject: Re: [PATCH 2/2] thunderbolt: Use pre-boot DMA protection on AMD systems
Date: Tue, 15 Mar 2022 18:07:47 +0000	[thread overview]
Message-ID: <21d33a75-8c0e-7734-b3d1-dbe33cfe0ab0@arm.com> (raw)
In-Reply-To: <BL1PR12MB5157D7B7734122684D47923AE2109@BL1PR12MB5157.namprd12.prod.outlook.com>

On 2022-03-15 16:54, Limonciello, Mario via iommu wrote:
> [Public]
> 
> 
>> On Tue, Mar 15, 2022 at 11:24:55AM -0500, Mario Limonciello wrote:
>>> -	 * handled natively using IOMMU. It is enabled when IOMMU is
>>> -	 * enabled and ACPI DMAR table has DMAR_PLATFORM_OPT_IN set.
>>> +	 * handled natively using IOMMU. It is enabled when the IOMMU is
>>> +	 * enabled and either:
>>> +	 * ACPI DMAR table has DMAR_PLATFORM_OPT_IN set
>>> +	 * or
>>> +	 * ACPI IVRS table has DMA_REMAP bitset
>>>   	 */
>>>   	return sprintf(buf, "%d\n",
>>> -		       iommu_present(&pci_bus_type) &&
>> dmar_platform_optin());
>>> +		       iommu_present(&pci_bus_type) &&
>>> +		       (dmar_platform_optin() || amd_ivrs_remap_support()));
>>
>> Yikes.  No, the thunderbot code does not have any business poking into
>> either dmar_platform_optin or amd_ivrs_remap_support.  This needs
>> a proper abstration from the IOMMU code.
> 
> To make sure I follow your ask - it's to make a new generic iommu function
> That would check dmar/ivrs, and switch out thunderbolt domain.c to use the
> symbol?
> 
> I'm happy to rework that if that is what you want.
> Do you have a preferred proposed function name for that?

But why? Either IOMMU translation is enabled or it isn't, and if it is, 
what's to gain from guessing at *why* it might have been? And even if 
the IOMMU's firmware table did tell the IOMMU driver to enable the 
IOMMU, why should that be Thunderbolt's business?

Furthermore, looking at patch #1 I can only conclude that this is 
entirely meaningless anyway. AFAICS it's literally reporting whether the 
firmware flag was set or not. Not whether it's actually been honoured 
and the IOMMU is enforcing any kind of DMA protection at all. Even on 
Intel where the flag does at least have some effect on the IOMMU driver, 
that can still be overridden.

I already have a patch refactoring this to get rid of iommu_present(), 
but at the time I wasn't looking to closely at what it's trying to *do* 
with the information. If it's supposed to accurately reflect whether the 
Thunderbolt device is subject to IOMMU translation and not bypassed, I 
can fix that too (and unexport dmar_platform_optin() in the process...)

Robin.