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

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Wed, Sep 04, 2019 at 06:50:25AM +0000, Tian, Kevin wrote:
> > From: Roger Pau Monné [mailto:roger.pau@xxxxxxxxxx]
> > 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@xxxxxxxxxx]
> > > > > 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@xxxxxxxxxx>
> > > > > ---
> > > > > Cc: Jun Nakajima <jun.nakajima@xxxxxxxxx>
> > > > > Cc: Kevin Tian <kevin.tian@xxxxxxxxx>
> > > > > Cc: George Dunlap <george.dunlap@xxxxxxxxxxxxx>
> > > > > Cc: Jan Beulich <jbeulich@xxxxxxxx>
> > > > > Cc: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
> > > > > Cc: Wei Liu <wl@xxxxxxx>
> > > > > Cc: Sergey Dyasli <sergey.dyasli@xxxxxxxxxx>
> > > > > Cc: Paul Durrant <paul.durrant@xxxxxxxxxx>
> > > > > ---
> > > > >   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@xxxxxxxxx>.
> > > >
> > > > 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@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel