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.
---
v4: Restore mistakenly dropped mfn_eq(mfn, INVALID_MFN).
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,17 @@ 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 &&
+ 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);
On 03.02.2022 14:57, Jan Beulich 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.
> ---
> v4: Restore mistakenly dropped mfn_eq(mfn, INVALID_MFN).
I think this was the only open item I needed to deal with. Any chance
I could get an ack or R-b here, please?
Thanks, Jan
> 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,17 @@ 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 &&
> + 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);
>
>
On Thu, Feb 24, 2022 at 8:10 AM Jan Beulich <jbeulich@suse.com> wrote: > > On 03.02.2022 14:57, Jan Beulich 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. > > --- > > v4: Restore mistakenly dropped mfn_eq(mfn, INVALID_MFN). > > I think this was the only open item I needed to deal with. Any chance > I could get an ack or R-b here, please? > > Thanks, Jan Reviewed-by: Tamas K Lengyel <tamas@tklengyel.com>
© 2016 - 2024 Red Hat, Inc.