[PATCH] xen/arm: Fix ARM build following c/s 11c397c

[PATCH] xen/arm: Fix ARM build following c/s 11c397c

[Andrew Cooper]

c/s 11c397c broke the ARM build by introducing a common ACCESS_ONCE() which is
different to the definiton in smmu.c

The SMMU code included a scalar typecheck, which is worth keeping in the
common case, given ACCESS_ONCE()'s restrictions.  However, express the
typecheck differently so as to avoid Coverity compliants about unused
variables.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Stefano Stabellini <sstabellini@kernel.org>
CC: Julien Grall <julien.grall@arm.com>
---
 xen/drivers/passthrough/arm/smmu.c | 10 ----------
 xen/include/xen/lib.h              |  5 ++++-
 2 files changed, 4 insertions(+), 11 deletions(-)

diff --git a/xen/drivers/passthrough/arm/smmu.c b/xen/drivers/passthrough/arm/smmu.c
index cf8b8b8..06c0a45 100644
--- a/xen/drivers/passthrough/arm/smmu.c
+++ b/xen/drivers/passthrough/arm/smmu.c
@@ -198,16 +198,6 @@ typedef paddr_t phys_addr_t;
 
 #define VA_BITS		0	/* Only used for configuring stage-1 input size */
 
-/* The macro ACCESS_ONCE start to be replaced in Linux in favor of
- * {READ, WRITE}_ONCE. Rather than introducing in the common code, keep a
- * version here. We will have to drop it when the SMMU code in Linux will
- * switch to {READ, WRITE}_ONCE.
- */
-#define __ACCESS_ONCE(x) ({ \
-	 __maybe_unused typeof(x) __var = 0; \
-	(volatile typeof(x) *)&(x); })
-#define ACCESS_ONCE(x) (*__ACCESS_ONCE(x))
-
 #define MODULE_DEVICE_TABLE(type, name)
 #define module_param_named(name, value, type, perm)
 #define MODULE_PARM_DESC(_parm, desc)
diff --git a/xen/include/xen/lib.h b/xen/include/xen/lib.h
index 1976e4b..995a85a 100644
--- a/xen/include/xen/lib.h
+++ b/xen/include/xen/lib.h
@@ -56,7 +56,10 @@
 
 #define ARRAY_SIZE(x) (sizeof(x) / sizeof((x)[0]) + __must_be_array(x))
 
-#define ACCESS_ONCE(x) (*(volatile typeof(x) *)&(x))
+#define __ACCESS_ONCE(x) ({                             \
+            (void)(typeof(x))0; /* Scalar typecheck. */ \
+            (volatile typeof(x) *)&(x); })
+#define ACCESS_ONCE(x) (*__ACCESS_ONCE(x))
 
 #define MASK_EXTR(v, m) (((v) & (m)) / ((m) & -(m)))
 #define MASK_INSR(v, m) (((v) * ((m) & -(m))) & (m))
-- 
2.1.4


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH] xen/arm: Fix ARM build following c/s 11c397c

[Julien Grall]

Hi Andrew,

On 08/02/17 19:10, Andrew Cooper wrote:
> c/s 11c397c broke the ARM build by introducing a common ACCESS_ONCE() which is
> different to the definiton in smmu.c
>
> The SMMU code included a scalar typecheck, which is worth keeping in the
> common case, given ACCESS_ONCE()'s restrictions.  However, express the
> typecheck differently so as to avoid Coverity compliants about unused

s/compliants/complaint/

> variables.

OOI, the variable is marked as "__maybe_unused", so why Coverity would 
complaint?

>
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Acked-by: Julien Grall <julien.grall@arm.com>

Cheers,

> ---
> CC: Jan Beulich <JBeulich@suse.com>
> CC: Stefano Stabellini <sstabellini@kernel.org>
> CC: Julien Grall <julien.grall@arm.com>
> ---
>  xen/drivers/passthrough/arm/smmu.c | 10 ----------
>  xen/include/xen/lib.h              |  5 ++++-
>  2 files changed, 4 insertions(+), 11 deletions(-)
>
> diff --git a/xen/drivers/passthrough/arm/smmu.c b/xen/drivers/passthrough/arm/smmu.c
> index cf8b8b8..06c0a45 100644
> --- a/xen/drivers/passthrough/arm/smmu.c
> +++ b/xen/drivers/passthrough/arm/smmu.c
> @@ -198,16 +198,6 @@ typedef paddr_t phys_addr_t;
>
>  #define VA_BITS		0	/* Only used for configuring stage-1 input size */
>
> -/* The macro ACCESS_ONCE start to be replaced in Linux in favor of
> - * {READ, WRITE}_ONCE. Rather than introducing in the common code, keep a
> - * version here. We will have to drop it when the SMMU code in Linux will
> - * switch to {READ, WRITE}_ONCE.
> - */
> -#define __ACCESS_ONCE(x) ({ \
> -	 __maybe_unused typeof(x) __var = 0; \
> -	(volatile typeof(x) *)&(x); })
> -#define ACCESS_ONCE(x) (*__ACCESS_ONCE(x))
> -
>  #define MODULE_DEVICE_TABLE(type, name)
>  #define module_param_named(name, value, type, perm)
>  #define MODULE_PARM_DESC(_parm, desc)
> diff --git a/xen/include/xen/lib.h b/xen/include/xen/lib.h
> index 1976e4b..995a85a 100644
> --- a/xen/include/xen/lib.h
> +++ b/xen/include/xen/lib.h
> @@ -56,7 +56,10 @@
>
>  #define ARRAY_SIZE(x) (sizeof(x) / sizeof((x)[0]) + __must_be_array(x))
>
> -#define ACCESS_ONCE(x) (*(volatile typeof(x) *)&(x))
> +#define __ACCESS_ONCE(x) ({                             \
> +            (void)(typeof(x))0; /* Scalar typecheck. */ \
> +            (volatile typeof(x) *)&(x); })
> +#define ACCESS_ONCE(x) (*__ACCESS_ONCE(x))
>
>  #define MASK_EXTR(v, m) (((v) & (m)) / ((m) & -(m)))
>  #define MASK_INSR(v, m) (((v) * ((m) & -(m))) & (m))
>

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH] xen/arm: Fix ARM build following c/s 11c397c

[Andrew Cooper]

On 08/02/17 19:13, Julien Grall wrote:
> Hi Andrew,
>
> On 08/02/17 19:10, Andrew Cooper wrote:
>> c/s 11c397c broke the ARM build by introducing a common ACCESS_ONCE()
>> which is
>> different to the definiton in smmu.c
>>
>> The SMMU code included a scalar typecheck, which is worth keeping in the
>> common case, given ACCESS_ONCE()'s restrictions.  However, express the
>> typecheck differently so as to avoid Coverity compliants about unused
>
> s/compliants/complaint/

In this case, it is multiple individual complains about unused
individual variables, so "complaints" is scans perfectly well.

An alternative would be "to avoid Coverity complaining about..." if you
prefer?

>
>> variables.
>
> OOI, the variable is marked as "__maybe_unused", so why Coverity would
> complaint?

The entire purpose of Coverity is to second guess what the programmer
actually wrote when it looks suspicious.

As for this specific example, I believe that the annotation doesn't even
survive into the GCC Abstract Syntax Tree, which means Coverity doesn't
get to see it.  Even if it does, the complaint of "This variable is
unused - why do you need it?" is still valid.

~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH] xen/arm: Fix ARM build following c/s 11c397c

[Julien Grall]

On 08/02/17 19:21, Andrew Cooper wrote:
> On 08/02/17 19:13, Julien Grall wrote:
>> Hi Andrew,
>>
>> On 08/02/17 19:10, Andrew Cooper wrote:
>>> c/s 11c397c broke the ARM build by introducing a common ACCESS_ONCE()
>>> which is
>>> different to the definiton in smmu.c

Forgot this one s/definiton/definition/

>>>
>>> The SMMU code included a scalar typecheck, which is worth keeping in the
>>> common case, given ACCESS_ONCE()'s restrictions.  However, express the
>>> typecheck differently so as to avoid Coverity compliants about unused
>>
>> s/compliants/complaint/
>
> In this case, it is multiple individual complains about unused
> individual variables, so "complaints" is scans perfectly well.
>
> An alternative would be "to avoid Coverity complaining about..." if you
> prefer?

Sorry I was flagging the typo "i" and "a" inverted and not the plural.

>
>>
>>> variables.
>>
>> OOI, the variable is marked as "__maybe_unused", so why Coverity would
>> complaint?
>
> The entire purpose of Coverity is to second guess what the programmer
> actually wrote when it looks suspicious.
>
> As for this specific example, I believe that the annotation doesn't even
> survive into the GCC Abstract Syntax Tree, which means Coverity doesn't
> get to see it.  Even if it does, the complaint of "This variable is
> unused - why do you need it?" is still valid.

Oh, thank you for the explanation.

Cheers,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH] xen/arm: Fix ARM build following c/s 11c397c

[Andrew Cooper]

On 08/02/2017 19:24, Julien Grall wrote:
>
>
> On 08/02/17 19:21, Andrew Cooper wrote:
>> On 08/02/17 19:13, Julien Grall wrote:
>>> Hi Andrew,
>>>
>>> On 08/02/17 19:10, Andrew Cooper wrote:
>>>> c/s 11c397c broke the ARM build by introducing a common ACCESS_ONCE()
>>>> which is
>>>> different to the definiton in smmu.c
>
> Forgot this one s/definiton/definition/
>
>>>>
>>>> The SMMU code included a scalar typecheck, which is worth keeping
>>>> in the
>>>> common case, given ACCESS_ONCE()'s restrictions.  However, express the
>>>> typecheck differently so as to avoid Coverity compliants about unused
>>>
>>> s/compliants/complaint/
>>
>> In this case, it is multiple individual complains about unused
>> individual variables, so "complaints" is scans perfectly well.
>>
>> An alternative would be "to avoid Coverity complaining about..." if you
>> prefer?
>
> Sorry I was flagging the typo "i" and "a" inverted and not the plural.

Ah, sorry.  I hadn't even spotted that.

Will fix both.

~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH] xen/arm: Fix ARM build following c/s 11c397c

[Stefano Stabellini]

On Wed, 8 Feb 2017, Andrew Cooper wrote:
> On 08/02/2017 19:24, Julien Grall wrote:
> >
> >
> > On 08/02/17 19:21, Andrew Cooper wrote:
> >> On 08/02/17 19:13, Julien Grall wrote:
> >>> Hi Andrew,
> >>>
> >>> On 08/02/17 19:10, Andrew Cooper wrote:
> >>>> c/s 11c397c broke the ARM build by introducing a common ACCESS_ONCE()
> >>>> which is
> >>>> different to the definiton in smmu.c
> >
> > Forgot this one s/definiton/definition/
> >
> >>>>
> >>>> The SMMU code included a scalar typecheck, which is worth keeping
> >>>> in the
> >>>> common case, given ACCESS_ONCE()'s restrictions.  However, express the
> >>>> typecheck differently so as to avoid Coverity compliants about unused
> >>>
> >>> s/compliants/complaint/
> >>
> >> In this case, it is multiple individual complains about unused
> >> individual variables, so "complaints" is scans perfectly well.
> >>
> >> An alternative would be "to avoid Coverity complaining about..." if you
> >> prefer?
> >
> > Sorry I was flagging the typo "i" and "a" inverted and not the plural.
> 
> Ah, sorry.  I hadn't even spotted that.
> 
> Will fix both.
>

I fixed the commit message and applied.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel