* Re: changeset 22526:7a5ee3800417
[not found] <AANLkTimqNJWRWzSj1BYPP5T7GAhSK_=70T9dq5CW94iB@mail.gmail.com>
@ 2011-03-07 16:20 ` George Dunlap
2011-03-07 16:41 ` George Dunlap
0 siblings, 1 reply; 12+ messages in thread
From: George Dunlap @ 2011-03-07 16:20 UTC (permalink / raw)
To: 张鹏飞; +Cc: George Dunlap, xen devel
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
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: Re: changeset 22526:7a5ee3800417
2011-03-07 16:20 ` changeset 22526:7a5ee3800417 George Dunlap
@ 2011-03-07 16:41 ` George Dunlap
2011-03-07 16:48 ` Tim Deegan
2011-03-09 13:59 ` George Dunlap
0 siblings, 2 replies; 12+ messages in thread
From: George Dunlap @ 2011-03-07 16:41 UTC (permalink / raw)
To: George Dunlap; +Cc: xen devel, 张鹏飞
[-- 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
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: Re: changeset 22526:7a5ee3800417
2011-03-07 16:41 ` George Dunlap
@ 2011-03-07 16:48 ` Tim Deegan
2011-03-07 16:57 ` George Dunlap
2011-03-09 13:59 ` George Dunlap
1 sibling, 1 reply; 12+ messages in thread
From: Tim Deegan @ 2011-03-07 16:48 UTC (permalink / raw)
To: George Dunlap; +Cc: 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)
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: Re: changeset 22526:7a5ee3800417
2011-03-07 16:48 ` Tim Deegan
@ 2011-03-07 16:57 ` George Dunlap
2011-03-09 8:44 ` Tim Deegan
0 siblings, 1 reply; 12+ messages in thread
From: George Dunlap @ 2011-03-07 16:57 UTC (permalink / raw)
To: Tim Deegan; +Cc: George Dunlap, 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
>
>
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: Re: changeset 22526:7a5ee3800417
2011-03-07 16:57 ` George Dunlap
@ 2011-03-09 8:44 ` Tim Deegan
2011-03-09 13:57 ` George Dunlap
0 siblings, 1 reply; 12+ messages in thread
From: Tim Deegan @ 2011-03-09 8:44 UTC (permalink / raw)
To: George Dunlap; +Cc: 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)
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: Re: changeset 22526:7a5ee3800417
2011-03-09 8:44 ` Tim Deegan
@ 2011-03-09 13:57 ` George Dunlap
0 siblings, 0 replies; 12+ messages in thread
From: George Dunlap @ 2011-03-09 13:57 UTC (permalink / raw)
To: Tim Deegan; +Cc: 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
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: Re: changeset 22526:7a5ee3800417
2011-03-07 16:41 ` George Dunlap
2011-03-07 16:48 ` Tim Deegan
@ 2011-03-09 13:59 ` George Dunlap
2011-03-10 9:41 ` George Dunlap
1 sibling, 1 reply; 12+ messages in thread
From: George Dunlap @ 2011-03-09 13:59 UTC (permalink / raw)
To: George Dunlap; +Cc: 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
>>
>
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: Re: changeset 22526:7a5ee3800417
2011-03-09 13:59 ` George Dunlap
@ 2011-03-10 9:41 ` George Dunlap
2011-03-10 9:43 ` George Dunlap
0 siblings, 1 reply; 12+ messages in thread
From: George Dunlap @ 2011-03-10 9:41 UTC (permalink / raw)
To: George Dunlap; +Cc: 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
>>>
>>
>
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: Re: changeset 22526:7a5ee3800417
2011-03-10 9:41 ` George Dunlap
@ 2011-03-10 9:43 ` George Dunlap
2011-03-10 12:49 ` 答复: [Xen-devel] " zpfalpc23
2011-03-16 10:27 ` zpfalpc23
0 siblings, 2 replies; 12+ messages in thread
From: George Dunlap @ 2011-03-10 9:43 UTC (permalink / raw)
To: George Dunlap; +Cc: xen devel, 张鹏飞
[-- 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
^ permalink raw reply [flat|nested] 12+ messages in thread
* 答复: [Xen-devel] Re: changeset 22526:7a5ee3800417
2011-03-10 9:43 ` George Dunlap
@ 2011-03-10 12:49 ` zpfalpc23
2011-03-16 10:27 ` zpfalpc23
1 sibling, 0 replies; 12+ messages in thread
From: zpfalpc23 @ 2011-03-10 12:49 UTC (permalink / raw)
To: 'George Dunlap'; +Cc: '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
>>>>
>>>
>>
>
^ permalink raw reply [flat|nested] 12+ messages in thread
* 答复: [Xen-devel] Re: changeset 22526:7a5ee3800417
2011-03-10 9:43 ` George Dunlap
2011-03-10 12:49 ` 答复: [Xen-devel] " zpfalpc23
@ 2011-03-16 10:27 ` zpfalpc23
2011-03-17 12:01 ` 答复: " George Dunlap
1 sibling, 1 reply; 12+ messages in thread
From: zpfalpc23 @ 2011-03-16 10:27 UTC (permalink / raw)
To: 'xen devel'; +Cc: 'George Dunlap', 'George Dunlap'
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
>>>>
>>>
>>
>
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: 答复: Re: changeset 22526:7a5ee3800417
2011-03-16 10:27 ` zpfalpc23
@ 2011-03-17 12:01 ` George Dunlap
0 siblings, 0 replies; 12+ messages in thread
From: George Dunlap @ 2011-03-17 12:01 UTC (permalink / raw)
To: zpfalpc23; +Cc: George Dunlap, '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
> >>>>
> >>>
> >>
> >
>
^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2011-03-17 12:01 UTC | newest]
Thread overview: 12+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <AANLkTimqNJWRWzSj1BYPP5T7GAhSK_=70T9dq5CW94iB@mail.gmail.com>
2011-03-07 16:20 ` changeset 22526:7a5ee3800417 George Dunlap
2011-03-07 16:41 ` George Dunlap
2011-03-07 16:48 ` Tim Deegan
2011-03-07 16:57 ` George Dunlap
2011-03-09 8:44 ` Tim Deegan
2011-03-09 13:57 ` George Dunlap
2011-03-09 13:59 ` George Dunlap
2011-03-10 9:41 ` George Dunlap
2011-03-10 9:43 ` George Dunlap
2011-03-10 12:49 ` 答复: [Xen-devel] " zpfalpc23
2011-03-16 10:27 ` zpfalpc23
2011-03-17 12:01 ` 答复: " George Dunlap
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).