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...
