From mboxrd@z Thu Jan 1 00:00:00 1970 From: Julien Grall Subject: Re: [PATCH 1/3] xen/arm: p2m: extend create_p2m_entries to support read-only mapping Date: Tue, 12 Nov 2013 13:13:02 +0000 Message-ID: <5282295E.4030005@linaro.org> References: <1382605365-19244-1-git-send-email-julien.grall@linaro.org> <1382605365-19244-2-git-send-email-julien.grall@linaro.org> <1384176236.19117.26.camel@kazak.uk.xensource.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: Received: from mail6.bemta14.messagelabs.com ([193.109.254.103]) by lists.xen.org with esmtp (Exim 4.72) (envelope-from ) id 1VgDmH-0006o6-8X for xen-devel@lists.xenproject.org; Tue, 12 Nov 2013 13:13:09 +0000 Received: by mail-ea0-f174.google.com with SMTP id n15so2867066ead.33 for ; Tue, 12 Nov 2013 05:13:06 -0800 (PST) In-Reply-To: <1384176236.19117.26.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, stefano.stabellini@eu.citrix.com, tim@xen.org, patches@linaro.org List-Id: xen-devel@lists.xenproject.org On 11/11/2013 01:23 PM, Ian Campbell wrote: > On Thu, 2013-10-24 at 10:02 +0100, Julien Grall wrote: >> Signed-off-by: Julien Grall >> --- >> xen/arch/arm/p2m.c | 19 +++++++++++-------- >> xen/include/asm-arm/page.h | 5 +++-- >> 2 files changed, 14 insertions(+), 10 deletions(-) >> >> diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c >> index 2d09fef..fdbb07b 100644 >> --- a/xen/arch/arm/p2m.c >> +++ b/xen/arch/arm/p2m.c >> @@ -111,7 +111,7 @@ static int p2m_create_table(struct domain *d, >> clear_page(p); >> unmap_domain_page(p); >> >> - pte = mfn_to_p2m_entry(page_to_mfn(page), MATTR_MEM); >> + pte = mfn_to_p2m_entry(page_to_mfn(page), MATTR_MEM, 1); > > If this is a bool_t then true/false might be more appropriate? (and all > the other such too) > > I have a weak preference for avoiding such boolean parameters, since it > is never obvious what a given 1/0/true/false is actually is without > having to look at the prototype. At least there is only 1 such param > here (now). > > We don't need full p2m typing yet but perhaps we can make a start by > defining "typdef enum { p2m_ram_rw = N; p2m_ram_ro = M } p2m_type_t" for > this? Sounds good. I will update the patch series with that. >> @@ -213,14 +213,15 @@ static inline lpae_t mfn_to_xen_entry(unsigned long mfn) >> return e; >> } >> >> -static inline lpae_t mfn_to_p2m_entry(unsigned long mfn, unsigned int mattr) >> +static inline lpae_t mfn_to_p2m_entry(unsigned long mfn, unsigned int mattr, >> + bool_t rw) >> { >> paddr_t pa = ((paddr_t) mfn) << PAGE_SHIFT; >> lpae_t e = (lpae_t) { >> .p2m.xn = 0, >> .p2m.af = 1, >> .p2m.sh = LPAE_SH_OUTER, >> - .p2m.write = 1, >> + .p2m.write = !!rw, > > I don't think the !! is strictly necessary for either a bool_t nor a > single bit bitfield. I will use p2m_type_t here and check if the type is equal to p2m_ram_rw. -- Julien Grall