Re: Re: changeset 22526:7a5ee3800417

[George Dunlap <George.Dunlap@eu.citrix.com>]

[-- Attachment #1: Type: text/plain, Size: 2813 bytes --]

Oops, forgot to attach the patch...

 -George

On Thu, Mar 10, 2011 at 9:41 AM, George Dunlap
<George.Dunlap@eu.citrix.com> wrote:
> Peng fei,
>
> Can you (1) test this patch, and (2) make sure I've romanized your
> name properly?
>
> Thanks,
>  -George
>
> On Wed, Mar 9, 2011 at 1:59 PM, George Dunlap
> <George.Dunlap@eu.citrix.com> wrote:
>> Peng fei,
>>
>> I just noticed this patch has a bug -- it doesn't set new_entry.mfn if
>> the new mfn is the same as the one already in ept_entry!  Standby for
>> a new one...
>>
>>  -George
>>
>> On Mon, Mar 7, 2011 at 4:41 PM, George Dunlap
>> <George.Dunlap@eu.citrix.com> wrote:
>>> Peng fei,
>>>
>>> Can you test the attached patch (to xen-unstable)?  I don't have
>>> EPT-enabled hardware handy...
>>>
>>>  -George
>>>
>>> On Mon, Mar 7, 2011 at 4:20 PM, George Dunlap <george.dunlap@citrix.com> wrote:
>>>> Hmm, yeah, that's obviously not right!  Let me take a look...
>>>>
>>>>  -George
>>>>
>>>> On Mon, 2011-03-07 at 09:55 +0000, 张鹏飞 wrote:
>>>>> Hi,
>>>>>     Recently, I did a research on the p2m and EPT,and did apply your
>>>>> patch(22526) to the source code of mine. But there is one place
>>>>> confused me:
>>>>>
>>>>>
>>>>>
>>>>> ept_entry = table + index;
>>>>> 1.91
>>>>>     1.92 -        ept_entry->emt = epte_get_entry_emt(d, gfn, mfn, &ipat, direct_mmio);
>>>>>     1.93 -        ept_entry->ipat = ipat;
>>>>>     1.94 -        ept_entry->sp = i ? 1 : 0;
>>>>>     1.95 -        ept_entry->avail1 = p2mt;
>>>>>     1.96 -        ept_entry->avail2 = 0;
>>>>>     1.97 +        new_entry.emt = epte_get_entry_emt(d, gfn, mfn, &ipat, direct_mmio);
>>>>>     1.98 +        new_entry.ipat = ipat;
>>>>>     1.99 +        new_entry.sp = i ? 1 : 0;
>>>>>    1.100 +        new_entry.avail1 = p2mt;
>>>>>    1.101 +        new_entry.avail2 = 0;
>>>>>    1.102
>>>>>    1.103 -        if ( ept_entry->mfn == mfn_x(mfn) )
>>>>>    1.104 +        if ( new_entry.mfn == mfn_x(mfn) )
>>>>>    1.105               need_modify_vtd_table = 0;
>>>>>    1.106          else /* the caller should take care of the previous page */
>>>>>    1.107 -            ept_entry->mfn = mfn_x(mfn);
>>>>>    1.108 +            new_entry.mfn = mfn_x(mfn);
>>>>>
>>>>> I think, The new_entry.mfn is used without assigned any value. Maybe there was something else I did not think of.
>>>>> Thank you for your kindness!
>>>>>
>>>>> http://xenbits.xen.org/xen-unstable.hg/rev/7a5ee3800417
>>>>
>>>>
>>>>
>>>> _______________________________________________
>>>> Xen-devel mailing list
>>>> Xen-devel@lists.xensource.com
>>>> http://lists.xensource.com/xen-devel
>>>>
>>>
>>
>

[-- Attachment #2: ept-use-existing-entry.diff --]
[-- Type: text/plain, Size: 2289 bytes --]

ept: Fix bug in changeset 22526:7a5ee3800417

This fixes a bug in changeset 22526:7a5ee3800417, where the wrong
value is read when deciding whether to flush the VTd tables.  The
effect is minor: in situations where the p2m entry is changed but the
mfn is the same, the VTd tables will be unnecessarily flushed.

old_entry is left untouched in the second case, since having a present
old_entry will cause the an unnecessary check to be taken at the end
of the function.

Spotted-by: Zhang, Peng Fei <zpfalpc23@gmail.com>
Signed-off-by: George Dunlap <george.dunlap@eu.citrix.com>

diff -r f071d8e9f744 xen/arch/x86/mm/hap/p2m-ept.c
--- a/xen/arch/x86/mm/hap/p2m-ept.c	Tue Mar 08 10:23:52 2011 +0000
+++ b/xen/arch/x86/mm/hap/p2m-ept.c	Wed Mar 09 16:58:51 2011 +0000
@@ -374,7 +374,9 @@
             needs_sync = 0;
 
         /* If we're replacing a non-leaf entry with a leaf entry (1GiB or 2MiB),
-         * the intermediate tables will be freed below after the ept flush */
+         * the intermediate tables will be freed below after the ept flush
+         *
+         * Read-then-write is OK because we hold the p2m lock. */
         old_entry = *ept_entry;
 
         if ( mfn_valid(mfn_x(mfn)) || direct_mmio || p2m_is_paged(p2mt) ||
@@ -390,10 +392,10 @@
             new_entry.access = p2ma;
             new_entry.rsvd2_snp = (iommu_enabled && iommu_snoop);
 
-            if ( new_entry.mfn == mfn_x(mfn) )
+            new_entry.mfn = mfn_x(mfn);
+
+            if ( old_entry.mfn == new_entry.mfn )
                 need_modify_vtd_table = 0;
-            else
-                new_entry.mfn = mfn_x(mfn);
 
             ept_p2m_type_to_flags(&new_entry, p2mt, p2ma);
         }
@@ -438,10 +440,12 @@
         new_entry.access = p2ma;
         new_entry.rsvd2_snp = (iommu_enabled && iommu_snoop);
 
-        if ( new_entry.mfn == mfn_x(mfn) )
+        /* the caller should take care of the previous page */
+        new_entry.mfn = mfn_x(mfn);
+
+        /* Safe to read-then-write because we hold the p2m lock */
+        if ( ept_entry->mfn == new_entry.mfn )
              need_modify_vtd_table = 0;
-        else /* the caller should take care of the previous page */
-            new_entry.mfn = mfn_x(mfn);
 
         ept_p2m_type_to_flags(&new_entry, p2mt, p2ma);
 

[-- Attachment #3: Type: text/plain, Size: 138 bytes --]

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