From mboxrd@z Thu Jan 1 00:00:00 1970 From: Julien Grall Subject: Re: [PATCH for-4.5 v8 12/19] xen/arm: p2m changes for mem_access support Date: Wed, 24 Sep 2014 15:43:21 +0100 Message-ID: <5422D889.2060608@linaro.org> References: <1411478070-13836-1-git-send-email-tklengyel@sec.in.tum.de> <1411478070-13836-13-git-send-email-tklengyel@sec.in.tum.de> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <1411478070-13836-13-git-send-email-tklengyel@sec.in.tum.de> 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 , xen-devel@lists.xen.org Cc: ian.campbell@citrix.com, tim@xen.org, ian.jackson@eu.citrix.com, stefano.stabellini@citrix.com, andres@lagarcavilla.org, jbeulich@suse.com, dgdegra@tycho.nsa.gov List-Id: xen-devel@lists.xenproject.org On 09/23/2014 02:14 PM, Tamas K Lengyel wrote: > +/* List of possible type for each page in the p2m entry. > + * The number of available bit per page in the pte for this purpose is 4 bits. > + * So it's possible to only have 16 fields. If we run out of value in the > + * future, it's possible to use higher value for pseudo-type and don't store > + * them in the p2m entry. > + */ > +typedef enum { > + p2m_invalid = 0, /* Nothing mapped here */ > + p2m_ram_rw, /* Normal read/write guest RAM */ > + p2m_ram_ro, /* Read-only; writes are silently dropped */ > + p2m_mmio_direct, /* Read/write mapping of genuine MMIO area */ > + p2m_map_foreign, /* Ram pages from foreign domain */ > + p2m_grant_map_rw, /* Read/write grant mapping */ > + p2m_grant_map_ro, /* Read-only grant mapping */ > + /* The types below are only used to decide the page attribute in the P2M */ > + p2m_iommu_map_rw, /* Read/write iommu mapping */ > + p2m_iommu_map_ro, /* Read-only iommu mapping */ > + p2m_max_real_type, /* Types after this won't be store in the p2m */ > +} p2m_type_t; In the context of this patch, this clearly looks a spurious change. As said on a previous version, moving this enum only for style is not a good reason. It makes more difficult to use git-blame. Please avoid to move this enum. > + > +/* Look up a GFN and take a reference count on the backing page. */ > +typedef unsigned int p2m_query_t; > +#define P2M_ALLOC (1u<<0) /* Populate PoD and paged-out entries */ > +#define P2M_UNSHARE (1u<<1) /* Break CoW sharing */ > + > /* Per-p2m-table state */ > struct p2m_domain { > /* Lock that protects updates to the p2m */ > @@ -48,27 +75,20 @@ struct p2m_domain { > /* If true, and an access fault comes in and there is no mem_event listener, > * pause domain. Otherwise, remove access restrictions. */ > bool_t access_required; > -}; > > -/* List of possible type for each page in the p2m entry. > - * The number of available bit per page in the pte for this purpose is 4 bits. > - * So it's possible to only have 16 fields. If we run out of value in the > - * future, it's possible to use higher value for pseudo-type and don't store > - * them in the p2m entry. > - */ > -typedef enum { > - p2m_invalid = 0, /* Nothing mapped here */ > - p2m_ram_rw, /* Normal read/write guest RAM */ > - p2m_ram_ro, /* Read-only; writes are silently dropped */ > - p2m_mmio_direct, /* Read/write mapping of genuine MMIO area */ > - p2m_map_foreign, /* Ram pages from foreign domain */ > - p2m_grant_map_rw, /* Read/write grant mapping */ > - p2m_grant_map_ro, /* Read-only grant mapping */ > - /* The types below are only used to decide the page attribute in the P2M */ > - p2m_iommu_map_rw, /* Read/write iommu mapping */ > - p2m_iommu_map_ro, /* Read-only iommu mapping */ > - p2m_max_real_type, /* Types after this won't be store in the p2m */ > -} p2m_type_t; > + /* Defines if mem_access is in use for the domain to avoid uneccessary radix unecessary > + * lookups. */ > + bool_t access_in_use; Actually, you are using this boolean for more than avoid lookup to the radix. I would drop the end of the sentence i.e: "to avoid uneccessary radix lookups". I gave a look to the radix tree lookups functions. Even if it's not obvious, those functions have nearly the same cost as checking a boolean when the radix tree is empty. It might be interesting to see if we can avoid some if (p2m->access_in_use) check in the p2m code. Regards, -- Julien Grall