Re: [PATCH for-4.5 v6 13/17] xen/arm: Data abort exception (R/W) mem_events.

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

On Tue, 2014-09-16 at 12:07 +0200, Tamas K Lengyel wrote:

>         In the trap handlers only permission faults are checked
>         against the mem_access radix tree, so that already cuts down
>         overhead to a conditional. In my experiments I haven't seen a
>         single instance of a permission fault happening which I didn't
>         cause myself. In p2m modifications as long as the default
>         mem_access is rwx, the code path is the same as before for
>         large pages. For 4k pages when adding them with rwx permission
>         it does have an extra radix tree lookup to clean any potential
>         setting in the radix tree for that page, but that is really
>         just a safety check on my part and if overhead with it is a
>         problem it can be removed. IMHO in the default case the radix
>         tree is empty, so that lookup is essentially just another
>         conditional.

With the radix tree lookup functions it isn't trivially easy to reason
that they turn into an almost-nop on an empty tree, so I'm not sure.
It's an out of line function call and at least 2 pointer indirections
from the looks of it.

Perhaps we could add an explicit value for p2m->default_access which
causes the tree never to even get touched?

> 
> While the code logic is in theory the same, unfortunately there are
> significant differences between handling locks, which makes it not
> possible to have this code in common. There are also some style
> differences, like ARM doesn't have set_entry/get_entry pointers in the
> p2m_domain, as on ARM we don't need to dynamically support different
> types for those functions (no need to abstract them). The parts that
> could be in common are only a couple lines here and there which don't
> really justify having them in common as separate functions.

We can add new arch-generic wrappers for things if it makes sense, like
e.g. guest_physmap_add_entry or map_mmio_regions etc. Do you tihnk that
would help/make sense here?

Ian.