The current logic in epte_get_entry_emt() will split any page marked
as special with order greater than zero, without checking whether the
super page is all special.

Fix this by only splitting the page only if it's not all marked as
special, in order to prevent unneeded super page shuttering.

Fixes: ca24b2ffdb ('x86/hvm: set 'ipat' in EPT for special pages')
Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
Cc: Paul Durrant <paul@xen.org>
---
It would seem weird to me to have a super page entry in EPT with
ranges marked as special and not the full page.  I guess it's better
to be safe, but I don't see an scenario where we could end up in that
situation.

I've been trying to find a clarification in the original patch
submission about how it's possible to have such super page EPT entry,
but haven't been able to find any justification.

Might be nice to expand the commit message as to why it's possible to
have such mixed super page entries that would need splitting.
---
 xen/arch/x86/mm/p2m-ept.c | 20 +++++++++++---------
 1 file changed, 11 insertions(+), 9 deletions(-)

diff --git a/xen/arch/x86/mm/p2m-ept.c b/xen/arch/x86/mm/p2m-ept.c
index b04ca6dbe8..b4919bad51 100644
--- a/xen/arch/x86/mm/p2m-ept.c
+++ b/xen/arch/x86/mm/p2m-ept.c
@@ -491,7 +491,7 @@ int epte_get_entry_emt(struct domain *d, gfn_t gfn, mfn_t mfn,
 {
     int gmtrr_mtype, hmtrr_mtype;
     struct vcpu *v = current;
-    unsigned long i;
+    unsigned long i, special_pgs;
 
     *ipat = false;
 
@@ -525,15 +525,17 @@ int epte_get_entry_emt(struct domain *d, gfn_t gfn, mfn_t mfn,
         return MTRR_TYPE_WRBACK;
     }
 
-    for ( i = 0; i < (1ul << order); i++ )
-    {
+    for ( special_pgs = i = 0; i < (1ul << order); i++ )
         if ( is_special_page(mfn_to_page(mfn_add(mfn, i))) )
-        {
-            if ( order )
-                return -1;
-            *ipat = true;
-            return MTRR_TYPE_WRBACK;
-        }
+            special_pgs++;
+
+    if ( special_pgs )
+    {
+        if ( special_pgs != (1ul << order) )
+            return -1;
+
+        *ipat = true;
+        return MTRR_TYPE_WRBACK;
     }
 
     switch ( type )
-- 
2.36.1

On 27.06.2022 12:01, Roger Pau Monne wrote:
> The current logic in epte_get_entry_emt() will split any page marked
> as special with order greater than zero, without checking whether the
> super page is all special.
> 
> Fix this by only splitting the page only if it's not all marked as
> special, in order to prevent unneeded super page shuttering.
> 
> Fixes: ca24b2ffdb ('x86/hvm: set 'ipat' in EPT for special pages')
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> ---
> Cc: Paul Durrant <paul@xen.org>
> ---
> It would seem weird to me to have a super page entry in EPT with
> ranges marked as special and not the full page.  I guess it's better
> to be safe, but I don't see an scenario where we could end up in that
> situation.
> 
> I've been trying to find a clarification in the original patch
> submission about how it's possible to have such super page EPT entry,
> but haven't been able to find any justification.

I think the loop there was added "just in case", whereas in reality
it was only single special pages that would ever be mapped. So ...

> Might be nice to expand the commit message as to why it's possible to
> have such mixed super page entries that would need splitting.

... there may not be anything to add. I don't mind this being re-done,
hence
Reviewed-by: Jan Beulich <jbeulich@suse.com>
Yet I'm not sure I understand what case this actually fixes (and
hence why you did add a Fixes: tag) - are there cases where non-
order-0 special pages are mapped in reality?

As to the Fixes: tag - I think we mean to follow Linux there and
hence want 12-digit hashes to be specified. See also
docs/process/sending-patches.pandoc. (But no need to resend just
for this.)

Jan