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

[Oleksandr <olekstysh@gmail.com>]

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