From mboxrd@z Thu Jan 1 00:00:00 1970 From: Tamas K Lengyel Subject: Re: [PATCH v3 10/15] xen/arm: Data abort exception (R/W) mem_events. Date: Wed, 3 Sep 2014 23:56:43 +0200 Message-ID: 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> <540777F9.8020006@linaro.org> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="===============2455204693524369322==" Return-path: In-Reply-To: <540777F9.8020006@linaro.org> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Julien Grall 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 --===============2455204693524369322== Content-Type: multipart/alternative; boundary=001a11c29feec63d810502304f9a --001a11c29feec63d810502304f9a Content-Type: text/plain; charset=ISO-8859-1 On Wed, Sep 3, 2014 at 10:20 PM, Julien Grall wrote: > > 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. > > I'm not entirely sure I could easily extend apply_one_level/apply_p2m_changes without significant changes and a potential to affect something else along the way.. I can give it a try though but I'm a bit hesitant.. Do you see an immediate performance reason to try to hook it into those functions instead of keeping it separate? The current function is pretty straight forward in what it is doing as it is. > >> >> + { >> + >> + 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! Hm, I guess I should set rc in that case to an appropriate -errno. I don't think we should continue setting permission if we had a failure midway through. I'll look into this a bit more. > > > 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). Not for every pte but only for those pte's where the access permission is not p2m_access_rwx. This is what I currently have in plans for v4: https://github.com/tklengyel/xen/compare/arm_memaccess4?expand=1#diff-448087f66572e941e5aab286c05c8efaR481 I'm also thinking this function should attempt to delete the node from the radix tree in case the setting being set is p2m_access_rwx as to remove stale entries. > > + 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. > Yeap, you are right, it does work with unsigned from the beginning. > Regards, > > -- > Julien Grall > Thanks! Tamas --001a11c29feec63d810502304f9a Content-Type: text/html; charset=ISO-8859-1 Content-Transfer-Encoding: quoted-printable

= On Wed, Sep 3, 2014 at 10:20 PM, Julien Grall <julien.grall@linaro.o= rg> wrote:

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 mak= e
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 ca= se. I would prefer that you extend the function rather than creating a new = function.


I'm= not entirely sure I could easily extend apply_one_level/apply_p2m_changes = without significant changes and a potential to affect something else along = the way.. I can give it a try though but I'm a bit hesitant.. Do you se= e an immediate performance reason to try to hook it into those functions in= stead of keeping it separate? The current function is pretty straight forwa= rd in what it is doing as it is.




=A0 =A0 =A0 =A0 +=A0 =A0 {
=A0 =A0 =A0 =A0 +
=A0 =A0 =A0 =A0 +=A0 =A0 =A0 =A0 bool_t pte_update =3D p2m_set_entry(d, pfn= _to_paddr(pfn), a);
=A0 =A0 =A0 =A0 +
=A0 =A0 =A0 =A0 +=A0 =A0 =A0 =A0 if ( !pte_update )
=A0 =A0 =A0 =A0 +=A0 =A0 =A0 =A0 =A0 =A0 break;


=A0 =A0 Shouldn't you continue here? The other pages in the batch may =A0 =A0 require updates.


This is the same approach as in x86.

Hmmm ... no. x86 will break if an error has occured (i.e rc !=3D 0) and ret= urn the rc later.

Here, you p2m_set_entry will return a boolean (I don't really understan= d 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 re= turn 0 (rc has been initialized to 0 earlier). Therefore the userspace appl= ication will think everything has been correctly updated but it's wrong= !

Hm, I guess I should set rc in that case to an appropri= ate -errno. I don't think we should continue setting permission if we h= ad a failure midway through. I'll look into this a bit more.
=A0


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 ra= dix tree. Xen will waste memory when xen access is not used (IHMO, the feat= ure is only used in few cases).

Not for every pte but only for those pte's where th= e access permission is not p2m_access_rwx. This is what I currently have in= plans for v4: https://github= .com/tklengyel/xen/compare/arm_memaccess4?expand=3D1#diff-448087f66572e941e= 5aab286c05c8efaR481

I'm also thinking this function should attemp= t to delete the node from the radix tree in case the setting being set is p= 2m_access_rwx as to remove stale entries.



=A0 =A0 =A0 =A0 +=A0 =A0 =A0 =A0 return -ESRCH;
=A0 =A0 =A0 =A0 +
=A0 =A0 =A0 =A0 +=A0 =A0 index =3D radix_tree_ptr_to_int(i);
=A0 =A0 =A0 =A0 +
=A0 =A0 =A0 =A0 +=A0 =A0 if ( (unsigned) index >=3D ARRAY_SIZE(memaccess= ) )
=A0 =A0 =A0 =A0 +=A0 =A0 =A0 =A0 return -ERANGE;


=A0 =A0 You are casting to unsigned all usage of index within this function= .
=A0 =A0 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.

Yeap, you are right, it does work with uns= igned from the beginning.
=A0
Regards,

--
Julien Grall

Thank= s!
Tamas
--001a11c29feec63d810502304f9a-- --===============2455204693524369322== Content-Type: text/plain; charset="us-ascii" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit Content-Disposition: inline _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel --===============2455204693524369322==--