From mboxrd@z Thu Jan  1 00:00:00 1970
From: Julien Grall <julien.grall@citrix.com>
Subject: Re: [PATCH V14 3/7] xen/arm: Allow hypervisor access to
 mem_access protected pages
Date: Wed, 8 Apr 2015 15:33:06 +0100
Message-ID: <55253C22.5030701@citrix.com>
References: <1427407531-31694-1-git-send-email-tklengyel@sec.in.tum.de>
	<1427407531-31694-4-git-send-email-tklengyel@sec.in.tum.de>
Mime-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
Return-path: <xen-devel-bounces@lists.xen.org>
In-Reply-To: <1427407531-31694-4-git-send-email-tklengyel@sec.in.tum.de>
List-Unsubscribe: <http://lists.xen.org/cgi-bin/mailman/options/xen-devel>,
	<mailto:xen-devel-request@lists.xen.org?subject=unsubscribe>
List-Post: <mailto:xen-devel@lists.xen.org>
List-Help: <mailto:xen-devel-request@lists.xen.org?subject=help>
List-Subscribe: <http://lists.xen.org/cgi-bin/mailman/listinfo/xen-devel>,
	<mailto:xen-devel-request@lists.xen.org?subject=subscribe>
Sender: xen-devel-bounces@lists.xen.org
Errors-To: xen-devel-bounces@lists.xen.org
To: Tamas K Lengyel <tklengyel@sec.in.tum.de>, xen-devel@lists.xen.org
Cc: wei.liu2@citrix.com, ian.campbell@citrix.com, stefano.stabellini@eu.citrix.com, ian.jackson@eu.citrix.com, julien.grall@linaro.org, tim@xen.org, stefano.stabellini@citrix.com, jbeulich@suse.com, keir@xen.org
List-Id: xen-devel@lists.xenproject.org

Hi Tamas,

One minor question.

On 26/03/15 22:05, Tamas K Lengyel wrote:
> +    /*
> +     * We had a mem_access permission limiting the access, but the page type
> +     * could also be limiting, so we need to check that as well.
> +     */
> +    maddr = p2m_lookup(current->domain, ipa, &t);
> +    if ( maddr == INVALID_PADDR )
> +        goto err;
> +
> +    mfn = maddr >> PAGE_SHIFT;
> +    if ( !mfn_valid(mfn) )
> +        goto err;
> +
> +    /*
> +     * Base type doesn't allow r/w
> +     */
> +    if ( t != p2m_ram_rw )
> +        goto err;
> +
> +    page = mfn_to_page(mfn);
> +
> +    if ( unlikely(!get_page(page, current->domain)) )
> +        page = NULL;

I just found that this code is very similar to get_page_from_gfn. Would
it make sense to use it?

> diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
> index ad046e8..5d90609 100644
> --- a/xen/arch/arm/traps.c
> +++ b/xen/arch/arm/traps.c
> @@ -1988,7 +1988,7 @@ static void do_trap_data_abort_guest(struct cpu_user_regs *regs,
>      if (dabt.s1ptw)
>          goto bad_data_abort;
>  
> -    rc = gva_to_ipa(info.gva, &info.gpa);
> +    rc = gva_to_ipa(info.gva, &info.gpa, GV2M_READ);

The correct value would be to use dabt.write in order to give either
GV2M_READ or GV2M_WRITE. Although you keep compatibility and it's out of
the scope of this patch.

I will send a follow-up when your series will be pushed.

Regards,

-- 
Julien Grall