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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

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.

>
>>>> 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.

~Andrew

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