thr3ads.net - Xen devel - [Xen-devel] [PATCH][VTD] bug fix for EPT/VT-d table sharing [Jan 2011]

If this information is useful, please help other people find it:
Share via:

Kay, Allen M

2011-Jan-05 19:36 UTC

[Xen-devel] [PATCH][VTD] bug fix for EPT/VT-d table sharing

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>


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Jan Beulich

2011-Jan-06 08:43 UTC

head link

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


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Kay, Allen M

2011-Jan-07 02:49 UTC

head link

RE: [Xen-devel] [PATCH][VTD] bug fix for EPT/VT-d table sharing

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



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Jan Beulich

2011-Jan-07 08:25 UTC

head link

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

Kay, Allen M

2011-Jan-07 18:55 UTC

head link

RE: [Xen-devel] [PATCH][VTD] bug fix for EPT/VT-d table sharing

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

Jan Beulich

2011-Jan-10 08:25 UTC

head link

RE: [Xen-devel] [PATCH][VTD] bug fix for EPT/VT-d table sharing

>>> 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


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Keir Fraser

2011-Jan-10 08:38 UTC

head link

Re: [Xen-devel] [PATCH][VTD] bug fix for EPT/VT-d table sharing

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


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Olaf Hering

2011-Jan-10 09:52 UTC

head link

Re: [Xen-devel] [PATCH][VTD] bug fix for EPT/VT-d table sharing

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


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Keir Fraser

2011-Jan-10 10:32 UTC

head link

Re: [Xen-devel] [PATCH][VTD] bug fix for EPT/VT-d table sharing

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
> 


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Xen devel - Jan 2011 - [PATCH][VTD] bug fix for EPT/VT-d table sharing

[Xen-devel] [PATCH][VTD] bug fix for EPT/VT-d table sharing

Re: [Xen-devel] [PATCH][VTD] bug fix for EPT/VT-d table sharing

RE: [Xen-devel] [PATCH][VTD] bug fix for EPT/VT-d table sharing

RE: [Xen-devel] [PATCH][VTD] bug fix for EPT/VT-d table sharing

RE: [Xen-devel] [PATCH][VTD] bug fix for EPT/VT-d table sharing

RE: [Xen-devel] [PATCH][VTD] bug fix for EPT/VT-d table sharing

Re: [Xen-devel] [PATCH][VTD] bug fix for EPT/VT-d table sharing

Re: [Xen-devel] [PATCH][VTD] bug fix for EPT/VT-d table sharing

Re: [Xen-devel] [PATCH][VTD] bug fix for EPT/VT-d table sharing