When adding the HASH_CALLBACKS_CHECK() I failed to properly recognize
the (somewhat unusually formatted) if() around the call to
hash_domain_foreach()). Gcc 11 is absolutely right in pointing out the
apparently misleading indentation. Besides adding the missing braces,
also adjust the two oddly formatted if()-s in the macro.

Fixes: 90629587e16e ("x86/shadow: replace stale literal numbers in hash_{vcpu,domain}_foreach()")
Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
I'm puzzled as to why this bug didn't cause any fallout.

--- a/xen/arch/x86/mm/shadow/common.c
+++ b/xen/arch/x86/mm/shadow/common.c
@@ -2220,8 +2220,8 @@ void sh_remove_shadows(struct domain *d,
      */
 #define DO_UNSHADOW(_type) do {                                         \
     t = (_type);                                                        \
-    if( !(pg->count_info & PGC_page_table)                              \
-        || !(pg->shadow_flags & (1 << t)) )                             \
+    if ( !(pg->count_info & PGC_page_table) ||                          \
+         !(pg->shadow_flags & (1 << t)) )                               \
         break;                                                          \
     smfn = shadow_hash_lookup(d, mfn_x(gmfn), t);                       \
     if ( unlikely(!mfn_valid(smfn)) )                                   \
@@ -2235,11 +2235,13 @@ void sh_remove_shadows(struct domain *d,
         sh_unpin(d, smfn);                                              \
     else if ( sh_type_has_up_pointer(d, t) )                            \
         sh_remove_shadow_via_pointer(d, smfn);                          \
-    if( !fast                                                           \
-        && (pg->count_info & PGC_page_table)                            \
-        && (pg->shadow_flags & (1 << t)) )                              \
+    if ( !fast &&                                                       \
+         (pg->count_info & PGC_page_table) &&                           \
+         (pg->shadow_flags & (1 << t)) )                                \
+    {                                                                   \
         HASH_CALLBACKS_CHECK(SHF_page_type_mask);                       \
         hash_domain_foreach(d, masks[t], callbacks, smfn);              \
+    }                                                                   \
 } while (0)
 
     DO_UNSHADOW(SH_type_l2_32_shadow);

> On 19 May 2021, at 13:36, Jan Beulich <jbeulich@suse.com> wrote:
> 
> When adding the HASH_CALLBACKS_CHECK() I failed to properly recognize
> the (somewhat unusually formatted) if() around the call to
> hash_domain_foreach()). Gcc 11 is absolutely right in pointing out the
> apparently misleading indentation. Besides adding the missing braces,
> also adjust the two oddly formatted if()-s in the macro.
> 
> Fixes: 90629587e16e ("x86/shadow: replace stale literal numbers in hash_{vcpu,domain}_foreach()")
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Reviewed-by: Luca Fancellu <luca.fancellu@arm.com>

Cheers,
Luca

> ---
> I'm puzzled as to why this bug didn't cause any fallout.
> 
> --- a/xen/arch/x86/mm/shadow/common.c
> +++ b/xen/arch/x86/mm/shadow/common.c
> @@ -2220,8 +2220,8 @@ void sh_remove_shadows(struct domain *d,
>      */
> #define DO_UNSHADOW(_type) do {                                         \
>     t = (_type);                                                        \
> -    if( !(pg->count_info & PGC_page_table)                              \
> -        || !(pg->shadow_flags & (1 << t)) )                             \
> +    if ( !(pg->count_info & PGC_page_table) ||                          \
> +         !(pg->shadow_flags & (1 << t)) )                               \
>         break;                                                          \
>     smfn = shadow_hash_lookup(d, mfn_x(gmfn), t);                       \
>     if ( unlikely(!mfn_valid(smfn)) )                                   \
> @@ -2235,11 +2235,13 @@ void sh_remove_shadows(struct domain *d,
>         sh_unpin(d, smfn);                                              \
>     else if ( sh_type_has_up_pointer(d, t) )                            \
>         sh_remove_shadow_via_pointer(d, smfn);                          \
> -    if( !fast                                                           \
> -        && (pg->count_info & PGC_page_table)                            \
> -        && (pg->shadow_flags & (1 << t)) )                              \
> +    if ( !fast &&                                                       \
> +         (pg->count_info & PGC_page_table) &&                           \
> +         (pg->shadow_flags & (1 << t)) )                                \
> +    {                                                                   \
>         HASH_CALLBACKS_CHECK(SHF_page_type_mask);                       \
>         hash_domain_foreach(d, masks[t], callbacks, smfn);              \
> +    }                                                                   \
> } while (0)
> 
>     DO_UNSHADOW(SH_type_l2_32_shadow);
> 

At 14:36 +0200 on 19 May (1621434982), Jan Beulich wrote:
> When adding the HASH_CALLBACKS_CHECK() I failed to properly recognize
> the (somewhat unusually formatted) if() around the call to
> hash_domain_foreach()). Gcc 11 is absolutely right in pointing out the
> apparently misleading indentation. Besides adding the missing braces,
> also adjust the two oddly formatted if()-s in the macro.
> 
> Fixes: 90629587e16e ("x86/shadow: replace stale literal numbers in hash_{vcpu,domain}_foreach()")
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Reviewed-by: Tim Deegan <tim@xen.org>