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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 17.01.2023 19:48, Andrew Cooper wrote:
> 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.

Well, I can undo that aspect, but just to get done with the change. I do
consider ...

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

... your view here at least questionable: I'm unaware of us doing so
explicitly, and I've pointed out numerous times that the C standard
specifies very clearly what underscore-prefixed names may be used for. 

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

That's simply the result of wanting to limit how much of a change I
make to the macro, such that the actual changes are easier to spot.

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

Will do. I may retain this in some form of comment.

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

Indeed, and I was unable to come up with good criteria whether to leave
it (for consistency with the other macros) or  change it. Since you're
...

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

... explicitly asking for it, I'll change then. Would you mind if I then
also added a semicolon after _code, to make things look more sensible?

Jan