From mboxrd@z Thu Jan 1 00:00:00 1970 From: Julien Grall Subject: Re: [PATCH v5 12/17] xen/arm: Data abort exception (R/W) mem_events. Date: Fri, 12 Sep 2014 13:35:39 -0700 Message-ID: <5413591B.1050908@linaro.org> References: <1410355726-5599-1-git-send-email-tklengyel@sec.in.tum.de> <1410355726-5599-13-git-send-email-tklengyel@sec.in.tum.de> <541211F3.4050103@linaro.org> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" 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: Ian Campbell , Tim Deegan , Ian Jackson , "xen-devel@lists.xen.org" , Stefano Stabellini , Andres Lagar-Cavilla , Jan Beulich , Daniel De Graaf , Tamas K Lengyel List-Id: xen-devel@lists.xenproject.org Hello Tamas, On 12/09/14 01:46, Tamas K Lengyel wrote: > /* New mapping is superpage aligned, make it */ > pte = mfn_to_p2m_entry(*maddr >> PAGE_SHIFT, > mattr, t, a); > if ( level < 3 ) > @@ -663,6 +737,7 @@ static int apply_one_level(struct domain *d, > > memset(&pte, 0x00, sizeof(pte)); > p2m_write_pte(entry, pte, flush_cache); > + radix_tree_delete(&p2m->mem___access_settings, > paddr_to_pfn(*addr)); > > *addr += level_size; > *maddr += level_size; > @@ -707,6 +782,53 @@ 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)++; > > > Why increment by 1? You the PTE doesn't contain valid mapping you > want to skip the whole level range. ie: > > *addr += level_size; > > > It doesn't make a difference, apply_p2m_changes is called with > start=paddr, end=paddr+1 from a separate loop. So just incrementing it > by one or a whole level achieves the same effect, that is, the > apply_p2m_changes loop breaks. Actually it makes a lots of difference. If you increment by 1 the address, you will call up to level_size time your code before effectively going to the next level entry. This function can be called with *multiple page*. > + return P2M_ONE_PROGRESS_NOP; > + } > + > + /* Shatter large pages as we descend */ > + if ( p2m_mapping(orig_pte) ) > + { > + rc = p2m_create_table(d, entry, > + level_shift - PAGE_SHIFT, > flush_cache); > + if ( rc < 0 ) > + return rc; > + > + p2m->stats.shattered[level]++; > + p2m->stats.mappings[level]--; > + p2m->stats.mappings[level+1] += LPAE_ENTRIES; > + } /* else: an existing table mapping -> descend */ > + > > > This piece of code is exactly the same in INSERT, REMOVE and now > MEMACCESS. I would create an helper to shatter and update the stats. > > > Ack. > > > + return P2M_ONE_DESCEND; > + } > + else > + { > + pte = orig_pte; > + > + if ( !p2m_table(pte) ) > + pte.bits = 0; > + > + if ( p2m_valid(pte) ) > + { > + ASSERT(pte.p2m.type != p2m_invalid); > + > + rc = p2m_mem_access_radix_set(p2m, > paddr_to_pfn(*addr), a); > + if ( rc < 0 ) > + return rc; > + > + p2m_set_permission(&pte, pte.p2m.type, a); > + p2m_write_pte(entry, pte, flush_cache); > + } > + > + (*addr)++; > > > *addr += PAGE_SIZE; > > [..] > > > +/* Set access type for a region of pfns. > + * If start_pfn == -1ul, sets the default access type */ > +long p2m_set_mem_access(struct domain *d, unsigned long pfn, > uint32_t nr, > + uint32_t start, uint32_t mask, > xenmem_access_t access) > +{ > + struct p2m_domain *p2m = p2m_get_hostp2m(d); > + p2m_access_t a; > + long rc = 0; > + paddr_t paddr; > + > + static const p2m_access_t memaccess[] = { > +#define ACCESS(ac) [XENMEM_access_##ac] = p2m_access_##ac > + ACCESS(n), > + ACCESS(r), > + ACCESS(w), > + ACCESS(rw), > + ACCESS(x), > + ACCESS(rx), > + ACCESS(wx), > + ACCESS(rwx), > +#undef ACCESS > + }; > + > + switch ( access ) > + { > + case 0 ... ARRAY_SIZE(memaccess) - 1: > + a = memaccess[access]; > + break; > + case XENMEM_access_default: > + a = p2m->default_access; > + break; > + default: > + return -EINVAL; > + } > + > + /* If request to set default access */ > + if ( pfn == ~0ul ) > + { > + p2m->default_access = a; > + return 0; > + } > + > + for ( pfn += start; nr > start; ++pfn ) > + { > + paddr = pfn_to_paddr(pfn); > + rc = apply_p2m_changes(d, MEMACCESS, paddr, paddr+1, 0, > MATTR_MEM, 0, a); > > > Hmmm... why didn't you call directly apply_p2m_changes with the > whole range? > > > Because the hypercall continuation. Setting mem_access permissions needs > to be preemptible and it has its own separate routine to do that here. > See http://xenbits.xen.org/xsa/advisory-89.html for more info. We do have hypercall continuation in apply_p2m_changes (see for relinquish). Please do the same for MEMACCESS rather than using your own loop. Hence, with your solution, the p2m lookup is taken/released at each loop. This is inefficient. Regards, -- Julien Grall