Re: [Xen-devel] [PATCH] x86/mm: drop further relics of translated PV domains

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 12.06.17 at 12:52, <andrew.cooper3@xxxxxxxxxx> wrote:
> On 12/06/17 11:44, Jan Beulich wrote:
>>>>> On 12.06.17 at 12:37, <andrew.cooper3@xxxxxxxxxx> wrote:
>>> On 12/06/17 07:28, Jan Beulich wrote:
>>>>>>> On 09.06.17 at 19:38, <andrew.cooper3@xxxxxxxxxx> wrote:
>>>>> On 08/06/17 16:30, Jan Beulich wrote:
>>>>>> For PV domains paging_mode_{refcounts,translate}() are always false as
>>>>>> of commits 4045953527 ("x86/paging: Enforce PG_external == PG_translate
>>>>>> == PG_refcounts") and 92942fd3d4 ("x86/mm: drop
>>>>>> guest_{map,get_eff}_l1e() hooks").
>>>>>>
>>>>>> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
>>>>> Reviewed-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
>>>> Thanks.
>>>>
>>>>> There are more cases as well.  I will rebase my series over this patch
>>>>> when you commit it, because the extra cases only become obvious after
>>>>> the other cleanup which is still pending. 
>>>> Oh, interesting. I'm curious to see what further ones I didn't spot.
>>> There is a pattern in several do_mmuext_op() subops which is:
>>>
>>> if ( currd != pg_owner )
>>>     rc = -EPERM;
>>> else if ( paging_mode_translate(currd) )
>>>     rc = -EINVAL;
>>>
>>> This is equivalent to paging_mode_translate(pg_owner).
>> But pg_owner can generally be translated (i.e. HVM).
> 
> Not in this case.  The start of do_mmuext_op() does
> 
>     if ( !is_pv_domain(pg_owner) )
>     {
>         put_pg_owner(pg_owner);
>         return -EINVAL;
>     }
> 
> meaning that do_mmuext_op() can strictly only operate on PV domains.

Ah, I see. paging_mode_refcounts() checks are then pointless
there too.

>>>>> One style query though...
>>>>>
>>>>>> @@ -3384,11 +3368,9 @@ long do_mmuext_op(
>>>>>>  
>>>>>>              if ( op.arg1.mfn != 0 )
>>>>>>              {
>>>>>> -                if ( paging_mode_refcounts(d) )
>>>>>> -                    rc = get_page_from_pagenr(op.arg1.mfn, d) ? 0 : 
>>>>>> -EINVAL;
>>>>>> -                else
>>>>>> -                    rc = get_page_and_type_from_pagenr(
>>>>>> -                        op.arg1.mfn, PGT_root_page_table, d, 0, 1);
>>>>>> +                rc = get_page_and_type_from_pagenr(op.arg1.mfn,
>>>>>> +                                                   PGT_root_page_table,
>>>>>> +                                                   d, 0, 1);
>>>>> Why do you choose to squash the parameters on the right hand side?  For
>>>>> cases like this, the style of the old code is neater IMO.
>>>> I think this alternative style is contrary to general style guidelines,
>>> Which guidelines where?
>> Well, I admit "Long lines should be split at sensible places and the
>> trailing portions indented" can be read in various different ways,
>> especially with there being nothing said on what the indented
>> trailing portion should align with. So I guess it's rather my
>> interpretation of that rule.
> 
> Do you honestly think that squashing everything on the right hand side
> is neater or easier to read?  Because I certainly don't.

The longer the function name gets, the less I'd be of that opinion,
but generally I think keeping at least the first argument on the
same line as the function (and the thus resulting deeper indentation
for the others) make the function call better stand out as such (to
ease separation from other kinds of expressions / statements).

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel