From mboxrd@z Thu Jan 1 00:00:00 1970 From: Julien Grall Subject: Re: [PATCH v3 10/15] xen/arm: Data abort exception (R/W) mem_events. Date: Wed, 03 Sep 2014 13:20:09 -0700 Message-ID: <540777F9.8020006@linaro.org> References: <1409581329-2607-1-git-send-email-tklengyel@sec.in.tum.de> <1409581329-2607-11-git-send-email-tklengyel@sec.in.tum.de> <5404E02E.7010406@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 02/09/14 02:06, Tamas K Lengyel wrote: > Honestly, I tried to wrap my head around apply_p2m_changes and it is > already quite complex. While I see I could apply the type/access > permissions with it over a range, I'm not entirely sure how I would make > it force-shatter all large pages. It was just easier (for now) to do it > manually. To shatter large pages, you can give a look to the REMOVE case in apply_one_level. I would even create an helper to shatter a page apply_one_level doesn't look very complex. Almost everything is in a case. I would prefer that you extend the function rather than creating a new function. > > > + { > + > + bool_t pte_update = p2m_set_entry(d, pfn_to_paddr(pfn), a); > + > + if ( !pte_update ) > + break; > > > Shouldn't you continue here? The other pages in the batch may > require updates. > > > This is the same approach as in x86. Hmmm ... no. x86 will break if an error has occured (i.e rc != 0) and return the rc later. Here, you p2m_set_entry will return a boolean (I don't really understand why because you are mixing bool and int for rc withing this function). If p2m_set_entry returns false, you will break in p2m_set_mem_access and return 0 (rc has been initialized to 0 earlier). Therefore the userspace application will think everything has been correctly updated but it's wrong! > Right, I missed page additions with default_access. With so few software > programmable bits available in the PTE, we have no other choice but to > store the permission settings separately. I just need to make sure that > the radix tree is updated when a pte is added/removed. I'm not sure to fully understand your plan. Do you intend to add every page in the radix tree? If so, I'm a bit worry about the size of the radix tree. Xen will waste memory when xen access is not used (IHMO, the feature is only used in few cases). > + return -ESRCH; > + > + index = radix_tree_ptr_to_int(i); > + > + if ( (unsigned) index >= ARRAY_SIZE(memaccess) ) > + return -ERANGE; > > > You are casting to unsigned all usage of index within this function. > Why not directly define index as an "unsigned int"? > > > The radix tree returns an int but I guess could do that. Which is fine because, IIRC, the compiler will implicitly cast the value in unsigned. Regards, -- Julien Grall