Re: [PATCH v3 2/5] arch, arm: add consistency checks to REMOVE p2m changes

[Julien Grall <julien.grall@linaro.org>]

On 15/03/14 22:36, Arianna Avanzini wrote:
> On 03/15/2014 11:19 PM, Julien Grall wrote:
>> Did you try remove path? apply_p2m_changes is taking p2m->lock which is also
>> taken by p2m_lookup. With this solution it will end up to a deadlock.
>>
>
> The lookup is performed before grabbing p2m->lock, as stated in the comment.
> I'll certainly remove it as it is useless, thank you for the feedback and for
> the many suggestions.

Oh right, didn't pay attention about it. In any case, you need to keep 
in my mind that p2m_lookup is "slow" (it will map and unmap several 
page). If we can avoid it, it's better.

>> Anyway, you don't need to use p2m_lookup because you already have all the data
>> in pte (if pte.p2m.valid == 1):
>>     - pte.p2m.type  = p2m type
>>     - pte.p2m.base  = MFN
>>
>>>        spin_lock(&p2m->lock);
>>>
>>>        if ( d != current->domain )
>>> @@ -367,9 +382,23 @@ static int apply_p2m_changes(struct domain *d,
>>>                        maddr += PAGE_SIZE;
>>>                    }
>>>                    break;
>>> -            case RELINQUISH:
>>>                case REMOVE:
>>>                    {
>>> +                    /*
>>> +                     * Ensure that, if we are trying to unmap I/O memory
>>> +                     * ranges, the given gfn is p2m_mmio_direct.
>>> +                     */
>>
>>> +                    if ( t == p2m_mmio_direct ? _t != p2m_mmio_direct : 0 ||
>>> +                         paddr_to_pfn(_maddr) == INVALID_MFN ||
>>
>> Testing pte.p2m.valid instead of paddr_to(_maddr)... is right answer.
>>
>> Moreover, why do you need to check t? Every call to guest_physmap_remove_page is
>> done with a valid mfn (I guess it can be enhanced by a BUG_ON(mfn !=
>> INVALID_MFN) in this function).
>>
>
> I might be wrong, but it seems to me that apply_p2m_changes() is called with op
> == REMOVE also from guest_physmap_remove_page(), and in that case t == p2m_invalid.

The important bit is the MFN. I don't think we care about "t".

-- 
Julien Grall