From mboxrd@z Thu Jan 1 00:00:00 1970 From: Tamas K Lengyel Subject: Re: [PATCH for-4.5 v6 13/17] xen/arm: Data abort exception (R/W) mem_events. Date: Tue, 16 Sep 2014 12:07:11 +0200 Message-ID: References: <1410789775-24197-1-git-send-email-tklengyel@sec.in.tum.de> <1410789775-24197-14-git-send-email-tklengyel@sec.in.tum.de> <1410821634.6908.27.camel@hastur.hellion.org.uk> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="===============8784408822893470525==" 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: Ian Campbell , "xen-devel@lists.xen.org" List-Id: xen-devel@lists.xenproject.org --===============8784408822893470525== Content-Type: multipart/alternative; boundary=001a11c29fee3ac36305032bea4b --001a11c29fee3ac36305032bea4b Content-Type: text/plain; charset=ISO-8859-1 Dropped xen-devel by accident on my previous msg. On Tue, Sep 16, 2014 at 12:53 AM, Ian Campbell > wrote: > >> On Mon, 2014-09-15 at 16:02 +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, where the key is the pfn >> > of a 4k page. Only settings other than p2m_access_rwx are saved >> > in the Radix tree. >> >> It is here that it is absolutely essential to consider the overhead for >> the non-xenaccess use case, and I think it should be written here in the >> commit message. >> >> I'm worried because I'm not seeing the obvious "xenaccess is disabled, >> skip all this stuff" path I was hoping for. >> >> I think the overhead of p2m modifications and on fault needs separate >> consideration. >> >> For cases where xenaccess isn't enabled IMHO we need to be talking about >> overhead in the ballpark of a pointer deref, compare, conditional jump. >> Certainly not any kind of radix tree lookup etc. >> > > In the trap handlers only permission faults are checked against the > mem_access radix tree, so that already cuts down overhead to a conditional. > In my experiments I haven't seen a single instance of a permission fault > happening which I didn't cause myself. In p2m modifications as long as the > default mem_access is rwx, the code path is the same as before for large > pages. For 4k pages when adding them with rwx permission it does have an > extra radix tree lookup to clean any potential setting in the radix tree > for that page, but that is really just a safety check on my part and if > overhead with it is a problem it can be removed. IMHO in the default case > the radix tree is empty, so that lookup is essentially just another > conditional. > > Do you see any other paths that are a cause for concern? > > >> >> > @@ -149,6 +152,74 @@ static lpae_t *p2m_map_first(struct p2m_domain >> *p2m, paddr_t addr) >> > return __map_domain_page(page); >> > } >> > >> > +static void p2m_set_permission(lpae_t *e, p2m_type_t t, p2m_access_t a) >> > +{ >> >> It would have been good to refactor this, and probably p2m_shatter_page >> in precursor patches to reduce the size of this aleady pretty big patch. >> > > Ack, that would make sense. > > >> >> > @@ -1159,6 +1329,236 @@ err: >> > return page; >> > } >> > >> > +bool_t p2m_mem_access_check(paddr_t gpa, vaddr_t gla, const struct >> npfec npfec) >> >> Can a bunch of this function not be common code? i.e. the inject an >> event into a ring bits? Or even the automatic conversion of rx2rw etc. >> > > I'll look into it, some bits might be. > While the code logic is in theory the same, unfortunately there are significant differences between handling locks, which makes it not possible to have this code in common. There are also some style differences, like ARM doesn't have set_entry/get_entry pointers in the p2m_domain, as on ARM we don't need to dynamically support different types for those functions (no need to abstract them). The parts that could be in common are only a couple lines here and there which don't really justify having them in common as separate functions. > > >> >> > @@ -248,6 +249,23 @@ static inline bool_t >> p2m_mem_event_sanity_check(struct domain *d) >> > return 1; >> > } >> > >> > +/* Send mem event based on the access (gla is -1ull if not available). >> Boolean >> >> Is a "gla" a "guest linear address"? I don't think we use that term on >> ARM. >> > > Yea, that's copy-pasta and will clean it up.. on ARM we always have the > faulting vaddr it seems to me anyway. > > >> >> Ian >> > > Thanks, > Tamas > --001a11c29fee3ac36305032bea4b Content-Type: text/html; charset=ISO-8859-1 Content-Transfer-Encoding: quoted-printable
Dropped xen-devel by accident on my previous msg.
=
On Tue, Sep 16, 2014 at 12:53 AM, Ian Campbell <= span dir=3D"ltr"><ian.campbell@citrix.com> wrote:
On Mon, 2014-09-15 at 16:02 +0200, Tamas K Lengyel wrot= e:
> This patch enables to store, set, check and deliver LPAE R/W mem_event= s.
> As the LPAE PTE's lack enough available software programmable bits= ,
> we store the permissions in a Radix tree, where the key is the pfn
> of a 4k page. Only settings other than p2m_access_rwx are saved
> in the Radix tree.

It is here that it is absolutely essential to consider the overhead = for
the non-xenaccess use case, and I think it should be written here in the commit message.

I'm worried because I'm not seeing the obvious "xenaccess is d= isabled,
skip all this stuff" path I was hoping for.

I think the overhead of p2m modifications and on fault needs separate
consideration.

For cases where xenaccess isn't enabled IMHO we need to be talking abou= t
overhead in the ballpark of a pointer deref, compare, conditional jump.
Certainly not any kind of radix tree lookup etc.

<= /div>
In the trap handlers only permission faults are checked ag= ainst the mem_access radix tree, so that already cuts down overhead to a co= nditional. In my experiments I haven't seen a single instance of a perm= ission fault happening which I didn't cause myself. In p2m modification= s as long as the default mem_access is rwx, the code path is the same as be= fore for large pages. For 4k pages when adding them with rwx permission it = does have an extra radix tree lookup to clean any potential setting in the = radix tree for that page, but that is really just a safety check on my part= and if overhead with it is a problem it can be removed. IMHO in the defaul= t case the radix tree is empty, so that lookup is essentially just another = conditional.

Do you see any other paths that are a cause = for concern?
=A0

> @@ -149,6 +152,74 @@ static lpae_t *p2m_map_first(struct p2m_domain *p= 2m, paddr_t addr)
>=A0 =A0 =A0 return __map_domain_page(page);
>=A0 }
>
> +static void p2m_set_permission(lpae_t *e, p2m_type_t t, p2m_access_t = a)
> +{

It would have been good to refactor this, and probably p2m_shatter_p= age
in precursor patches to reduce the size of this aleady pretty big patch.

Ack, that would make sense.
=A0

> @@ -1159,6 +1329,236 @@ err:
>=A0 =A0 =A0 return page;
>=A0 }
>
> +bool_t p2m_mem_access_check(paddr_t gpa, vaddr_t gla, const struct np= fec npfec)

Can a bunch of this function not be common code? i.e. the inject an<= br> event into a ring bits? Or even the automatic conversion of rx2rw etc.
<= /blockquote>

I'll look into it, some bits mig= ht be.

While th= e code logic is in theory the same, unfortunately there are significant dif= ferences between handling locks, which makes it not possible to have this c= ode in common. There are also some style differences, like ARM doesn't = have set_entry/get_entry pointers in the p2m_domain, as on ARM we don't= need to dynamically support different types for those functions (no need t= o abstract them). The parts that could be in common are only a couple lines= here and there which don't really justify having them in common as sep= arate functions.
=A0
=A0

> @@ -248,6 +249,23 @@ static inline bool_t p2m_mem_event_sanity_check(s= truct domain *d)
>=A0 =A0 =A0 return 1;
>=A0 }
>
> +/* Send mem event based on the access (gla is -1ull if not available)= . Boolean

Is a "gla" a "guest linear address"? I don't= think we use that term on
ARM.

Yea, that's copy-pasta = and will clean it up.. on ARM we always have the faulting vaddr it seems to= me anyway.
=A0

Ian

Thanks,
Tamas
=

--001a11c29fee3ac36305032bea4b-- --===============8784408822893470525== 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 --===============8784408822893470525==--