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
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 mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
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) _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
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 > >_______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
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) _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
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 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
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 mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
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 mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
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 mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
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 mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
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 >>>> >>> >> >_______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
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 > >>>> > >>> > >> > > >_______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel