From mboxrd@z Thu Jan 1 00:00:00 1970 From: Julien Grall Subject: Re: [PATCH for-4.5 v8 15/19] xen/arm: Data abort exception (R/W) mem_events. Date: Wed, 24 Sep 2014 17:51:33 +0100 Message-ID: <5422F695.3000809@linaro.org> References: <1411478070-13836-1-git-send-email-tklengyel@sec.in.tum.de> <1411478070-13836-16-git-send-email-tklengyel@sec.in.tum.de> <5422E4AA.4060600@linaro.org> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Tamas K Lengyel Cc: Ian Campbell , Tim Deegan , Ian Jackson , "xen-devel@lists.xen.org" , Stefano Stabellini , Andres Lagar-Cavilla , Jan Beulich , Daniel De Graaf , Tamas K Lengyel List-Id: xen-devel@lists.xenproject.org Hello Tamas, On 09/24/2014 05:27 PM, Tamas K Lengyel wrote: > > /* Put any references on the single 4K page referenced by pte. TODO: > > @@ -553,13 +584,22 @@ static int apply_one_level(struct domain *d, > > if ( p2m_valid(orig_pte) ) > > return P2M_ONE_DESCEND; > > > > - if ( is_mapping_aligned(*addr, end_gpaddr, 0, level_size) ) > > + if ( is_mapping_aligned(*addr, end_gpaddr, 0, level_size) && > > + /* We only create superpages when mem_access is not in use. */ > > + (level == 3 || (level < 3 && !p2m->access_in_use)) ) > > Can't this check be moved in is_mapping_aligned? You have nearly the > same few lines below. > > > Unfortunately not, I already checked and it is used in REMOVE as well in > which case we would need an exception.. and that wasn't very straight > forward. Ok. > > [..] > > > + case MEMACCESS: > > + if ( level < 3 ) > > + { > > + if ( !p2m_valid(orig_pte) ) > > + { > > + *addr += level_size; > > + return P2M_ONE_PROGRESS_NOP; > > + } > > + > > + /* Shatter large pages as we descend */ > > + if ( p2m_mapping(orig_pte) ) > > + { > > + rc = p2m_shatter_page(d, entry, level, flush_cache); > > + > > + if ( rc < 0 ) > > + return rc; > > + } /* else: an existing table mapping -> descend */ > > + > > + return P2M_ONE_DESCEND; > > + } > > + else > > + { > > + pte = orig_pte; > > + > > + if ( !p2m_table(pte) ) > > + pte.bits = 0; > > + > > + if ( p2m_valid(pte) ) > > + { > > + ASSERT(pte.p2m.type != p2m_invalid); > > Why the ASSERT? I don't see why we wouldn't want to set permission for > this type of page. > > > Not sure, this I copied from p2m_lookup. Can it even happen that > something passes p2m_valid() but have a type of p2m_invalid? I think > that just signals that something is very wrong. The ASSERT has been added in p2m_lookup, because p2m_invalid means the MFN is wrong. Hence, p2m_invalid is only used for page table. In your case, you don't need to use the MFN. So, IHMO, this ASSERT is not necessary. > > > + && hypercall_preempt_check() ) > > + { > > + rc = progress; > > + goto out; > > Jumping directly to the label "out" will skip flushing the TLB for the > domain. While it wasn't critical until now, partial redo during > insertion/allocation or hypercall preemption only for relinquish, the > guest may use the wrong permission because the TLB hasn't been flushed. > > At the same time, it looks like you never request to flush for the > MEMACCESS operation (see *flush = true). Does memaccess does a TLB flush > somewhere else? > > > Yes, at the end of p2m_set_mem_access once all PTEs are updated > successfully. I guess we could flush the TLB as we are progressing as > well, it wouldn't hurt. We should flush the TLB as we are progressing because the guest may technically continue to run... > [..] > > > +bool_t p2m_mem_access_check(paddr_t gpa, vaddr_t gla, const struct npfec npfec) > > +{ > > + int rc; > > + bool_t violation; > > + xenmem_access_t xma; > > + mem_event_request_t *req; > > + struct vcpu *v = current; > > + struct p2m_domain *p2m = p2m_get_hostp2m(v->domain); > > + > > + /* Mem_access is not in use. */ > > + if ( !p2m->access_in_use ) > > + return true; > > AFAIU, it's not possible to call this function when mem access is not in > use. I would turn this check into an ASSERT. > > > It is possible to call this function when mem_access is not in use and > it is called every time there is a permission fault in the second stage > translation. This check here just makes sure the function returns as > fast as possible when not in use. Oh right, sorry for the noise. This case made me also think about another possible issue. Permission are checked in raw_copy_{from,to}_guest_helper during virtual address translation to a physical address. As you modified the attribute in the P2M, the copy may failed because of the lake of permission. Regards, -- Julien Grall