Re: [PATCH V2 16/23] xen/mm: Handle properly reference in set_foreign_p2m_entry() on Arm

[Oleksandr <olekstysh@xxxxxxxxx>]

On 12.11.20 14:18, Jan Beulich wrote:

Hi Jan

> On 15.10.2020 18:44, Oleksandr Tyshchenko wrote:
> > --- a/xen/arch/arm/p2m.c
> > +++ b/xen/arch/arm/p2m.c
> > @@ -1380,6 +1380,27 @@ int guest_physmap_remove_page(struct domain *d, gfn_t 
> > gfn, mfn_t mfn,
> >       return p2m_remove_mapping(d, gfn, (1 << page_order), mfn);
> >   }
> >   
> > +int set_foreign_p2m_entry(struct domain *d, const struct domain *fd,
> > +                          unsigned long gfn, mfn_t mfn)
> > +{
> > +    struct page_info *page = mfn_to_page(mfn);
> > +    int rc;
> > +
> > +    if ( !get_page(page, fd) )
> > +        return -EINVAL;
> > +
> > +    /*
> > +     * It is valid to always use p2m_map_foreign_rw here as if this gets
> > +     * called that d != fd. A case when d == fd would be rejected by
> > +     * rcu_lock_remote_domain_by_id() earlier.
> > +     */
> Are you describing things here on the assumption that no new
> callers may surface later on? To catch such, I'd recommend
> adding at least a respective ASSERT().
Agree, will add.


> > +    rc = guest_physmap_add_entry(d, _gfn(gfn), mfn, 0, p2m_map_foreign_rw);
> > +    if ( rc )
> > +        put_page(page);
> > +
> > +    return 0;
> I can't imagine it's correct to not signal the error to the
> caller(s).
It is not correct, I have just missed to place return rc. Thank you for 
the catch.

> > --- a/xen/common/memory.c
> > +++ b/xen/common/memory.c
> > @@ -1099,7 +1099,8 @@ static int acquire_resource(
> >        *        reference counted, it is unsafe to allow mapping of
> >        *        resource pages unless the caller is the hardware domain.
> >        */
> > -    if ( paging_mode_translate(currd) && !is_hardware_domain(currd) )
> > +    if ( paging_mode_translate(currd) && !is_hardware_domain(currd) &&
> > +         !arch_refcounts_p2m() )
> >           return -EACCES;
> I guess the hook may want naming differently, as both prior parts of
> the condition should be needed only on the x86 side, and there (for
> PV) there's no p2m involved in the refcounting. Maybe
> arch_acquire_resource_check()? And then the comment wants moving into
> the x86 hook as well. You may want to consider leaving a more generic
> one here...
ok, again, I am fine with the name). Will follow everything suggested 
above.
--
Regards,

Oleksandr Tyshchenko