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

[Andrew Cooper <Andrew.Cooper3@xxxxxxxxxx>]

On 11/01/2023 1:54 pm, Jan Beulich wrote:
> First of all move the almost loop-invariant condition out of the loop;
> transform it into an altered loop boundary. Since the new local variable
> wants to be "unsigned int" and named without violating name space rules,
> convert the loop induction variable accordingly.

I'm firmly -1 against using trailing underscores.

Mainly, I object to the attempt to justify doing so based on a rule we
explicitly choose to violate for code consistency and legibility reasons.

But in this case, you're taking a block of logic which was cleanly in
one style, and making it mixed, even amongst only the local variables.

> --- a/xen/arch/x86/mm/shadow/multi.c
> +++ b/xen/arch/x86/mm/shadow/multi.c
> @@ -863,23 +863,20 @@ 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 ( !shadow_mode_external(_dom) &&                                     \
> +         is_pv_32bit_domain(_dom) &&                                        \

The second clause here implies the first.  Given that all we're trying
to say here is "are there Xen entries to skip", I think we'd be fine
dropping the external() check entirely.

> +         mfn_to_page(_sl2mfn)->u.sh.type != SH_type_l2_64_shadow )          \
> +        end_ = COMPAT_L2_PAGETABLE_FIRST_XEN_SLOT(_dom);                    \
> +    for ( i_ = 0; i_ < end_; ++i_ )                                         \
>      {                                                                       \
> -        if ( (!(_xen))                                                      \
> -             || !is_pv_32bit_domain(_dom)                                   \
> -             || mfn_to_page(_sl2mfn)->u.sh.type == SH_type_l2_64_shadow     \
> -             || (_i < COMPAT_L2_PAGETABLE_FIRST_XEN_SLOT(_dom)) )           \
> -        {                                                                   \
> -            (_sl2e) = _sp + _i;                                             \
> -            if ( shadow_l2e_get_flags(*(_sl2e)) & _PAGE_PRESENT )           \
> -                {_code}                                                     \
> -            if ( _done ) break;                                             \
> -            increment_ptr_to_guest_entry(_gl2p);                            \
> -        }                                                                   \
> +        (_sl2e) = _sp + i_;                                                 \
> +        if ( shadow_l2e_get_flags(*(_sl2e)) & _PAGE_PRESENT )               \
> +            { _code }                                                       \

This doesn't match either of our two styles. 

if ( ... )
{ _code }

would be closer to Xen's normal style, but  ...

> +        if ( _done ) break;                                                 \

... with this too, I think it would still be better to write it out
fully, so:

if ( ... )
{
    _code
}
if ( _done )
    break;

These macros are already big enough that trying to save 3 lines seems
futile.

~Andrew

> +        increment_ptr_to_guest_entry(_gl2p);                                \

P.S. I'm pretty sure I had encountered this before, but I'm re-reminded
of how much this function seems like a bad idea, even in the context of
this macroset taking arbitrary _done and _code blobs...