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

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Mon, May 06, 2024 at 12:41:49PM +0200, Jan Beulich wrote:
> 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.

Good point, I will disable those initially, as I don't think there's a
need right now for foreign mapping in altp2m-s and nested p2ms.

> > +    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.

Oh, I've added that outer assert later, and then didn't remove this
comment.

> > +            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.

I see, thanks for spotting that.  Will change the logic to ensure the
index is not updated past the failed to remove entry.

> > +        }
> > +
> > +        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.

Yeah, it's difficult to strike a good balance, short of counting by
processed entries rather than using the gfn value.  I'm fine with
checking more often than required, as long as we always ensure that
progress is made on each function call.

Thanks, Roger.