| Message ID | 27a7245c-f933-5b2b-5685-d9ba2dbd4a8c@suse.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | x86/shadow: misc tidying | expand |
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...
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
--- 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) && \ + 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 } \ + if ( _done ) break; \ + increment_ptr_to_guest_entry(_gl2p); \ } \ unmap_domain_page(_sp); \ } while (0)
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. Signed-off-by: Jan Beulich <jbeulich@suse.com> --- v2: New.