Skip to contentSkip to conversation

amd iommu: interfaces for p2m sharing

PATCH In review Wei Wang2 2 participants 4 messages Apr 15, 2011 – Apr 18, 2011

lore
Share & tools
lore

Wei Wang2

author
#0 lore raw

PATCH v2 4/4 amd iommu: interfaces for p2m sharing

--
Advanced Micro Devices GmbH
Sitz: Dornach, Gemeinde Aschheim,
Landkreis München Registergericht München,
HRB Nr. 43632
WEEE-Reg-Nr: DE 12919551
Geschäftsführer:
Alberto Bozzo, Andrew Bowd

# HG changeset patch
# User Wei Wang <[email protected]>
# Node ID b5c197b1d2ac80881d0683cdadf12ac33b762614
# Parent 1d9b0e45566ec3cd0e293212d9a454920116b2b1
Add a generic interface for both vtd and amd iommu p2m sharing. Also introduce a new parameter (iommu=sharept) to enable this feature.

xen/arch/x86/mm/p2m.c +2 -2
diff -r 1d9b0e45566e -r b5c197b1d2ac xen/arch/x86/mm/p2m.c
@@ -2061,8 +2061,8 @@ int p2m_alloc_table(struct p2m_domain *p
 
     p2m->phys_table = pagetable_from_mfn(page_to_mfn(p2m_top));
 
-    if ( hap_enabled(d) && (boot_cpu_data.x86_vendor == X86_VENDOR_INTEL) )
-        iommu_set_pgd(d);
+    if ( hap_enabled(d) )
+        iommu_share_p2m_table(d);
 
     P2M_PRINTK("populating p2m table\n");
 
xen/drivers/passthrough/amd/iommu_map.c +6 -0
diff -r 1d9b0e45566e -r b5c197b1d2ac xen/drivers/passthrough/amd/iommu_map.c
@@ -716,6 +716,12 @@ void amd_iommu_share_p2m(struct domain *
 
     ASSERT( is_hvm_domain(d) && d->arch.hvm_domain.hap_enabled );
 
+    if ( !iommu_sharept )
+    {
+        iommu_hap_pt_share = 0;
+        return;
+    }
+
     pgd_mfn = pagetable_get_mfn(p2m_get_pagetable(p2m_get_hostp2m(d)));
     p2m_table = mfn_to_page(mfn_x(pgd_mfn));
 
xen/drivers/passthrough/amd/pci_amd_iommu.c +1 -0
diff -r 1d9b0e45566e -r b5c197b1d2ac xen/drivers/passthrough/amd/pci_amd_iommu.c
@@ -458,4 +458,5 @@ const struct iommu_ops amd_iommu_ops = {
     .read_msi_from_ire = amd_iommu_read_msi_from_ire,
     .suspend = amd_iommu_suspend,
     .resume = amd_iommu_resume,
+    .share_p2m = amd_iommu_share_p2m,
 };
xen/drivers/passthrough/iommu.c +11 -0
diff -r 1d9b0e45566e -r b5c197b1d2ac xen/drivers/passthrough/iommu.c
@@ -50,6 +50,7 @@ bool_t __read_mostly iommu_hap_pt_share;
 bool_t __read_mostly iommu_hap_pt_share;
 bool_t __read_mostly amd_iommu_debug;
 bool_t __read_mostly amd_iommu_perdev_intremap;
+bool_t __read_mostly iommu_sharept;
 
 static void __init parse_iommu_param(char *s)
 {
@@ -82,6 +83,8 @@ static void __init parse_iommu_param(cha
             iommu_passthrough = 1;
         else if ( !strcmp(s, "dom0-strict") )
             iommu_dom0_strict = 1;
+        else if ( !strcmp(s, "sharept") )
+            iommu_sharept = 1;
 
         s = ss + 1;
     } while ( ss );
@@ -419,6 +422,14 @@ void iommu_suspend()
         ops->suspend();
 }
 
+void iommu_share_p2m_table(struct domain* d)
+{
+    const struct iommu_ops *ops = iommu_get_ops();
+
+    if ( iommu_enabled && is_hvm_domain(d) )
+        ops->share_p2m(d);
+}
+
 /*
  * Local variables:
  * mode: C
xen/drivers/passthrough/vtd/iommu.c +2 -1
diff -r 1d9b0e45566e -r b5c197b1d2ac xen/drivers/passthrough/vtd/iommu.c
@@ -1747,7 +1747,7 @@ static bool_t vtd_ept_share(void)
     bool_t share = TRUE;
 
     /* sharept defaults to 0 for now, default to 1 when feature matures */
-    if ( !sharept )
+    if ( !iommu_sharept )
         share = FALSE;
 
     /*
@@ -2299,6 +2299,7 @@ const struct iommu_ops intel_iommu_ops =
     .read_msi_from_ire = msi_msg_read_remap_rte,
     .suspend = vtd_suspend,
     .resume = vtd_resume,
+    .share_p2m = iommu_set_pgd,
 };
 
 /*
xen/include/xen/iommu.h +3 -0
diff -r 1d9b0e45566e -r b5c197b1d2ac xen/include/xen/iommu.h
@@ -33,6 +33,7 @@ extern bool_t iommu_hap_pt_share;
 extern bool_t iommu_hap_pt_share;
 extern bool_t amd_iommu_debug;
 extern bool_t amd_iommu_perdev_intremap;
+extern bool_t iommu_sharept;
 
 extern struct rangeset *mmio_ro_ranges;
 
@@ -132,6 +133,7 @@ struct iommu_ops {
     unsigned int (*read_apic_from_ire)(unsigned int apic, unsigned int reg);
     void (*suspend)(void);
     void (*resume)(void);
+    void (*share_p2m)(struct domain *d);
 };
 
 void iommu_update_ire_from_apic(unsigned int apic, unsigned int reg, unsigned int value);
@@ -143,5 +145,6 @@ void iommu_resume(void);
 void iommu_resume(void);
 
 void iommu_set_dom0_mapping(struct domain *d);
+void iommu_share_p2m_table(struct domain *d);
 
 #endif /* _IOMMU_H_ */

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

#1 lore raw
Quoted 9 lines · At 13:14 +0100 on 15 Apr (1302873244), Wei Wang2 wrote:

--
Advanced Micro Devices GmbH
Sitz: Dornach, Gemeinde Aschheim,
Landkreis München Registergericht München,
HRB Nr. 43632
WEEE-Reg-Nr: DE 12919551
Geschäftsführer:
Alberto Bozzo, Andrew Bowd

Quoted 6 lines

# HG changeset patch
# User Wei Wang <[email protected]>
# Node ID b5c197b1d2ac80881d0683cdadf12ac33b762614
# Parent 1d9b0e45566ec3cd0e293212d9a454920116b2b1
Add a generic interface for both vtd and amd iommu p2m sharing. Also introduce a new parameter (iommu=sharept) to enable this feature.

Why does this introduce a separate variable? Surely "iommu=sharept" should just set iommu_hap_pt_share directly. Also, it looks like "iommu=sharept" has different semantics on AMD and Intel machines (i.e., it does nothing on Intel). Was that the intention?

The rest of this series looks OK to me. Is it safe to apply the rest without this patch or should I wait for the next version and apply them all together?

Cheers,

Tim.

Quoted 122 lines

diff -r 1d9b0e45566e -r b5c197b1d2ac xen/arch/x86/mm/p2m.c

xen/arch/x86/mm/p2m.c +2 -2
@@ -2061,8 +2061,8 @@ int p2m_alloc_table(struct p2m_domain *p
 
     p2m->phys_table = pagetable_from_mfn(page_to_mfn(p2m_top));
 
-    if ( hap_enabled(d) && (boot_cpu_data.x86_vendor == X86_VENDOR_INTEL) )
-        iommu_set_pgd(d);
+    if ( hap_enabled(d) )
+        iommu_share_p2m_table(d);
 
     P2M_PRINTK("populating p2m table\n");
 

diff -r 1d9b0e45566e -r b5c197b1d2ac xen/drivers/passthrough/amd/iommu_map.c

xen/drivers/passthrough/amd/iommu_map.c +6 -0
@@ -716,6 +716,12 @@ void amd_iommu_share_p2m(struct domain *
 
     ASSERT( is_hvm_domain(d) && d->arch.hvm_domain.hap_enabled );
 
+    if ( !iommu_sharept )
+    {
+        iommu_hap_pt_share = 0;
+        return;
+    }
+
     pgd_mfn = pagetable_get_mfn(p2m_get_pagetable(p2m_get_hostp2m(d)));
     p2m_table = mfn_to_page(mfn_x(pgd_mfn));
 

diff -r 1d9b0e45566e -r b5c197b1d2ac xen/drivers/passthrough/amd/pci_amd_iommu.c

xen/drivers/passthrough/amd/pci_amd_iommu.c +1 -0
@@ -458,4 +458,5 @@ const struct iommu_ops amd_iommu_ops = {
     .read_msi_from_ire = amd_iommu_read_msi_from_ire,
     .suspend = amd_iommu_suspend,
     .resume = amd_iommu_resume,
+    .share_p2m = amd_iommu_share_p2m,
 };

diff -r 1d9b0e45566e -r b5c197b1d2ac xen/drivers/passthrough/iommu.c

xen/drivers/passthrough/iommu.c +11 -0
@@ -50,6 +50,7 @@ bool_t __read_mostly iommu_hap_pt_share;
 bool_t __read_mostly iommu_hap_pt_share;
 bool_t __read_mostly amd_iommu_debug;
 bool_t __read_mostly amd_iommu_perdev_intremap;
+bool_t __read_mostly iommu_sharept;
 
 static void __init parse_iommu_param(char *s)
 {
@@ -82,6 +83,8 @@ static void __init parse_iommu_param(cha
             iommu_passthrough = 1;
         else if ( !strcmp(s, "dom0-strict") )
             iommu_dom0_strict = 1;
+        else if ( !strcmp(s, "sharept") )
+            iommu_sharept = 1;
 
         s = ss + 1;
     } while ( ss );
@@ -419,6 +422,14 @@ void iommu_suspend()
         ops->suspend();
 }
 
+void iommu_share_p2m_table(struct domain* d)
+{
+    const struct iommu_ops *ops = iommu_get_ops();
+
+    if ( iommu_enabled && is_hvm_domain(d) )
+        ops->share_p2m(d);
+}
+
 /*
  * Local variables:
  * mode: C

diff -r 1d9b0e45566e -r b5c197b1d2ac xen/drivers/passthrough/vtd/iommu.c

xen/drivers/passthrough/vtd/iommu.c +2 -1
@@ -1747,7 +1747,7 @@ static bool_t vtd_ept_share(void)
     bool_t share = TRUE;
 
     /* sharept defaults to 0 for now, default to 1 when feature matures */
-    if ( !sharept )
+    if ( !iommu_sharept )
         share = FALSE;
 
     /*
@@ -2299,6 +2299,7 @@ const struct iommu_ops intel_iommu_ops =
     .read_msi_from_ire = msi_msg_read_remap_rte,
     .suspend = vtd_suspend,
     .resume = vtd_resume,
+    .share_p2m = iommu_set_pgd,
 };
 
 /*

diff -r 1d9b0e45566e -r b5c197b1d2ac xen/include/xen/iommu.h

xen/include/xen/iommu.h +3 -0
@@ -33,6 +33,7 @@ extern bool_t iommu_hap_pt_share;
 extern bool_t iommu_hap_pt_share;
 extern bool_t amd_iommu_debug;
 extern bool_t amd_iommu_perdev_intremap;
+extern bool_t iommu_sharept;
 
 extern struct rangeset *mmio_ro_ranges;
 
@@ -132,6 +133,7 @@ struct iommu_ops {
     unsigned int (*read_apic_from_ire)(unsigned int apic, unsigned int reg);
     void (*suspend)(void);
     void (*resume)(void);
+    void (*share_p2m)(struct domain *d);
 };
 
 void iommu_update_ire_from_apic(unsigned int apic, unsigned int reg, unsigned int value);
@@ -143,5 +145,6 @@ void iommu_resume(void);
 void iommu_resume(void);
 
 void iommu_set_dom0_mapping(struct domain *d);
+void iommu_share_p2m_table(struct domain *d);
 
 #endif /* _IOMMU_H_ */
Signature · 3 lines
Tim Deegan <[email protected]>
Principal Software Engineer, Xen Platform Team
Citrix Systems UK Ltd.  (Company #02937203, SL9 0BG)
author
#2 lore raw
Quoted 17 lines · On Monday 18 April 2011 12:26:36 Tim Deegan wrote:
Quoted 9 lines · At 13:14 +0100 on 15 Apr (1302873244), Wei Wang2 wrote:

--
Advanced Micro Devices GmbH
Sitz: Dornach, Gemeinde Aschheim,
Landkreis München Registergericht München,
HRB Nr. 43632
WEEE-Reg-Nr: DE 12919551
Geschäftsführer:
Alberto Bozzo, Andrew Bowd

Quoted 6 lines

# HG changeset patch
# User Wei Wang <[email protected]>
# Node ID b5c197b1d2ac80881d0683cdadf12ac33b762614
# Parent 1d9b0e45566ec3cd0e293212d9a454920116b2b1
Add a generic interface for both vtd and amd iommu p2m sharing. Also introduce a new parameter (iommu=sharept) to enable this feature.

Quoted 4 lines

Why does this introduce a separate variable? Surely "iommu=sharept" should just set iommu_hap_pt_share directly. Also, it looks like "iommu=sharept" has different semantics on AMD and Intel machines (i.e., it does nothing on Intel). Was that the intention?

Hi Tim
True, that is a little weird.. It looks like some vtd systems have compatible issue of re-using p2m table for iommu, so vtd code only turns iommu_hap_pt_share on after some compatible checks (in vtd_ept_share). To avoid breaking vtd, I intended to keep this in the new code. But your suggestion also looks good to me. We could set iommu_hap_pt_share directly and turn it off if vtd_ept_share() fails. New patch has been attached.

Quoted 3 lines

The rest of this series looks OK to me. Is it safe to apply the rest without this patch or should I wait for the next version and apply them all together?

Yes, it should be OK to just apply the rest. Without the last one, p2m sharing will not been enabled.
Thanks,
Wei

Quoted 121 lines

Cheers,

Tim.

Quoted 117 lines

diff -r 1d9b0e45566e -r b5c197b1d2ac xen/arch/x86/mm/p2m.c

xen/arch/x86/mm/p2m.c +2 -1
@@ -2061,8 +2061,8 @@ int p2m_alloc_table(struct p2m_domain *p
 
     p2m->phys_table = pagetable_from_mfn(page_to_mfn(p2m_top));
 
-    if ( hap_enabled(d) && (boot_cpu_data.x86_vendor ==
 X86_VENDOR_INTEL) ) -        iommu_set_pgd(d);
+    if ( hap_enabled(d) )
+        iommu_share_p2m_table(d);
 
     P2M_PRINTK("populating p2m table\n");

diff -r 1d9b0e45566e -r b5c197b1d2ac xen/drivers/passthrough/amd/iommu_map.c ---
a/xen/drivers/passthrough/amd/iommu_map.c Fri Apr 15 12:10:39 2011 +0200
+++ b/xen/drivers/passthrough/amd/iommu_map.c Fri Apr 15 12:13:24 2011
+0200 @@ -716,6 +716,12 @@ void amd_iommu_share_p2m(struct domain *

     ASSERT( is_hvm_domain(d) && d->arch.hvm_domain.hap_enabled );
xen/arch/x86/mm/p2m.c +6 -0
+    if ( !iommu_sharept )
+    {
+        iommu_hap_pt_share = 0;
+        return;
+    }
+
     pgd_mfn = pagetable_get_mfn(p2m_get_pagetable(p2m_get_hostp2m(d)));
     p2m_table = mfn_to_page(mfn_x(pgd_mfn));

diff -r 1d9b0e45566e -r b5c197b1d2ac xen/drivers/passthrough/amd/pci_amd_iommu.c ---
a/xen/drivers/passthrough/amd/pci_amd_iommu.c Fri Apr 15 12:10:39 2011
+0200 +++ b/xen/drivers/passthrough/amd/pci_amd_iommu.c Fri Apr 15
12:13:24 2011 +0200 @@ -458,4 +458,5 @@ const struct iommu_ops amd_iommu_ops = {
.read_msi_from_ire = amd_iommu_read_msi_from_ire, .suspend = amd_iommu_suspend,
.resume = amd_iommu_resume,
+ .share_p2m = amd_iommu_share_p2m,
};
diff -r 1d9b0e45566e -r b5c197b1d2ac xen/drivers/passthrough/iommu.c

xen/drivers/passthrough/iommu.c +11 -0
@@ -50,6 +50,7 @@ bool_t __read_mostly iommu_hap_pt_share;
 bool_t __read_mostly iommu_hap_pt_share;
 bool_t __read_mostly amd_iommu_debug;
 bool_t __read_mostly amd_iommu_perdev_intremap;
+bool_t __read_mostly iommu_sharept;
 
 static void __init parse_iommu_param(char *s)
 {
@@ -82,6 +83,8 @@ static void __init parse_iommu_param(cha
             iommu_passthrough = 1;
         else if ( !strcmp(s, "dom0-strict") )
             iommu_dom0_strict = 1;
+        else if ( !strcmp(s, "sharept") )
+            iommu_sharept = 1;
 
         s = ss + 1;
     } while ( ss );
@@ -419,6 +422,14 @@ void iommu_suspend()
         ops->suspend();
 }
 
+void iommu_share_p2m_table(struct domain* d)
+{
+    const struct iommu_ops *ops = iommu_get_ops();
+
+    if ( iommu_enabled && is_hvm_domain(d) )
+        ops->share_p2m(d);
+}
+
 /*
  * Local variables:
  * mode: C

diff -r 1d9b0e45566e -r b5c197b1d2ac xen/drivers/passthrough/vtd/iommu.c

xen/drivers/passthrough/vtd/iommu.c +2 -0
@@ -1747,7 +1747,7 @@ static bool_t vtd_ept_share(void)
     bool_t share = TRUE;
 
     /* sharept defaults to 0 for now, default to 1 when feature matures
 */ -    if ( !sharept )
+    if ( !iommu_sharept )
         share = FALSE;
 
     /*
@@ -2299,6 +2299,7 @@ const struct iommu_ops intel_iommu_ops =
     .read_msi_from_ire = msi_msg_read_remap_rte,
     .suspend = vtd_suspend,
     .resume = vtd_resume,
+    .share_p2m = iommu_set_pgd,
 };
 
 /*

diff -r 1d9b0e45566e -r b5c197b1d2ac xen/include/xen/iommu.h

xen/include/xen/iommu.h +2 -0
@@ -33,6 +33,7 @@ extern bool_t iommu_hap_pt_share;
 extern bool_t iommu_hap_pt_share;
 extern bool_t amd_iommu_debug;
 extern bool_t amd_iommu_perdev_intremap;
+extern bool_t iommu_sharept;
 
 extern struct rangeset *mmio_ro_ranges;
 
@@ -132,6 +133,7 @@ struct iommu_ops {
     unsigned int (*read_apic_from_ire)(unsigned int apic, unsigned int
 reg); void (*suspend)(void);
     void (*resume)(void);
+    void (*share_p2m)(struct domain *d);
 };
 
Quoted 8 lines
Quoted 8 lines
xen/include/xen/iommu.h +0 -0
 void iommu_update_ire_from_apic(unsigned int apic, unsigned int reg,

unsigned int value); @@ -143,5 +145,6 @@ void iommu_resume(void);
void iommu_resume(void);

void iommu_set_dom0_mapping(struct domain *d);
+void iommu_share_p2m_table(struct domain *d);

#endif /* _IOMMU_H_ */

# HG changeset patch
# User Wei Wang <[email protected]>
# Node ID 15a9daf8a8aedf21e941a9ed3cb250536ac6972f
# Parent 5846cc03cf0c1a4b02eba76a7b4aa4884de5e957
Add a generic interface for both vtd and amd iommu p2m sharing. Also introduce a new parameter (iommu=sharept) to enable this feature.

xen/arch/x86/mm/p2m.c +2 -2
diff -r 5846cc03cf0c -r 15a9daf8a8ae xen/arch/x86/mm/p2m.c
@@ -2061,8 +2061,8 @@ int p2m_alloc_table(struct p2m_domain *p
 
     p2m->phys_table = pagetable_from_mfn(page_to_mfn(p2m_top));
 
-    if ( hap_enabled(d) && (boot_cpu_data.x86_vendor == X86_VENDOR_INTEL) )
-        iommu_set_pgd(d);
+    if ( hap_enabled(d) )
+        iommu_share_p2m_table(d);
 
     P2M_PRINTK("populating p2m table\n");
 
xen/drivers/passthrough/amd/iommu_map.c +3 -2
diff -r 5846cc03cf0c -r 15a9daf8a8ae xen/drivers/passthrough/amd/iommu_map.c
@@ -716,6 +716,9 @@ void amd_iommu_share_p2m(struct domain *
 
     ASSERT( is_hvm_domain(d) && d->arch.hvm_domain.hap_enabled );
 
+    if ( !iommu_hap_pt_share )
+        return;
+
     pgd_mfn = pagetable_get_mfn(p2m_get_pagetable(p2m_get_hostp2m(d)));
     p2m_table = mfn_to_page(mfn_x(pgd_mfn));
 
@@ -726,8 +729,6 @@ void amd_iommu_share_p2m(struct domain *
 
         /* When sharing p2m with iommu, paging mode = 4 */
         hd->paging_mode = IOMMU_PAGING_MODE_LEVEL_4;
-        iommu_hap_pt_share = 1;
-
         AMD_IOMMU_DEBUG("Share p2m table with iommu: p2m table = 0x%lx\n",
                         mfn_x(pgd_mfn));
     }
xen/drivers/passthrough/amd/pci_amd_iommu.c +1 -0
diff -r 5846cc03cf0c -r 15a9daf8a8ae xen/drivers/passthrough/amd/pci_amd_iommu.c
@@ -458,4 +458,5 @@ const struct iommu_ops amd_iommu_ops = {
     .read_msi_from_ire = amd_iommu_read_msi_from_ire,
     .suspend = amd_iommu_suspend,
     .resume = amd_iommu_resume,
+    .share_p2m = amd_iommu_share_p2m,
 };
xen/drivers/passthrough/iommu.c +10 -0
diff -r 5846cc03cf0c -r 15a9daf8a8ae xen/drivers/passthrough/iommu.c
@@ -82,6 +82,8 @@ static void __init parse_iommu_param(cha
             iommu_passthrough = 1;
         else if ( !strcmp(s, "dom0-strict") )
             iommu_dom0_strict = 1;
+        else if ( !strcmp(s, "sharept") )
+            iommu_hap_pt_share = 1;
 
         s = ss + 1;
     } while ( ss );
@@ -419,6 +421,14 @@ void iommu_suspend()
         ops->suspend();
 }
 
+void iommu_share_p2m_table(struct domain* d)
+{
+    const struct iommu_ops *ops = iommu_get_ops();
+
+    if ( iommu_enabled && is_hvm_domain(d) )
+        ops->share_p2m(d);
+}
+
 /*
  * Local variables:
  * mode: C
xen/drivers/passthrough/vtd/iommu.c +2 -4
diff -r 5846cc03cf0c -r 15a9daf8a8ae xen/drivers/passthrough/vtd/iommu.c
@@ -45,9 +45,6 @@
 #define nr_ioapics              iosapic_get_nr_iosapics()
 #endif
 
-static int sharept = 0;
-boolean_param("sharept", sharept);
-
 int nr_iommus;
 
 static void setup_dom0_devices(struct domain *d);
@@ -1747,7 +1744,7 @@ static bool_t vtd_ept_share(void)
     bool_t share = TRUE;
 
     /* sharept defaults to 0 for now, default to 1 when feature matures */
-    if ( !sharept )
+    if ( !iommu_hap_pt_share )
         share = FALSE;
 
     /*
@@ -2299,6 +2296,7 @@ const struct iommu_ops intel_iommu_ops =
     .read_msi_from_ire = msi_msg_read_remap_rte,
     .suspend = vtd_suspend,
     .resume = vtd_resume,
+    .share_p2m = iommu_set_pgd,
 };
 
 /*
xen/include/xen/iommu.h +2 -0
diff -r 5846cc03cf0c -r 15a9daf8a8ae xen/include/xen/iommu.h
@@ -132,6 +132,7 @@ struct iommu_ops {
     unsigned int (*read_apic_from_ire)(unsigned int apic, unsigned int reg);
     void (*suspend)(void);
     void (*resume)(void);
+    void (*share_p2m)(struct domain *d);
 };
 
 void iommu_update_ire_from_apic(unsigned int apic, unsigned int reg, unsigned int value);
@@ -143,5 +144,6 @@ void iommu_resume(void);
 void iommu_resume(void);
 
 void iommu_set_dom0_mapping(struct domain *d);
+void iommu_share_p2m_table(struct domain *d);
 
 #endif /* _IOMMU_H_ */

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

#3 lore raw

Hi,

I've applied all of these.

Cc'ing Allen: this series changes the xen command-line rune to share EPT and VT-D pagetables from "sharept" to "iommu=sharept". That seems to me like an improvement but if you don't like it, shout.

Cheers,

Tim.

Quoted 299 lines · At 14:48 +0100 on 18 Apr (1303138135), Wei Wang2 wrote:
Quoted 17 lines · On Monday 18 April 2011 12:26:36 Tim Deegan wrote:
Quoted 9 lines · At 13:14 +0100 on 15 Apr (1302873244), Wei Wang2 wrote:

--
Advanced Micro Devices GmbH
Sitz: Dornach, Gemeinde Aschheim,
Landkreis München Registergericht München,
HRB Nr. 43632
WEEE-Reg-Nr: DE 12919551
Geschäftsführer:
Alberto Bozzo, Andrew Bowd

Quoted 6 lines

# HG changeset patch
# User Wei Wang <[email protected]>
# Node ID b5c197b1d2ac80881d0683cdadf12ac33b762614
# Parent 1d9b0e45566ec3cd0e293212d9a454920116b2b1
Add a generic interface for both vtd and amd iommu p2m sharing. Also introduce a new parameter (iommu=sharept) to enable this feature.

Quoted 4 lines

Why does this introduce a separate variable? Surely "iommu=sharept" should just set iommu_hap_pt_share directly. Also, it looks like "iommu=sharept" has different semantics on AMD and Intel machines (i.e., it does nothing on Intel). Was that the intention?

Hi Tim
True, that is a little weird.. It looks like some vtd systems have compatible issue of re-using p2m table for iommu, so vtd code only turns iommu_hap_pt_share on after some compatible checks (in vtd_ept_share). To avoid breaking vtd, I intended to keep this in the new code. But your suggestion also looks good to me. We could set iommu_hap_pt_share directly and turn it off if vtd_ept_share() fails. New patch has been attached.

Quoted 3 lines

The rest of this series looks OK to me. Is it safe to apply the rest without this patch or should I wait for the next version and apply them all together?

Yes, it should be OK to just apply the rest. Without the last one, p2m sharing will not been enabled.
Thanks,
Wei

Quoted 121 lines

Cheers,

Tim.

Quoted 117 lines

diff -r 1d9b0e45566e -r b5c197b1d2ac xen/arch/x86/mm/p2m.c

xen/arch/x86/mm/p2m.c +2 -1
@@ -2061,8 +2061,8 @@ int p2m_alloc_table(struct p2m_domain *p
 
     p2m->phys_table = pagetable_from_mfn(page_to_mfn(p2m_top));
 
-    if ( hap_enabled(d) && (boot_cpu_data.x86_vendor ==
 X86_VENDOR_INTEL) ) -        iommu_set_pgd(d);
+    if ( hap_enabled(d) )
+        iommu_share_p2m_table(d);
 
     P2M_PRINTK("populating p2m table\n");

diff -r 1d9b0e45566e -r b5c197b1d2ac xen/drivers/passthrough/amd/iommu_map.c ---
a/xen/drivers/passthrough/amd/iommu_map.c Fri Apr 15 12:10:39 2011 +0200
+++ b/xen/drivers/passthrough/amd/iommu_map.c Fri Apr 15 12:13:24 2011
+0200 @@ -716,6 +716,12 @@ void amd_iommu_share_p2m(struct domain *

     ASSERT( is_hvm_domain(d) && d->arch.hvm_domain.hap_enabled );
xen/arch/x86/mm/p2m.c +6 -0
+    if ( !iommu_sharept )
+    {
+        iommu_hap_pt_share = 0;
+        return;
+    }
+
     pgd_mfn = pagetable_get_mfn(p2m_get_pagetable(p2m_get_hostp2m(d)));
     p2m_table = mfn_to_page(mfn_x(pgd_mfn));

diff -r 1d9b0e45566e -r b5c197b1d2ac xen/drivers/passthrough/amd/pci_amd_iommu.c ---
a/xen/drivers/passthrough/amd/pci_amd_iommu.c Fri Apr 15 12:10:39 2011
+0200 +++ b/xen/drivers/passthrough/amd/pci_amd_iommu.c Fri Apr 15
12:13:24 2011 +0200 @@ -458,4 +458,5 @@ const struct iommu_ops amd_iommu_ops = {
.read_msi_from_ire = amd_iommu_read_msi_from_ire, .suspend = amd_iommu_suspend,
.resume = amd_iommu_resume,
+ .share_p2m = amd_iommu_share_p2m,
};
diff -r 1d9b0e45566e -r b5c197b1d2ac xen/drivers/passthrough/iommu.c

xen/drivers/passthrough/iommu.c +11 -0
@@ -50,6 +50,7 @@ bool_t __read_mostly iommu_hap_pt_share;
 bool_t __read_mostly iommu_hap_pt_share;
 bool_t __read_mostly amd_iommu_debug;
 bool_t __read_mostly amd_iommu_perdev_intremap;
+bool_t __read_mostly iommu_sharept;
 
 static void __init parse_iommu_param(char *s)
 {
@@ -82,6 +83,8 @@ static void __init parse_iommu_param(cha
             iommu_passthrough = 1;
         else if ( !strcmp(s, "dom0-strict") )
             iommu_dom0_strict = 1;
+        else if ( !strcmp(s, "sharept") )
+            iommu_sharept = 1;
 
         s = ss + 1;
     } while ( ss );
@@ -419,6 +422,14 @@ void iommu_suspend()
         ops->suspend();
 }
 
+void iommu_share_p2m_table(struct domain* d)
+{
+    const struct iommu_ops *ops = iommu_get_ops();
+
+    if ( iommu_enabled && is_hvm_domain(d) )
+        ops->share_p2m(d);
+}
+
 /*
  * Local variables:
  * mode: C

diff -r 1d9b0e45566e -r b5c197b1d2ac xen/drivers/passthrough/vtd/iommu.c

xen/drivers/passthrough/vtd/iommu.c +2 -0
@@ -1747,7 +1747,7 @@ static bool_t vtd_ept_share(void)
     bool_t share = TRUE;
 
     /* sharept defaults to 0 for now, default to 1 when feature matures
 */ -    if ( !sharept )
+    if ( !iommu_sharept )
         share = FALSE;
 
     /*
@@ -2299,6 +2299,7 @@ const struct iommu_ops intel_iommu_ops =
     .read_msi_from_ire = msi_msg_read_remap_rte,
     .suspend = vtd_suspend,
     .resume = vtd_resume,
+    .share_p2m = iommu_set_pgd,
 };
 
 /*

diff -r 1d9b0e45566e -r b5c197b1d2ac xen/include/xen/iommu.h

xen/include/xen/iommu.h +2 -0
@@ -33,6 +33,7 @@ extern bool_t iommu_hap_pt_share;
 extern bool_t iommu_hap_pt_share;
 extern bool_t amd_iommu_debug;
 extern bool_t amd_iommu_perdev_intremap;
+extern bool_t iommu_sharept;
 
 extern struct rangeset *mmio_ro_ranges;
 
@@ -132,6 +133,7 @@ struct iommu_ops {
     unsigned int (*read_apic_from_ire)(unsigned int apic, unsigned int
 reg); void (*suspend)(void);
     void (*resume)(void);
+    void (*share_p2m)(struct domain *d);
 };
 
Quoted 8 lines
Quoted 8 lines
xen/include/xen/iommu.h +0 -0
 void iommu_update_ire_from_apic(unsigned int apic, unsigned int reg,

unsigned int value); @@ -143,5 +145,6 @@ void iommu_resume(void);
void iommu_resume(void);

void iommu_set_dom0_mapping(struct domain *d);
+void iommu_share_p2m_table(struct domain *d);

#endif /* _IOMMU_H_ */

# HG changeset patch
# User Wei Wang <[email protected]>
# Node ID 15a9daf8a8aedf21e941a9ed3cb250536ac6972f
# Parent 5846cc03cf0c1a4b02eba76a7b4aa4884de5e957
Add a generic interface for both vtd and amd iommu p2m sharing. Also introduce a new parameter (iommu=sharept) to enable this feature.

diff -r 5846cc03cf0c -r 15a9daf8a8ae xen/arch/x86/mm/p2m.c

xen/arch/x86/mm/p2m.c +2 -2
@@ -2061,8 +2061,8 @@ int p2m_alloc_table(struct p2m_domain *p
 
     p2m->phys_table = pagetable_from_mfn(page_to_mfn(p2m_top));
 
-    if ( hap_enabled(d) && (boot_cpu_data.x86_vendor == X86_VENDOR_INTEL) )
-        iommu_set_pgd(d);
+    if ( hap_enabled(d) )
+        iommu_share_p2m_table(d);
 
     P2M_PRINTK("populating p2m table\n");
 

diff -r 5846cc03cf0c -r 15a9daf8a8ae xen/drivers/passthrough/amd/iommu_map.c

xen/drivers/passthrough/amd/iommu_map.c +3 -2
@@ -716,6 +716,9 @@ void amd_iommu_share_p2m(struct domain *
 
     ASSERT( is_hvm_domain(d) && d->arch.hvm_domain.hap_enabled );
 
+    if ( !iommu_hap_pt_share )
+        return;
+
     pgd_mfn = pagetable_get_mfn(p2m_get_pagetable(p2m_get_hostp2m(d)));
     p2m_table = mfn_to_page(mfn_x(pgd_mfn));
 
@@ -726,8 +729,6 @@ void amd_iommu_share_p2m(struct domain *
 
         /* When sharing p2m with iommu, paging mode = 4 */
         hd->paging_mode = IOMMU_PAGING_MODE_LEVEL_4;
-        iommu_hap_pt_share = 1;
-
         AMD_IOMMU_DEBUG("Share p2m table with iommu: p2m table = 0x%lx\n",
                         mfn_x(pgd_mfn));
     }

diff -r 5846cc03cf0c -r 15a9daf8a8ae xen/drivers/passthrough/amd/pci_amd_iommu.c

xen/drivers/passthrough/amd/pci_amd_iommu.c +1 -0
@@ -458,4 +458,5 @@ const struct iommu_ops amd_iommu_ops = {
     .read_msi_from_ire = amd_iommu_read_msi_from_ire,
     .suspend = amd_iommu_suspend,
     .resume = amd_iommu_resume,
+    .share_p2m = amd_iommu_share_p2m,
 };

diff -r 5846cc03cf0c -r 15a9daf8a8ae xen/drivers/passthrough/iommu.c

xen/drivers/passthrough/iommu.c +10 -0
@@ -82,6 +82,8 @@ static void __init parse_iommu_param(cha
             iommu_passthrough = 1;
         else if ( !strcmp(s, "dom0-strict") )
             iommu_dom0_strict = 1;
+        else if ( !strcmp(s, "sharept") )
+            iommu_hap_pt_share = 1;
 
         s = ss + 1;
     } while ( ss );
@@ -419,6 +421,14 @@ void iommu_suspend()
         ops->suspend();
 }
 
+void iommu_share_p2m_table(struct domain* d)
+{
+    const struct iommu_ops *ops = iommu_get_ops();
+
+    if ( iommu_enabled && is_hvm_domain(d) )
+        ops->share_p2m(d);
+}
+
 /*
  * Local variables:
  * mode: C

diff -r 5846cc03cf0c -r 15a9daf8a8ae xen/drivers/passthrough/vtd/iommu.c

xen/drivers/passthrough/vtd/iommu.c +2 -4
@@ -45,9 +45,6 @@
 #define nr_ioapics              iosapic_get_nr_iosapics()
 #endif
 
-static int sharept = 0;
-boolean_param("sharept", sharept);
-
 int nr_iommus;
 
 static void setup_dom0_devices(struct domain *d);
@@ -1747,7 +1744,7 @@ static bool_t vtd_ept_share(void)
     bool_t share = TRUE;
 
     /* sharept defaults to 0 for now, default to 1 when feature matures */
-    if ( !sharept )
+    if ( !iommu_hap_pt_share )
         share = FALSE;
 
     /*
@@ -2299,6 +2296,7 @@ const struct iommu_ops intel_iommu_ops =
     .read_msi_from_ire = msi_msg_read_remap_rte,
     .suspend = vtd_suspend,
     .resume = vtd_resume,
+    .share_p2m = iommu_set_pgd,
 };
 
 /*

diff -r 5846cc03cf0c -r 15a9daf8a8ae xen/include/xen/iommu.h

xen/include/xen/iommu.h +2 -0
@@ -132,6 +132,7 @@ struct iommu_ops {
     unsigned int (*read_apic_from_ire)(unsigned int apic, unsigned int reg);
     void (*suspend)(void);
     void (*resume)(void);
+    void (*share_p2m)(struct domain *d);
 };
 
 void iommu_update_ire_from_apic(unsigned int apic, unsigned int reg, unsigned int value);
@@ -143,5 +144,6 @@ void iommu_resume(void);
 void iommu_resume(void);
 
 void iommu_set_dom0_mapping(struct domain *d);
+void iommu_share_p2m_table(struct domain *d);
 
 #endif /* _IOMMU_H_ */
Signature · 3 lines
Tim Deegan <[email protected]>
Principal Software Engineer, Xen Platform Team
Citrix Systems UK Ltd.  (Company #02937203, SL9 0BG)