From mboxrd@z Thu Jan 1 00:00:00 1970 From: Julien Grall Subject: Re: [PATCH v2] xen/arm: p2m: Correctly flush TLB in create_p2m_entries Date: Mon, 13 Jan 2014 17:37:49 +0000 Message-ID: <52D4246D.1050400@linaro.org> References: <1389285240-7116-1-git-send-email-julien.grall@linaro.org> <1389633015.13654.109.camel@kazak.uk.xensource.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from mail6.bemta5.messagelabs.com ([195.245.231.135]) by lists.xen.org with esmtp (Exim 4.72) (envelope-from ) id 1W2lSV-0000Lj-0w for xen-devel@lists.xenproject.org; Mon, 13 Jan 2014 17:37:55 +0000 Received: by mail-wi0-f182.google.com with SMTP id ex4so692086wid.3 for ; Mon, 13 Jan 2014 09:37:53 -0800 (PST) In-Reply-To: <1389633015.13654.109.camel@kazak.uk.xensource.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Ian Campbell Cc: xen-devel@lists.xenproject.org, patches@linaro.org, stefano.stabellini@citrix.com, tim@xen.org List-Id: xen-devel@lists.xenproject.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