From mboxrd@z Thu Jan 1 00:00:00 1970 From: Dario Faggioli Subject: Re: [PATCH 06/15] xen: trace IRQ enabling/disabling Date: Wed, 7 Jun 2017 17:22:04 +0200 Message-ID: <1496848924.9462.1.camel@citrix.com> References: <149633614204.12814.14390287626133023934.stgit@Solace.fritz.box> <149633845700.12814.7130992212550379105.stgit@Solace.fritz.box> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="===============0824219606845056877==" Return-path: Received: from mail6.bemta3.messagelabs.com ([195.245.230.39]) by lists.xenproject.org with esmtp (Exim 4.84_2) (envelope-from ) id 1dIcml-0005TG-UQ for xen-devel@lists.xenproject.org; Wed, 07 Jun 2017 15:22:16 +0000 In-Reply-To: List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Errors-To: xen-devel-bounces@lists.xen.org Sender: "Xen-devel" To: Julien Grall , xen-devel@lists.xenproject.org Cc: George Dunlap , Andrew Cooper , Stefano Stabellini , Jennifer Herbert , Jan Beulich List-Id: xen-devel@lists.xenproject.org --===============0824219606845056877== Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="=-gu3WTf2q/IeY3ewLs08r" --=-gu3WTf2q/IeY3ewLs08r Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable On Wed, 2017-06-07 at 12:16 +0100, Julien Grall wrote: > Hi Dario, >=20 Hi, > On 01/06/17 18:34, Dario Faggioli wrote: > > diff --git a/xen/common/spinlock.c b/xen/common/spinlock.c > > index 2a06406..33b903e 100644 > > --- a/xen/common/spinlock.c > > +++ b/xen/common/spinlock.c > > @@ -150,7 +150,9 @@ void _spin_lock(spinlock_t *lock) > > =C2=A0void _spin_lock_irq(spinlock_t *lock) > > =C2=A0{ > > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0ASSERT(local_irq_is_enabled()); > > -=C2=A0=C2=A0=C2=A0=C2=A0local_irq_disable(); > > +=C2=A0=C2=A0=C2=A0=C2=A0_local_irq_disable(); > > +=C2=A0=C2=A0=C2=A0=C2=A0if ( unlikely(tb_init_done) ) >=20 > __trace_var already contain a "if ( !tb_init_done )". It sounds=C2=A0 > pointless to do it twice, and also I think it is not obvious to=C2=A0 > understand the meaning of the check (i.e what is tb_init_done). >=20 > Would it be possible to hide this check and avoid check tb_init_done > twice? >=20 I totally agree. And in fact, in another patch, I remove the tb_init_done check in __trace_var(). Reason behind this is that all the callers of __trace_var() (the main one being trace_var()), already checks for tb_init_done themselves. In fact, the explicit check in the caller, is the (only) basis on which one decides to call either trace_var() or __trace_var(). This here is one of the call sites where I think the check is better done in the caller. > > diff --git a/xen/include/asm-arm/arm32/system.h b/xen/include/asm- > > arm/arm32/system.h > > index c617b40..20871ad 100644 > > --- a/xen/include/asm-arm/arm32/system.h > > +++ b/xen/include/asm-arm/arm32/system.h > > @@ -4,6 +4,8 @@ > >=20 > > =C2=A0#include > >=20 > > +#include > > + > > =C2=A0#define local_irq_disable() asm volatile ( "cpsid i @ > > local_irq_disable\n" : : : "cc" ) > > =C2=A0#define local_irq_enable()=C2=A0=C2=A0asm volatile ( "cpsie i @ > > local_irq_enable\n" : : : "cc" ) > >=20 > > @@ -41,6 +43,16 @@ static inline int local_irq_is_enabled(void) > > =C2=A0#define local_abort_enable() __asm__("cpsie a=C2=A0=C2=A0@ __sta\= n" : : : > > "memory", "cc") > > =C2=A0#define local_abort_disable() __asm__("cpsid a @ __sta\n" : : : > > "memory", "cc") > >=20 > > +/* We do not support tracing (at all) yet */ >=20 > I know that some bits are missing for ARM, but the patch #5 > contradicts=C2=A0 > this comment as you have CONFIG_TRACE=3Dy by default. >=20 No sure what you mean. Tracing is de facto on by default right now, despite it not being implemented for ARM. So what I'm doing is basically keeping things as they are. If you think it should be off by default, well, let's talk about it... but I'm not sure this is really what you are saying/asking. > > +#define trace_irq_disable_ret()=C2=A0=C2=A0=C2=A0do { } while ( 0 ) > > +#define trace_irq_enable_ret()=C2=A0=C2=A0=C2=A0=C2=A0do { } while ( 0= ) > > +#define trace_irq_save_ret(_x)=C2=A0=C2=A0=C2=A0=C2=A0do { } while ( 0= ) > > +#define trace_irq_restore_ret(_x) do { } while ( 0 ) > > +#define _local_irq_disable()=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0local_= irq_disable() > > +#define _local_irq_enable()=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0l= ocal_irq_enable() > > +#define _local_irq_save(_x)=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0l= ocal_irq_save(_x) > > +#define _local_irq_restore(_x)=C2=A0=C2=A0=C2=A0=C2=A0local_irq_restor= e(_x) > > + >=20 > This does not need to be duplicated in both asm- > arm/arm{32,64}/system.h.=C2=A0 > You could directly implement them in asm-arm/system.h. >=20 Ok, thanks. > Also, I would prefer if you stay consistent with x86. I.e non- > underscore=C2=A0 > versions are calling the underscore versions and not the invert. >=20 Well, I know it is counterintuitive, but it's the easiest way of getting over this, i.e., without the need of a lot of stubs, and touching as few existing code as possible. But I certainly can give it a try doing it as you say, and see how it ends up looking like. Regards, Dario --=20 <> (Raistlin Majere) ----------------------------------------------------------------- Dario Faggioli, Ph.D, http://about.me/dario.faggioli Senior Software Engineer, Citrix Systems R&D Ltd., Cambridge (UK) --=-gu3WTf2q/IeY3ewLs08r Content-Type: application/pgp-signature; name="signature.asc" Content-Description: This is a digitally signed message part Content-Transfer-Encoding: 7bit -----BEGIN PGP SIGNATURE----- Version: GnuPG v2 iQIcBAABCAAGBQJZOBocAAoJEBZCeImluHPu0p4P/2slaPRbdQNWvmpyKSBQwaoK 5I7WTMFnX0S4a91UgfwjzaFIdH/uSOaZVYJQqeo4B2C+WKQx+z+4iZNtf63UIpsP ZlIxPIheoYRrAez2vsEvl1FEYz0jCFHjcW9PRGDe4pcKw4wrQ82HW/D21x0fgew/ 1rglLV2q1LzfuBw1WFheZqns4gY87dnDQP9GAQM4Wq9D4TZtN3zSW1fOvc2+0Gu/ kbNNymnU9LigzfyBbU2ryN4aCggXnNhuzeQhM673XHnOGhz94RJclTUKgH4wRcgP mqW4NAhZ/tTOrhUyPMvCzb+CAxZD6Lks+eG+rZyIZrh04VQHtEnonFc+pLmUr08/ W8rXXx9aHOY0oFr8/CHpWPxkaZXjQZYsutDViVJ2kLd+dyXbUaEdUJGZaa1KzsCX 6s0FCVy3cHXUjdWsw/gFCkjGfgkWoG1IDIqtlg7GrBUZSwSRKyuhwN92WTy853vQ WNnzRSdig3xqcIVmzCqsIV8cBQRRKZBHE109gH0OMsWaHghonCsQjMIgtagsYvWK jVXaPildcVlLqgm742fkWr4Ki1shBM5HFEMSeSuOkTgKS9Lo94ZUZ8tKeRAcuibf voBAfbLMzqVSsdChEWJ5R+a+Teco+4UT3Q6l8h/xIKNHGBPYpWgK7J97M9wqCcYd ZRklLlvP4ip/vO/XIM9X =Lr4O -----END PGP SIGNATURE----- --=-gu3WTf2q/IeY3ewLs08r-- --===============0824219606845056877== Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: base64 Content-Disposition: inline X19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX18KWGVuLWRldmVs IG1haWxpbmcgbGlzdApYZW4tZGV2ZWxAbGlzdHMueGVuLm9yZwpodHRwczovL2xpc3RzLnhlbi5v cmcveGVuLWRldmVsCg== --===============0824219606845056877==--