[Xen-devel] [PATCH] x86/ept: pass correct level to p2m_entry_modify

Roger Pau Monne posted 1 patch 16 weeks ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/xen tags/patchew/20190703094322.1551-1-roger.pau@citrix.com
xen/arch/x86/mm/p2m-ept.c | 2 +-
xen/include/asm-x86/p2m.h | 2 +-
2 files changed, 2 insertions(+), 2 deletions(-)

[Xen-devel] [PATCH] x86/ept: pass correct level to p2m_entry_modify

Posted by Roger Pau Monne 16 weeks ago
EPT differs from NPT and shadow when translating page orders to levels
in the physmap page tables. EPT page tables level for order 0 pages is
0, while NPT and shadow instead use 1, ie: EPT page tables levels
starts at 0 while NPT and shadow starts at 1.

Fix the p2m_entry_modify call in atomic_write_ept_entry to always add
one to the level, in order to match NPT and shadow usage.

While there also fix p2m_entry_modify BUG condition to trigger when
foreign or ioreq entries with level different than 0 are attempted.
That should allow to catch future errors related to the level
parameter.

Fixes: c7a4c0 ('x86/mm: split p2m ioreq server pages special handling into helper')
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>
---
 xen/arch/x86/mm/p2m-ept.c | 2 +-
 xen/include/asm-x86/p2m.h | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/xen/arch/x86/mm/p2m-ept.c b/xen/arch/x86/mm/p2m-ept.c
index e3044bee2e..6b8468c793 100644
--- a/xen/arch/x86/mm/p2m-ept.c
+++ b/xen/arch/x86/mm/p2m-ept.c
@@ -51,7 +51,7 @@ static int atomic_write_ept_entry(struct p2m_domain *p2m,
                                   int level)
 {
     int rc = p2m_entry_modify(p2m, new.sa_p2mt, entryptr->sa_p2mt,
-                              _mfn(new.mfn), _mfn(entryptr->mfn), level);
+                              _mfn(new.mfn), _mfn(entryptr->mfn), level + 1);
 
     if ( rc )
         return rc;
diff --git a/xen/include/asm-x86/p2m.h b/xen/include/asm-x86/p2m.h
index 09ef7e02fd..756929d5c0 100644
--- a/xen/include/asm-x86/p2m.h
+++ b/xen/include/asm-x86/p2m.h
@@ -946,7 +946,7 @@ static inline int p2m_entry_modify(struct p2m_domain *p2m, p2m_type_t nt,
                                    p2m_type_t ot, mfn_t nfn, mfn_t ofn,
                                    unsigned int level)
 {
-    BUG_ON(level > 1 && (nt == p2m_ioreq_server || nt == p2m_map_foreign));
+    BUG_ON(level != 1 && (nt == p2m_ioreq_server || nt == p2m_map_foreign));
 
     if ( level != 1 || (nt == ot && mfn_eq(nfn, ofn)) )
         return 0;
-- 
2.20.1 (Apple Git-117)


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH] x86/ept: pass correct level to p2m_entry_modify

Posted by Jan Beulich 16 weeks ago
On 03.07.2019 11:43, Roger Pau Monne wrote:
> EPT differs from NPT and shadow when translating page orders to levels
> in the physmap page tables. EPT page tables level for order 0 pages is
> 0, while NPT and shadow instead use 1, ie: EPT page tables levels
> starts at 0 while NPT and shadow starts at 1.
> 
> Fix the p2m_entry_modify call in atomic_write_ept_entry to always add
> one to the level, in order to match NPT and shadow usage.
> 
> While there also fix p2m_entry_modify BUG condition to trigger when
> foreign or ioreq entries with level different than 0 are attempted.
> That should allow to catch future errors related to the level
> parameter.
> 
> Fixes: c7a4c0 ('x86/mm: split p2m ioreq server pages special handling into helper')

A 6-digit hash is definitely too short in the long run. I understand
that this then wants backporting to the 4.12 tree.

> --- a/xen/include/asm-x86/p2m.h
> +++ b/xen/include/asm-x86/p2m.h
> @@ -946,7 +946,7 @@ static inline int p2m_entry_modify(struct p2m_domain *p2m, p2m_type_t nt,
>                                      p2m_type_t ot, mfn_t nfn, mfn_t ofn,
>                                      unsigned int level)
>   {
> -    BUG_ON(level > 1 && (nt == p2m_ioreq_server || nt == p2m_map_foreign));
> +    BUG_ON(level != 1 && (nt == p2m_ioreq_server || nt == p2m_map_foreign));

Wouldn't you better leave this alone and add BUG_ON(!level)?

Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH] x86/ept: pass correct level to p2m_entry_modify

Posted by Roger Pau Monné 16 weeks ago
On Wed, Jul 03, 2019 at 10:22:03AM +0000, Jan Beulich wrote:
> On 03.07.2019 11:43, Roger Pau Monne wrote:
> > EPT differs from NPT and shadow when translating page orders to levels
> > in the physmap page tables. EPT page tables level for order 0 pages is
> > 0, while NPT and shadow instead use 1, ie: EPT page tables levels
> > starts at 0 while NPT and shadow starts at 1.
> > 
> > Fix the p2m_entry_modify call in atomic_write_ept_entry to always add
> > one to the level, in order to match NPT and shadow usage.
> > 
> > While there also fix p2m_entry_modify BUG condition to trigger when
> > foreign or ioreq entries with level different than 0 are attempted.
> > That should allow to catch future errors related to the level
> > parameter.
> > 
> > Fixes: c7a4c0 ('x86/mm: split p2m ioreq server pages special handling into helper')
> 
> A 6-digit hash is definitely too short in the long run. I understand
> that this then wants backporting to the 4.12 tree.

Yes.

Is there consensus on how many digits to use 8, 12, 16?

> > --- a/xen/include/asm-x86/p2m.h
> > +++ b/xen/include/asm-x86/p2m.h
> > @@ -946,7 +946,7 @@ static inline int p2m_entry_modify(struct p2m_domain *p2m, p2m_type_t nt,
> >                                      p2m_type_t ot, mfn_t nfn, mfn_t ofn,
> >                                      unsigned int level)
> >   {
> > -    BUG_ON(level > 1 && (nt == p2m_ioreq_server || nt == p2m_map_foreign));
> > +    BUG_ON(level != 1 && (nt == p2m_ioreq_server || nt == p2m_map_foreign));
> 
> Wouldn't you better leave this alone and add BUG_ON(!level)?

That's an option also. I guess your check is better because it will
trigger for any call with level == 0, while mine would only do if such
call is also to add an entry of type ioreq or foreign.

Thanks, Roger.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH] x86/ept: pass correct level to p2m_entry_modify

Posted by Jan Beulich 16 weeks ago
On 03.07.2019 13:01, Roger Pau Monné  wrote:
> On Wed, Jul 03, 2019 at 10:22:03AM +0000, Jan Beulich wrote:
>> On 03.07.2019 11:43, Roger Pau Monne wrote:
>>> EPT differs from NPT and shadow when translating page orders to levels
>>> in the physmap page tables. EPT page tables level for order 0 pages is
>>> 0, while NPT and shadow instead use 1, ie: EPT page tables levels
>>> starts at 0 while NPT and shadow starts at 1.
>>>
>>> Fix the p2m_entry_modify call in atomic_write_ept_entry to always add
>>> one to the level, in order to match NPT and shadow usage.
>>>
>>> While there also fix p2m_entry_modify BUG condition to trigger when
>>> foreign or ioreq entries with level different than 0 are attempted.
>>> That should allow to catch future errors related to the level
>>> parameter.
>>>
>>> Fixes: c7a4c0 ('x86/mm: split p2m ioreq server pages special handling into helper')
>>
>> A 6-digit hash is definitely too short in the long run. I understand
>> that this then wants backporting to the 4.12 tree.
> 
> Yes.
> 
> Is there consensus on how many digits to use 8, 12, 16?

Consensus - no, I don't think so. But anything below 8 seems
open for collisions in the foreseeable future (albeit I don't
know if git has separate hash name spaces for commits and
objects in general). I've been using 10 for the last so many
years ...

Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel