| Message ID | 7d2dd28a-a821-e906-6245-ab26e5518706@suse.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | x86/shadow: misc tidying | expand |
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@suse.com> Acked-by: Andrew Cooper <andrew.cooper3@citrix.com> > --- 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
On 09.02.2023 18:26, Andrew Cooper wrote: > 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@suse.com> > > Acked-by: Andrew Cooper <andrew.cooper3@citrix.com> Thanks. >> --- 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. I would certainly have done it without pushing out the escape, but both alternative variants look worse to me: In /* Implies !shadow_mode_external(_dom) */ \ if ( 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); \ it isn't clear that the comment applies to only the first part of the conditions, whereas if ( /* Implies !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); \ looks more clumsy to me. I'm not very likely to accept a suggestion to for the former route; if you think the latter variant is strictly better than the original, I might make the change while committing. Hmm, or maybe if ( mfn_to_page(_sl2mfn)->u.sh.type != SH_type_l2_64_shadow ) \ /* Implies !shadow_mode_external(_dom): */ \ is_pv_32bit_domain(_dom) && \ _end = COMPAT_L2_PAGETABLE_FIRST_XEN_SLOT(_dom); \ ? Jan
On 10/02/2023 7:07 am, Jan Beulich wrote: > On 09.02.2023 18:26, Andrew Cooper wrote: >> 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@suse.com> >> Acked-by: Andrew Cooper <andrew.cooper3@citrix.com> > Thanks. > >>> --- 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. > I would certainly have done it without pushing out the escape, but both > alternative variants look worse to me: In > > /* Implies !shadow_mode_external(_dom) */ \ > if ( 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); \ > > it isn't clear that the comment applies to only the first part of the > conditions, whereas > > if ( /* Implies !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); \ > > looks more clumsy to me. I'm not very likely to accept a suggestion to > for the former route; if you think the latter variant is strictly > better than the original, I might make the change while committing. > > Hmm, or maybe > > if ( mfn_to_page(_sl2mfn)->u.sh.type != SH_type_l2_64_shadow ) \ > /* Implies !shadow_mode_external(_dom): */ \ > is_pv_32bit_domain(_dom) && \ > _end = COMPAT_L2_PAGETABLE_FIRST_XEN_SLOT(_dom); \ > > ? I simply meant: - for ( _i = 0; _i < SHADOW_L2_PAGETABLE_ENTRIES; _i++ ) \ + if ( is_pv_32bit_domain(_dom) /* implies !shadow_mode_external */ && \ (If this renderers properly.) It is not important for the comment to be syntactically valid C, especially as you're saying one predicate implies the other. ~Andrew
--- 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) */ && \ + 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 ) \ { \ - (_sl2e) = _sp + _i; \ - if ( shadow_l2e_get_flags(*(_sl2e)) & _PAGE_PRESENT ) \ - {_code} \ - if ( _done ) break; \ - increment_ptr_to_guest_entry(_gl2p); \ + _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, 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@suse.com> --- v3: Drop shadow_mode_external(). Switch back from using trailing underscores. Convert style to be fully conformant. v2: New.