[PATCH] intel-iommu: Default to non-coherent for domains unattached to iommus

[PATCH] intel-iommu: Default to non-coherent for domains unattached to iommus

[Donald Dutile]

This patch was posted back in Nov 2011:
  http://lists.linuxfoundation.org/pipermail/iommu/2011-November/003086.html

and due to discussion about the patch, it was never pulled in.
Although the thread discussed an alternate patch to
default to non-coherent if any IOMMU didn't support coherency,
this alternate method was never implemented, and this bug persists.

This patch has been in RHEL6 for quite some time,
and it wasn't noticed that it didn't get into linux upstream, 
until a RH partner reported this error when running upstream kernels,
and noticed how it doesn't occur on RHEL6 kernels.
Applying this patch to an upstream kernel resolved this issue.


 domain_update_iommu_coherency() currently defaults to setting
 domains as coherent when the domain is not attached to any iommus.
 This allows for a window in domain_context_mapping_one() where such a
 domain can update context entries non-coherently, and only after
 update the domain capability to clear iommu_coherency.
 This can be seen using KVM device assignment on VT-d systems that
 do not support coherency in the ecap register.  When a device is
 added to a guest, a domain is created (iommu_coherency = 0), the
 device is attached, and ranges are mapped.  If we then hot unplug
 the device, the coherency is updated and set to the default (1)
 since no iommus are attached to the domain.  A subsequent attach
 of a device makes use of the same dmar domain (now marked coherent)
 updates context entries with coherency enabled, and only disables
 coherency as the last step in the process.
 To fix this, switch domain_update_iommu_coherency() to use the
 safer, non-coherent default for domains not attached to iommus.

Signed-off-by: Donald Dutile <ddutile-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
cc: Alex Williamson <alex.williamson at redhat.com>
---
 drivers/iommu/intel-iommu.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/iommu/intel-iommu.c b/drivers/iommu/intel-iommu.c
index 2297ec1..52ff63f 100644
--- a/drivers/iommu/intel-iommu.c
+++ b/drivers/iommu/intel-iommu.c
@@ -589,7 +589,9 @@ static void domain_update_iommu_coherency(struct dmar_domain *domain)
 {
 	int i;
 
-	domain->iommu_coherency = 1;
+	i = find_first_bit(domain->iommu_bmp, g_num_of_iommus);
+
+	domain->iommu_coherency = i < g_num_of_iommus ? 1: 0;
 
 	for_each_set_bit(i, domain->iommu_bmp, g_num_of_iommus) {
 		if (!ecap_coherent(g_iommus[i]->ecap)) {
-- 
1.7.10.2.552.gaa3bb87

Re: [PATCH] intel-iommu: Default to non-coherent for domains unattached to iommus

[Joerg Roedel]

On Wed, Sep 12, 2012 at 03:55:05PM -0400, Donald Dutile wrote:
> This patch was posted back in Nov 2011:
>   http://lists.linuxfoundation.org/pipermail/iommu/2011-November/003086.html
> 
> and due to discussion about the patch, it was never pulled in.
> Although the thread discussed an alternate patch to
> default to non-coherent if any IOMMU didn't support coherency,
> this alternate method was never implemented, and this bug persists.
> 
> This patch has been in RHEL6 for quite some time,
> and it wasn't noticed that it didn't get into linux upstream, 
> until a RH partner reported this error when running upstream kernels,
> and noticed how it doesn't occur on RHEL6 kernels.
> Applying this patch to an upstream kernel resolved this issue.
> 
> 
>  domain_update_iommu_coherency() currently defaults to setting
>  domains as coherent when the domain is not attached to any iommus.
>  This allows for a window in domain_context_mapping_one() where such a
>  domain can update context entries non-coherently, and only after
>  update the domain capability to clear iommu_coherency.
>  This can be seen using KVM device assignment on VT-d systems that
>  do not support coherency in the ecap register.  When a device is
>  added to a guest, a domain is created (iommu_coherency = 0), the
>  device is attached, and ranges are mapped.  If we then hot unplug
>  the device, the coherency is updated and set to the default (1)
>  since no iommus are attached to the domain.  A subsequent attach
>  of a device makes use of the same dmar domain (now marked coherent)
>  updates context entries with coherency enabled, and only disables
>  coherency as the last step in the process.
>  To fix this, switch domain_update_iommu_coherency() to use the
>  safer, non-coherent default for domains not attached to iommus.
> 
> Signed-off-by: Donald Dutile <ddutile-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> cc: Alex Williamson <alex.williamson at redhat.com>

Hmm, who is the author? The patch looks the same as what Alex submitted
last year. I applied Alex' patch because it includes also the Acked-bys
and he seems to be the author anyway. Oh, and I added a stable-tag.


	Joerg

Re: [PATCH] intel-iommu: Default to non-coherent for domains unattached to iommus

[Don Dutile]

On 09/18/2012 07:59 AM, Joerg Roedel wrote:
> On Wed, Sep 12, 2012 at 03:55:05PM -0400, Donald Dutile wrote:
>> This patch was posted back in Nov 2011:
>>    http://lists.linuxfoundation.org/pipermail/iommu/2011-November/003086.html
>>
>> and due to discussion about the patch, it was never pulled in.
>> Although the thread discussed an alternate patch to
>> default to non-coherent if any IOMMU didn't support coherency,
>> this alternate method was never implemented, and this bug persists.
>>
>> This patch has been in RHEL6 for quite some time,
>> and it wasn't noticed that it didn't get into linux upstream,
>> until a RH partner reported this error when running upstream kernels,
>> and noticed how it doesn't occur on RHEL6 kernels.
>> Applying this patch to an upstream kernel resolved this issue.
>>
>>
>>   domain_update_iommu_coherency() currently defaults to setting
>>   domains as coherent when the domain is not attached to any iommus.
>>   This allows for a window in domain_context_mapping_one() where such a
>>   domain can update context entries non-coherently, and only after
>>   update the domain capability to clear iommu_coherency.
>>   This can be seen using KVM device assignment on VT-d systems that
>>   do not support coherency in the ecap register.  When a device is
>>   added to a guest, a domain is created (iommu_coherency = 0), the
>>   device is attached, and ranges are mapped.  If we then hot unplug
>>   the device, the coherency is updated and set to the default (1)
>>   since no iommus are attached to the domain.  A subsequent attach
>>   of a device makes use of the same dmar domain (now marked coherent)
>>   updates context entries with coherency enabled, and only disables
>>   coherency as the last step in the process.
>>   To fix this, switch domain_update_iommu_coherency() to use the
>>   safer, non-coherent default for domains not attached to iommus.
>>
>> Signed-off-by: Donald Dutile<ddutile-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
>> cc: Alex Williamson<alex.williamson at redhat.com>
>
> Hmm, who is the author? The patch looks the same as what Alex submitted
> last year. I applied Alex' patch because it includes also the Acked-bys
> and he seems to be the author anyway. Oh, and I added a stable-tag.
>
>
> 	Joerg
>
>
Yes, Alex was the original author, thus the reason I cc'd him on the update.

And you can't apply Alex's original patch as-is, since the iommu_bmp
structure element changed from a ptr to an array,
(a snippet of) alex's patch looked like:
+    i = find_first_bit(&domain->iommu_bmp, g_num_of_iommus);

and the correct patch with the change to iommu_bmp is:
+    i = find_first_bit(domain->iommu_bmp, g_num_of_iommus);

and the reason why I re-posted it, vs fwd-ing the original patch
& asking for inclusion.
Additionally, the above patch is what the customer tested & verified.

So, if you made the above adjustment to Alex's patch,
then the patch is ok.  If not, the above adjustment must be made.

- Don

Re: [PATCH] intel-iommu: Default to non-coherent for domains unattached to iommus

[Joerg Roedel]

On Tue, Sep 18, 2012 at 09:57:58AM -0400, Don Dutile wrote:
> And you can't apply Alex's original patch as-is, since the iommu_bmp
> structure element changed from a ptr to an array,
> (a snippet of) alex's patch looked like:
> +    i = find_first_bit(&domain->iommu_bmp, g_num_of_iommus);
> 
> and the correct patch with the change to iommu_bmp is:
> +    i = find_first_bit(domain->iommu_bmp, g_num_of_iommus);
> 
> and the reason why I re-posted it, vs fwd-ing the original patch
> & asking for inclusion.
> Additionally, the above patch is what the customer tested & verified.
> 
> So, if you made the above adjustment to Alex's patch,
> then the patch is ok.  If not, the above adjustment must be made.

Okay, fine then. I made the adjustment to Alex' patch and will push my
updated tree out after some compile-testing.


	Joerg

-- 
AMD Operating System Research Center

Advanced Micro Devices GmbH Einsteinring 24 85609 Dornach
General Managers: Alberto Bozzo
Registration: Dornach, Landkr. Muenchen; Registerger. Muenchen, HRB Nr. 43632