On 2/7/19 5:57 PM, Roger Pau Monné wrote:
> On Thu, Feb 07, 2019 at 05:49:16PM +0000, George Dunlap wrote:
>> On 1/30/19 10:36 AM, Roger Pau Monne wrote:
>>> diff --git a/xen/include/asm-x86/p2m.h b/xen/include/asm-x86/p2m.h
>>> index 834d49d2d4..1cc8acb3fe 100644
>>> --- a/xen/include/asm-x86/p2m.h
>>> +++ b/xen/include/asm-x86/p2m.h
>>> @@ -933,9 +933,12 @@ 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)
>>> + p2m_type_t ot, mfn_t nfn, mfn_t ofn,
>>> + unsigned int level)
>>> {
>>> - if ( level != 1 || nt == ot )
>>> + struct page_info *pg;
>>> +
>>> + if ( level != 1 || (nt == ot && mfn_eq(nfn, ofn)) )
>>> return;
>>
>> Are you sure that foreign mappings (or ioreq server pages, for that
>> matter) can never be level > 1?
>
> Not given the current Xen interface, see
> XENMEM_add_to_physmap{_batch}. This will have to change if the
> interface is expanded to allow 2M or 1G mappings.
Right; but the question really meant to say: If such an interface
expansion happened, are you sure the person who did it would remember to
handle larger pages down here?
I think we need at least an ASSERT() for ioreq_server types as well
(although we might want to take a look to see what badness could happen
if the count was out of sync -- adding that condition to the BUG_ON()
might be necessary as well).
>> The alternate would be to have this return an error value, which would
>> 1) cause the p2m write to fail, and 2) be checked all the way up the chain.
>>
>> Less worried about the removal side, as if we have BUG_ON's on the
>> insertion side, they *really* shouldn't happen.
>
> I would go for the BUG_ON ATM, because it's a simpler solution and
> callers of p2m_entry_modify should make sure the operation is correct.
Ack.
-George
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
