From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ian Campbell Subject: Re: [PATCH for-4.5 v6 13/17] xen/arm: Data abort exception (R/W) mem_events. Date: Tue, 16 Sep 2014 17:50:43 +0100 Message-ID: <1410886243.23505.16.camel@citrix.com> References: <1410789775-24197-1-git-send-email-tklengyel@sec.in.tum.de> <1410789775-24197-14-git-send-email-tklengyel@sec.in.tum.de> <1410821634.6908.27.camel@hastur.hellion.org.uk> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Tamas K Lengyel Cc: "xen-devel@lists.xen.org" List-Id: xen-devel@lists.xenproject.org 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.