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.
--- 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)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
© 2016 - 2026 Red Hat, Inc.