Re: [Xen-devel] [PATCH] x86/mm: re-arrange get_page_from_l<N>e() vs pv_l1tf_check_l<N>e

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 29.08.18 at 15:16, <andrew.cooper3@xxxxxxxxxx> wrote:
> On 17/08/18 07:42, Jan Beulich wrote:
>> Restore symmetry between get_page_from_l<N>e(): pv_l1tf_check_l<N>e is
>> uniformly invoked from outside of them.
> 
> I'm not sure what symmetry you are referring to.

Just look at the current state: get_page_from_l[234]e() call
pv_l1tf_check_l[234]e(), but get_page_from_l1e() in the
same situation. Instead alloc_l1_table() does.

>>  They're no longer getting called
>> for non-present PTEs. This way the slightly odd three-way return value
>> meaning of the higher level ones can also be got rid of.
>>
>> Introduce local variables holding the page table entries processed, and
>> use them throughout the loop bodies instead of re-reading them from the
>> page table several times.
>>
>> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
> 
> With at least the above query resolved, Reviewed-by: Andrew Cooper
> <andrew.cooper3@xxxxxxxxxx>, but a style recommendation.

Thanks, but please let me know if the above clarifies this.

>> @@ -1396,8 +1369,7 @@ static int alloc_l1_table(struct page_in
>>              if ( ret )
>>                  goto out;
>>          }
>> -
>> -        switch ( ret = get_page_from_l1e(pl1e[i], d, d) )
>> +        else switch ( ret = get_page_from_l1e(pl1e[i], d, d) )
> 
> else switch isn't a usual construct (here and in ptwr_emulated_update). 
> I was expecting the misleading indentation warning to trigger, but GCC
> 8.2 does appear to be happy.

Presumably because then it would also trigger for "else if".

> Still, for code clarity, I'd suggest adding braces to the and indenting
> the switch statement.

At first I meant to do so, then decided against because of the extra
code churn in both places where I've chosen this approach (also
making looking at the patch itself less difficult). Would you be okay
with the re-indentation done in a separate follow-up patch?

Jan



_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel