| Message ID | 2c421077-81c2-a45e-f7c3-9827d3cb1abf@suse.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v3] x86/altp2m: p2m_altp2m_propagate_change() should honor present page order | expand |
On Thu, Jan 27, 2022 at 10:07 AM Jan Beulich <jbeulich@suse.com> wrote: > > For higher order mappings the comparison against p2m->min_remapped_gfn > needs to take the upper bound of the covered GFN range into account, not > just the base GFN. Otherwise, i.e. when dropping a mapping overlapping > the remapped range but the base GFN outside of that range, an altp2m may > wrongly not get reset. > > Note that there's no need to call get_gfn_type_access() ahead of the > check against the remapped range boundaries: None of its outputs are > needed earlier, and p2m_reset_altp2m() doesn't require the lock to be > held. In fact this avoids a latent lock order violation: With per-GFN > locking p2m_reset_altp2m() not only doesn't require the GFN lock to be > held, but holding such a lock would actually not be allowed, as the > function acquires a P2M lock. > > Local variables are moved into the more narrow scope (one is deleted > altogether) to help see their actual life ranges. > > Signed-off-by: Jan Beulich <jbeulich@suse.com> > --- > Note that this addresses only half of the problem: get_gfn_type_access() > would also need invoking for all of the involved GFNs, not just the 1st > one. > --- > v3: Don't pass the minimum of both orders to p2m_set_entry() (as was the > case in the original code). Restore get_gfn_type_access() return > value checking. > > --- a/xen/arch/x86/mm/p2m.c > +++ b/xen/arch/x86/mm/p2m.c > @@ -2534,9 +2534,6 @@ int p2m_altp2m_propagate_change(struct d > p2m_type_t p2mt, p2m_access_t p2ma) > { > struct p2m_domain *p2m; > - p2m_access_t a; > - p2m_type_t t; > - mfn_t m; > unsigned int i; > unsigned int reset_count = 0; > unsigned int last_reset_idx = ~0; > @@ -2549,15 +2546,16 @@ int p2m_altp2m_propagate_change(struct d > > for ( i = 0; i < MAX_ALTP2M; i++ ) > { > + p2m_type_t t; > + p2m_access_t a; > + > if ( d->arch.altp2m_eptp[i] == mfn_x(INVALID_MFN) ) > continue; > > p2m = d->arch.altp2m_p2m[i]; > - m = get_gfn_type_access(p2m, gfn_x(gfn), &t, &a, 0, NULL); > > /* Check for a dropped page that may impact this altp2m */ > - if ( mfn_eq(mfn, INVALID_MFN) && > - gfn_x(gfn) >= p2m->min_remapped_gfn && > + if ( gfn_x(gfn) + (1UL << page_order) > p2m->min_remapped_gfn && > gfn_x(gfn) <= p2m->max_remapped_gfn ) Why are you dropping the mfn_eq(mfn, INVALID_MFN) check here? Tamas
On 28.01.2022 17:49, Tamas K Lengyel wrote: > On Thu, Jan 27, 2022 at 10:07 AM Jan Beulich <jbeulich@suse.com> wrote: >> @@ -2549,15 +2546,16 @@ int p2m_altp2m_propagate_change(struct d >> >> for ( i = 0; i < MAX_ALTP2M; i++ ) >> { >> + p2m_type_t t; >> + p2m_access_t a; >> + >> if ( d->arch.altp2m_eptp[i] == mfn_x(INVALID_MFN) ) >> continue; >> >> p2m = d->arch.altp2m_p2m[i]; >> - m = get_gfn_type_access(p2m, gfn_x(gfn), &t, &a, 0, NULL); >> >> /* Check for a dropped page that may impact this altp2m */ >> - if ( mfn_eq(mfn, INVALID_MFN) && >> - gfn_x(gfn) >= p2m->min_remapped_gfn && >> + if ( gfn_x(gfn) + (1UL << page_order) > p2m->min_remapped_gfn && >> gfn_x(gfn) <= p2m->max_remapped_gfn ) > > Why are you dropping the mfn_eq(mfn, INVALID_MFN) check here? Thanks for spotting - this is a mistake in the v3 rework. Jan
--- a/xen/arch/x86/mm/p2m.c +++ b/xen/arch/x86/mm/p2m.c @@ -2534,9 +2534,6 @@ int p2m_altp2m_propagate_change(struct d p2m_type_t p2mt, p2m_access_t p2ma) { struct p2m_domain *p2m; - p2m_access_t a; - p2m_type_t t; - mfn_t m; unsigned int i; unsigned int reset_count = 0; unsigned int last_reset_idx = ~0; @@ -2549,15 +2546,16 @@ int p2m_altp2m_propagate_change(struct d for ( i = 0; i < MAX_ALTP2M; i++ ) { + p2m_type_t t; + p2m_access_t a; + if ( d->arch.altp2m_eptp[i] == mfn_x(INVALID_MFN) ) continue; p2m = d->arch.altp2m_p2m[i]; - m = get_gfn_type_access(p2m, gfn_x(gfn), &t, &a, 0, NULL); /* Check for a dropped page that may impact this altp2m */ - if ( mfn_eq(mfn, INVALID_MFN) && - gfn_x(gfn) >= p2m->min_remapped_gfn && + if ( gfn_x(gfn) + (1UL << page_order) > p2m->min_remapped_gfn && gfn_x(gfn) <= p2m->max_remapped_gfn ) { if ( !reset_count++ ) @@ -2568,8 +2566,6 @@ int p2m_altp2m_propagate_change(struct d else { /* At least 2 altp2m's impacted, so reset everything */ - __put_gfn(p2m, gfn_x(gfn)); - for ( i = 0; i < MAX_ALTP2M; i++ ) { if ( i == last_reset_idx || @@ -2583,16 +2579,19 @@ int p2m_altp2m_propagate_change(struct d break; } } - else if ( !mfn_eq(m, INVALID_MFN) ) + else if ( !mfn_eq(get_gfn_type_access(p2m, gfn_x(gfn), &t, &a, 0, + NULL), INVALID_MFN) ) { int rc = p2m_set_entry(p2m, gfn, mfn, page_order, p2mt, p2ma); /* Best effort: Don't bail on error. */ if ( !ret ) ret = rc; - } - __put_gfn(p2m, gfn_x(gfn)); + __put_gfn(p2m, gfn_x(gfn)); + } + else + __put_gfn(p2m, gfn_x(gfn)); } altp2m_list_unlock(d);
For higher order mappings the comparison against p2m->min_remapped_gfn needs to take the upper bound of the covered GFN range into account, not just the base GFN. Otherwise, i.e. when dropping a mapping overlapping the remapped range but the base GFN outside of that range, an altp2m may wrongly not get reset. Note that there's no need to call get_gfn_type_access() ahead of the check against the remapped range boundaries: None of its outputs are needed earlier, and p2m_reset_altp2m() doesn't require the lock to be held. In fact this avoids a latent lock order violation: With per-GFN locking p2m_reset_altp2m() not only doesn't require the GFN lock to be held, but holding such a lock would actually not be allowed, as the function acquires a P2M lock. Local variables are moved into the more narrow scope (one is deleted altogether) to help see their actual life ranges. Signed-off-by: Jan Beulich <jbeulich@suse.com> --- Note that this addresses only half of the problem: get_gfn_type_access() would also need invoking for all of the involved GFNs, not just the 1st one. --- v3: Don't pass the minimum of both orders to p2m_set_entry() (as was the case in the original code). Restore get_gfn_type_access() return value checking.