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

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

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"?

> 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.

Sorry, I seem to have forgotten to read the blurb after the cut. I'll
have to have a think about that aspect tomorrow.

> >>  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?

On x86 they seem to be in steal_page, which ARM doesn't implement.

> 
> > 
> >>     - 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);

So it does, I missed that when I looked.

Thanks,
Ian.



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