Re: [PATCH v3 4/4] x86/shadow: re-work 4-level SHADOW_FOREACH_L2E()

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 08/02/2023 2:38 pm, Jan Beulich wrote:
> First of all move the almost loop-invariant condition out of the loop;
> transform it into an altered loop boundary, noting that the updating of
> _gl2p is relevant only at one use site, and then also only inside the
> _code blob it provides. Then drop the shadow_mode_external() part of the
> condition as being redundant with the is_pv_32bit_domain() check.
> Further, since the new local variable wants to be "unsigned int",
> convert the loop induction variable accordingly. Finally also adjust
> formatting as most code needs touching anyway.
>
> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>

Acked-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>

> --- a/xen/arch/x86/mm/shadow/multi.c
> +++ b/xen/arch/x86/mm/shadow/multi.c
> @@ -861,23 +861,22 @@ do {
>  /* 64-bit l2: touch all entries except for PAE compat guests. */
>  #define SHADOW_FOREACH_L2E(_sl2mfn, _sl2e, _gl2p, _done, _dom, _code)       \
>  do {                                                                        \
> -    int _i;                                                                 \
> -    int _xen = !shadow_mode_external(_dom);                                 \
> +    unsigned int _i, _end = SHADOW_L2_PAGETABLE_ENTRIES;                    \
>      shadow_l2e_t *_sp = map_domain_page((_sl2mfn));                         \
>      ASSERT_VALID_L2(mfn_to_page(_sl2mfn)->u.sh.type);                       \
> -    for ( _i = 0; _i < SHADOW_L2_PAGETABLE_ENTRIES; _i++ )                  \
> +    if ( is_pv_32bit_domain(_dom) /* implies !shadow_mode_external(_dom) */ 
> && \

As this is a comment, I think can reasonably be

/* implies !shadow_mode_external */

which shortens it enough to maintain the RHS justification.

~Andrew