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

[Tamas K Lengyel <tamas.lengyel@xxxxxxxxxxxx>]

On Mon, Sep 29, 2014 at 4:50 PM, Julien Grall <julien.grall@xxxxxxxxxx> 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).

 

>     > +    ASSERT(*page);
>
>     mfn_to_page only returns a valid pointer if the MFN is valid (see
>     mfn_valid).
>
>
> Hm, I copied this from get_page_from_gva but I'll remove it.

I meant, you forgot to copy the if ( !mfn_valid(mfn) ) return -EFAULT;

But the ASSERT is pointless as the function may return an invalid
pointer which is not NULL.

Ah I see. OK.

 

Regards,

--
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel