Re: [PATCH for-4.19? 2/2] xen/x86: remove foreign mappings from the p2m on teardown

[Jan Beulich <jbeulich@xxxxxxxx>]

On 30.04.2024 18:58, Roger Pau Monne wrote:
> @@ -2695,6 +2691,70 @@ int p2m_set_altp2m_view_visibility(struct domain *d, 
> unsigned int altp2m_idx,
>      return rc;
>  }
>  
> +/*
> + * Remove foreign mappings from the p2m, as that drops the page reference 
> taken
> + * when mapped.
> + */
> +int relinquish_p2m_mapping(struct domain *d)
> +{
> +    struct p2m_domain *p2m = p2m_get_hostp2m(d);

Are there any guarantees made anywhere that altp2m-s and nested P2Ms can't
hold foreign mappings? p2m_entry_modify() certainly treats them all the same.

> +    unsigned long gfn = gfn_x(p2m->max_gfn);
> +    int rc = 0;
> +
> +    if ( !paging_mode_translate(d) )
> +        return 0;
> +
> +    BUG_ON(!d->is_dying);
> +
> +    p2m_lock(p2m);
> +
> +    /* Iterate over the whole p2m on debug builds to ensure correctness. */
> +    while ( gfn && (IS_ENABLED(CONFIG_DEBUG) || p2m->nr_foreign) )
> +    {
> +        unsigned int order;
> +        p2m_type_t t;
> +        p2m_access_t a;
> +
> +        _get_gfn_type_access(p2m, _gfn(gfn - 1), &t, &a, 0, &order, 0);
> +        ASSERT(IS_ALIGNED(gfn, 1u << order));

This heavily relies on the sole place where max_gfn is updated being indeed
sufficient.

> +        gfn -= 1 << order;

Please be consistent with the kind of 1 you shift left. Perhaps anyway both
better as 1UL.

> +        if ( t == p2m_map_foreign )
> +        {
> +            ASSERT(p2m->nr_foreign);
> +            ASSERT(order == 0);
> +            /*
> +             * Foreign mappings can only be of order 0, hence there's no need
> +             * to align the gfn to the entry order.  Otherwise we would need 
> to
> +             * adjust gfn to point to the start of the page if order > 0.
> +             */

I'm a little irritated by this comment. Ahead of the enclosing if() you
already rely on (and assert) GFN being suitably aligned.

> +            rc = p2m_set_entry(p2m, _gfn(gfn), INVALID_MFN, order, 
> p2m_invalid,
> +                               p2m->default_access);
> +            if ( rc )
> +            {
> +                printk(XENLOG_ERR
> +                       "%pd: failed to unmap foreign page %" PRI_gfn " order 
> %u error %d\n",
> +                       d, gfn, order, rc);
> +                ASSERT_UNREACHABLE();
> +                break;
> +            }

Together with the updating of ->max_gfn further down, for a release build
this means: A single attempt to clean up the domain would fail when such a
set-entry fails. However, another attempt to clean up despite the earlier
error would then not retry for the failed GFN, but continue one below.
That's unexpected: I'd either see such a domain remain as a zombie forever,
or a best effort continuation of all cleanup right away.

> +        }
> +
> +        if ( !(gfn & 0xfff) && hypercall_preempt_check() )

By going from gfn's low bits you may check way more often than necessary
when encountering large pages.

Jan