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:[email protected]] >>>> 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é <[email protected]> >>>> --- >>>> Cc: Jun Nakajima <[email protected]> >>>> Cc: Kevin Tian <[email protected]> >>>> Cc: George Dunlap <[email protected]> >>>> Cc: Jan Beulich <[email protected]> >>>> Cc: Andrew Cooper <[email protected]> >>>> Cc: Wei Liu <[email protected]> >>>> Cc: Sergey Dyasli <[email protected]> >>>> Cc: Paul Durrant <[email protected]> >>>> --- >>>> 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 <[email protected]>. >>> >>> 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 [email protected] https://lists.xenproject.org/mailman/listinfo/xen-devel
