Xen project Mailing List

On Tue, Sep 16, 2014 at 6:50 PM, Ian Campbell <ian.campbell@xxxxxxxxxx> wrote:

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?

It wouldn't really be via the default_access, I would just have a separate function that is used to remove pre-existing mem_access entries, as that would only happen when the user initiates a mem_access op with rwx permissions. Right now I just bundled that into this path, but as this also gets called by non-memaccess paths its better to separate the two.

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

Not particularly, because we are locking at a different level here.. x86 locks on a gfn level as that's where the permission are stored vs. in ARM we need to lock on a p2m level because the tree might get modified otherwise. So abstracting this code while having the appropriate locks sprinkled into it is not very nice.. Essentially at all lock location for both archs we would need to call down to the arch specific function which would determine where it needs to lock.. and that wouldn't really help in making the code easy to follow.

Tamas

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