Re: [Xen-devel] [PATCH 7/8] x86/mm: handle foreign mappings in p2m_entry_modify

[George Dunlap <george.dunlap@xxxxxxxxxx>]

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@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel