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

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

> From: Roger Pau Monné <roger.pau@citrix.com>
> Sent: Wednesday, June 29, 2022 5:11 PM
> 
> On Wed, Jun 29, 2022 at 08:41:43AM +0000, Tian, Kevin wrote:
> > > From: Roger Pau Monne <roger.pau@citrix.com>
> > > 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@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.
> >
> > 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]
> 
> But the APIC access page is always added using set_mmio_p2m_entry(),
> which will unconditionally create an entry for it in the EPT, hence
> there's no explicit need to check for it's presence inside of higher
> order pages.
> 
> > 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@suse.com>
> > 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@suse.com>
> >     Acked-by: Kevin Tian <kevin.tian@intel.com>
> >     Reviewed-by: Tim Deegan <tim@xen.org>
> >
> > I suppose Jan may actually find such mixed super page entry case
> > to bring this fix in.
> 
> Hm, I guess theoretically it could be possible for contiguous entries
> to be coalesced (if we ever implement that for p2m page tables), and
> hence result in super pages being created from smaller entries.
> 
> It that case it would be less clear to assert that special pages
> cannot be coalesced with other contiguous entries.

With Jan's confirmation I'm fine with this change too. Just below...

> >
> > Did you actually observe an impact w/o this fix?
> 
> Yes, the original change has caused a performance regression in some
> GPU pass through workloads for XenServer.  I think it's reasonable to
> avoid super page shattering if the resulting pages would all end up
> using ipat and WRBACK cache attribute, as there's no reason for the
> split in the first case.
> 

... I'd appreciate mentioning the regression case in the commit msg.

With that,

Reviewed-by: Kevin Tian <kevin.tian@intel.com>