On 2/18/19 11:01 AM, Roger Pau Monné wrote:
> On Fri, Feb 15, 2019 at 06:48:25PM +0100, George Dunlap wrote:
>>
>>> On Feb 15, 2019, at 2:18 PM, Roger Pau Monne <roger....@citrix.com> wrote:
>>>
>>> This is in preparation for also changing p2m_entry_modify to return an
>>> error code.
>>>
>>> No functional change intended.
>>
>> I think you need to explain wheny/why you’re using BUG_ON() rather than
>> ASSERT() or passing the caller up the stack.
>>
>>
>> Just in general:
>>
>> * Passing things up the stack should be used when the caller is already
>> expecting to handle errors, and the state when the error was discovered
>> isn’t broken, or too hard to fix.
>>
>> * BUG_ON() should be used when you can’t pass things up the stack, and
>> continuing would certainly cause a vulnerability.
>>
>> * ASSERT() should be used when continuing might work, or might have an
>> effect later whose badness is equal or less than that of a host crash; OR
>> whose truth can be clearly observed from the code directly surrounding it.
>>
>> For instance...
>>
>>> diff --git a/xen/arch/x86/mm/p2m-pt.c b/xen/arch/x86/mm/p2m-pt.c
>>> index 04e9d81cf6..44abd65999 100644
>>> --- a/xen/arch/x86/mm/p2m-pt.c
>>> +++ b/xen/arch/x86/mm/p2m-pt.c
>>> @@ -202,7 +202,7 @@ p2m_next_level(struct p2m_domain *p2m, void **table,
>>> new_entry = l1e_from_mfn(mfn, P2M_BASE_FLAGS | _PAGE_RW);
>>>
>>> p2m_add_iommu_flags(&new_entry, level,
>>> IOMMUF_readable|IOMMUF_writable);
>>> - p2m->write_p2m_entry(p2m, gfn, p2m_entry, new_entry, level + 1);
>>> + BUG_ON(p2m->write_p2m_entry(p2m, gfn, p2m_entry, new_entry, level
>>> + 1));
>>
>> In this case, a few lines above we have `return -ENOMEM`; so:
>> 1. The caller is expecting to handle error values, and
>> 2. There’s no unusual state to try to clean up.
>
> I'm not that familiar with the p2m code, but there's a call to
> p2m_alloc_ptp just further up, and while I think failing here would
> only imply that a page has been added to p2m->pages but has not
> actually been used in the p2m page tables because the addition of the
> entry failed.
>
> I think adding an entry that expands the page table structure should
> never fail, and hence I think the following:
>
> rc = p2m->write_p2m_entry(p2m, gfn, p2m_entry, new_entry, level + 1);
> if ( rc )
> {
> ASSERT_UNREACHABLE();
> return rc;
> }
>
> Would be the best way to handle the return code from write_p2m_entry
> in p2m_next_level.
That works for me.
>>> }
>>> else
>>> ASSERT(flags & _PAGE_PRESENT);
>>> @@ -321,7 +321,7 @@ static int p2m_pt_set_recalc_range(struct p2m_domain
>>> *p2m,
>>> if ( (l1e_get_flags(e) & _PAGE_PRESENT) && !needs_recalc(l1, e)
>>> )
>>> {
>>> set_recalc(l1, e);
>>> - p2m->write_p2m_entry(p2m, first_gfn, pent, e, level);
>>> + BUG_ON(p2m->write_p2m_entry(p2m, first_gfn, pent, e,
>>> level));
>>> }
>>
>> Again here; theoretically, the only change has been that RECALC_FLAGS have
>> been added.
>>
>> And so on.
>>
>> Thoughts? (Looking for input from Jan here as well.)
>
> As you say here and above, the only caller that's expected to fail is
> p2m_pt_set_entry, the rest just do recalc or add intermediate entries
> to the p2m, which must always succeed.
>
>> If not, it seems like we should also be modifying the places in p2m-ept.c to
>> do BUG_ON() rather than ASSERT()’ing that the page write succeeded.
>
> I can apply the same treatment that's done in p2m-ept.c, so that we
> have consistency between both implementations.
Sounds good.
Thanks!
-George
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
