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

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

On 12/03/15 15:26, Tamas K Lengyel wrote:
> 
> 
> On Thu, Mar 12, 2015 at 4:13 PM, Julien Grall <julien.grall@linaro.org
> <mailto:julien.grall@linaro.org>> wrote:
> 
>     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.
> 
> 
> Well, I don't really see an obvious way how the two could be combined.

The preemption for relinquish has been chosen arbitrarily so it would be
possible to change it.

> 
>     [..]
> 
>     > +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)
> 
> 
> There was just a discussion how there is no difference between 0/1 and
> false/true other than maybe style. If one is preferred over the other,
> I'm fine with either. Is this really an issue?

You are mixing 2 different types. bool/false/true are part of stdbool.h
rather than bool_t is part of asm/types.h


By mistake we mix both type in an handful of places in ARM. We should
avoid to introduce more.

FWIW, the headers stdbool.h has been introduced in order to share libelf
with the tools.

Regards,

-- 
Julien Grall