Re: [PATCH v3] x86/altp2m: p2m_altp2m_propagate_change() should honor present page order

[Tamas K Lengyel <tamas@xxxxxxxxxxxxx>]

On Thu, Jan 27, 2022 at 10:07 AM Jan Beulich <jbeulich@xxxxxxxx> 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@xxxxxxxx>
> ---
> 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