George Dunlap
2011-Mar-15 15:41 UTC
[Xen-devel] [PATCH] 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 3caed2112c65 -r 56b32e6fd315 xen/arch/x86/mm/hap/p2m-ept.c
--- a/xen/arch/x86/mm/hap/p2m-ept.c Tue Mar 15 10:14:27 2011 +0000
+++ b/xen/arch/x86/mm/hap/p2m-ept.c Tue Mar 15 15:41:17 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);
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel
Tim Deegan
2011-Mar-16 09:40 UTC
Re: [Xen-devel] [PATCH] ept: Fix bug in changeset 22526:7a5ee3800417
At 15:41 +0000 on 15 Mar (1300203711), George Dunlap wrote:> 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>Applied, thanks. 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
MaoXiaoyun
2011-Mar-16 11:08 UTC
RE: Re: [Xen-devel] [PATCH] ept: Fix bug in changeset 22526:7a5ee3800417
So system doesn''t use VTD is not affectted, right?> Date: Wed, 16 Mar 2011 09:40:15 +0000 > From: Tim Deegan <Tim.Deegan@citrix.com> > Subject: Re: [Xen-devel] [PATCH] ept: Fix bug in changeset > 22526:7a5ee3800417 > To: George Dunlap <george.dunlap@eu.citrix.com> > Cc: "xen-devel@lists.xensource.com" <xen-devel@lists.xensource.com> > > At 15:41 +0000 on 15 Mar (1300203711), George Dunlap wrote: > > 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> > > Applied, thanks. > > 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
George Dunlap
2011-Mar-16 11:34 UTC
RE: Re: [Xen-devel] [PATCH] ept: Fix bug in changeset 22526:7a5ee3800417
I think so, yes. -George On Wed, 2011-03-16 at 11:08 +0000, MaoXiaoyun wrote:> > So system doesn''t use VTD is not affectted, right? > > > Date: Wed, 16 Mar 2011 09:40:15 +0000 > > From: Tim Deegan <Tim.Deegan@citrix.com> > > Subject: Re: [Xen-devel] [PATCH] ept: Fix bug in changeset > > 22526:7a5ee3800417 > > To: George Dunlap <george.dunlap@eu.citrix.com> > > Cc: "xen-devel@lists.xensource.com" <xen-devel@lists.xensource.com> > > > > At 15:41 +0000 on 15 Mar (1300203711), George Dunlap wrote: > > > 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> > > > > Applied, thanks. > > > > 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
Ian Jackson
2011-Mar-17 16:18 UTC
Re: [Xen-devel] [PATCH] ept: Fix bug in changeset 22526:7a5ee3800417
George Dunlap writes ("[Xen-devel] [PATCH] 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.
Does this need applying to 4.1 before the release ?
Ian.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel
Tim Deegan
2011-Mar-17 16:30 UTC
Re: [Xen-devel] [PATCH] ept: Fix bug in changeset 22526:7a5ee3800417
At 16:18 +0000 on 17 Mar (1300378713), Ian Jackson wrote:> George Dunlap writes ("[Xen-devel] [PATCH] 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. > > Does this need applying to 4.1 before the release ?I don''t think so, but it should be backported afterwards. 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
George Dunlap
2011-Mar-17 17:45 UTC
Re: [Xen-devel] [PATCH] ept: Fix bug in changeset 22526:7a5ee3800417
On Thu, 2011-03-17 at 16:30 +0000, Tim Deegan wrote:> At 16:18 +0000 on 17 Mar (1300378713), Ian Jackson wrote: > > George Dunlap writes ("[Xen-devel] [PATCH] 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. > > > > Does this need applying to 4.1 before the release ? > > I don''t think so, but it should be backported afterwards.I tend to agree. -George _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel