xen/arch/x86/mm/p2m-ept.c | 25 +++++++++++++++++++++++--
1 files changed, 23 insertions(+), 2 deletions(-)
The current test for a present ept entry checks for a permission bit
to be set.
While this is valid in contexts in which we want to know whether an entry
will fault, it is not correct when it comes to testing whether an entry is
valid. Specifically, in the ept_change_entry_type_page function which is
used to set entries to the log dirty type.
In combination with a p2m access type like n or n2rwx, log dirty will not be
set for ept entries for which it should.
Signed-off-by: Andres Lagar-Cavilla <andres@lagarcavilla.org>
Signed-off-by: Adin Scannell <adin@scannell.ca>
diff -r e7028b298fe3 -r fa59531b6daa xen/arch/x86/mm/p2m-ept.c
--- a/xen/arch/x86/mm/p2m-ept.c
+++ b/xen/arch/x86/mm/p2m-ept.c
@@ -39,7 +39,8 @@
#define atomic_write_ept_entry(__pepte, __epte) \
write_atomic(&(__pepte)->epte, (__epte).epte)
-#define is_epte_present(ept_entry) ((ept_entry)->epte & 0x7)
+#define epte_permissions(ept_entry) ((ept_entry)->epte & 0x7)
+#define is_epte_present(ept_entry) (!!(epte_permissions(ept_entry)))
#define is_epte_superpage(ept_entry) ((ept_entry)->sp)
/* Non-ept "lock-and-check" wrapper */
@@ -139,6 +140,26 @@ static void ept_p2m_type_to_flags(ept_en
}
+static inline bool_t is_epte_valid(ept_entry_t *e)
+{
+ ept_entry_t rights = { .epte = 0 };
+
+ /* Not valid if empty */
+ if (e->epte == 0) return 0;
+
+ /* Not valid if mfn not valid */
+ if ( !mfn_valid(e->mfn) ) return 0;
+
+ /* Not valid if rights different from those of
+ * its p2m type and access combination */
+ ept_p2m_type_to_flags(&rights, e->sa_p2mt, e->access);
+ if ( epte_permissions(&rights) != epte_permissions(e) )
+ return 0;
+
+ return 1;
+}
+
+
#define GUEST_TABLE_MAP_FAILED 0
#define GUEST_TABLE_NORMAL_PAGE 1
#define GUEST_TABLE_SUPER_PAGE 2
@@ -777,7 +798,7 @@ static void ept_change_entry_type_page(m
for ( int i = 0; i < EPT_PAGETABLE_ENTRIES; i++ )
{
- if ( !is_epte_present(epte + i) )
+ if ( !is_epte_valid(epte + i) )
continue;
if ( (ept_page_level > 0) && !is_epte_superpage(epte + i) )
Hi, I agree with the idea of this patch but:> +static inline bool_t is_epte_valid(ept_entry_t *e) > +{ > + ept_entry_t rights = { .epte = 0 }; > + > + /* Not valid if empty */ > + if (e->epte == 0) return 0; > + > + /* Not valid if mfn not valid */ > + if ( !mfn_valid(e->mfn) ) return 0; > + > + /* Not valid if rights different from those of > + * its p2m type and access combination */ > + ept_p2m_type_to_flags(&rights, e->sa_p2mt, e->access); > + if ( epte_permissions(&rights) != epte_permissions(e) ) > + return 0;This is a rather odd set of tests. Since we construct EPT pagetables ourselves, I think that we only need to test for uninitialised entries and explicitly invalid entries. Does this patch work for you? diff -r f5b366c6c4c6 xen/arch/x86/mm/p2m-ept.c --- a/xen/arch/x86/mm/p2m-ept.c Thu Jan 19 10:42:42 2012 +0000 +++ b/xen/arch/x86/mm/p2m-ept.c Thu Jan 19 11:01:34 2012 +0000 @@ -41,6 +41,10 @@ #define is_epte_present(ept_entry) ((ept_entry)->epte & 0x7) #define is_epte_superpage(ept_entry) ((ept_entry)->sp) +static inline bool_t is_epte_valid(ept_entry_t *e) +{ + return (e->epte != 0 && e->sa_p2mt != p2m_invalid); +} /* Non-ept "lock-and-check" wrapper */ static int ept_pod_check_and_populate(struct p2m_domain *p2m, unsigned long gfn, @@ -777,7 +781,7 @@ static void ept_change_entry_type_page(m for ( int i = 0; i < EPT_PAGETABLE_ENTRIES; i++ ) { - if ( !is_epte_present(epte + i) ) + if ( !is_epte_valid(epte + i) ) continue; if ( (ept_page_level > 0) && !is_epte_superpage(epte + i) )
> Hi, > > I agree with the idea of this patch but: > >> +static inline bool_t is_epte_valid(ept_entry_t *e) >> +{ >> + ept_entry_t rights = { .epte = 0 }; >> + >> + /* Not valid if empty */ >> + if (e->epte == 0) return 0; >> + >> + /* Not valid if mfn not valid */ >> + if ( !mfn_valid(e->mfn) ) return 0; >> + >> + /* Not valid if rights different from those of >> + * its p2m type and access combination */ >> + ept_p2m_type_to_flags(&rights, e->sa_p2mt, e->access); >> + if ( epte_permissions(&rights) != epte_permissions(e) ) >> + return 0; > > This is a rather odd set of tests. Since we construct EPT pagetables > ourselves, I think that we only need to test for uninitialised entries > and explicitly invalid entries. > > Does this patch work for you?It will achieve the stated goal. I am curious as to whether we should check for p2m_broken. In any case: Acked-by: Andres Lagar-Cavilla <andres@lagarcavilla.org>> > diff -r f5b366c6c4c6 xen/arch/x86/mm/p2m-ept.c > --- a/xen/arch/x86/mm/p2m-ept.c Thu Jan 19 10:42:42 2012 +0000 > +++ b/xen/arch/x86/mm/p2m-ept.c Thu Jan 19 11:01:34 2012 +0000 > @@ -41,6 +41,10 @@ > > #define is_epte_present(ept_entry) ((ept_entry)->epte & 0x7) > #define is_epte_superpage(ept_entry) ((ept_entry)->sp) > +static inline bool_t is_epte_valid(ept_entry_t *e) > +{ > + return (e->epte != 0 && e->sa_p2mt != p2m_invalid); > +} > > /* Non-ept "lock-and-check" wrapper */ > static int ept_pod_check_and_populate(struct p2m_domain *p2m, unsigned > long gfn, > @@ -777,7 +781,7 @@ static void ept_change_entry_type_page(m > > for ( int i = 0; i < EPT_PAGETABLE_ENTRIES; i++ ) > { > - if ( !is_epte_present(epte + i) ) > + if ( !is_epte_valid(epte + i) ) > continue; > > if ( (ept_page_level > 0) && !is_epte_superpage(epte + i) ) >
At 04:58 -0800 on 19 Jan (1326949088), Andres Lagar-Cavilla wrote:> > Hi, > > > > I agree with the idea of this patch but: > > > >> +static inline bool_t is_epte_valid(ept_entry_t *e) > >> +{ > >> + ept_entry_t rights = { .epte = 0 }; > >> + > >> + /* Not valid if empty */ > >> + if (e->epte == 0) return 0; > >> + > >> + /* Not valid if mfn not valid */ > >> + if ( !mfn_valid(e->mfn) ) return 0; > >> + > >> + /* Not valid if rights different from those of > >> + * its p2m type and access combination */ > >> + ept_p2m_type_to_flags(&rights, e->sa_p2mt, e->access); > >> + if ( epte_permissions(&rights) != epte_permissions(e) ) > >> + return 0; > > > > This is a rather odd set of tests. Since we construct EPT pagetables > > ourselves, I think that we only need to test for uninitialised entries > > and explicitly invalid entries. > > > > Does this patch work for you? > > It will achieve the stated goal. I am curious as to whether we should > check for p2m_broken. In any case: > Acked-by: Andres Lagar-Cavilla <andres@lagarcavilla.org>Thanks. I''ve committed the patch as-is; I think in the current code p2m_ram_broken doesn''t need a special case. Cheers, Tim.