Re: [PATCH V14 3/7] xen/arm: Allow hypervisor access to mem_access protected pages

[Julien Grall <julien.grall@citrix.com>]

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