RE: [PATCH] x86/ept: fix shattering of special pages

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

> From: Roger Pau Monne <roger.pau@xxxxxxxxxx>
> Sent: Monday, June 27, 2022 6:01 PM
> 
> 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@xxxxxxxxxx>
> ---
> Cc: Paul Durrant <paul@xxxxxxx>
> ---
> 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.

Here is what I dig out.

When reviewing v1 of adding special page check, Jan suggested
that APIC access page was also covered hence the old logic for APIC
access page can be removed. [1]

Then when reviewing v2 he found that the order check in removed
logic was not carried to the new check on special page. [2] 

The original order check in old APIC access logic came from:

commit 126018f2acd5416434747423e61a4690108b9dc9
Author: Jan Beulich <jbeulich@xxxxxxxx>
Date:   Fri May 2 10:48:48 2014 +0200

    x86/EPT: consider page order when checking for APIC MFN

    This was overlooked in 3d90d6e6 ("x86/EPT: split super pages upon
    mismatching memory types").

    Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
    Acked-by: Kevin Tian <kevin.tian@xxxxxxxxx>
    Reviewed-by: Tim Deegan <tim@xxxxxxx>

I suppose Jan may actually find such mixed super page entry case
to bring this fix in.

Not sure whether Jan still remember the history.

[1] https://lists.xenproject.org/archives/html/xen-devel/2020-07/msg01648.html
[2] https://lore.kernel.org/all/a4856c33-8bb0-4afa-cc71-3af4c229bc27@xxxxxxxx/

> ---
>  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;

Did you actually observe an impact w/o this fix? 

>      }
> 
>      switch ( type )
> --
> 2.36.1