| Message ID | 2e5c460b-9123-bfc3-d5c8-0922f7b2e667@suse.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | x86/mm: address observations made while working on XSA-388 | expand |
On Tue, Jan 4, 2022 at 4:48 AM Jan Beulich <jbeulich@suse.com> wrote: > > Prior to XSA-304 the only caller merely happened to not use any further > the order value that it passes into the function. Already then this was > a latent issue: The function really should, in the "get" case, hand back > the order the underlying mapping actually uses (or actually the smaller > of the two), such that (going forward) there wouldn't be any action on > unrelated mappings (in particular ones which did already diverge from > the host P2M). > > Similarly in the "propagate" case only the smaller of the two orders > should actually get used for creating the new entry, again to avoid > altering mappings which did already diverge from the host P2M. I don't really understand the reason why you want to return the page_order from the altp2m here. The only check that uses the page_order following is the super-page shattering check for XSA-304 but that's done on the hostp2m. So you would want to know what the page_order is on the hosp2m, not the altp2m, no? In either case, in all the setups we use altp2m we never use any superpages, the recommendation is to boot with hap_1gb=0 hap_2mb=0. I never trusted the complexity of superpage shattering and its implementation in Xen as it is very hard to follow. Tamas
On 04.01.2022 16:00, Tamas K Lengyel wrote: > On Tue, Jan 4, 2022 at 4:48 AM Jan Beulich <jbeulich@suse.com> wrote: >> >> Prior to XSA-304 the only caller merely happened to not use any further >> the order value that it passes into the function. Already then this was >> a latent issue: The function really should, in the "get" case, hand back >> the order the underlying mapping actually uses (or actually the smaller >> of the two), such that (going forward) there wouldn't be any action on >> unrelated mappings (in particular ones which did already diverge from >> the host P2M). >> >> Similarly in the "propagate" case only the smaller of the two orders >> should actually get used for creating the new entry, again to avoid >> altering mappings which did already diverge from the host P2M. > > I don't really understand the reason why you want to return the > page_order from the altp2m here. The only check that uses the > page_order following is the super-page shattering check for XSA-304 > but that's done on the hostp2m. So you would want to know what the > page_order is on the hosp2m, not the altp2m, no? From what I see I would say the XSA-304 action is on the altp2m, not the host one - the p2m_set_entry() invocation passes "p2m", which gets set immediately prior to the call to p2m_altp2m_get_or_propagate(). This is also what gets passed into the function. It's the host p2m only when !altp2m_active(currd). > In either case, in all the setups we use altp2m we never use any > superpages, the recommendation is to boot with hap_1gb=0 hap_2mb=0. I > never trusted the complexity of superpage shattering and its > implementation in Xen as it is very hard to follow. Hmm, interesting. If you're aware of bugs there, would you mind letting us know the details? There shouldn't be a need to use command line options to make altp2m actually work. If there's an incompatibility, and if we can't get that fixed, perhaps use of superpages would want suppressing when altp2m gets enabled for a guest? Right now, without this being enforced (nor spelled out anywhere in, say, a code comment), I don't think we should make assumptions like this. Hence the patch. Jan
On Tue, Jan 4, 2022 at 11:14 AM Jan Beulich <jbeulich@suse.com> wrote: > > On 04.01.2022 16:00, Tamas K Lengyel wrote: > > On Tue, Jan 4, 2022 at 4:48 AM Jan Beulich <jbeulich@suse.com> wrote: > >> > >> Prior to XSA-304 the only caller merely happened to not use any further > >> the order value that it passes into the function. Already then this was > >> a latent issue: The function really should, in the "get" case, hand back > >> the order the underlying mapping actually uses (or actually the smaller > >> of the two), such that (going forward) there wouldn't be any action on > >> unrelated mappings (in particular ones which did already diverge from > >> the host P2M). > >> > >> Similarly in the "propagate" case only the smaller of the two orders > >> should actually get used for creating the new entry, again to avoid > >> altering mappings which did already diverge from the host P2M. > > > > I don't really understand the reason why you want to return the > > page_order from the altp2m here. The only check that uses the > > page_order following is the super-page shattering check for XSA-304 > > but that's done on the hostp2m. So you would want to know what the > > page_order is on the hosp2m, not the altp2m, no? > > From what I see I would say the XSA-304 action is on the altp2m, > not the host one - the p2m_set_entry() invocation passes "p2m", > which gets set immediately prior to the call to > p2m_altp2m_get_or_propagate(). This is also what gets passed into > the function. It's the host p2m only when !altp2m_active(currd). Oh, yea, I see. OK, makes sense. Reviewed-by: Tamas K Lengyel <tamas@tklengyel.com> > > > In either case, in all the setups we use altp2m we never use any > > superpages, the recommendation is to boot with hap_1gb=0 hap_2mb=0. I > > never trusted the complexity of superpage shattering and its > > implementation in Xen as it is very hard to follow. > > Hmm, interesting. If you're aware of bugs there, would you mind > letting us know the details? There shouldn't be a need to use > command line options to make altp2m actually work. If there's an > incompatibility, and if we can't get that fixed, perhaps use of > superpages would want suppressing when altp2m gets enabled for a > guest? Right now, without this being enforced (nor spelled out > anywhere in, say, a code comment), I don't think we should make > assumptions like this. Hence the patch. I don't know of any bugs, it's just the complexity of page-shattering always had a "smell", thus I opted to recommend superpages being always disabled. With the added complexity of altp2m it's even more so the case. Tamas
--- a/xen/arch/x86/hvm/hvm.c +++ b/xen/arch/x86/hvm/hvm.c @@ -1832,7 +1832,7 @@ int hvm_hap_nested_page_fault(paddr_t gp * altp2m. */ if ( p2m_altp2m_get_or_propagate(p2m, gfn, &mfn, &p2mt, - &p2ma, page_order) ) + &p2ma, &page_order) ) { /* Entry was copied from host -- retry fault */ rc = 1; --- a/xen/arch/x86/mm/p2m.c +++ b/xen/arch/x86/mm/p2m.c @@ -2198,10 +2198,11 @@ bool_t p2m_switch_vcpu_altp2m_by_id(stru */ bool p2m_altp2m_get_or_propagate(struct p2m_domain *ap2m, unsigned long gfn_l, mfn_t *mfn, p2m_type_t *p2mt, - p2m_access_t *p2ma, unsigned int page_order) + p2m_access_t *p2ma, unsigned int *page_order) { p2m_type_t ap2mt; p2m_access_t ap2ma; + unsigned int cur_order; unsigned long mask; gfn_t gfn; mfn_t amfn; @@ -2214,7 +2215,10 @@ bool p2m_altp2m_get_or_propagate(struct */ p2m_lock(ap2m); - amfn = get_gfn_type_access(ap2m, gfn_l, &ap2mt, &ap2ma, 0, NULL); + amfn = get_gfn_type_access(ap2m, gfn_l, &ap2mt, &ap2ma, 0, &cur_order); + + if ( cur_order > *page_order ) + cur_order = *page_order; if ( !mfn_eq(amfn, INVALID_MFN) ) { @@ -2222,6 +2226,7 @@ bool p2m_altp2m_get_or_propagate(struct *mfn = amfn; *p2mt = ap2mt; *p2ma = ap2ma; + *page_order = cur_order; return false; } @@ -2229,6 +2234,7 @@ bool p2m_altp2m_get_or_propagate(struct if ( mfn_eq(*mfn, INVALID_MFN) ) { p2m_unlock(ap2m); + *page_order = cur_order; return false; } @@ -2237,11 +2243,11 @@ bool p2m_altp2m_get_or_propagate(struct * to the start of the superpage. NB that we repupose `amfn` * here. */ - mask = ~((1UL << page_order) - 1); + mask = ~((1UL << cur_order) - 1); amfn = _mfn(mfn_x(*mfn) & mask); gfn = _gfn(gfn_l & mask); - rc = p2m_set_entry(ap2m, gfn, amfn, page_order, *p2mt, *p2ma); + rc = p2m_set_entry(ap2m, gfn, amfn, cur_order, *p2mt, *p2ma); p2m_unlock(ap2m); if ( rc ) --- a/xen/arch/x86/include/asm/p2m.h +++ b/xen/arch/x86/include/asm/p2m.h @@ -852,7 +852,7 @@ void p2m_flush_altp2m(struct domain *d); /* Alternate p2m paging */ bool p2m_altp2m_get_or_propagate(struct p2m_domain *ap2m, unsigned long gfn_l, mfn_t *mfn, p2m_type_t *p2mt, - p2m_access_t *p2ma, unsigned int page_order); + p2m_access_t *p2ma, unsigned int *page_order); /* Make a specific alternate p2m valid */ int p2m_init_altp2m_by_id(struct domain *d, unsigned int idx);
Prior to XSA-304 the only caller merely happened to not use any further the order value that it passes into the function. Already then this was a latent issue: The function really should, in the "get" case, hand back the order the underlying mapping actually uses (or actually the smaller of the two), such that (going forward) there wouldn't be any action on unrelated mappings (in particular ones which did already diverge from the host P2M). Similarly in the "propagate" case only the smaller of the two orders should actually get used for creating the new entry, again to avoid altering mappings which did already diverge from the host P2M. Signed-off-by: Jan Beulich <jbeulich@suse.com>