Re: [PATCH v2] xen/arm: p2m: Correctly flush TLB in create_p2m_entries

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

On 01/13/2014 05:57 PM, Ian Campbell wrote:
> On Mon, 2014-01-13 at 17:37 +0000, Julien Grall wrote:
>> On 01/13/2014 05:10 PM, Ian Campbell wrote:
>>> Hrm, our TLB flush discipline is horribly confused isn't it...
>>>
>>> On Thu, 2014-01-09 at 16:34 +0000, Julien Grall wrote:
>>>> The p2m is shared between VCPUs for each domain. Currently Xen only flush
>>>> TLB on the local PCPU. This could result to mismatch between the mapping in the
>>>> p2m and TLBs.
>>>>
>>>> Flush TLB entries used by this domain on every PCPU. The flush can also be
>>>> moved out of the loop because:
>>>>     - ALLOCATE: only called for dom0 RAM allocation, so the flush is never called
>>>
>>> OK.
>>>
>>> An ASSERT(!third[third_table_offset(addr)].p2m.valid) might be
>>> worthwhile if that is the case.
>>
>> Will add it.
> 
> Thanks.
> 
>>> (I'm not sure why ALLOCATE can't be replaced by allocation followed by
>>> an INSERT, it's seems very special case)
>>>
>>>>     - INSERT: if valid = 1 that would means with have replaced a
>>>>     page that already belongs to the domain. A VCPU can write on the wrong page.
>>>>     This can append for dom0 with the 1:1 mapping because the mapping is not
>>>>     removed from the p2m.
>>>
>>> "append"? Do you mean "happen"?
>>
>> I meant "happen".
>>
>>>
>>> In the non-dom0 1:1 case eventually the page will be freed, I guess by a
>>> subsequent put_page elsewhere -- do they all contain the correct
>>> flushing? Or do we just leak?
>>
>> As for foreign mapping the INSERT function should be hardened. We don't
> 
> Did you mean "handled"?

I meant both :). Actually we don't have any check in this function as
for REMOVE case.

I don't think it's possible to do it for 4.4, we take a reference on the
mapping every time a new entrie is added in the p2m.


-- 
Julien Grall