| Message ID | 7a80d08b-edd7-43c8-a7ce-42eb85d6f3be@suse.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | x86/P2M: allow 2M superpage use for shadowed guests | expand |
> On 9 Dec 2021, at 11:27, Jan Beulich <jbeulich@suse.com> wrote: > > For guests in shadow mode the P2M table gets used only by software. The > only place where it matters whether superpages in the P2M can be dealt > with is sh_unshadow_for_p2m_change(). It’s easy to verify that this patch is doing what it claims to do; but whether it’s correct or not depends on the veracity of this claim here. Rather than me having to duplicate whatever work you did to come to this conclusion, can you briefly explain why it’s true in a way that I can easily verify? e.g., all other accesses to the p2m in the shadow code are via get_gfn_[something](), which (because it’s in the p2m code) handles p2m superpages correctly? Everything else looks good here. > That function has been capabale of > handling them even before commit 0ca1669871f8a ("P2M: check whether hap > mode is enabled before using 2mb pages") disabled 2M use in this case > for dubious reasons ("potential errors when hap is disabled"). I’m glad the days of random patches being checked in without comment or discussion are behind us... -George
On 24.06.2022 21:27, George Dunlap wrote: > > >> On 9 Dec 2021, at 11:27, Jan Beulich <jbeulich@suse.com> wrote: >> >> For guests in shadow mode the P2M table gets used only by software. The >> only place where it matters whether superpages in the P2M can be dealt >> with is sh_unshadow_for_p2m_change(). > > It’s easy to verify that this patch is doing what it claims to do; but whether it’s correct or not depends on the veracity of this claim here. Rather than me having to duplicate whatever work you did to come to this conclusion, can you briefly explain why it’s true in a way that I can easily verify? Would "The table is never made accessible by hardware for address translation, and the only checks of _PAGE_PSE in P2M entries in shadow code are in this function (all others are against guest page table entries)." look sufficient to you? > e.g., all other accesses to the p2m in the shadow code are via get_gfn_[something](), which (because it’s in the p2m code) handles p2m superpages correctly? Well, yes - I don't think I need to reason about generic P2M code being super-page aware? Jan
> On 27 Jun 2022, at 16:33, Jan Beulich <jbeulich@suse.com> wrote: > > On 24.06.2022 21:27, George Dunlap wrote: >> >> >>> On 9 Dec 2021, at 11:27, Jan Beulich <jbeulich@suse.com> wrote: >>> >>> For guests in shadow mode the P2M table gets used only by software. The >>> only place where it matters whether superpages in the P2M can be dealt >>> with is sh_unshadow_for_p2m_change(). >> >> It’s easy to verify that this patch is doing what it claims to do; but whether it’s correct or not depends on the veracity of this claim here. Rather than me having to duplicate whatever work you did to come to this conclusion, can you briefly explain why it’s true in a way that I can easily verify? > > Would > > "The table is never made accessible by hardware for address translation, > and the only checks of _PAGE_PSE in P2M entries in shadow code are in > this function (all others are against guest page table entries)." > > look sufficient to you? Sorry for the delay responding to this — yes, I think this would do, thanks. -George
--- a/xen/arch/x86/mm/p2m.c +++ b/xen/arch/x86/mm/p2m.c @@ -631,28 +631,22 @@ struct page_info *p2m_get_page_from_gfn( int p2m_set_entry(struct p2m_domain *p2m, gfn_t gfn, mfn_t mfn, unsigned int page_order, p2m_type_t p2mt, p2m_access_t p2ma) { - struct domain *d = p2m->domain; + bool hap = hap_enabled(p2m->domain); unsigned long todo = 1ul << page_order; - unsigned int order; int set_rc, rc = 0; ASSERT(gfn_locked_by_me(p2m, gfn)); while ( todo ) { - if ( hap_enabled(d) ) - { - unsigned long fn_mask = !mfn_eq(mfn, INVALID_MFN) ? mfn_x(mfn) : 0; - - fn_mask |= gfn_x(gfn) | todo; - - order = (!(fn_mask & ((1ul << PAGE_ORDER_1G) - 1)) && - hap_has_1gb) ? PAGE_ORDER_1G : - (!(fn_mask & ((1ul << PAGE_ORDER_2M) - 1)) && - hap_has_2mb) ? PAGE_ORDER_2M : PAGE_ORDER_4K; - } - else - order = 0; + unsigned long fn_mask = (!mfn_eq(mfn, INVALID_MFN) ? mfn_x(mfn) : 0) | + gfn_x(gfn) | todo; + unsigned int order = (!(fn_mask & ((1ul << PAGE_ORDER_1G) - 1)) && + hap && hap_has_1gb) + ? PAGE_ORDER_1G + : (!(fn_mask & ((1ul << PAGE_ORDER_2M) - 1)) && + (!hap || hap_has_2mb)) + ? PAGE_ORDER_2M : PAGE_ORDER_4K; set_rc = p2m->set_entry(p2m, gfn, mfn, order, p2mt, p2ma, -1); if ( set_rc )
For guests in shadow mode the P2M table gets used only by software. The only place where it matters whether superpages in the P2M can be dealt with is sh_unshadow_for_p2m_change(). That function has been capabale of handling them even before commit 0ca1669871f8a ("P2M: check whether hap mode is enabled before using 2mb pages") disabled 2M use in this case for dubious reasons ("potential errors when hap is disabled"). While doing this, move "order" into more narrow scope and replace the local variable "d" by a new "hap" one. Signed-off-by: Jan Beulich <jbeulich@suse.com> --- Strictly speaking "fn_mask" could also be "unsigned int"; I wasn't sure whether changing that would cause objections. While at least sh_unshadow_for_p2m_change() presently relies on this behavior, it is somewhat odd (and inefficient) for p2m_set_entry() to split even non-present mappings.