On 15/05/2020 16:15, Jan Beulich wrote:
>>> + domain_crash(d);
> This already leaves a file/line combination as a (minimal hint).
First, that is still tantamount to useless in logs from a user.
Second, the use of __LINE__ is why it breaks livepatching, and people
using livepatching is still carrying an out-of-tree patch to unbreak it.
> I can make a patch to add a gprintk() as you ask for, but I'm not
> sure it's worth it for this almost dead code.
"page in unexpected state" would be better than nothing, but given the
comment, it might also be better as ASSERT_UNREACHABLE(), and we now
have a lot of cases where we declare unreachable, and kill the domain in
release builds.
>
>>> @@ -1843,13 +1852,24 @@ int p2m_mem_paging_prep(struct domain *d
>>> ret = p2m_set_entry(p2m, gfn, mfn, PAGE_ORDER_4K,
>>> paging_mode_log_dirty(d) ? p2m_ram_logdirty
>>> : p2m_ram_rw, a);
>>> - set_gpfn_from_mfn(mfn_x(mfn), gfn_l);
>>> + if ( !ret )
>>> + {
>>> + set_gpfn_from_mfn(mfn_x(mfn), gfn_l);
>>>
>>> - if ( !page_extant )
>>> - atomic_dec(&d->paged_pages);
>>> + if ( !page_extant )
>>> + atomic_dec(&d->paged_pages);
>>> + }
>>>
>>> out:
>>> gfn_unlock(p2m, gfn, 0);
>>> +
>>> + if ( page )
>>> + {
>>> + if ( ret )
>>> + put_page_alloc_ref(page);
>>> + put_page(page);
>> This is a very long way from clear enough to follow, and buggy if anyone
>> inserts a new goto out path.
> What alternatives do you see?
/* Fully free the page on error. Drop our temporary reference in all
cases. */
would at least help someone trying to figure out what is going on here,
especially as put_page_alloc_ref() is not the obvious freeing function
for alloc_domheap_page().
~Andrew
