Re: [PATCH] x86/shadow: fix DO_UNSHADOW()

[Luca Fancellu <luca.fancellu@xxxxxxx>]

> On 19 May 2021, at 13:36, Jan Beulich <jbeulich@xxxxxxxx> 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@xxxxxxxx>

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

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);
>