From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ian Campbell Subject: Re: [PATCH for-4.5 v8 15/19] xen/arm: Data abort exception (R/W) mem_events. Date: Wed, 24 Sep 2014 16:02:51 +0100 Message-ID: <1411570971.28127.63.camel@kazak.uk.xensource.com> References: <1411478070-13836-1-git-send-email-tklengyel@sec.in.tum.de> <1411478070-13836-16-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-16-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 Cc: tim@xen.org, julien.grall@linaro.org, ian.jackson@eu.citrix.com, xen-devel@lists.xen.org, stefano.stabellini@citrix.com, andres@lagarcavilla.org, jbeulich@suse.com, dgdegra@tycho.nsa.gov List-Id: xen-devel@lists.xenproject.org On Tue, 2014-09-23 at 15:14 +0200, Tamas K Lengyel wrote: > This patch enables to store, set, check and deliver LPAE R/W mem_events. > As the LPAE PTE's lack enough available software programmable bits, > we store the permissions in a Radix tree. A custom boolean, access_in_use, > specifies if the tree is in use to avoid uneccessary lookups on an empty tree. "unnecessary". > @@ -414,12 +417,40 @@ static int p2m_create_table(struct domain *d, lpae_t *entry, > return 0; > } > > +static long p2m_mem_access_radix_set(struct p2m_domain *p2m, unsigned long pfn, > + p2m_access_t a) > +{ > + long rc; > + if ( p2m_access_rwx == a ) I think I mentioned this before, but please write your conditionals the more conventional way around. > + { > + if ( p2m->access_in_use ) > + radix_tree_delete(&p2m->mem_access_settings, pfn); > + > + return 0; > + } > + > + rc = radix_tree_insert(&p2m->mem_access_settings, pfn, > + radix_tree_int_to_ptr(a)); > + if ( -EEXIST == rc ) > + { > + /* If a setting existed already, change it to the new one */ > + radix_tree_replace_slot( > + radix_tree_lookup_slot( > + &p2m->mem_access_settings, pfn), > + radix_tree_int_to_ptr(a)); What a shame that the API doesn't allow you to do an insert or replace in one go! > @@ -591,9 +631,14 @@ static int apply_one_level(struct domain *d, > > case INSERT: > if ( is_mapping_aligned(*addr, end_gpaddr, *maddr, level_size) && > - /* We do not handle replacing an existing table with a superpage */ > - (level == 3 || !p2m_table(orig_pte)) ) > + /* We do not handle replacing an existing table with a superpage > + * or when mem_access is in use. */ > + (level == 3 || (!p2m_table(orig_pte) && !p2m->access_in_use)) ) > { > + rc = p2m_mem_access_radix_set(p2m, paddr_to_pfn(*addr), a); > + if ( rc < 0 ) > + return rc; I expect the answer is yes, but just to check: It is desirable to be able to insert with a specific non-defualt access perms, as opposed to INSERT with defaults then MEMACCESS? > @@ -753,6 +799,49 @@ static int apply_one_level(struct domain *d, > *addr += PAGE_SIZE; > return P2M_ONE_PROGRESS_NOP; > } > + > + case MEMACCESS: > + if ( level < 3 ) > + { > + if ( !p2m_valid(orig_pte) ) > + { > + *addr += level_size; > + return P2M_ONE_PROGRESS_NOP; > + } > + > + /* Shatter large pages as we descend */ > + if ( p2m_mapping(orig_pte) ) > + { > + rc = p2m_shatter_page(d, entry, level, flush_cache); > + > + if ( rc < 0 ) > + return rc; I don't know if that matter for this specific sub op but what is supposed to happen on error? Should partial work be undone? > @@ -776,6 +865,8 @@ static int apply_p2m_changes(struct domain *d, > unsigned int cur_root_table = ~0; > unsigned int cur_offset[4] = { ~0, }; > unsigned int count = 0; > + unsigned long start_gpfn = paddr_to_pfn(start_gpaddr), > + end_gpfn = paddr_to_pfn(end_gpaddr); You don't need to update end_gpaddr up, do you? > bool_t flush = false; > bool_t flush_pt; > > @@ -821,6 +912,21 @@ static int apply_p2m_changes(struct domain *d, > count = 0; > } > > + /* > + * Preempt setting mem_access permissions as required by XSA-89, > + * if it's not the last iteration. > + */ > + if ( op == MEMACCESS && count ) > + { > + int progress = paddr_to_pfn(addr) - start_gpfn + 1; > + if ( (end_gpfn-start_gpfn) > progress && !(progress & mask) This differs from the x86 equivalent in that it is not "++progress". Doesn't that mean that we can fail to make any progress at all? > +int p2m_get_mem_access(struct domain *d, unsigned long gpfn, > + xenmem_access_t *access) > +{ > + struct p2m_domain *p2m = p2m_get_hostp2m(d); > + void *i; > + unsigned int index; > + > + static const xenmem_access_t memaccess[] = { > +#define ACCESS(ac) [p2m_access_##ac] = XENMEM_access_##ac > + ACCESS(n), > + ACCESS(r), > + ACCESS(w), > + ACCESS(rw), > + ACCESS(x), > + ACCESS(rx), > + ACCESS(wx), > + ACCESS(rwx), > + ACCESS(rx2rw), > + ACCESS(n2rwx), > +#undef ACCESS Unlike x86 I think you have pretty much complete freedom to define p2m_access_*? Why not just make them 1:1? (one for a future cleanup if you prefer) > + /* Seting was found in the Radix tree. */ "Setting"