[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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




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

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


 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.