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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

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@xxxxxxxx>
>> Acked-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
> 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