amd iommu: disable iommu emulation on non-iommu systems

PATCH In review Wei Wang 2 participants 5 messages Jan 26, 2012 – Feb 3, 2012

Share & tools

Wei Wang

author

2012-01-26 10:56 UTC #0 lore raw

PATCH 1/1 amd iommu: disable iommu emulation on non-iommu systems

Introduce a new flag to disable iommu emulation on old iommu systems.
This patch is taken from my v4 patch queue, which is till pending, to make old or non-iommu system to run cleanly without interfered by iommuv2 codes. This might be helpful to isolate iommuv2 code in debugging unstable regressions. The reset part of v4 will be re-based.

Thanks,
Wei

Signed-off-by Wei Wang <[email protected]>

xen/drivers/passthrough/amd/iommu_guest.c +5 -2

diff -r a2a8089b1ffb xen/drivers/passthrough/amd/iommu_guest.c
@@ -805,6 +805,9 @@ int guest_iommu_set_base(struct domain *
     p2m_type_t t;
     struct guest_iommu *iommu = domain_iommu(d);
 
+    if ( !is_hvm_domain(d) || !iommu_enabled || !iommuv2_enabled )
+        return 0;
+
     if ( !iommu )
         return -EACCES;

@@ -867,7 +870,7 @@ int guest_iommu_init(struct domain* d)
     struct guest_iommu *iommu;
     struct hvm_iommu *hd  = domain_hvm_iommu(d);
 
-    if ( !is_hvm_domain(d) )
+    if ( !is_hvm_domain(d) || !iommu_enabled || !iommuv2_enabled )
         return 0;
 
     iommu = xzalloc(struct guest_iommu);

@@ -893,7 +896,7 @@ void guest_iommu_destroy(struct domain *
 {
     struct guest_iommu *iommu;
 
-    if ( !is_hvm_domain(d) )
+    if ( !is_hvm_domain(d) || !iommu_enabled || !iommuv2_enabled )
         return;
 
     iommu = domain_iommu(d);

xen/drivers/passthrough/amd/iommu_init.c +5 -0

diff -r a2a8089b1ffb xen/drivers/passthrough/amd/iommu_init.c
@@ -38,6 +38,7 @@ unsigned short ivrs_bdf_entries;
 static struct radix_tree_root ivrs_maps;
 struct list_head amd_iommu_head;
 struct table_struct device_table;
+bool_t iommuv2_enabled = 0;
 
 static int iommu_has_ht_flag(struct amd_iommu *iommu, u8 mask)
 {

@@ -887,6 +888,9 @@ static int __init amd_iommu_init_one(str
 
     get_iommu_features(iommu);
 
+    if ( iommu->features )
+        iommuv2_enabled = 1;
+
     if ( allocate_cmd_buffer(iommu) == NULL )
         goto error_out;

@@ -950,6 +954,7 @@ static void __init amd_iommu_init_cleanu
     iommu_enabled = 0;
     iommu_passthrough = 0;
     iommu_intremap = 0;
+    iommuv2_enabled = 0;
 }
 
 /*

xen/include/asm-x86/amd-iommu.h +2 -0

diff -r a2a8089b1ffb xen/include/asm-x86/amd-iommu.h
@@ -182,4 +182,6 @@ struct guest_iommu {
     struct guest_iommu_msi  msi;
 };
 
+extern bool_t iommuv2_enabled;
+
 #endif /* _ASM_X86_64_AMD_IOMMU_H */

_______________________________________________
Xen-devel mailing list
[email protected]
http://lists.xensource.com/xen-devel

Jan Beulich

2012-02-01 15:58 UTC #1 lore raw

xen/drivers/passthrough/amd/iommu_guest.c int guest_iommu_set_base(struct domain * · line 809

Quoted 9 lines

Quoted 1 line

Quoted 1 line · Wei Wang

xen/drivers/passthrough/amd/iommu_guest.c +2 -0

@@ -805,6 +805,9 @@ int guest_iommu_set_base(struct domain *
    p2m_type_t t;
    struct guest_iommu *iommu = domain_iommu(d);
 
+    if ( !is_hvm_domain(d) || !iommu_enabled || !iommuv2_enabled )
+        return 0;

Is it really appropriate/correct to return 0 here, while ...

xen/drivers/passthrough/amd/iommu_guest.c int guest_iommu_set_base(struct domain * · line 813

Quoted 4 lines

xen/drivers/passthrough/amd/iommu_guest.c +1 -0
+
    if ( !iommu )
        return -EACCES;
 

... here it is -EACCES? Further, are the extra checks needed at all (i.e. wouldn't domain_iommu() return NULL in all of these cases anyway due to the same checks being added to guest_iommu_init())? If so, the checks you add to guest_iommu_destroy() are pointless too.

Jan

Wei Wang

author

2012-02-03 14:09 UTC #2 lore raw

Quoted 11 lines · Jan Beulich

xen/drivers/passthrough/amd/iommu_guest.c int guest_iommu_set_base(struct domain * · line 809
Quoted 9 lines
Quoted 1 line
Quoted 1 line · Wei Wang
xen/drivers/passthrough/amd/iommu_guest.c +2 -0
@@ -805,6 +805,9 @@ int guest_iommu_set_base(struct domain *
     p2m_type_t t;
     struct guest_iommu *iommu = domain_iommu(d);
 
+    if ( !is_hvm_domain(d) || !iommu_enabled || !iommuv2_enabled )
+        return 0;
Is it really appropriate/correct to return 0 here, while ...

good point, will be fixed in the next try. No one use guest_iommu_set_base so far until remaining patches got committed.

Quoted 10 lines

xen/drivers/passthrough/amd/iommu_guest.c int guest_iommu_set_base(struct domain * · line 812
Quoted 3 lines
xen/drivers/passthrough/amd/iommu_guest.c +1 -0
+
     if ( !iommu )
         return -EACCES;
... here it is -EACCES? Further, are the extra checks needed at all (i.e. wouldn't domain_iommu() return NULL in all of these cases anyway due to the same checks being added to guest_iommu_init())? If so, the checks you add to guest_iommu_destroy() are pointless too.

It is just to make sure those functions are not called by an unexpected code path since it is non-static. But I can remove it if you prefer that.

Thanks,
Wei

Quoted 3 lines

Jan

Jan Beulich

2012-02-03 14:11 UTC #3 lore raw

Quoted 10 lines

Quoted 1 line
Quoted 1 line · Wei Wang
Quoted 5 lines · Jan Beulich
... Further, are the extra checks needed at all
(i.e. wouldn't domain_iommu() return NULL in all of these cases anyway due to the same checks being added to guest_iommu_init())? If so, the checks you add to guest_iommu_destroy() are pointless too.
It is just to make sure those functions are not called by an unexpected code path since it is non-static. But I can remove it if you prefer that.

Keir already committed the patch, but converting things like this (where the impossible is being checked) to assertions is preferred imo (meaning less redundant, possibly dead code in production builds), so a follow-up patch would be appreciated.

Jan

Wei Wang

author

2012-02-03 14:28 UTC #4 lore raw

Quoted 8 lines · Jan Beulich

Quoted 8 lines
Quoted 1 line
Quoted 1 line · Wei Wang
Quoted 5 lines · Jan Beulich
... Further, are the extra checks needed at all
(i.e. wouldn't domain_iommu() return NULL in all of these cases anyway due to the same checks being added to guest_iommu_init())? If so, the checks you add to guest_iommu_destroy() are pointless too.

Quoted 7 lines

Quoted 2 lines
It is just to make sure those functions are not called by an unexpected code path since it is non-static. But I can remove it if you prefer that.
Keir already committed the patch, but converting things like this (where the impossible is being checked) to assertions is preferred imo (meaning less redundant, possibly dead code in production builds), so a follow-up patch would be appreciated.

No problem. Do it right away.
Thanks,
Wei

Quoted 3 lines

Jan