* [PATCH][VTD] bug fix for EPT/VT-d table sharing
@ 2011-01-05 19:36 Kay, Allen M
2011-01-06 8:43 ` Jan Beulich
0 siblings, 1 reply; 9+ messages in thread
From: Kay, Allen M @ 2011-01-05 19:36 UTC (permalink / raw)
To: xen-devel@lists.xensource.com; +Cc: Keir Fraser, Tim Deegan
[-- Attachment #1: Type: text/plain, Size: 439 bytes --]
This patch makes following changes: 1) Moves EPT/VT-d sharing initialization back to when it is actually needed to make sure vmx_ept_vpid_cap has been initialized. 2) added page order parameter to iommu_pte_flush() to tell VT-d what size of page to flush. 3) added hap_2mb flag to ease performance studies between base 4KB EPT size and when 2MB and 1GB page size support are enabled.
Signed-off-by: Allen Kay <allen.m.kay@intel.com>
[-- Attachment #2: share0105.patch --]
[-- Type: application/octet-stream, Size: 7017 bytes --]
diff -r 39194f457534 xen/arch/x86/mm/hap/p2m-ept.c
--- a/xen/arch/x86/mm/hap/p2m-ept.c Wed Jan 05 09:57:15 2011 +0000
+++ b/xen/arch/x86/mm/hap/p2m-ept.c Thu Dec 23 05:51:20 2010 -0800
@@ -413,7 +413,7 @@
if ( rv && iommu_enabled && need_iommu(p2m->domain) && need_modify_vtd_table )
{
if ( iommu_hap_pt_share )
- iommu_pte_flush(d, gfn, (u64*)ept_entry, vtd_pte_present);
+ iommu_pte_flush(d, gfn, (u64*)ept_entry, order, vtd_pte_present);
else
{
if ( p2mt == p2m_ram_rw )
diff -r 39194f457534 xen/arch/x86/mm/p2m.c
--- a/xen/arch/x86/mm/p2m.c Wed Jan 05 09:57:15 2011 +0000
+++ b/xen/arch/x86/mm/p2m.c Thu Dec 23 05:51:20 2010 -0800
@@ -43,6 +43,9 @@
static bool_t __read_mostly opt_hap_1gb = 1;
boolean_param("hap_1gb", opt_hap_1gb);
+static bool_t __read_mostly opt_hap_2mb = 1;
+boolean_param("hap_2mb", opt_hap_2mb);
+
/* Printouts */
#define P2M_PRINTK(_f, _a...) \
debugtrace_printk("p2m: %s(): " _f, __func__, ##_a)
@@ -1772,7 +1775,7 @@
order = ( (((gfn | mfn_x(mfn) | todo) & ((1ul << 18) - 1)) == 0) &&
hvm_hap_has_1gb(d) && opt_hap_1gb ) ? 18 :
((((gfn | mfn_x(mfn) | todo) & ((1ul << 9) - 1)) == 0) &&
- hvm_hap_has_2mb(d)) ? 9 : 0;
+ hvm_hap_has_2mb(d) && opt_hap_2mb) ? 9 : 0;
else
order = 0;
diff -r 39194f457534 xen/drivers/passthrough/vtd/iommu.c
--- a/xen/drivers/passthrough/vtd/iommu.c Wed Jan 05 09:57:15 2011 +0000
+++ b/xen/drivers/passthrough/vtd/iommu.c Thu Dec 23 05:51:20 2010 -0800
@@ -535,7 +535,7 @@
static int inline iommu_flush_iotlb_psi(
struct iommu *iommu, u16 did, u64 addr, unsigned int pages,
- int flush_non_present_entry, int flush_dev_iotlb)
+ int page_shift, int flush_non_present_entry, int flush_dev_iotlb)
{
unsigned int align;
struct iommu_flush *flush = iommu_get_flush(iommu);
@@ -552,13 +552,14 @@
* PSI requires page size is 2 ^ x, and the base address is naturally
* aligned to the size
*/
- align = get_alignment(addr >> PAGE_SHIFT_4K, pages);
+ align = get_alignment(addr >> page_shift, pages);
+
/* Fallback to domain selective flush if size is too big */
if ( align > cap_max_amask_val(iommu->cap) )
return iommu_flush_iotlb_dsi(iommu, did, flush_non_present_entry, flush_dev_iotlb);
- addr >>= PAGE_SHIFT_4K + align;
- addr <<= PAGE_SHIFT_4K + align;
+ addr >>= page_shift + align;
+ addr <<= page_shift + align;
/* apply platform specific errata workarounds */
vtd_ops_preamble_quirk(iommu);
@@ -635,7 +636,8 @@
if ( iommu_domid == -1 )
continue;
if ( iommu_flush_iotlb_psi(iommu, iommu_domid,
- addr, 1, 0, flush_dev_iotlb) )
+ addr, 1, PAGE_SHIFT_4K,
+ 0, flush_dev_iotlb) )
iommu_flush_write_buffer(iommu);
}
}
@@ -1711,6 +1713,7 @@
continue;
if ( iommu_flush_iotlb_psi(iommu, iommu_domid,
(paddr_t)gfn << PAGE_SHIFT_4K, 1,
+ PAGE_SHIFT_4K,
!dma_pte_present(old), flush_dev_iotlb) )
iommu_flush_write_buffer(iommu);
}
@@ -1729,13 +1732,15 @@
return 0;
}
-void iommu_pte_flush(struct domain *d, u64 gfn, u64 *pte, int present)
+void iommu_pte_flush(struct domain *d, u64 gfn, u64 *pte,
+ u32 order, int present)
{
struct acpi_drhd_unit *drhd;
struct iommu *iommu = NULL;
struct hvm_iommu *hd = domain_hvm_iommu(d);
int flush_dev_iotlb;
int iommu_domid;
+ int page_shift = (order == 0) ? PAGE_SHIFT_4K : PAGE_SHIFT_4K + order;
iommu_flush_cache_entry(pte, sizeof(struct dma_pte));
@@ -1750,8 +1755,8 @@
if ( iommu_domid == -1 )
continue;
if ( iommu_flush_iotlb_psi(iommu, iommu_domid,
- (paddr_t)gfn << PAGE_SHIFT_4K, 1,
- !present, flush_dev_iotlb) )
+ (paddr_t)gfn << page_shift, 1,
+ page_shift, !present, flush_dev_iotlb) )
iommu_flush_write_buffer(iommu);
}
}
@@ -1769,6 +1774,28 @@
return 1;
}
+static bool_t vtd_ept_share(void)
+{
+ struct acpi_drhd_unit *drhd;
+ struct iommu *iommu;
+ bool_t share = TRUE;
+
+ /* sharept defaults to 0 for now, default to 1 when feature matures */
+ if ( !sharept )
+ share = FALSE;
+
+ /*
+ * Determine whether EPT and VT-d page tables can be shared or not.
+ */
+ for_each_drhd_unit ( drhd )
+ {
+ iommu = drhd->iommu;
+ if ( !vtd_ept_page_compatible(drhd->iommu) )
+ share = FALSE;
+ }
+ return share;
+}
+
/*
* set VT-d page table directory to EPT table if allowed
*/
@@ -1779,11 +1806,13 @@
ASSERT( is_hvm_domain(d) && d->arch.hvm_domain.hap_enabled );
- if ( !iommu_hap_pt_share )
- return;
-
+ iommu_hap_pt_share = vtd_ept_share();
pgd_mfn = pagetable_get_mfn(p2m_get_pagetable(p2m_get_hostp2m(d)));
hd->pgd_maddr = pagetable_get_paddr(pagetable_from_mfn(pgd_mfn));
+
+ dprintk(XENLOG_INFO VTDPREFIX,
+ "VT-d page table %s with EPT table\n",
+ iommu_hap_pt_share ? "shares" : "not sharing");
}
static int domain_rmrr_mapped(struct domain *d,
@@ -2036,27 +2065,6 @@
}
}
iommu_flush_all();
-
- /*
- * Determine whether EPT and VT-d page tables can be shared or not.
- */
- iommu_hap_pt_share = TRUE;
- for_each_drhd_unit ( drhd )
- {
- iommu = drhd->iommu;
- if ( (drhd->iommu->nr_pt_levels != VTD_PAGE_TABLE_LEVEL_4) ||
- !vtd_ept_page_compatible(drhd->iommu) )
- iommu_hap_pt_share = FALSE;
- }
-
- /* keep boot flag sharept as safe fallback. remove after feature matures */
- if ( !sharept )
- iommu_hap_pt_share = FALSE;
-
- dprintk(XENLOG_INFO VTDPREFIX,
- "VT-d page table %sshared with EPT table\n",
- iommu_hap_pt_share ? "" : "not ");
-
return 0;
}
diff -r 39194f457534 xen/include/xen/iommu.h
--- a/xen/include/xen/iommu.h Wed Jan 05 09:57:15 2011 +0000
+++ b/xen/include/xen/iommu.h Thu Dec 23 05:51:20 2010 -0800
@@ -85,7 +85,7 @@
int iommu_map_page(struct domain *d, unsigned long gfn, unsigned long mfn,
unsigned int flags);
int iommu_unmap_page(struct domain *d, unsigned long gfn);
-void iommu_pte_flush(struct domain *d, u64 gfn, u64 *pte, int present);
+void iommu_pte_flush(struct domain *d, u64 gfn, u64 *pte, u32 order, int present);
void iommu_set_pgd(struct domain *d);
void iommu_domain_teardown(struct domain *d);
int hvm_do_IRQ_dpci(struct domain *d, unsigned int irq);
[-- Attachment #3: Type: text/plain, Size: 138 bytes --]
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH][VTD] bug fix for EPT/VT-d table sharing 2011-01-05 19:36 [PATCH][VTD] bug fix for EPT/VT-d table sharing Kay, Allen M @ 2011-01-06 8:43 ` Jan Beulich 2011-01-07 2:49 ` Kay, Allen M 0 siblings, 1 reply; 9+ messages in thread From: Jan Beulich @ 2011-01-06 8:43 UTC (permalink / raw) To: Allen M Kay; +Cc: xen-devel@lists.xensource.com, Keir Fraser, Tim Deegan >>> On 05.01.11 at 20:36, "Kay, Allen M" <allen.m.kay@intel.com> wrote: >@@ -1729,13 +1732,15 @@ > return 0; > } > >-void iommu_pte_flush(struct domain *d, u64 gfn, u64 *pte, int present) >+void iommu_pte_flush(struct domain *d, u64 gfn, u64 *pte, >+ u32 order, int present) u32? Shouldn't fixed-size types only be used where really necessary? > { > struct acpi_drhd_unit *drhd; > struct iommu *iommu = NULL; > struct hvm_iommu *hd = domain_hvm_iommu(d); > int flush_dev_iotlb; > int iommu_domid; >+ int page_shift = (order == 0) ? PAGE_SHIFT_4K : PAGE_SHIFT_4K + order; Why not simply int page_shift = PAGE_SHIFT_4K + order; > > iommu_flush_cache_entry(pte, sizeof(struct dma_pte)); > >@@ -1750,8 +1755,8 @@ > if ( iommu_domid == -1 ) > continue; > if ( iommu_flush_iotlb_psi(iommu, iommu_domid, >- (paddr_t)gfn << PAGE_SHIFT_4K, 1, >- !present, flush_dev_iotlb) ) >+ (paddr_t)gfn << page_shift, 1, This certainly isn't correct, at least as long as "gfn" is what its name says (and since the only caller simply adds an "order" argument without adjusting the "gfn" one, I'm sure it is). >+ page_shift, !present, flush_dev_iotlb) ) > iommu_flush_write_buffer(iommu); > } > } Jan ^ permalink raw reply [flat|nested] 9+ messages in thread
* RE: [PATCH][VTD] bug fix for EPT/VT-d table sharing 2011-01-06 8:43 ` Jan Beulich @ 2011-01-07 2:49 ` Kay, Allen M 2011-01-07 8:25 ` Jan Beulich 0 siblings, 1 reply; 9+ messages in thread From: Kay, Allen M @ 2011-01-07 2:49 UTC (permalink / raw) To: Jan Beulich; +Cc: xen-devel@lists.xensource.com, Keir Fraser, Tim Deegan [-- Attachment #1: Type: text/plain, Size: 2151 bytes --] Thanks for your feedback. I have incorporated all of them in the attached patch. Hopefully my understanding of gfn is correct now. Also I have found an pre-existing EPT 1GB page issue. I have included the fix in this patch also: - if ( order == EPT_TABLE_ORDER ) + if ( order > 0 ) I also remembered to use "hg diff -p" this time to make review easier. Allen -----Original Message----- From: Jan Beulich [mailto:JBeulich@novell.com] Sent: Thursday, January 06, 2011 12:43 AM To: Kay, Allen M Cc: Tim Deegan; xen-devel@lists.xensource.com; Keir Fraser Subject: Re: [Xen-devel] [PATCH][VTD] bug fix for EPT/VT-d table sharing >>> On 05.01.11 at 20:36, "Kay, Allen M" <allen.m.kay@intel.com> wrote: >@@ -1729,13 +1732,15 @@ > return 0; > } > >-void iommu_pte_flush(struct domain *d, u64 gfn, u64 *pte, int present) >+void iommu_pte_flush(struct domain *d, u64 gfn, u64 *pte, >+ u32 order, int present) u32? Shouldn't fixed-size types only be used where really necessary? > { > struct acpi_drhd_unit *drhd; > struct iommu *iommu = NULL; > struct hvm_iommu *hd = domain_hvm_iommu(d); > int flush_dev_iotlb; > int iommu_domid; >+ int page_shift = (order == 0) ? PAGE_SHIFT_4K : PAGE_SHIFT_4K + order; Why not simply int page_shift = PAGE_SHIFT_4K + order; > > iommu_flush_cache_entry(pte, sizeof(struct dma_pte)); > >@@ -1750,8 +1755,8 @@ > if ( iommu_domid == -1 ) > continue; > if ( iommu_flush_iotlb_psi(iommu, iommu_domid, >- (paddr_t)gfn << PAGE_SHIFT_4K, 1, >- !present, flush_dev_iotlb) ) >+ (paddr_t)gfn << page_shift, 1, This certainly isn't correct, at least as long as "gfn" is what its name says (and since the only caller simply adds an "order" argument without adjusting the "gfn" one, I'm sure it is). >+ page_shift, !present, flush_dev_iotlb) ) > iommu_flush_write_buffer(iommu); > } > } Jan [-- Attachment #2: share0106.patch --] [-- Type: application/octet-stream, Size: 8031 bytes --] diff -r 39194f457534 xen/arch/x86/mm/hap/p2m-ept.c --- a/xen/arch/x86/mm/hap/p2m-ept.c Wed Jan 05 09:57:15 2011 +0000 +++ b/xen/arch/x86/mm/hap/p2m-ept.c Fri Dec 24 13:15:17 2010 -0800 @@ -413,12 +413,12 @@ out: if ( rv && iommu_enabled && need_iommu(p2m->domain) && need_modify_vtd_table ) { if ( iommu_hap_pt_share ) - iommu_pte_flush(d, gfn, (u64*)ept_entry, vtd_pte_present); + iommu_pte_flush(d, gfn, (u64*)ept_entry, order, vtd_pte_present); else { if ( p2mt == p2m_ram_rw ) { - if ( order == EPT_TABLE_ORDER ) + if ( order > 0 ) { for ( i = 0; i < (1 << order); i++ ) iommu_map_page( @@ -431,7 +431,7 @@ out: } else { - if ( order == EPT_TABLE_ORDER ) + if ( order > 0 ) { for ( i = 0; i < (1 << order); i++ ) iommu_unmap_page(p2m->domain, gfn - offset + i); diff -r 39194f457534 xen/arch/x86/mm/p2m.c --- a/xen/arch/x86/mm/p2m.c Wed Jan 05 09:57:15 2011 +0000 +++ b/xen/arch/x86/mm/p2m.c Fri Dec 24 13:15:17 2010 -0800 @@ -43,6 +43,9 @@ static bool_t __read_mostly opt_hap_1gb = 1; boolean_param("hap_1gb", opt_hap_1gb); +static bool_t __read_mostly opt_hap_2mb = 1; +boolean_param("hap_2mb", opt_hap_2mb); + /* Printouts */ #define P2M_PRINTK(_f, _a...) \ debugtrace_printk("p2m: %s(): " _f, __func__, ##_a) @@ -1772,7 +1775,7 @@ int set_p2m_entry(struct p2m_domain *p2m order = ( (((gfn | mfn_x(mfn) | todo) & ((1ul << 18) - 1)) == 0) && hvm_hap_has_1gb(d) && opt_hap_1gb ) ? 18 : ((((gfn | mfn_x(mfn) | todo) & ((1ul << 9) - 1)) == 0) && - hvm_hap_has_2mb(d)) ? 9 : 0; + hvm_hap_has_2mb(d) && opt_hap_2mb) ? 9 : 0; else order = 0; diff -r 39194f457534 xen/drivers/passthrough/vtd/iommu.c --- a/xen/drivers/passthrough/vtd/iommu.c Wed Jan 05 09:57:15 2011 +0000 +++ b/xen/drivers/passthrough/vtd/iommu.c Fri Dec 24 13:15:17 2010 -0800 @@ -518,24 +518,9 @@ static int inline iommu_flush_iotlb_dsi( return status; } -static int inline get_alignment(u64 base, unsigned int size) -{ - int t = 0; - u64 end; - - end = base + size - 1; - while ( base != end ) - { - t++; - base >>= 1; - end >>= 1; - } - return t; -} - static int inline iommu_flush_iotlb_psi( struct iommu *iommu, u16 did, u64 addr, unsigned int pages, - int flush_non_present_entry, int flush_dev_iotlb) + int order, int flush_non_present_entry, int flush_dev_iotlb) { unsigned int align; struct iommu_flush *flush = iommu_get_flush(iommu); @@ -548,17 +533,12 @@ static int inline iommu_flush_iotlb_psi( if ( !cap_pgsel_inv(iommu->cap) ) return iommu_flush_iotlb_dsi(iommu, did, flush_non_present_entry, flush_dev_iotlb); - /* - * PSI requires page size is 2 ^ x, and the base address is naturally - * aligned to the size - */ - align = get_alignment(addr >> PAGE_SHIFT_4K, pages); /* Fallback to domain selective flush if size is too big */ - if ( align > cap_max_amask_val(iommu->cap) ) + if ( order > cap_max_amask_val(iommu->cap) ) return iommu_flush_iotlb_dsi(iommu, did, flush_non_present_entry, flush_dev_iotlb); - addr >>= PAGE_SHIFT_4K + align; - addr <<= PAGE_SHIFT_4K + align; + addr >>= PAGE_SHIFT_4K + order; + addr <<= PAGE_SHIFT_4K + order; /* apply platform specific errata workarounds */ vtd_ops_preamble_quirk(iommu); @@ -635,7 +615,8 @@ static void dma_pte_clear_one(struct dom if ( iommu_domid == -1 ) continue; if ( iommu_flush_iotlb_psi(iommu, iommu_domid, - addr, 1, 0, flush_dev_iotlb) ) + addr, 1, PAGE_SHIFT_4K, + 0, flush_dev_iotlb) ) iommu_flush_write_buffer(iommu); } } @@ -1711,6 +1692,7 @@ static int intel_iommu_map_page( continue; if ( iommu_flush_iotlb_psi(iommu, iommu_domid, (paddr_t)gfn << PAGE_SHIFT_4K, 1, + PAGE_SHIFT_4K, !dma_pte_present(old), flush_dev_iotlb) ) iommu_flush_write_buffer(iommu); } @@ -1729,7 +1711,8 @@ static int intel_iommu_unmap_page(struct return 0; } -void iommu_pte_flush(struct domain *d, u64 gfn, u64 *pte, int present) +void iommu_pte_flush(struct domain *d, u64 gfn, u64 *pte, + int order, int present) { struct acpi_drhd_unit *drhd; struct iommu *iommu = NULL; @@ -1751,7 +1734,7 @@ void iommu_pte_flush(struct domain *d, u continue; if ( iommu_flush_iotlb_psi(iommu, iommu_domid, (paddr_t)gfn << PAGE_SHIFT_4K, 1, - !present, flush_dev_iotlb) ) + order, !present, flush_dev_iotlb) ) iommu_flush_write_buffer(iommu); } } @@ -1769,6 +1752,28 @@ static int vtd_ept_page_compatible(struc return 1; } +static bool_t vtd_ept_share(void) +{ + struct acpi_drhd_unit *drhd; + struct iommu *iommu; + bool_t share = TRUE; + + /* sharept defaults to 0 for now, default to 1 when feature matures */ + if ( !sharept ) + share = FALSE; + + /* + * Determine whether EPT and VT-d page tables can be shared or not. + */ + for_each_drhd_unit ( drhd ) + { + iommu = drhd->iommu; + if ( !vtd_ept_page_compatible(drhd->iommu) ) + share = FALSE; + } + return share; +} + /* * set VT-d page table directory to EPT table if allowed */ @@ -1779,11 +1784,13 @@ void iommu_set_pgd(struct domain *d) ASSERT( is_hvm_domain(d) && d->arch.hvm_domain.hap_enabled ); - if ( !iommu_hap_pt_share ) - return; - + iommu_hap_pt_share = vtd_ept_share(); pgd_mfn = pagetable_get_mfn(p2m_get_pagetable(p2m_get_hostp2m(d))); hd->pgd_maddr = pagetable_get_paddr(pagetable_from_mfn(pgd_mfn)); + + dprintk(XENLOG_INFO VTDPREFIX, + "VT-d page table %s with EPT table\n", + iommu_hap_pt_share ? "shares" : "not sharing"); } static int domain_rmrr_mapped(struct domain *d, @@ -2036,27 +2043,6 @@ static int init_vtd_hw(void) } } iommu_flush_all(); - - /* - * Determine whether EPT and VT-d page tables can be shared or not. - */ - iommu_hap_pt_share = TRUE; - for_each_drhd_unit ( drhd ) - { - iommu = drhd->iommu; - if ( (drhd->iommu->nr_pt_levels != VTD_PAGE_TABLE_LEVEL_4) || - !vtd_ept_page_compatible(drhd->iommu) ) - iommu_hap_pt_share = FALSE; - } - - /* keep boot flag sharept as safe fallback. remove after feature matures */ - if ( !sharept ) - iommu_hap_pt_share = FALSE; - - dprintk(XENLOG_INFO VTDPREFIX, - "VT-d page table %sshared with EPT table\n", - iommu_hap_pt_share ? "" : "not "); - return 0; } diff -r 39194f457534 xen/include/xen/iommu.h --- a/xen/include/xen/iommu.h Wed Jan 05 09:57:15 2011 +0000 +++ b/xen/include/xen/iommu.h Fri Dec 24 13:15:17 2010 -0800 @@ -85,7 +85,7 @@ int iommu_get_device_group(struct domain int iommu_map_page(struct domain *d, unsigned long gfn, unsigned long mfn, unsigned int flags); int iommu_unmap_page(struct domain *d, unsigned long gfn); -void iommu_pte_flush(struct domain *d, u64 gfn, u64 *pte, int present); +void iommu_pte_flush(struct domain *d, u64 gfn, u64 *pte, int order, int present); void iommu_set_pgd(struct domain *d); void iommu_domain_teardown(struct domain *d); int hvm_do_IRQ_dpci(struct domain *d, unsigned int irq); [-- Attachment #3: Type: text/plain, Size: 138 bytes --] _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel ^ permalink raw reply [flat|nested] 9+ messages in thread
* RE: [PATCH][VTD] bug fix for EPT/VT-d table sharing 2011-01-07 2:49 ` Kay, Allen M @ 2011-01-07 8:25 ` Jan Beulich 2011-01-07 18:55 ` Kay, Allen M 0 siblings, 1 reply; 9+ messages in thread From: Jan Beulich @ 2011-01-07 8:25 UTC (permalink / raw) To: Allen M Kay; +Cc: xen-devel@lists.xensource.com, Keir Fraser, Tim Deegan >>> On 07.01.11 at 03:49, "Kay, Allen M" <allen.m.kay@intel.com> wrote: With this >@@ -548,17 +533,12 @@ static int inline iommu_flush_iotlb_psi( > if ( !cap_pgsel_inv(iommu->cap) ) > return iommu_flush_iotlb_dsi(iommu, did, flush_non_present_entry, flush_dev_iotlb); > >- /* >- * PSI requires page size is 2 ^ x, and the base address is naturally >- * aligned to the size >- */ >- align = get_alignment(addr >> PAGE_SHIFT_4K, pages); > /* Fallback to domain selective flush if size is too big */ >- if ( align > cap_max_amask_val(iommu->cap) ) >+ if ( order > cap_max_amask_val(iommu->cap) ) > return iommu_flush_iotlb_dsi(iommu, did, flush_non_present_entry, flush_dev_iotlb); > >- addr >>= PAGE_SHIFT_4K + align; >- addr <<= PAGE_SHIFT_4K + align; >+ addr >>= PAGE_SHIFT_4K + order; >+ addr <<= PAGE_SHIFT_4K + order; > > /* apply platform specific errata workarounds */ > vtd_ops_preamble_quirk(iommu); I suppose that here >@@ -635,7 +615,8 @@ static void dma_pte_clear_one(struct dom > if ( iommu_domid == -1 ) > continue; > if ( iommu_flush_iotlb_psi(iommu, iommu_domid, >- addr, 1, 0, flush_dev_iotlb) ) >+ addr, 1, PAGE_SHIFT_4K, >+ 0, flush_dev_iotlb) ) > iommu_flush_write_buffer(iommu); > } > } >@@ -1711,6 +1692,7 @@ static int intel_iommu_map_page( > continue; > if ( iommu_flush_iotlb_psi(iommu, iommu_domid, > (paddr_t)gfn << PAGE_SHIFT_4K, 1, >+ PAGE_SHIFT_4K, > !dma_pte_present(old), flush_dev_iotlb) ) > iommu_flush_write_buffer(iommu); > } you need to pass 0 instead of PAGE_SHIFT_4K. Jan ^ permalink raw reply [flat|nested] 9+ messages in thread
* RE: [PATCH][VTD] bug fix for EPT/VT-d table sharing 2011-01-07 8:25 ` Jan Beulich @ 2011-01-07 18:55 ` Kay, Allen M 2011-01-10 8:25 ` Jan Beulich ` (2 more replies) 0 siblings, 3 replies; 9+ messages in thread From: Kay, Allen M @ 2011-01-07 18:55 UTC (permalink / raw) To: Jan Beulich; +Cc: xen-devel@lists.xensource.com, Keir Fraser, Tim Deegan [-- Attachment #1: Type: text/plain, Size: 2441 bytes --] Thanks for catching this. After I change the iommu_flush_iotlb_psi() parameter from page_shift to order, I forgot revisit the callers for non shared cases. I have fixed them in attached patch. Allen -----Original Message----- From: Jan Beulich [mailto:JBeulich@novell.com] Sent: Friday, January 07, 2011 12:25 AM To: Kay, Allen M Cc: Tim Deegan; xen-devel@lists.xensource.com; Keir Fraser Subject: RE: [Xen-devel] [PATCH][VTD] bug fix for EPT/VT-d table sharing >>> On 07.01.11 at 03:49, "Kay, Allen M" <allen.m.kay@intel.com> wrote: With this >@@ -548,17 +533,12 @@ static int inline iommu_flush_iotlb_psi( > if ( !cap_pgsel_inv(iommu->cap) ) > return iommu_flush_iotlb_dsi(iommu, did, flush_non_present_entry, flush_dev_iotlb); > >- /* >- * PSI requires page size is 2 ^ x, and the base address is naturally >- * aligned to the size >- */ >- align = get_alignment(addr >> PAGE_SHIFT_4K, pages); > /* Fallback to domain selective flush if size is too big */ >- if ( align > cap_max_amask_val(iommu->cap) ) >+ if ( order > cap_max_amask_val(iommu->cap) ) > return iommu_flush_iotlb_dsi(iommu, did, flush_non_present_entry, flush_dev_iotlb); > >- addr >>= PAGE_SHIFT_4K + align; >- addr <<= PAGE_SHIFT_4K + align; >+ addr >>= PAGE_SHIFT_4K + order; >+ addr <<= PAGE_SHIFT_4K + order; > > /* apply platform specific errata workarounds */ > vtd_ops_preamble_quirk(iommu); I suppose that here >@@ -635,7 +615,8 @@ static void dma_pte_clear_one(struct dom > if ( iommu_domid == -1 ) > continue; > if ( iommu_flush_iotlb_psi(iommu, iommu_domid, >- addr, 1, 0, flush_dev_iotlb) ) >+ addr, 1, PAGE_SHIFT_4K, >+ 0, flush_dev_iotlb) ) > iommu_flush_write_buffer(iommu); > } > } >@@ -1711,6 +1692,7 @@ static int intel_iommu_map_page( > continue; > if ( iommu_flush_iotlb_psi(iommu, iommu_domid, > (paddr_t)gfn << PAGE_SHIFT_4K, 1, >+ PAGE_SHIFT_4K, > !dma_pte_present(old), flush_dev_iotlb) ) > iommu_flush_write_buffer(iommu); > } you need to pass 0 instead of PAGE_SHIFT_4K. Jan [-- Attachment #2: share0107.patch --] [-- Type: application/octet-stream, Size: 8156 bytes --] diff -r 39194f457534 xen/arch/x86/mm/hap/p2m-ept.c --- a/xen/arch/x86/mm/hap/p2m-ept.c Wed Jan 05 09:57:15 2011 +0000 +++ b/xen/arch/x86/mm/hap/p2m-ept.c Sat Dec 25 05:25:45 2010 -0800 @@ -413,12 +413,12 @@ out: if ( rv && iommu_enabled && need_iommu(p2m->domain) && need_modify_vtd_table ) { if ( iommu_hap_pt_share ) - iommu_pte_flush(d, gfn, (u64*)ept_entry, vtd_pte_present); + iommu_pte_flush(d, gfn, (u64*)ept_entry, order, vtd_pte_present); else { if ( p2mt == p2m_ram_rw ) { - if ( order == EPT_TABLE_ORDER ) + if ( order > 0 ) { for ( i = 0; i < (1 << order); i++ ) iommu_map_page( @@ -431,7 +431,7 @@ out: } else { - if ( order == EPT_TABLE_ORDER ) + if ( order > 0 ) { for ( i = 0; i < (1 << order); i++ ) iommu_unmap_page(p2m->domain, gfn - offset + i); diff -r 39194f457534 xen/arch/x86/mm/p2m.c --- a/xen/arch/x86/mm/p2m.c Wed Jan 05 09:57:15 2011 +0000 +++ b/xen/arch/x86/mm/p2m.c Sat Dec 25 05:25:45 2010 -0800 @@ -43,6 +43,9 @@ static bool_t __read_mostly opt_hap_1gb = 1; boolean_param("hap_1gb", opt_hap_1gb); +static bool_t __read_mostly opt_hap_2mb = 1; +boolean_param("hap_2mb", opt_hap_2mb); + /* Printouts */ #define P2M_PRINTK(_f, _a...) \ debugtrace_printk("p2m: %s(): " _f, __func__, ##_a) @@ -1772,7 +1775,7 @@ int set_p2m_entry(struct p2m_domain *p2m order = ( (((gfn | mfn_x(mfn) | todo) & ((1ul << 18) - 1)) == 0) && hvm_hap_has_1gb(d) && opt_hap_1gb ) ? 18 : ((((gfn | mfn_x(mfn) | todo) & ((1ul << 9) - 1)) == 0) && - hvm_hap_has_2mb(d)) ? 9 : 0; + hvm_hap_has_2mb(d) && opt_hap_2mb) ? 9 : 0; else order = 0; diff -r 39194f457534 xen/drivers/passthrough/vtd/iommu.c --- a/xen/drivers/passthrough/vtd/iommu.c Wed Jan 05 09:57:15 2011 +0000 +++ b/xen/drivers/passthrough/vtd/iommu.c Sat Dec 25 05:25:45 2010 -0800 @@ -518,24 +518,9 @@ static int inline iommu_flush_iotlb_dsi( return status; } -static int inline get_alignment(u64 base, unsigned int size) -{ - int t = 0; - u64 end; - - end = base + size - 1; - while ( base != end ) - { - t++; - base >>= 1; - end >>= 1; - } - return t; -} - static int inline iommu_flush_iotlb_psi( struct iommu *iommu, u16 did, u64 addr, unsigned int pages, - int flush_non_present_entry, int flush_dev_iotlb) + int order, int flush_non_present_entry, int flush_dev_iotlb) { unsigned int align; struct iommu_flush *flush = iommu_get_flush(iommu); @@ -548,17 +533,12 @@ static int inline iommu_flush_iotlb_psi( if ( !cap_pgsel_inv(iommu->cap) ) return iommu_flush_iotlb_dsi(iommu, did, flush_non_present_entry, flush_dev_iotlb); - /* - * PSI requires page size is 2 ^ x, and the base address is naturally - * aligned to the size - */ - align = get_alignment(addr >> PAGE_SHIFT_4K, pages); /* Fallback to domain selective flush if size is too big */ - if ( align > cap_max_amask_val(iommu->cap) ) + if ( order > cap_max_amask_val(iommu->cap) ) return iommu_flush_iotlb_dsi(iommu, did, flush_non_present_entry, flush_dev_iotlb); - addr >>= PAGE_SHIFT_4K + align; - addr <<= PAGE_SHIFT_4K + align; + addr >>= PAGE_SHIFT_4K + order; + addr <<= PAGE_SHIFT_4K + order; /* apply platform specific errata workarounds */ vtd_ops_preamble_quirk(iommu); @@ -634,8 +614,8 @@ static void dma_pte_clear_one(struct dom iommu_domid= domain_iommu_domid(domain, iommu); if ( iommu_domid == -1 ) continue; - if ( iommu_flush_iotlb_psi(iommu, iommu_domid, - addr, 1, 0, flush_dev_iotlb) ) + if ( iommu_flush_iotlb_psi(iommu, iommu_domid, addr, + 1, 0, 0, flush_dev_iotlb) ) iommu_flush_write_buffer(iommu); } } @@ -1710,7 +1690,7 @@ static int intel_iommu_map_page( if ( iommu_domid == -1 ) continue; if ( iommu_flush_iotlb_psi(iommu, iommu_domid, - (paddr_t)gfn << PAGE_SHIFT_4K, 1, + (paddr_t)gfn << PAGE_SHIFT_4K, 1, 0, !dma_pte_present(old), flush_dev_iotlb) ) iommu_flush_write_buffer(iommu); } @@ -1729,7 +1709,8 @@ static int intel_iommu_unmap_page(struct return 0; } -void iommu_pte_flush(struct domain *d, u64 gfn, u64 *pte, int present) +void iommu_pte_flush(struct domain *d, u64 gfn, u64 *pte, + int order, int present) { struct acpi_drhd_unit *drhd; struct iommu *iommu = NULL; @@ -1751,7 +1732,7 @@ void iommu_pte_flush(struct domain *d, u continue; if ( iommu_flush_iotlb_psi(iommu, iommu_domid, (paddr_t)gfn << PAGE_SHIFT_4K, 1, - !present, flush_dev_iotlb) ) + order, !present, flush_dev_iotlb) ) iommu_flush_write_buffer(iommu); } } @@ -1769,6 +1750,28 @@ static int vtd_ept_page_compatible(struc return 1; } +static bool_t vtd_ept_share(void) +{ + struct acpi_drhd_unit *drhd; + struct iommu *iommu; + bool_t share = TRUE; + + /* sharept defaults to 0 for now, default to 1 when feature matures */ + if ( !sharept ) + share = FALSE; + + /* + * Determine whether EPT and VT-d page tables can be shared or not. + */ + for_each_drhd_unit ( drhd ) + { + iommu = drhd->iommu; + if ( !vtd_ept_page_compatible(drhd->iommu) ) + share = FALSE; + } + return share; +} + /* * set VT-d page table directory to EPT table if allowed */ @@ -1779,11 +1782,13 @@ void iommu_set_pgd(struct domain *d) ASSERT( is_hvm_domain(d) && d->arch.hvm_domain.hap_enabled ); - if ( !iommu_hap_pt_share ) - return; - + iommu_hap_pt_share = vtd_ept_share(); pgd_mfn = pagetable_get_mfn(p2m_get_pagetable(p2m_get_hostp2m(d))); hd->pgd_maddr = pagetable_get_paddr(pagetable_from_mfn(pgd_mfn)); + + dprintk(XENLOG_INFO VTDPREFIX, + "VT-d page table %s with EPT table\n", + iommu_hap_pt_share ? "shares" : "not sharing"); } static int domain_rmrr_mapped(struct domain *d, @@ -2036,27 +2041,6 @@ static int init_vtd_hw(void) } } iommu_flush_all(); - - /* - * Determine whether EPT and VT-d page tables can be shared or not. - */ - iommu_hap_pt_share = TRUE; - for_each_drhd_unit ( drhd ) - { - iommu = drhd->iommu; - if ( (drhd->iommu->nr_pt_levels != VTD_PAGE_TABLE_LEVEL_4) || - !vtd_ept_page_compatible(drhd->iommu) ) - iommu_hap_pt_share = FALSE; - } - - /* keep boot flag sharept as safe fallback. remove after feature matures */ - if ( !sharept ) - iommu_hap_pt_share = FALSE; - - dprintk(XENLOG_INFO VTDPREFIX, - "VT-d page table %sshared with EPT table\n", - iommu_hap_pt_share ? "" : "not "); - return 0; } diff -r 39194f457534 xen/include/xen/iommu.h --- a/xen/include/xen/iommu.h Wed Jan 05 09:57:15 2011 +0000 +++ b/xen/include/xen/iommu.h Sat Dec 25 05:25:45 2010 -0800 @@ -85,7 +85,7 @@ int iommu_get_device_group(struct domain int iommu_map_page(struct domain *d, unsigned long gfn, unsigned long mfn, unsigned int flags); int iommu_unmap_page(struct domain *d, unsigned long gfn); -void iommu_pte_flush(struct domain *d, u64 gfn, u64 *pte, int present); +void iommu_pte_flush(struct domain *d, u64 gfn, u64 *pte, int order, int present); void iommu_set_pgd(struct domain *d); void iommu_domain_teardown(struct domain *d); int hvm_do_IRQ_dpci(struct domain *d, unsigned int irq); [-- Attachment #3: Type: text/plain, Size: 138 bytes --] _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel ^ permalink raw reply [flat|nested] 9+ messages in thread
* RE: [PATCH][VTD] bug fix for EPT/VT-d table sharing 2011-01-07 18:55 ` Kay, Allen M @ 2011-01-10 8:25 ` Jan Beulich 2011-01-10 8:38 ` Keir Fraser 2011-01-10 9:52 ` Olaf Hering 2 siblings, 0 replies; 9+ messages in thread From: Jan Beulich @ 2011-01-10 8:25 UTC (permalink / raw) To: Allen M Kay; +Cc: xen-devel@lists.xensource.com, Keir Fraser, Tim Deegan >>> On 07.01.11 at 19:55, "Kay, Allen M" <allen.m.kay@intel.com> wrote: > Thanks for catching this. After I change the iommu_flush_iotlb_psi() > parameter from page_shift to order, I forgot revisit the callers for non > shared cases. > > I have fixed them in attached patch. Looks good to me now. Thanks Allen! Jan ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH][VTD] bug fix for EPT/VT-d table sharing 2011-01-07 18:55 ` Kay, Allen M 2011-01-10 8:25 ` Jan Beulich @ 2011-01-10 8:38 ` Keir Fraser 2011-01-10 9:52 ` Olaf Hering 2 siblings, 0 replies; 9+ messages in thread From: Keir Fraser @ 2011-01-10 8:38 UTC (permalink / raw) To: Kay, Allen M, Jan Beulich; +Cc: xen-devel@lists.xensource.com, Tim Deegan I'll apply this when Jan Acks it. -- Keir On 07/01/2011 18:55, "Kay, Allen M" <allen.m.kay@intel.com> wrote: > Thanks for catching this. After I change the iommu_flush_iotlb_psi() > parameter from page_shift to order, I forgot revisit the callers for non > shared cases. > > I have fixed them in attached patch. > > Allen > > -----Original Message----- > From: Jan Beulich [mailto:JBeulich@novell.com] > Sent: Friday, January 07, 2011 12:25 AM > To: Kay, Allen M > Cc: Tim Deegan; xen-devel@lists.xensource.com; Keir Fraser > Subject: RE: [Xen-devel] [PATCH][VTD] bug fix for EPT/VT-d table sharing > >>>> On 07.01.11 at 03:49, "Kay, Allen M" <allen.m.kay@intel.com> wrote: > > With this > >> @@ -548,17 +533,12 @@ static int inline iommu_flush_iotlb_psi( >> if ( !cap_pgsel_inv(iommu->cap) ) >> return iommu_flush_iotlb_dsi(iommu, did, flush_non_present_entry, >> flush_dev_iotlb); >> >> - /* >> - * PSI requires page size is 2 ^ x, and the base address is naturally >> - * aligned to the size >> - */ >> - align = get_alignment(addr >> PAGE_SHIFT_4K, pages); >> /* Fallback to domain selective flush if size is too big */ >> - if ( align > cap_max_amask_val(iommu->cap) ) >> + if ( order > cap_max_amask_val(iommu->cap) ) >> return iommu_flush_iotlb_dsi(iommu, did, flush_non_present_entry, >> flush_dev_iotlb); >> >> - addr >>= PAGE_SHIFT_4K + align; >> - addr <<= PAGE_SHIFT_4K + align; >> + addr >>= PAGE_SHIFT_4K + order; >> + addr <<= PAGE_SHIFT_4K + order; >> >> /* apply platform specific errata workarounds */ >> vtd_ops_preamble_quirk(iommu); > > I suppose that here > >> @@ -635,7 +615,8 @@ static void dma_pte_clear_one(struct dom >> if ( iommu_domid == -1 ) >> continue; >> if ( iommu_flush_iotlb_psi(iommu, iommu_domid, >> - addr, 1, 0, flush_dev_iotlb) ) >> + addr, 1, PAGE_SHIFT_4K, >> + 0, flush_dev_iotlb) ) >> iommu_flush_write_buffer(iommu); >> } >> } >> @@ -1711,6 +1692,7 @@ static int intel_iommu_map_page( >> continue; >> if ( iommu_flush_iotlb_psi(iommu, iommu_domid, >> (paddr_t)gfn << PAGE_SHIFT_4K, 1, >> + PAGE_SHIFT_4K, >> !dma_pte_present(old), flush_dev_iotlb) ) >> iommu_flush_write_buffer(iommu); >> } > > you need to pass 0 instead of PAGE_SHIFT_4K. > > Jan > > _______________________________________________ > Xen-devel mailing list > Xen-devel@lists.xensource.com > http://lists.xensource.com/xen-devel ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH][VTD] bug fix for EPT/VT-d table sharing 2011-01-07 18:55 ` Kay, Allen M 2011-01-10 8:25 ` Jan Beulich 2011-01-10 8:38 ` Keir Fraser @ 2011-01-10 9:52 ` Olaf Hering 2011-01-10 10:32 ` Keir Fraser 2 siblings, 1 reply; 9+ messages in thread From: Olaf Hering @ 2011-01-10 9:52 UTC (permalink / raw) To: Kay, Allen M Cc: xen-devel@lists.xensource.com, Keir Fraser, Tim Deegan, Jan Beulich On Fri, Jan 07, Kay, Allen M wrote: > >@@ -548,17 +533,12 @@ static int inline iommu_flush_iotlb_psi( > > if ( !cap_pgsel_inv(iommu->cap) ) > > return iommu_flush_iotlb_dsi(iommu, did, flush_non_present_entry, flush_dev_iotlb); > > > >- /* > >- * PSI requires page size is 2 ^ x, and the base address is naturally > >- * aligned to the size > >- */ > >- align = get_alignment(addr >> PAGE_SHIFT_4K, pages); > > /* Fallback to domain selective flush if size is too big */ > >- if ( align > cap_max_amask_val(iommu->cap) ) > >+ if ( order > cap_max_amask_val(iommu->cap) ) This leaves 'align' uninitialized when flush->iotlb() is called. iommu.c:546: error: 'align' may be used uninitialized in this function Olaf ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH][VTD] bug fix for EPT/VT-d table sharing 2011-01-10 9:52 ` Olaf Hering @ 2011-01-10 10:32 ` Keir Fraser 0 siblings, 0 replies; 9+ messages in thread From: Keir Fraser @ 2011-01-10 10:32 UTC (permalink / raw) To: Olaf Hering, Kay, Allen M Cc: xen-devel@lists.xensource.com, Tim Deegan, Jan Beulich On 10/01/2011 09:52, "Olaf Hering" <olaf@aepfle.de> wrote: > On Fri, Jan 07, Kay, Allen M wrote: > >>> @@ -548,17 +533,12 @@ static int inline iommu_flush_iotlb_psi( >>> if ( !cap_pgsel_inv(iommu->cap) ) >>> return iommu_flush_iotlb_dsi(iommu, did, flush_non_present_entry, >>> flush_dev_iotlb); >>> >>> - /* >>> - * PSI requires page size is 2 ^ x, and the base address is naturally >>> - * aligned to the size >>> - */ >>> - align = get_alignment(addr >> PAGE_SHIFT_4K, pages); >>> /* Fallback to domain selective flush if size is too big */ >>> - if ( align > cap_max_amask_val(iommu->cap) ) >>> + if ( order > cap_max_amask_val(iommu->cap) ) > > This leaves 'align' uninitialized when flush->iotlb() is called. > > iommu.c:546: error: 'align' may be used uninitialized in this function I fixed this as of c/s 22696. -- Keir > > Olaf > ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2011-01-10 10:32 UTC | newest] Thread overview: 9+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2011-01-05 19:36 [PATCH][VTD] bug fix for EPT/VT-d table sharing Kay, Allen M 2011-01-06 8:43 ` Jan Beulich 2011-01-07 2:49 ` Kay, Allen M 2011-01-07 8:25 ` Jan Beulich 2011-01-07 18:55 ` Kay, Allen M 2011-01-10 8:25 ` Jan Beulich 2011-01-10 8:38 ` Keir Fraser 2011-01-10 9:52 ` Olaf Hering 2011-01-10 10:32 ` Keir Fraser
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.