From mboxrd@z Thu Jan 1 00:00:00 1970 From: Julien Grall Subject: Re: [PATCH v4 11/16] xen/arm: Data abort exception (R/W) mem_events. Date: Mon, 08 Sep 2014 17:25:39 -0700 Message-ID: <540E4903.90001@linaro.org> References: <1409907524-12509-1-git-send-email-tklengyel@sec.in.tum.de> <1409907524-12509-12-git-send-email-tklengyel@sec.in.tum.de> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <1409907524-12509-12-git-send-email-tklengyel@sec.in.tum.de> 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 , xen-devel@lists.xen.org Cc: ian.campbell@citrix.com, tim@xen.org, ian.jackson@eu.citrix.com, stefano.stabellini@citrix.com, andres@lagarcavilla.org, jbeulich@suse.com, dgdegra@tycho.nsa.gov List-Id: xen-devel@lists.xenproject.org Hi Tamas, On 05/09/14 01:58, Tamas K Lengyel wrote: > #define P2M_ONE_DESCEND 0 > #define P2M_ONE_PROGRESS_NOP 0x1 > #define P2M_ONE_PROGRESS 0x10 > @@ -502,6 +568,14 @@ static int apply_one_level(struct domain *d, > pte = mfn_to_p2m_entry(page_to_mfn(page), mattr, t, a); > if ( level < 3 ) > pte.p2m.table = 0; > + > + if ( p2m_mem_access_radix_set(p2m, paddr_to_pfn(*addr), a) < 0 ) > + { > + /* If we fail to save the setting in the Radix tree, we > + * need to clear the PTE mem_access permissions. */ > + p2m_set_permission(&pte, pte.p2m.type, p2m_access_rwx); > + } > + [..] > @@ -538,6 +612,13 @@ static int apply_one_level(struct domain *d, > if ( level < 3 ) > pte.p2m.table = 0; /* Superpage entry */ > > + if ( p2m_mem_access_radix_set(p2m, paddr_to_pfn(*addr), a) < 0 ) > + { > + /* If we fail to save the setting in the Radix tree, we > + * need to clear the PTE mem_access permissions. */ > + p2m_set_permission(&pte, pte.p2m.type, p2m_access_rwx); > + } > + Can't this be abstract to another helper? Also is it acceptable from mem access POV to unconditionally set the access to rwx when an error occured? Shouldn't you report an error? > +static int p2m_set_entry(struct domain *d, paddr_t paddr, p2m_access_t p2ma) > +{ As I said on the previous version, I still think this can be merge in apply_p2m_changes without any difficulties. The main reason is to avoid increasing the number of function that have a common pattern to use the p2m. Also apply_p2m_changes have some interesting performance improvement (such as avoid map/unmap every time page table). Finally, it will make life simpler when 4-Level page table will be supported (see Ian's series [1]) [..] > +bool_t p2m_mem_access_check(paddr_t gpa, vaddr_t gla, struct npfec npfec) const struct npfect [..] > diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c > index 019991f..7eb875a 100644 > --- a/xen/arch/arm/traps.c > +++ b/xen/arch/arm/traps.c > @@ -1852,13 +1852,38 @@ static void do_trap_data_abort_guest(struct cpu_user_regs *regs, [..] > + switch ( dabt.dfsc ) > + { > + case DABT_DFSC_PERMISSION_1: > + case DABT_DFSC_PERMISSION_2: > + case DABT_DFSC_PERMISSION_3: > + { > + struct npfec npfec = { const as you won't modify it later. Regards, [1] http://lists.xen.org/archives/html/xen-devel/2014-09/msg00596.html -- Julien Grall