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

[Julien Grall <julien.grall@xxxxxxxxxx>]

On 01/14/2014 12:54 PM, Ian Campbell wrote:
> On Tue, 2014-01-14 at 12:41 +0000, Julien Grall wrote:
>> 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.
> 
> Can we pull the:
>                 /* TODO: Handle other p2m type */
>                     if ( p2m_is_foreign(pte.p2m.type) )
>                     {
>                         ASSERT(mfn_valid(mfn));
>                         put_page(mfn_to_page(mfn));
>                     }
> out to above the switch and have it be:
>                                 pte = third[third_table_offset(addr)]
>                                 flsuh = pte.p2m.valid
>                                 
>                 /* TODO: Handle other p2m type */
>                                 if (pte.p2m.valid &&
>                                 p2m_is_foreign(pte.p2m.type)
>                                 {
>                                     ASSERT(mfn_valid(mfn));
>                                     put_page(mfn_to_page(mfn));
>                                 }
> I think that would be acceptable for 4.4, unless there is some
> complication I'm not foreseeing?

As discussed IRL, this is the only {get,put}_page to the foreign page
for this domain. So if the foreign domain has release all reference to
this page, the put_page will free the page.

In the tiny timeslice before the flush (at the end of the loop), the
page could be reallocated to another domain.
But hopefully we are safe (assuming my patch "xen/arm: correct
flush_tlb_mask behaviour" is also in the tree), because page_alloc will
call flush TLB.

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel