Re: changeset 22526:7a5ee3800417

Re: changeset 22526:7a5ee3800417

[George Dunlap]

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

Re: Re: changeset 22526:7a5ee3800417

[George Dunlap]

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

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/x-diff, Size: 891 bytes --]

diff -r 809ea782d2e6 -r 7ddd021a2079 xen/arch/x86/mm/hap/p2m-ept.c
--- a/xen/arch/x86/mm/hap/p2m-ept.c	Mon Mar 07 16:33:45 2011 +0000
+++ b/xen/arch/x86/mm/hap/p2m-ept.c	Mon Mar 07 16:41:02 2011 +0000
@@ -390,7 +390,7 @@
             new_entry.access = p2ma;
             new_entry.rsvd2_snp = (iommu_enabled && iommu_snoop);
 
-            if ( new_entry.mfn == mfn_x(mfn) )
+            if ( ept_entry->mfn == mfn_x(mfn) )
                 need_modify_vtd_table = 0;
             else
                 new_entry.mfn = mfn_x(mfn);
@@ -438,7 +438,7 @@
         new_entry.access = p2ma;
         new_entry.rsvd2_snp = (iommu_enabled && iommu_snoop);
 
-        if ( new_entry.mfn == mfn_x(mfn) )
+        if ( ept_entry->mfn == mfn_x(mfn) )
              need_modify_vtd_table = 0;
         else /* the caller should take care of the previous page */
             new_entry.mfn = mfn_x(mfn);

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

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

Re: Re: changeset 22526:7a5ee3800417

[Tim Deegan]

Hi, 

At 16:41 +0000 on 07 Mar (1299516062), George Dunlap wrote:
> diff -r 809ea782d2e6 -r 7ddd021a2079 xen/arch/x86/mm/hap/p2m-ept.c
> --- a/xen/arch/x86/mm/hap/p2m-ept.c	Mon Mar 07 16:33:45 2011 +0000
> +++ b/xen/arch/x86/mm/hap/p2m-ept.c	Mon Mar 07 16:41:02 2011 +0000
> @@ -390,7 +390,7 @@
>              new_entry.access = p2ma;
>              new_entry.rsvd2_snp = (iommu_enabled && iommu_snoop);
>  
> -            if ( new_entry.mfn == mfn_x(mfn) )
> +            if ( ept_entry->mfn == mfn_x(mfn) )

Better to use old_entry.mfn, in the spirit of the original cset
("access-once semantics")?  In fact, I suspect that to be safe, you need
to do an atomic RMW instead of just an atomic set, and then decide
whether the VT-d tables will need to be synced.

Cheers,

Tim. 

>                  need_modify_vtd_table = 0;
>              else
>                  new_entry.mfn = mfn_x(mfn);
> @@ -438,7 +438,7 @@
>          new_entry.access = p2ma;
>          new_entry.rsvd2_snp = (iommu_enabled && iommu_snoop);
>  
> -        if ( new_entry.mfn == mfn_x(mfn) )
> +        if ( ept_entry->mfn == mfn_x(mfn) )
>               need_modify_vtd_table = 0;
>          else /* the caller should take care of the previous page */
>              new_entry.mfn = mfn_x(mfn);

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


-- 
Tim Deegan <Tim.Deegan@citrix.com>
Principal Software Engineer, Xen Platform Team
Citrix Systems UK Ltd.  (Company #02937203, SL9 0BG)

Re: Re: changeset 22526:7a5ee3800417

[George Dunlap]

On Mon, 2011-03-07 at 16:48 +0000, Tim Deegan wrote:
> Hi, 
> 
> At 16:41 +0000 on 07 Mar (1299516062), George Dunlap wrote:
> > diff -r 809ea782d2e6 -r 7ddd021a2079 xen/arch/x86/mm/hap/p2m-ept.c
> > --- a/xen/arch/x86/mm/hap/p2m-ept.c	Mon Mar 07 16:33:45 2011 +0000
> > +++ b/xen/arch/x86/mm/hap/p2m-ept.c	Mon Mar 07 16:41:02 2011 +0000
> > @@ -390,7 +390,7 @@
> >              new_entry.access = p2ma;
> >              new_entry.rsvd2_snp = (iommu_enabled && iommu_snoop);
> >  
> > -            if ( new_entry.mfn == mfn_x(mfn) )
> > +            if ( ept_entry->mfn == mfn_x(mfn) )
> 
> Better to use old_entry.mfn, in the spirit of the original cset
> ("access-once semantics")?  

I started to do that, but the one below didn't have an old_entry
already.  

> In fact, I suspect that to be safe, you need
> to do an atomic RMW instead of just an atomic set, and then decide
> whether the VT-d tables will need to be synced.

Are we not holding the p2m lock when writing entries?

Reading should be safe here if we are holding the p2m lock.  When
holding the lock, we only need write-once semantics (to avoid racing the
read-once semantics of someone reading without the lock).

 -George

> 
> Cheers,
> 
> Tim. 
> 
> >                  need_modify_vtd_table = 0;
> >              else
> >                  new_entry.mfn = mfn_x(mfn);
> > @@ -438,7 +438,7 @@
> >          new_entry.access = p2ma;
> >          new_entry.rsvd2_snp = (iommu_enabled && iommu_snoop);
> >  
> > -        if ( new_entry.mfn == mfn_x(mfn) )
> > +        if ( ept_entry->mfn == mfn_x(mfn) )
> >               need_modify_vtd_table = 0;
> >          else /* the caller should take care of the previous page */
> >              new_entry.mfn = mfn_x(mfn);
> 
> > _______________________________________________
> > Xen-devel mailing list
> > Xen-devel@lists.xensource.com
> > http://lists.xensource.com/xen-devel
> 
> 

Re: Re: changeset 22526:7a5ee3800417

[Tim Deegan]

At 16:57 +0000 on 07 Mar (1299517021), George Dunlap wrote:
> > Better to use old_entry.mfn, in the spirit of the original cset
> > ("access-once semantics")?  
> 
> I started to do that, but the one below didn't have an old_entry
> already.  
> 
> > In fact, I suspect that to be safe, you need
> > to do an atomic RMW instead of just an atomic set, and then decide
> > whether the VT-d tables will need to be synced.
> 
> Are we not holding the p2m lock when writing entries?

Good point. :)  I would prefer to use old_entry in both places anyway,
just for consistency with the general approach of reading once.  It
won't be any slower. 

Is this patch intended for 4.1.0?

Cheers,

Tim.

-- 
Tim Deegan <Tim.Deegan@citrix.com>
Principal Software Engineer, Xen Platform Team
Citrix Systems UK Ltd.  (Company #02937203, SL9 0BG)

Re: Re: changeset 22526:7a5ee3800417

[George Dunlap]

On Wed, Mar 9, 2011 at 8:44 AM, Tim Deegan <Tim.Deegan@citrix.com> wrote:
> At 16:57 +0000 on 07 Mar (1299517021), George Dunlap wrote:
>> > Better to use old_entry.mfn, in the spirit of the original cset
>> > ("access-once semantics")?
>>
>> I started to do that, but the one below didn't have an old_entry
>> already.
>>
>> > In fact, I suspect that to be safe, you need
>> > to do an atomic RMW instead of just an atomic set, and then decide
>> > whether the VT-d tables will need to be synced.
>>
>> Are we not holding the p2m lock when writing entries?
>
> Good point. :)  I would prefer to use old_entry in both places anyway,
> just for consistency with the general approach of reading once.  It
> won't be any slower.

Sure, why not. :-)

>
> Is this patch intended for 4.1.0?

It looks like by accident, the bug will only cause a performance
degradation -- it will unnecessarily flush the vtd table even if the
mfn for a page isn't changing.  Presumably this will mainly have an
impact on guests which are migrating with PCI pass-through -- is that
even possible?

 -George

Re: Re: changeset 22526:7a5ee3800417

[George Dunlap]

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

Re: Re: changeset 22526:7a5ee3800417

[George Dunlap]

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

Re: Re: changeset 22526:7a5ee3800417

[George Dunlap]

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

答复: [Xen-devel] Re: changeset 22526:7a5ee3800417

[zpfalpc23]

Hi,
Thank you for your new patch!
I will apply you patch to our version of xen source code, and test it in the next few days.
 -Pengfei zhang

-----邮件原件-----
发件人: dunlapg@gmail.com [mailto:dunlapg@gmail.com] 代表 George Dunlap
发送时间: 2011年3月10日 17:43
收件人: George Dunlap
抄送: 张鹏飞; xen devel
主题: Re: [Xen-devel] Re: changeset 22526:7a5ee3800417

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

答复: [Xen-devel] Re: changeset 22526:7a5ee3800417

[zpfalpc23]

Hi George,
	I have read you patch 22526 in xen-testing4.0 and xen-unstable carefully. There is one thing confused me a lot. why the patch for xen-testing4.0 did not handle the case In ept_set_entry where the superpage should be splitted. Maybe the race can't happen in this scene.
	Also When the race of updating p2m table happen, will the normal(be changed by ept_get_entry) page be POD again? In this situation, what is it for Xen and VM?
	Thanks a lot for enlighten me.

----Pengfei zhang

-----邮件原件-----
发件人: dunlapg@gmail.com [mailto:dunlapg@gmail.com] 代表 George Dunlap
发送时间: 2011年3月10日 17:43
收件人: George Dunlap
抄送: 张鹏飞; xen devel
主题: Re: [Xen-devel] Re: changeset 22526:7a5ee3800417

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

Re: 答复: Re: changeset 22526:7a5ee3800417

[George Dunlap]

Pengfei,

You're asking why I didn't make any changes to the superpage spit in
that patch?

The purpose of that patch was to make accesses to the p2m table follow
these rules:
* Reads without the lock are done only once
* Old entries are replaced with completely new entries all at once

The rules are meant to make it so that if you read the p2m table without
holding the p2m lock, even if you're racing with someone who's changing
it, you'll get a "consistent" view.

In the superpage-split case, replacing the old superpage entry with a
pointer to a non-superpage entry already followed those rules; so it
didn't need to be changed.

 -George

On Wed, 2011-03-16 at 10:27 +0000, zpfalpc23 wrote:
> Hi George,
> 	I have read you patch 22526 in xen-testing4.0 and xen-unstable carefully. There is one thing confused me a lot. why the patch for xen-testing4.0 did not handle the case In ept_set_entry where the superpage should be splitted. Maybe the race can't happen in this scene.
> 	Also When the race of updating p2m table happen, will the normal(be changed by ept_get_entry) page be POD again? In this situation, what is it for Xen and VM?
> 	Thanks a lot for enlighten me.
> 
> ----Pengfei zhang
> 
> -----邮件原件-----
> 发件人: dunlapg@gmail.com [mailto:dunlapg@gmail.com] 代表 George Dunlap
> 发送时间: 2011年3月10日 17:43
> 收件人: George Dunlap
> 抄送: 张鹏飞; xen devel
> 主题: Re: [Xen-devel] Re: changeset 22526:7a5ee3800417
> 
> 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
> >>>>
> >>>
> >>
> >
>