[PATCH] iommu/amd: Fix unity mapping initialization race

[PATCH] iommu/amd: Fix unity mapping initialization race

[Joerg Roedel]

From: Joerg Roedel <jroedel@suse.de>

There is a race condition in the AMD IOMMU init code that
causes requested unity mappings to be blocked by the IOMMU
for a short period of time. This results on boot failures
and IO_PAGE_FAULTs on some machines.

Fix this by making sure the unity mappings are installed
before all other DMA is blocked.

Fixes: aafd8ba0ca74 ('iommu/amd: Implement add_device and remove_device')
Cc: stable@vger.kernel.org # v4.2+
Signed-off-by: Joerg Roedel <jroedel@suse.de>
---
 drivers/iommu/amd_iommu_init.c | 14 ++++++++++++--
 1 file changed, 12 insertions(+), 2 deletions(-)

diff --git a/drivers/iommu/amd_iommu_init.c b/drivers/iommu/amd_iommu_init.c
index d091def..59741ea 100644
--- a/drivers/iommu/amd_iommu_init.c
+++ b/drivers/iommu/amd_iommu_init.c
@@ -1568,13 +1568,23 @@ static int __init amd_iommu_init_pci(void)
 			break;
 	}
 
+	/*
+	 * Order is important here to make sure any unity map requirements are
+	 * fulfilled. The unity mappings are created and written to the device
+	 * table during the amd_iommu_init_api() call.
+	 *
+	 * After that we call init_device_table_dma() to make sure any
+	 * uninitialized DTE will block DMA, and in the end we flush the caches
+	 * of all IOMMUs to make sure the changes to the device table are
+	 * active.
+	 */
+	ret = amd_iommu_init_api();
+
 	init_device_table_dma();
 
 	for_each_iommu(iommu)
 		iommu_flush_all_caches(iommu);
 
-	ret = amd_iommu_init_api();
-
 	if (!ret)
 		print_iommu_info();
 
-- 
2.6.6

Re: [PATCH] iommu/amd: Fix unity mapping initialization race

[Wan Zongshun]

On 2016年07月07日 00:00, Joerg Roedel wrote:
> From: Joerg Roedel <jroedel@suse.de>
>
> There is a race condition in the AMD IOMMU init code that
> causes requested unity mappings to be blocked by the IOMMU
> for a short period of time. This results on boot failures
> and IO_PAGE_FAULTs on some machines.
>
> Fix this by making sure the unity mappings are installed
> before all other DMA is blocked.
>
> Fixes: aafd8ba0ca74 ('iommu/amd: Implement add_device and remove_device')
> Cc: stable@vger.kernel.org # v4.2+
> Signed-off-by: Joerg Roedel <jroedel@suse.de>
> ---
>   drivers/iommu/amd_iommu_init.c | 14 ++++++++++++--
>   1 file changed, 12 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/iommu/amd_iommu_init.c b/drivers/iommu/amd_iommu_init.c
> index d091def..59741ea 100644
> --- a/drivers/iommu/amd_iommu_init.c
> +++ b/drivers/iommu/amd_iommu_init.c
> @@ -1568,13 +1568,23 @@ static int __init amd_iommu_init_pci(void)
>   			break;
>   	}
>
> +	/*
> +	 * Order is important here to make sure any unity map requirements are
> +	 * fulfilled. The unity mappings are created and written to the device
> +	 * table during the amd_iommu_init_api() call.
> +	 *
> +	 * After that we call init_device_table_dma() to make sure any
> +	 * uninitialized DTE will block DMA, and in the end we flush the caches
> +	 * of all IOMMUs to make sure the changes to the device table are
> +	 * active.
> +	 */

Joerg,

Do you mean we need enable the V and TV bits to DTE entry after all DTEs 
tables were initialized completely?

I checked this function 'init_device_table_dma', and find it just set
V and TV bit, to set translation info valid and DTE bits127:1 valid.

So I just think all things it should to do are to allow DMA access,
GPA-to-SPA translation should be active, why you add function comments 
below is to not allow DMA access and suppress all page faults?

/*
  * Init the device table to not allow DMA access for devices and
  * suppress all page faults
  */
static void init_device_table_dma(void)

> +	ret = amd_iommu_init_api();
> +
>   	init_device_table_dma();
>
>   	for_each_iommu(iommu)
>   		iommu_flush_all_caches(iommu);
>
> -	ret = amd_iommu_init_api();
> -
>   	if (!ret)
>   		print_iommu_info();
>
>
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH] iommu/amd: Fix unity mapping initialization race

[Joerg Roedel]

On Sun, Jul 10, 2016 at 07:40:53PM +0800, Wan Zongshun wrote:
> Do you mean we need enable the V and TV bits to DTE entry after all
> DTEs tables were initialized completely?

Yes, this is what my patch does and what fixes the bug that was
reported on machines which have unity-mapping entries.

> I checked this function 'init_device_table_dma', and find it just set
> V and TV bit, to set translation info valid and DTE bits127:1 valid.

Right, if no other bits are set this blocks all DMA from the gives
device-id.

> So I just think all things it should to do are to allow DMA access,
> GPA-to-SPA translation should be active, why you add function
> comments below is to not allow DMA access and suppress all page
> faults?
> 
> /*
>  * Init the device table to not allow DMA access for devices and
>  * suppress all page faults
>  */

Yeah, that comment needs to be updated. Not all DMA is blocked and
page-faults are not suppressed at all. Thanks for noticing.



	Joerg

Re: [PATCH] iommu/amd: Fix unity mapping initialization race

[Wan Zongshun]

On 2016年07月11日 15:19, Joerg Roedel wrote:
> On Sun, Jul 10, 2016 at 07:40:53PM +0800, Wan Zongshun wrote:
>> Do you mean we need enable the V and TV bits to DTE entry after all
>> DTEs tables were initialized completely?
>
> Yes, this is what my patch does and what fixes the bug that was
> reported on machines which have unity-mapping entries.

Okay, this patch should also better to general case not only unity-mapping.

How about the interrupt remap function? Do we need same considering for 
IV bit enable for interrupt remap?

>
>> I checked this function 'init_device_table_dma', and find it just set
>> V and TV bit, to set translation info valid and DTE bits127:1 valid.
>
> Right, if no other bits are set this blocks all DMA from the gives
> device-id.

Sorry, why you still say this 'init_device_table_dma' can block DMA?
I just think this function will enable DMA transfer, since  we set the V 
and TV bits, right? or I misunderstand what "block DMA" mean?

>
>> So I just think all things it should to do are to allow DMA access,
>> GPA-to-SPA translation should be active, why you add function
>> comments below is to not allow DMA access and suppress all page
>> faults?
>>
>> /*
>>   * Init the device table to not allow DMA access for devices and
>>   * suppress all page faults
>>   */
>
> Yeah, that comment needs to be updated. Not all DMA is blocked and
> page-faults are not suppressed at all. Thanks for noticing.
>
>
>
> 	Joerg
>
>

Re: [PATCH] iommu/amd: Fix unity mapping initialization race

[Joerg Roedel]

On Mon, Jul 11, 2016 at 05:25:29PM +0800, Wan Zongshun wrote:
> Okay, this patch should also better to general case not only unity-mapping.
> 
> How about the interrupt remap function? Do we need same considering
> for IV bit enable for interrupt remap?

No, there are no unity mappings for irqs, so we are not running into the
same race here.

> Sorry, why you still say this 'init_device_table_dma' can block DMA?
> I just think this function will enable DMA transfer, since  we set
> the V and TV bits, right? or I misunderstand what "block DMA" mean?

When the V and TV bits are not set, it means that all DMA from that
device-id is forwared untranslated by the IOMMU. But if we set V and TV
it means that there is translation information, and the IOMMU translates
the requests using the rest of the DTE information. As all other bits
are 0, this means that page-table-level is 0 (== no page-table) and that
the global IW and IR bits are 0 too (== no read and write permissions).
So all requests are blocked.



	Joerg

Re: [PATCH] iommu/amd: Fix unity mapping initialization race

[Wan ZongShun]

>
>> Sorry, why you still say this 'init_device_table_dma' can block DMA?
>> I just think this function will enable DMA transfer, since  we set
>> the V and TV bits, right? or I misunderstand what "block DMA" mean?
>
> When the V and TV bits are not set, it means that all DMA from that
> device-id is forwared untranslated by the IOMMU. But if we set V and TV
> it means that there is translation information, and the IOMMU translates
> the requests using the rest of the DTE information. As all other bits
> are 0, this means that page-table-level is 0 (== no page-table) and that
> the global IW and IR bits are 0 too (== no read and write permissions).
> So all requests are blocked.
>

Clearly.

Thanks.

>
>
>         Joerg
>
> _______________________________________________
> iommu mailing list
> iommu@lists.linux-foundation.org
> https://lists.linuxfoundation.org/mailman/listinfo/iommu



-- 
---
Vincent Wan(Zongshun)
www.mcuos.com