The level passed to ept_invalidate_emt corresponds to the EPT entry
passed as the mfn parameter, which is a pointer to an EPT page table,
hence the entries in that page table will have one level less than the
parent.
Fix the call to atomic_write_ept_entry to pass the correct level, ie:
one level less than the parent.
Fixes: 50fe6e73705 ('pvh dom0: add and remove foreign pages')
Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
Cc: Jun Nakajima <jun.nakajima@intel.com>
Cc: Kevin Tian <kevin.tian@intel.com>
Cc: George Dunlap <george.dunlap@eu.citrix.com>
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
Cc: Wei Liu <wl@xen.org>
Cc: Sergey Dyasli <sergey.dyasli@citrix.com>
Cc: Paul Durrant <paul.durrant@citrix.com>
---
xen/arch/x86/mm/p2m-ept.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/xen/arch/x86/mm/p2m-ept.c b/xen/arch/x86/mm/p2m-ept.c
index 6b8468c793..23ae6e9da3 100644
--- a/xen/arch/x86/mm/p2m-ept.c
+++ b/xen/arch/x86/mm/p2m-ept.c
@@ -367,7 +367,7 @@ static bool_t ept_invalidate_emt(struct p2m_domain *p2m, mfn_t mfn,
e.emt = MTRR_NUM_TYPES;
if ( recalc )
e.recalc = 1;
- rc = atomic_write_ept_entry(p2m, &epte[i], e, level);
+ rc = atomic_write_ept_entry(p2m, &epte[i], e, level - 1);
ASSERT(rc == 0);
changed = 1;
}
--
2.22.0
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
> From: Roger Pau Monne [mailto:roger.pau@citrix.com] > Sent: Tuesday, August 20, 2019 11:38 PM > > The level passed to ept_invalidate_emt corresponds to the EPT entry > passed as the mfn parameter, which is a pointer to an EPT page table, > hence the entries in that page table will have one level less than the > parent. > > Fix the call to atomic_write_ept_entry to pass the correct level, ie: > one level less than the parent. > > Fixes: 50fe6e73705 ('pvh dom0: add and remove foreign pages') > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> > --- > Cc: Jun Nakajima <jun.nakajima@intel.com> > Cc: Kevin Tian <kevin.tian@intel.com> > Cc: George Dunlap <george.dunlap@eu.citrix.com> > Cc: Jan Beulich <jbeulich@suse.com> > Cc: Andrew Cooper <andrew.cooper3@citrix.com> > Cc: Wei Liu <wl@xen.org> > Cc: Sergey Dyasli <sergey.dyasli@citrix.com> > Cc: Paul Durrant <paul.durrant@citrix.com> > --- > xen/arch/x86/mm/p2m-ept.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/xen/arch/x86/mm/p2m-ept.c b/xen/arch/x86/mm/p2m-ept.c > index 6b8468c793..23ae6e9da3 100644 > --- a/xen/arch/x86/mm/p2m-ept.c > +++ b/xen/arch/x86/mm/p2m-ept.c > @@ -367,7 +367,7 @@ static bool_t ept_invalidate_emt(struct p2m_domain > *p2m, mfn_t mfn, > e.emt = MTRR_NUM_TYPES; > if ( recalc ) > e.recalc = 1; > - rc = atomic_write_ept_entry(p2m, &epte[i], e, level); > + rc = atomic_write_ept_entry(p2m, &epte[i], e, level - 1); > ASSERT(rc == 0); > changed = 1; > } Reviewed-by: Kevin Tian <kevin.tian@intel.com>. One small comment about readability. What about renaming 'level' to 'parent_level' in this function? Thanks Kevin _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
On 23.08.2019 07:58, Tian, Kevin wrote: >> From: Roger Pau Monne [mailto:roger.pau@citrix.com] >> Sent: Tuesday, August 20, 2019 11:38 PM >> >> The level passed to ept_invalidate_emt corresponds to the EPT entry >> passed as the mfn parameter, which is a pointer to an EPT page table, >> hence the entries in that page table will have one level less than the >> parent. >> >> Fix the call to atomic_write_ept_entry to pass the correct level, ie: >> one level less than the parent. >> >> Fixes: 50fe6e73705 ('pvh dom0: add and remove foreign pages') >> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> >> --- >> Cc: Jun Nakajima <jun.nakajima@intel.com> >> Cc: Kevin Tian <kevin.tian@intel.com> >> Cc: George Dunlap <george.dunlap@eu.citrix.com> >> Cc: Jan Beulich <jbeulich@suse.com> >> Cc: Andrew Cooper <andrew.cooper3@citrix.com> >> Cc: Wei Liu <wl@xen.org> >> Cc: Sergey Dyasli <sergey.dyasli@citrix.com> >> Cc: Paul Durrant <paul.durrant@citrix.com> >> --- >> xen/arch/x86/mm/p2m-ept.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/xen/arch/x86/mm/p2m-ept.c b/xen/arch/x86/mm/p2m-ept.c >> index 6b8468c793..23ae6e9da3 100644 >> --- a/xen/arch/x86/mm/p2m-ept.c >> +++ b/xen/arch/x86/mm/p2m-ept.c >> @@ -367,7 +367,7 @@ static bool_t ept_invalidate_emt(struct p2m_domain >> *p2m, mfn_t mfn, >> e.emt = MTRR_NUM_TYPES; >> if ( recalc ) >> e.recalc = 1; >> - rc = atomic_write_ept_entry(p2m, &epte[i], e, level); >> + rc = atomic_write_ept_entry(p2m, &epte[i], e, level - 1); >> ASSERT(rc == 0); >> changed = 1; >> } > > Reviewed-by: Kevin Tian <kevin.tian@intel.com>. > > One small comment about readability. What about renaming 'level' > to 'parent_level' in this function? And also taking the opportunity and making it unsigned int? Jan _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
On Tue, Aug 27, 2019 at 05:23:33PM +0200, Jan Beulich wrote: > On 23.08.2019 07:58, Tian, Kevin wrote: > > > From: Roger Pau Monne [mailto:roger.pau@citrix.com] > > > Sent: Tuesday, August 20, 2019 11:38 PM > > > > > > The level passed to ept_invalidate_emt corresponds to the EPT entry > > > passed as the mfn parameter, which is a pointer to an EPT page table, > > > hence the entries in that page table will have one level less than the > > > parent. > > > > > > Fix the call to atomic_write_ept_entry to pass the correct level, ie: > > > one level less than the parent. > > > > > > Fixes: 50fe6e73705 ('pvh dom0: add and remove foreign pages') > > > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> > > > --- > > > Cc: Jun Nakajima <jun.nakajima@intel.com> > > > Cc: Kevin Tian <kevin.tian@intel.com> > > > Cc: George Dunlap <george.dunlap@eu.citrix.com> > > > Cc: Jan Beulich <jbeulich@suse.com> > > > Cc: Andrew Cooper <andrew.cooper3@citrix.com> > > > Cc: Wei Liu <wl@xen.org> > > > Cc: Sergey Dyasli <sergey.dyasli@citrix.com> > > > Cc: Paul Durrant <paul.durrant@citrix.com> > > > --- > > > xen/arch/x86/mm/p2m-ept.c | 2 +- > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > diff --git a/xen/arch/x86/mm/p2m-ept.c b/xen/arch/x86/mm/p2m-ept.c > > > index 6b8468c793..23ae6e9da3 100644 > > > --- a/xen/arch/x86/mm/p2m-ept.c > > > +++ b/xen/arch/x86/mm/p2m-ept.c > > > @@ -367,7 +367,7 @@ static bool_t ept_invalidate_emt(struct p2m_domain > > > *p2m, mfn_t mfn, > > > e.emt = MTRR_NUM_TYPES; > > > if ( recalc ) > > > e.recalc = 1; > > > - rc = atomic_write_ept_entry(p2m, &epte[i], e, level); > > > + rc = atomic_write_ept_entry(p2m, &epte[i], e, level - 1); > > > ASSERT(rc == 0); > > > changed = 1; > > > } > > > > Reviewed-by: Kevin Tian <kevin.tian@intel.com>. > > > > One small comment about readability. What about renaming 'level' > > to 'parent_level' in this function? > > And also taking the opportunity and making it unsigned int? I've been thinking about this, and I'm not sure renaming level to parent_level is correct, level actually matches the level of the mfn also passed as a parameter, and hence it's correct. It's the function itself that actually iterates over the page table entries, and hence descends one level into the page tables. ie: I could add something like ASSERT(level) to make sure no level 0 entries are passed to the function, but renaming doesn't seem correct to me. Thanks, Roger. _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
On 29.08.2019 12:26, Roger Pau Monné wrote: > On Tue, Aug 27, 2019 at 05:23:33PM +0200, Jan Beulich wrote: >> On 23.08.2019 07:58, Tian, Kevin wrote: >>>> From: Roger Pau Monne [mailto:roger.pau@citrix.com] >>>> Sent: Tuesday, August 20, 2019 11:38 PM >>>> >>>> The level passed to ept_invalidate_emt corresponds to the EPT entry >>>> passed as the mfn parameter, which is a pointer to an EPT page table, >>>> hence the entries in that page table will have one level less than the >>>> parent. >>>> >>>> Fix the call to atomic_write_ept_entry to pass the correct level, ie: >>>> one level less than the parent. >>>> >>>> Fixes: 50fe6e73705 ('pvh dom0: add and remove foreign pages') >>>> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> >>>> --- >>>> Cc: Jun Nakajima <jun.nakajima@intel.com> >>>> Cc: Kevin Tian <kevin.tian@intel.com> >>>> Cc: George Dunlap <george.dunlap@eu.citrix.com> >>>> Cc: Jan Beulich <jbeulich@suse.com> >>>> Cc: Andrew Cooper <andrew.cooper3@citrix.com> >>>> Cc: Wei Liu <wl@xen.org> >>>> Cc: Sergey Dyasli <sergey.dyasli@citrix.com> >>>> Cc: Paul Durrant <paul.durrant@citrix.com> >>>> --- >>>> xen/arch/x86/mm/p2m-ept.c | 2 +- >>>> 1 file changed, 1 insertion(+), 1 deletion(-) >>>> >>>> diff --git a/xen/arch/x86/mm/p2m-ept.c b/xen/arch/x86/mm/p2m-ept.c >>>> index 6b8468c793..23ae6e9da3 100644 >>>> --- a/xen/arch/x86/mm/p2m-ept.c >>>> +++ b/xen/arch/x86/mm/p2m-ept.c >>>> @@ -367,7 +367,7 @@ static bool_t ept_invalidate_emt(struct p2m_domain >>>> *p2m, mfn_t mfn, >>>> e.emt = MTRR_NUM_TYPES; >>>> if ( recalc ) >>>> e.recalc = 1; >>>> - rc = atomic_write_ept_entry(p2m, &epte[i], e, level); >>>> + rc = atomic_write_ept_entry(p2m, &epte[i], e, level - 1); >>>> ASSERT(rc == 0); >>>> changed = 1; >>>> } >>> >>> Reviewed-by: Kevin Tian <kevin.tian@intel.com>. >>> >>> One small comment about readability. What about renaming 'level' >>> to 'parent_level' in this function? >> >> And also taking the opportunity and making it unsigned int? > > I've been thinking about this, and I'm not sure renaming level to > parent_level is correct, level actually matches the level of the mfn > also passed as a parameter, and hence it's correct. It's the function > itself that actually iterates over the page table entries, and hence > descends one level into the page tables. > > ie: I could add something like ASSERT(level) to make sure no level 0 > entries are passed to the function, but renaming doesn't seem correct > to me. Hmm, I'm afraid I've made the change as requested by Kevin while committing. Personally I think either name is fine, but if Kevin agrees with your response, then maybe we should undo that adjustment. Jan _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
> From: Roger Pau Monné [mailto:roger.pau@citrix.com] > Sent: Thursday, August 29, 2019 6:26 PM > > On Tue, Aug 27, 2019 at 05:23:33PM +0200, Jan Beulich wrote: > > On 23.08.2019 07:58, Tian, Kevin wrote: > > > > From: Roger Pau Monne [mailto:roger.pau@citrix.com] > > > > Sent: Tuesday, August 20, 2019 11:38 PM > > > > > > > > The level passed to ept_invalidate_emt corresponds to the EPT entry > > > > passed as the mfn parameter, which is a pointer to an EPT page table, > > > > hence the entries in that page table will have one level less than the > > > > parent. > > > > > > > > Fix the call to atomic_write_ept_entry to pass the correct level, ie: > > > > one level less than the parent. > > > > > > > > Fixes: 50fe6e73705 ('pvh dom0: add and remove foreign pages') > > > > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> > > > > --- > > > > Cc: Jun Nakajima <jun.nakajima@intel.com> > > > > Cc: Kevin Tian <kevin.tian@intel.com> > > > > Cc: George Dunlap <george.dunlap@eu.citrix.com> > > > > Cc: Jan Beulich <jbeulich@suse.com> > > > > Cc: Andrew Cooper <andrew.cooper3@citrix.com> > > > > Cc: Wei Liu <wl@xen.org> > > > > Cc: Sergey Dyasli <sergey.dyasli@citrix.com> > > > > Cc: Paul Durrant <paul.durrant@citrix.com> > > > > --- > > > > xen/arch/x86/mm/p2m-ept.c | 2 +- > > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > > > diff --git a/xen/arch/x86/mm/p2m-ept.c b/xen/arch/x86/mm/p2m-ept.c > > > > index 6b8468c793..23ae6e9da3 100644 > > > > --- a/xen/arch/x86/mm/p2m-ept.c > > > > +++ b/xen/arch/x86/mm/p2m-ept.c > > > > @@ -367,7 +367,7 @@ static bool_t ept_invalidate_emt(struct > p2m_domain > > > > *p2m, mfn_t mfn, > > > > e.emt = MTRR_NUM_TYPES; > > > > if ( recalc ) > > > > e.recalc = 1; > > > > - rc = atomic_write_ept_entry(p2m, &epte[i], e, level); > > > > + rc = atomic_write_ept_entry(p2m, &epte[i], e, level - 1); > > > > ASSERT(rc == 0); > > > > changed = 1; > > > > } > > > > > > Reviewed-by: Kevin Tian <kevin.tian@intel.com>. > > > > > > One small comment about readability. What about renaming 'level' > > > to 'parent_level' in this function? > > > > And also taking the opportunity and making it unsigned int? > > I've been thinking about this, and I'm not sure renaming level to > parent_level is correct, level actually matches the level of the mfn > also passed as a parameter, and hence it's correct. It's the function > itself that actually iterates over the page table entries, and hence > descends one level into the page tables. > > ie: I could add something like ASSERT(level) to make sure no level 0 > entries are passed to the function, but renaming doesn't seem correct > to me. > The problem to me is that it is very obscure about what a 'level' actually means. Although I figured it out last time when reviewing this patch, now I'm getting confused again when looking at related code. e.g., you minus level by one in this patch when invoking atomic_write_ept_entry, while the latter increments the level by one when invoking p2m_entry_ modify. They are all about entry update according to the function name, but clearly the level means different thing. :/ specifically for this patch, maybe call ept_invalidate_emt_subtree can also improve readability a bit, along with ASSERT(level) thing? Thanks Kevin _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
On Wed, Sep 04, 2019 at 06:50:25AM +0000, Tian, Kevin wrote: > > From: Roger Pau Monné [mailto:roger.pau@citrix.com] > > Sent: Thursday, August 29, 2019 6:26 PM > > > > On Tue, Aug 27, 2019 at 05:23:33PM +0200, Jan Beulich wrote: > > > On 23.08.2019 07:58, Tian, Kevin wrote: > > > > > From: Roger Pau Monne [mailto:roger.pau@citrix.com] > > > > > Sent: Tuesday, August 20, 2019 11:38 PM > > > > > > > > > > The level passed to ept_invalidate_emt corresponds to the EPT entry > > > > > passed as the mfn parameter, which is a pointer to an EPT page table, > > > > > hence the entries in that page table will have one level less than the > > > > > parent. > > > > > > > > > > Fix the call to atomic_write_ept_entry to pass the correct level, ie: > > > > > one level less than the parent. > > > > > > > > > > Fixes: 50fe6e73705 ('pvh dom0: add and remove foreign pages') > > > > > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> > > > > > --- > > > > > Cc: Jun Nakajima <jun.nakajima@intel.com> > > > > > Cc: Kevin Tian <kevin.tian@intel.com> > > > > > Cc: George Dunlap <george.dunlap@eu.citrix.com> > > > > > Cc: Jan Beulich <jbeulich@suse.com> > > > > > Cc: Andrew Cooper <andrew.cooper3@citrix.com> > > > > > Cc: Wei Liu <wl@xen.org> > > > > > Cc: Sergey Dyasli <sergey.dyasli@citrix.com> > > > > > Cc: Paul Durrant <paul.durrant@citrix.com> > > > > > --- > > > > > xen/arch/x86/mm/p2m-ept.c | 2 +- > > > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > > > > > diff --git a/xen/arch/x86/mm/p2m-ept.c b/xen/arch/x86/mm/p2m-ept.c > > > > > index 6b8468c793..23ae6e9da3 100644 > > > > > --- a/xen/arch/x86/mm/p2m-ept.c > > > > > +++ b/xen/arch/x86/mm/p2m-ept.c > > > > > @@ -367,7 +367,7 @@ static bool_t ept_invalidate_emt(struct > > p2m_domain > > > > > *p2m, mfn_t mfn, > > > > > e.emt = MTRR_NUM_TYPES; > > > > > if ( recalc ) > > > > > e.recalc = 1; > > > > > - rc = atomic_write_ept_entry(p2m, &epte[i], e, level); > > > > > + rc = atomic_write_ept_entry(p2m, &epte[i], e, level - 1); > > > > > ASSERT(rc == 0); > > > > > changed = 1; > > > > > } > > > > > > > > Reviewed-by: Kevin Tian <kevin.tian@intel.com>. > > > > > > > > One small comment about readability. What about renaming 'level' > > > > to 'parent_level' in this function? > > > > > > And also taking the opportunity and making it unsigned int? > > > > I've been thinking about this, and I'm not sure renaming level to > > parent_level is correct, level actually matches the level of the mfn > > also passed as a parameter, and hence it's correct. It's the function > > itself that actually iterates over the page table entries, and hence > > descends one level into the page tables. > > > > ie: I could add something like ASSERT(level) to make sure no level 0 > > entries are passed to the function, but renaming doesn't seem correct > > to me. > > > > The problem to me is that it is very obscure about what a 'level' actually > means. Although I figured it out last time when reviewing this patch, > now I'm getting confused again when looking at related code. e.g., you > minus level by one in this patch when invoking atomic_write_ept_entry, That minus one is because the level of EPT entry in the atomic_write_ept_entry call is one level less than the parent, which is the parameter of the function. > while the latter increments the level by one when invoking p2m_entry_ > modify. They are all about entry update according to the function name, > but clearly the level means different thing. :/ That's unfortunate, but NPT/shadow considers that level starts at 1 (ie: 4K page-table entries are level 1), while EPT consider that level starts at 0 (ie: 4K page-table entries are level 0). That's IMO a very bad choice, I guess no one realized before of this difference because level was always internal to NPT or EPT code, but no generic functions using such level parameter where being called from both implementations. > specifically for this patch, maybe call ept_invalidate_emt_subtree can > also improve readability a bit, along with ASSERT(level) thing? I agree, let me try to prepare a patch. Thanks, Roger. _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
On Fri, Aug 23, 2019 at 05:58:29AM +0000, Tian, Kevin wrote: > > From: Roger Pau Monne [mailto:roger.pau@citrix.com] > > Sent: Tuesday, August 20, 2019 11:38 PM > > > > The level passed to ept_invalidate_emt corresponds to the EPT entry > > passed as the mfn parameter, which is a pointer to an EPT page table, > > hence the entries in that page table will have one level less than the > > parent. > > > > Fix the call to atomic_write_ept_entry to pass the correct level, ie: > > one level less than the parent. > > > > Fixes: 50fe6e73705 ('pvh dom0: add and remove foreign pages') > > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> > > --- > > Cc: Jun Nakajima <jun.nakajima@intel.com> > > Cc: Kevin Tian <kevin.tian@intel.com> > > Cc: George Dunlap <george.dunlap@eu.citrix.com> > > Cc: Jan Beulich <jbeulich@suse.com> > > Cc: Andrew Cooper <andrew.cooper3@citrix.com> > > Cc: Wei Liu <wl@xen.org> > > Cc: Sergey Dyasli <sergey.dyasli@citrix.com> > > Cc: Paul Durrant <paul.durrant@citrix.com> > > --- > > xen/arch/x86/mm/p2m-ept.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/xen/arch/x86/mm/p2m-ept.c b/xen/arch/x86/mm/p2m-ept.c > > index 6b8468c793..23ae6e9da3 100644 > > --- a/xen/arch/x86/mm/p2m-ept.c > > +++ b/xen/arch/x86/mm/p2m-ept.c > > @@ -367,7 +367,7 @@ static bool_t ept_invalidate_emt(struct p2m_domain > > *p2m, mfn_t mfn, > > e.emt = MTRR_NUM_TYPES; > > if ( recalc ) > > e.recalc = 1; > > - rc = atomic_write_ept_entry(p2m, &epte[i], e, level); > > + rc = atomic_write_ept_entry(p2m, &epte[i], e, level - 1); > > ASSERT(rc == 0); > > changed = 1; > > } > > Reviewed-by: Kevin Tian <kevin.tian@intel.com>. > > One small comment about readability. What about renaming 'level' > to 'parent_level' in this function? Sure, I guess this can be done while committing it, or else I can send a follow up. Thanks, Roger. _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
© 2016 - 2024 Red Hat, Inc.