On 2/18/19 11:05 AM, Roger Pau Monné wrote:
> On Fri, Feb 15, 2019 at 07:15:53PM +0100, George Dunlap wrote:
>>
>>
>>> On Feb 15, 2019, at 2:18 PM, Roger Pau Monne <roger....@citrix.com> wrote:
>>>
>>> So that the specific handling can be removed from
>>> atomic_write_ept_entry and be shared with npt and shadow code.
>>>
>>> This commit also removes the check that prevent non-ept PVH dom0 from
>>> mapping foreign pages.
>>>
>>> Signed-off-by: Roger Pau Monné <roger....@citrix.com>
>>
>> Mostly looks good; just a few comments...
>>>
>>> diff --git a/xen/include/asm-x86/p2m.h b/xen/include/asm-x86/p2m.h
>>> index f4ec2becbd..1687b31571 100644
>>> --- a/xen/include/asm-x86/p2m.h
>>> +++ b/xen/include/asm-x86/p2m.h
>>> @@ -932,11 +932,14 @@ int p2m_set_ioreq_server(struct domain *d, unsigned
>>> int flags,
>>> struct hvm_ioreq_server *p2m_get_ioreq_server(struct domain *d,
>>> unsigned int *flags);
>>>
>>> -static inline void p2m_entry_modify(struct p2m_domain *p2m, p2m_type_t nt,
>>> - p2m_type_t ot, unsigned int level)
>>> +static inline int p2m_entry_modify(struct p2m_domain *p2m, p2m_type_t nt,
>>> + p2m_type_t ot, mfn_t nfn, mfn_t ofn,
>>> + unsigned int level)
>>> {
>>> - if ( level != 1 || nt == ot )
>>> - return;
>>> + BUG_ON(level > 1 && (nt == p2m_ioreq_server || nt == p2m_map_foreign));
>>> +
>>> + if ( level != 1 || (nt == ot && mfn_eq(nfn, ofn)) )
>>> + return 0;
>>>
>>> switch ( nt )
>>> {
>>> @@ -948,6 +951,14 @@ static inline void p2m_entry_modify(struct p2m_domain
>>> *p2m, p2m_type_t nt,
>>> p2m->ioreq.entry_count++;
>>> break;
>>>
>>> + case p2m_map_foreign:
>>> + BUG_ON(!mfn_valid(nfn));
>>
>> Since we’re going to be returning errors anyway, why not retain the original
>> functionality of returning -EINVAL in this case, rather than BUG_ON?
>
> Ack. I added the BUG_ON because trying to add a foreign entry with an
> invalid mfn means something else went wrong in the caller, since it
> should not be possible to perform such action. As you pointed out
> however there's no reason to panic, since an error can be returned to
> the caller.
>
> Would you be fine with also adding an ASSERT_UNREACHABLE before
> returning the error?
>
>>> +
>>> + if ( !page_get_owner_and_reference(mfn_to_page(nfn)) )
>>> + return -EBUSY;
>>> +
>>> + break;
>>> +
>>> default:
>>> break;
>>> }
>>> @@ -959,9 +970,16 @@ static inline void p2m_entry_modify(struct p2m_domain
>>> *p2m, p2m_type_t nt,
>>> p2m->ioreq.entry_count--;
>>> break;
>>>
>>> + case p2m_map_foreign:
>>> + BUG_ON(!mfn_valid(ofn));
>>
>> If somehow this happened, then the bug isn’t here but somewhere else;
>> continuing on won’t make things any worse than they would be if this page
>> weren’t removed. I think this should probably be an ASSERT() (to help
>> narrow down where a bug may have come from), followed by a simple return.
>> Likely the worst that would happen here is an un-killable domain; no need to
>> crash production servers in this case.
>
> Ack, I think what I suggested above should also be used here:
>
> if ( !mfn_valid(ofn) )
> {
> ASSERT_UNREACHABLE();
> return -EINVAL;
> }
Yes to both ASSERTs. If something has broken one of our assumptions,
the sooner we find it out (during development) the better.
Thanks,
-George
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
