Xen project Mailing List

On Wed, Sep 3, 2014 at 10:20 PM, Julien Grall <julien.grall@xxxxxxxxxx> wrote:

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.

I'm not entirely sure I could easily extend apply_one_level/apply_p2m_changes without significant changes and a potential to affect something else along the way.. I can give it a try though but I'm a bit hesitant.. Do you see an immediate performance reason to try to hook it into those functions instead of keeping it separate? The current function is pretty straight forward in what it is doing as it is.

+ {
+
+ 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!

Hm, I guess I should set rc in that case to an appropriate -errno. I don't think we should continue setting permission if we had a failure midway through. I'll look into this a bit more.

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).

Not for every pte but only for those pte's where the access permission is not p2m_access_rwx. This is what I currently have in plans for v4: https://github.com/tklengyel/xen/compare/arm_memaccess4?expand=1#diff-448087f66572e941e5aab286c05c8efaR481

I'm also thinking this function should attempt to delete the node from the radix tree in case the setting being set is p2m_access_rwx as to remove stale entries.

+ 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.

Yeap, you are right, it does work with unsigned from the beginning.

Regards,

--
Julien Grall

Thanks!
Tamas

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