From: "Tian, Kevin" <kevin.tian@intel.com>
To: Roger Pau Monne <roger.pau@citrix.com>,
"xen-devel@lists.xenproject.org" <xen-devel@lists.xenproject.org>
Cc: "Nakajima, Jun" <jun.nakajima@intel.com>,
Jan Beulich <jbeulich@suse.com>,
"Cooper, Andrew" <andrew.cooper3@citrix.com>,
Wei Liu <wl@xen.org>, "George Dunlap" <george.dunlap@citrix.com>
Subject: RE: [PATCH 3/3] x86/ept: force WB cache attributes for grant and foreign maps
Date: Thu, 17 Jun 2021 09:31:28 +0000 [thread overview]
Message-ID: <MWHPR11MB188653F8277F861018DB00118C0E9@MWHPR11MB1886.namprd11.prod.outlook.com> (raw)
In-Reply-To: <20210528173935.29919-4-roger.pau@citrix.com>
> From: Roger Pau Monne <roger.pau@citrix.com>
> Sent: Saturday, May 29, 2021 1:40 AM
>
> Force WB type for grants and foreign pages. Those are usually mapped
> over unpopulated physical ranges in the p2m, and those ranges would
> usually be UC in the MTRR state, which is unlikely to be the correct
> cache attribute. It's also cumbersome (or even impossible) for the
> guest to be setting the MTRR type for all those mappings as WB, as
> MTRR ranges are finite.
>
> Note that on AMD we cannot force a cache attribute because of the lack
> of ignore PAT equivalent, so the behavior here slightly diverges
> between AMD and Intel (or EPT vs NPT/shadow).
>
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
Reviewed-by: Kevin Tian <kevin.tian@intel.com>
btw incorrect cache attribute brings functional/performance problem.
it'd be good to explain a bit why this problem doesn't hurt AMD in the
commit msg...
> ---
> xen/arch/x86/hvm/vmx/vmx.c | 2 +-
> xen/arch/x86/mm/p2m-ept.c | 35 ++++++++++++++++++++++++++-----
> xen/include/asm-x86/hvm/vmx/vmx.h | 2 +-
> 3 files changed, 32 insertions(+), 7 deletions(-)
>
> diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c
> index 0d4b47681b..e09b7e3af9 100644
> --- a/xen/arch/x86/hvm/vmx/vmx.c
> +++ b/xen/arch/x86/hvm/vmx/vmx.c
> @@ -423,7 +423,7 @@ static void domain_creation_finished(struct domain
> *d)
> return;
>
> ASSERT(epte_get_entry_emt(d, gfn, apic_access_mfn, 0, &ipat,
> - true) == MTRR_TYPE_WRBACK);
> + p2m_mmio_direct) == MTRR_TYPE_WRBACK);
> ASSERT(ipat);
>
> if ( set_mmio_p2m_entry(d, gfn, apic_access_mfn, PAGE_ORDER_4K) )
> diff --git a/xen/arch/x86/mm/p2m-ept.c b/xen/arch/x86/mm/p2m-ept.c
> index f1d1d07e92..59c0325473 100644
> --- a/xen/arch/x86/mm/p2m-ept.c
> +++ b/xen/arch/x86/mm/p2m-ept.c
> @@ -487,11 +487,12 @@ static int ept_invalidate_emt_range(struct
> p2m_domain *p2m,
> }
>
> int epte_get_entry_emt(struct domain *d, gfn_t gfn, mfn_t mfn,
> - unsigned int order, bool *ipat, bool direct_mmio)
> + unsigned int order, bool *ipat, p2m_type_t type)
> {
> int gmtrr_mtype, hmtrr_mtype;
> struct vcpu *v = current;
> unsigned long i;
> + bool direct_mmio = type == p2m_mmio_direct;
>
> *ipat = false;
>
> @@ -535,9 +536,33 @@ int epte_get_entry_emt(struct domain *d, gfn_t gfn,
> mfn_t mfn,
> }
> }
>
> - if ( direct_mmio )
> + switch ( type )
> + {
> + case p2m_mmio_direct:
> return MTRR_TYPE_UNCACHABLE;
>
> + case p2m_grant_map_ro:
> + case p2m_grant_map_rw:
> + case p2m_map_foreign:
> + /*
> + * Force WB type for grants and foreign pages. Those are usually
> mapped
> + * over unpopulated physical ranges in the p2m, and those would
> usually
> + * be UC in the MTRR state, which is unlikely to be the correct cache
> + * attribute. It's also cumbersome (or even impossible) for the guest
> + * to be setting the MTRR type for all those mappings as WB, as MTRR
> + * ranges are finite.
> + *
> + * Note that on AMD we cannot force a cache attribute because of the
> + * lack of ignore PAT equivalent, so the behavior here slightly
> + * diverges. See p2m_type_to_flags for the AMD attributes.
> + */
> + *ipat = true;
> + return MTRR_TYPE_WRBACK;
> +
> + default:
> + break;
> + }
> +
> gmtrr_mtype = hvm_get_mem_pinned_cacheattr(d, gfn, order);
> if ( gmtrr_mtype >= 0 )
> {
> @@ -640,7 +665,7 @@ static int resolve_misconfig(struct p2m_domain
> *p2m, unsigned long gfn)
> continue;
> e.emt = epte_get_entry_emt(p2m->domain, _gfn(gfn + i),
> _mfn(e.mfn), 0, &ipat,
> - e.sa_p2mt == p2m_mmio_direct);
> + e.sa_p2mt);
> e.ipat = ipat;
>
> nt = p2m_recalc_type(e.recalc, e.sa_p2mt, p2m, gfn + i);
> @@ -659,7 +684,7 @@ static int resolve_misconfig(struct p2m_domain
> *p2m, unsigned long gfn)
> int emt = epte_get_entry_emt(p2m->domain, _gfn(gfn),
> _mfn(e.mfn),
> level * EPT_TABLE_ORDER, &ipat,
> - e.sa_p2mt == p2m_mmio_direct);
> + e.sa_p2mt);
> bool_t recalc = e.recalc;
>
> if ( recalc && p2m_is_changeable(e.sa_p2mt) )
> @@ -895,7 +920,7 @@ ept_set_entry(struct p2m_domain *p2m, gfn_t gfn_,
> mfn_t mfn,
> bool ipat;
> int emt = epte_get_entry_emt(p2m->domain, _gfn(gfn), mfn,
> i * EPT_TABLE_ORDER, &ipat,
> - p2mt == p2m_mmio_direct);
> + p2mt);
>
> if ( emt >= 0 )
> new_entry.emt = emt;
> diff --git a/xen/include/asm-x86/hvm/vmx/vmx.h b/xen/include/asm-
> x86/hvm/vmx/vmx.h
> index f668ee1f09..0deb507490 100644
> --- a/xen/include/asm-x86/hvm/vmx/vmx.h
> +++ b/xen/include/asm-x86/hvm/vmx/vmx.h
> @@ -600,7 +600,7 @@ void ept_p2m_uninit(struct p2m_domain *p2m);
> void ept_walk_table(struct domain *d, unsigned long gfn);
> bool_t ept_handle_misconfig(uint64_t gpa);
> int epte_get_entry_emt(struct domain *d, gfn_t gfn, mfn_t mfn,
> - unsigned int order, bool *ipat, bool direct_mmio);
> + unsigned int order, bool *ipat, p2m_type_t type);
> void setup_ept_dump(void);
> void p2m_init_altp2m_ept(struct domain *d, unsigned int i);
> /* Locate an alternate p2m by its EPTP */
> --
> 2.31.1
next prev parent reply other threads:[~2021-06-17 9:31 UTC|newest]
Thread overview: 13+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-05-28 17:39 [PATCH 0/3] x86/ept: force WB to foreign and grant mappings Roger Pau Monne
2021-05-28 17:39 ` [PATCH 1/3] x86/mtrr: remove stale function prototype Roger Pau Monne
2021-05-31 6:48 ` Jan Beulich
2021-05-28 17:39 ` [PATCH 2/3] x86/mtrr: move epte_get_entry_emt to p2m-ept.c Roger Pau Monne
2021-05-31 7:00 ` Jan Beulich
2021-06-17 9:15 ` Tian, Kevin
2021-05-28 17:39 ` [PATCH 3/3] x86/ept: force WB cache attributes for grant and foreign maps Roger Pau Monne
2021-05-31 7:21 ` Jan Beulich
2021-06-02 9:36 ` Roger Pau Monné
2021-06-17 9:31 ` Tian, Kevin [this message]
2021-06-17 11:40 ` Roger Pau Monné
2021-06-17 11:57 ` Jan Beulich
2021-06-15 13:37 ` [PATCH 0/3] x86/ept: force WB to foreign and grant mappings Roger Pau Monné
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=MWHPR11MB188653F8277F861018DB00118C0E9@MWHPR11MB1886.namprd11.prod.outlook.com \
--to=kevin.tian@intel.com \
--cc=andrew.cooper3@citrix.com \
--cc=george.dunlap@citrix.com \
--cc=jbeulich@suse.com \
--cc=jun.nakajima@intel.com \
--cc=roger.pau@citrix.com \
--cc=wl@xen.org \
--cc=xen-devel@lists.xenproject.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).