Re: [Xen-devel] [PATCH] p2m/ept: pass correct level to atomic_write_ept_entry in ept_invalidate_emt

["Tian, Kevin" <kevin.tian@intel.com>]

> 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