Re: [PATCH 03/16] x86/shadow: drop redundant present bit checks from SHADOW_FOREACH_L<N>E() "bodys"

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 22/03/2023 9:31 am, Jan Beulich wrote:
> SHADOW_FOREACH_L<N>E() already invokes the "body" only when the present
> bit is set; no need to re-do the check.
>
> While there also
> - stop open-coding mfn_to_maddr() in code being touched (re-indented)
>   anyway,
> - stop open-coding mfn_eq() in code being touched or adjacent code,
> - drop local variables when they're no longer used at least twice.
>
> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>

While I agree this is the current behaviour, it's another spiketrap
waiting to happen.

There needs to be a rename of these macros to something like
FOREACH_PRESENT_SL?E(...).  The SHADOW prefix is a bit verbose and can
be ineligibly encoded with the L?E acronym.

After which this patch be trivially correct, and the resulting code will
read safely in context.

>
> --- a/xen/arch/x86/mm/shadow/multi.c
> +++ b/xen/arch/x86/mm/shadow/multi.c
> @@ -3647,13 +3632,10 @@ int cf_check sh_remove_l1_shadow(struct
>  {
>      shadow_l2e_t *sl2e;
>      int done = 0;
> -    int flags;
>  
>      SHADOW_FOREACH_L2E(sl2mfn, sl2e, 0, done, d,
>      {
> -        flags = shadow_l2e_get_flags(*sl2e);
> -        if ( (flags & _PAGE_PRESENT)
> -             && (mfn_x(shadow_l2e_get_mfn(*sl2e)) == mfn_x(sl1mfn)) )
> +        if ( mfn_x(shadow_l2e_get_mfn(*sl2e)) == mfn_x(sl1mfn) )

You've done mfn_eq() conversions thus far, but the final 3 hunks of the
patch could do with a conversion too.

With that, and subject to a suitable rename of the macros ahead of this
change, Reviewed-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>