> From: Roger Pau Monné <roger....@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....@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....@citrix.com> > > > --- > > > Cc: Paul Durrant <p...@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 <jbeul...@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 <jbeul...@suse.com> > > Acked-by: Kevin Tian <kevin.t...@intel.com> > > Reviewed-by: Tim Deegan <t...@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.t...@intel.com>