From mboxrd@z Thu Jan 1 00:00:00 1970 From: Julien Grall Subject: Re: [PATCH 13/34] xen/arm: gic: Introduce GIC_SGI_MAX Date: Tue, 25 Mar 2014 23:23:40 +0000 Message-ID: <53320FFC.4060707@linaro.org> References: <1395766541-23979-1-git-send-email-julien.grall@linaro.org> <1395766541-23979-14-git-send-email-julien.grall@linaro.org> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: Received: from mail6.bemta4.messagelabs.com ([85.158.143.247]) by lists.xen.org with esmtp (Exim 4.72) (envelope-from ) id 1WSah8-0001W3-Ie for xen-devel@lists.xenproject.org; Tue, 25 Mar 2014 23:23:46 +0000 Received: by mail-wi0-f170.google.com with SMTP id bs8so3987654wib.3 for ; Tue, 25 Mar 2014 16:23:44 -0700 (PDT) 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: Stefano Stabellini Cc: xen-devel@lists.xenproject.org, tim@xen.org, ian.campbell@citrix.com, stefano.stabellini@citrix.com List-Id: xen-devel@lists.xenproject.org Hi Stefano, On 25/03/14 18:19, Stefano Stabellini wrote: > On Tue, 25 Mar 2014, Julien Grall wrote: >> All the functions that send an SGI takes an enum. Therefore checking everytime >> if the value is in the range is not correct. >> >> Introduce GIC_SGI_MAX to check the enum will never reach more than 16 values. >> >> This is fix the compilation with Clang 3.5: >> >> gic.c:515:15: error: comparison of constant 16 with expression of type 'enum gic_sgi' is always true [-Werror,-Wtautological-constant-out-of-range-compare] >> ASSERT(sgi < 16); /* There are only 16 SGIs */ >> ~~~ ^ ~~ >> xen/xen/include/xen/lib.h:43:26: note: expanded from macro 'ASSERT' >> do { if ( unlikely(!(p)) ) assert_failed(#p); } while (0) >> ^ >> xen/xen/include/xen/compiler.h:11:41: note: expanded from macro 'unlikely' >> #define unlikely(x) __builtin_expect((x),0) >> >> Signed-off-by: Julien Grall >> Cc: Ian Campbell >> Cc: Stefano Stabellini >> Cc: Tim Deegan >> --- >> xen/arch/arm/gic.c | 7 ++++--- >> xen/include/asm-arm/gic.h | 2 ++ >> 2 files changed, 6 insertions(+), 3 deletions(-) >> >> diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c >> index 0095b97..41142a5 100644 >> --- a/xen/arch/arm/gic.c >> +++ b/xen/arch/arm/gic.c >> @@ -481,7 +481,8 @@ void send_SGI_mask(const cpumask_t *cpumask, enum gic_sgi sgi) >> unsigned int mask = 0; >> cpumask_t online_mask; >> >> - ASSERT(sgi < 16); /* There are only 16 SGIs */ >> + BUILD_BUG_ON(GIC_SGI_MAX >= 16); >> + ASSERT(sgi != GIC_SGI_MAX); > > These new checks wouldn't be able to cover the following case, while the > previous check could: > > enum gic_sgi sgi = (enum gic_sgi) integer_greater_than_16; > send_SGI_mask(something, sgi); Why people would do that? I think instead of an ASSERT, sgi & 0xff might better. It won't be harmless for the GIC, even debug is turned off. Right now, the developper can put the GIC in wrong state if the value is not in this range. If people wants to give a number instead of an enum, let them go to the hell! They should have used the right type. Regards, -- Julien Grall