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

[Julien Grall <julien.grall@xxxxxxxxxx>]

On 12/03/15 15:46, Ian Campbell wrote:
> 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@xxxxxxxxxx
>>> <mailto:julien.grall@xxxxxxxxxx>> 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?

Using true/false means we have to include stdbool.h. This file is only
there for library shared with the toolstack. It happens to be included
in p2m.c by mistake...

Anyway, we are taking the liberty for re-using different define for
another purpose. If we really want to use false/true with bool_t we
should at least move them in the same header.

Regards,

-- 
Julien Grall

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