Re: [PATCH V13 4/7] xen/arm: Data abort exception (R/W) mem_events.

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

Hi Tamas,

On 06/03/15 21:24, Tamas K Lengyel wrote:
> +        /*
> +         * Preempt setting mem_access permissions as required by XSA-89,
> +         * if it's not the last iteration.
> +         */
> +        if ( op == MEMACCESS && count )
> +        {
> +            uint32_t progress = paddr_to_pfn(addr) - sgfn + 1;
> +
> +            if ( (egfn - sgfn) > progress && !(progress & mask)
> +                 && hypercall_preempt_check() )
> +            {
> +                rc = progress;
> +                goto out;
> +            }
> +        }
> +

Would it be possible to merge the memaccess prempt check with the
relinquish one?

That may require some change in the relinquish version but I think it
would be cleaner.

[..]

> +bool_t p2m_mem_access_check(paddr_t gpa, vaddr_t gla, const struct npfec npfec)
> +{
> +    int rc;
> +    bool_t violation;
> +    xenmem_access_t xma;
> +    mem_event_request_t *req;
> +    struct vcpu *v = current;
> +    struct p2m_domain *p2m = p2m_get_hostp2m(v->domain);
> +
> +    /* Mem_access is not in use. */
> +    if ( !p2m->mem_access_enabled )
> +        return true;

true should be used with bool not boot_t.

> +
> +    rc = p2m_get_mem_access(v->domain, paddr_to_pfn(gpa), &xma);
> +    if ( rc )
> +        return true;

Ditto (I won't repeat for the few other place below)

> +
> +    /* Now check for mem_access violation. */
> +    switch ( xma )
> +    {
> +    case XENMEM_access_rwx:
> +        violation = false;
> +        break;
> +    case XENMEM_access_rw:
> +        violation = npfec.insn_fetch;
> +        break;
> +    case XENMEM_access_wx:
> +        violation = npfec.read_access;
> +        break;
> +    case XENMEM_access_rx:
> +    case XENMEM_access_rx2rw:
> +        violation = npfec.write_access;
> +        break;
> +    case XENMEM_access_x:
> +        violation = npfec.read_access || npfec.write_access;
> +        break;
> +    case XENMEM_access_w:
> +        violation = npfec.read_access || npfec.insn_fetch;
> +        break;
> +    case XENMEM_access_r:
> +        violation = npfec.write_access || npfec.insn_fetch;
> +        break;
> +    default:
> +    case XENMEM_access_n:
> +    case XENMEM_access_n2rwx:
> +        violation = true;
> +        break;
> +    }
> +
> +    if ( !violation )
> +        return true;

Ditto

> +
> +    /* First, handle rx2rw and n2rwx conversion automatically. */
> +    if ( npfec.write_access && xma == XENMEM_access_rx2rw )
> +    {
> +        rc = p2m_set_mem_access(v->domain, paddr_to_pfn(gpa), 1,
> +                                0, ~0, XENMEM_access_rw);
> +        return false;

Same as true.

[..]

> +    if ( !i )
> +    {
> +        /*
> +         * No setting was found in the Radix tree. Check if the
> +         * entry exists in the page-tables.
> +         */
> +        paddr_t maddr = p2m_lookup(d, gpfn << PAGE_SHIFT, NULL);
> +        if ( INVALID_PADDR == maddr )
> +            return -ESRCH;
> +
> +        /* If entry exists then its rwx. */
> +        *access = XENMEM_access_rwx;
> +    }
> +    else
> +    {
> +        /* Setting was found in the Radix tree. */
> +        index = radix_tree_ptr_to_int(i);
> +        if ( index >= ARRAY_SIZE(memaccess) )
> +            return -ERANGE;

We trust the value stored in the radix tree. So I think this could be
turned into an ASSERT/BUG_ON.

[..]

> @@ -243,12 +245,17 @@ static inline bool_t p2m_mem_event_sanity_check(struct domain *d)
>  
>  /* Get access type for a pfn
>   * If pfn == -1ul, gets the default access type */
> -static inline
>  int p2m_get_mem_access(struct domain *d, unsigned long pfn,
> -                       xenmem_access_t *access)
> -{
> -    return -ENOSYS;
> -}
> +                       xenmem_access_t *access);
> +

p2m_get_mem_access is called by the common code. So it should be moved
in xen/include/xen/p2m-common.h

In general, any function called by common code should be have the
prototype declared in common header.

Regards,

-- 
Julien Grall