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

[Ian Campbell <ian.campbell@citrix.com>]

On Thu, 2015-03-12 at 15:37 +0000, Julien Grall wrote:
> 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

I don't think this matters one jot.

        bool foo = true;
        bool_t foo = true;

are both perfectly fine, work as expected (since both are equivalent to
= !0), and are easier to read.

Why do you think this is so important?

Ian.