Re: [PATCH for-4.5 v11 5/9] xen/arm: Allow hypervisor access to mem_access protected pages

[Julien Grall <julien.grall@linaro.org>]

On 09/29/2014 03:57 PM, Tamas K Lengyel wrote:
> 
> 
> On Mon, Sep 29, 2014 at 4:50 PM, Julien Grall <julien.grall@linaro.org
> <mailto:julien.grall@linaro.org>> wrote:
> 
>     On 09/29/2014 03:44 PM, Tamas K Lengyel wrote:
>     >     > +    if ( rc < 0 )
>     >     > +        return rc;
>     >     > +
>     >     > +    /*
>     >     > +     * We do this first as this is faster in the default case when no
>     >     > +     * permission is set on the page.
>     >     > +     */
>     >     > +    rc = p2m_get_mem_access(current->domain, paddr_to_pfn(ipa), &xma);
>     >     > +    if ( rc < 0 )
>     >     > +        return rc;
>     >     > +
>     >     > +    /* Let's check if mem_access limited the access. */
>     >     > +    switch ( xma )
>     >     > +    {
>     >     > +    default:
>     >     > +    case XENMEM_access_rwx:
>     >
>     >     access_rwx is used to say there is no permission, right? If so, why
>     >     don't you continue to check permission?
>     >
>     >
>     > So things are backward here. There has already been a MMU fault
>     > (get_page_from_gva failed) and we are trying to determine the cause of
>     > that fault. If the mem_access permission is rwx or rw, that means it had
>     > nothing to do with it so we go back to the original path.
> 
>     This is very confusing. As we should never go there, I would add an
>     ASSERT to catch buggy implementation.
> 
> 
> Not sure what you mean. That's the best approach to really avoid adding
> any overhead to those cases where mem_access is not in use. We don't
> even have to check if its in use if get_page_from_gva just works (which
> I assume is the vast majority of the cases).

hmmmm... I'm wrong on this part from the beginning. Sorry for the noise.

Regards,

-- 
Julien Grall