Re: [PATCH v3 10/15] xen/arm: Data abort exception (R/W) mem_events.

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

Hello Tamas,

On 02/09/14 02:06, Tamas K Lengyel wrote:
> Honestly, I tried to wrap my head around apply_p2m_changes and it is
> already quite complex. While I see I could apply the type/access
> permissions with it over a range, I'm not entirely sure how I would make
> it force-shatter all large pages. It was just easier (for now) to do it
> manually.

To shatter large pages, you can give a look to the REMOVE case in 
apply_one_level. I would even create an helper to shatter a page

apply_one_level doesn't look very complex. Almost everything is in a 
case. I would prefer that you extend the function rather than creating a 
new function.

>
>
>         +    {
>         +
>         +        bool_t pte_update = p2m_set_entry(d, pfn_to_paddr(pfn), a);
>         +
>         +        if ( !pte_update )
>         +            break;
>
>
>     Shouldn't you continue here? The other pages in the batch may
>     require updates.
>
>
> This is the same approach as in x86.

Hmmm ... no. x86 will break if an error has occured (i.e rc != 0) and 
return the rc later.

Here, you p2m_set_entry will return a boolean (I don't really understand 
why because you are mixing bool and int for rc withing this function).

If p2m_set_entry returns false, you will break in p2m_set_mem_access and 
return 0 (rc has been initialized to 0 earlier). Therefore the userspace 
application will think everything has been correctly updated but it's wrong!

> Right, I missed page additions with default_access. With so few software
> programmable bits available in the PTE, we have no other choice but to
> store the permission settings separately. I just need to make sure that
> the radix tree is updated when a pte is added/removed.

I'm not sure to fully understand your plan. Do you intend to add every 
page in the radix tree? If so, I'm a bit worry about the size of the 
radix tree. Xen will waste memory when xen access is not used (IHMO, the 
feature is only used in few cases).

>         +        return -ESRCH;
>         +
>         +    index = radix_tree_ptr_to_int(i);
>         +
>         +    if ( (unsigned) index >= ARRAY_SIZE(memaccess) )
>         +        return -ERANGE;
>
>
>     You are casting to unsigned all usage of index within this function.
>     Why not directly define index as an "unsigned int"?
>
>
> The radix tree returns an int but I guess could do that.

Which is fine because, IIRC, the compiler will implicitly cast the value 
in unsigned.

Regards,

-- 
Julien Grall