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

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

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.

> 
> (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
have a protection against the guest is replacing a current valid mapping.

> What happens if the page being replaced is foreign? Do we leak a
> reference to another domain's page? (This is probably a separate issue
> though, I suspect the put_page needs pulling out of the switch(op)
> statement).

Right we leak a reference to another domain.

> 
>>     - REMOVE: except for grant-table (replace_grant_host_mapping),
> 
> What about this case? What makes it safe?

As specified a bit later, I can't say if it's safe or not. I was unabled
to find a proof in the code.

>>  each
>>     call to guest_physmap_remove_page are protected by the callers via a
>>         get_page -> .... -> guest_physmap_remove_page -> ... -> put_page. So
>>     the page can't be allocated for another domain until the last put_page.
> 
> I have confirmed this is the case for guest_remove_page, memory_exchange
> and XENMEM_remove_from_physmap.
> 
> There is one case I saw where this isn't true which is gnttab_transfer,
> AFAICT that will fail because steal_page unconditionally returns an
> error on arm. There is also a flush_tlb_mask there, FWIW.

hmmm... I forgot this one... why don't we have a {get,put}_page in this
function?

> 
>>     - RELINQUISH : the domain is not running anymore so we don't care...
> 
> At some point this page will be reused, as will the VMID, where/how is
> it ensured that a flush will happen before that point? 

When the VMID is reused, Xen will flush everything TLBs associated to
this VMID (see p2m_alloc_table);

-- 
Julien Grall