Re: [Xen-devel] [PATCH 16/17] xen/mm: Convert {s, g}et_gpfn_from_mfn() to use typesafe MFN

[Jan Beulich <jbeulich@xxxxxxxx>]

On 28.03.2020 12:14, Julien Grall wrote:
> On 27/03/2020 13:15, Jan Beulich wrote:
>> On 22.03.2020 17:14, julien@xxxxxxx wrote:
>>> @@ -983,19 +984,20 @@ long p2m_pt_audit_p2m(struct p2m_domain *p2m)
>>>                   /* check for 1GB super page */
>>>                   if ( l3e_get_flags(l3e[i3]) & _PAGE_PSE )
>>>                   {
>>> -                    mfn = l3e_get_pfn(l3e[i3]);
>>> -                    ASSERT(mfn_valid(_mfn(mfn)));
>>> +                    mfn = l3e_get_mfn(l3e[i3]);
>>> +                    ASSERT(mfn_valid(mfn));
>>>                       /* we have to cover 512x512 4K pages */
>>>                       for ( i2 = 0;
>>>                             i2 < (L2_PAGETABLE_ENTRIES * 
>>> L1_PAGETABLE_ENTRIES);
>>>                             i2++)
>>>                       {
>>> -                        m2pfn = get_gpfn_from_mfn(mfn+i2);
>>> +                        m2pfn = get_pfn_from_mfn(mfn_add(mfn, i2));
>>>                           if ( m2pfn != (gfn + i2) )
>>>                           {
>>>                               pmbad++;
>>> -                            P2M_PRINTK("mismatch: gfn %#lx -> mfn %#lx -> 
>>> gfn %#lx\n",
>>> -                                       gfn + i2, mfn + i2, m2pfn);
>>> +                            P2M_PRINTK("mismatch: gfn %#lx -> mfn 
>>> %"PRI_mfn" gfn %#lx\n",
>>> +                                       gfn + i2, mfn_x(mfn_add(mfn, i2)),
>>
>> As in the earlier patch, "mfn_x(mfn) + i2" would be shorter and
>> hence imo preferable, especially in printk() and alike invocations.
> 
> The goal of using typesafe is to make the code safer not try to
> open-code everything because it might be shorter to write.

I'm not talking about "everything". As soon as you use mfn_x()
_anywhere_, type-safety is gone. Since in printk() and alike you
unavoidably have to use it (at least for now), there's no win
from using e.g. mfn_add() as you do here, imo. And hence the
readability aspect gets even higher significance.

>> I would also prefer if you left %#lx alone, with the 2nd best
>> option being to also use PRI_gfn alongside PRI_mfn. Primarily
>> I'd like to avoid having a mixture.
> The two options would be wrong:
>     * gfn is an unsigned long and not gfn_t, so using PRI_gfn would be 
> incorrect
>     * mfn is now an mfn_t so using %lx would be incorrect
> 
> So the format string used in the patch is correct based on the types used.

Hmm, xen/mm.h suggests a partial connection between e.g. mfn_t
and PRI_mfn, yes, but I think this is unhelpful as long as
mfn_x() needs to be explicitly used when specifying the printk()
arguments. Instead I view PRI_mfn and alike as a more general
format usable also for MFNs stored in unsigned long rather than
mfn_t.

I agree though that views here may differ. Hence wider agreement
on what the intentions are (also mid/long term), and hence how
well formed code ought to look like, would seem necessary here.

> This...
> 
>>
>> Same (for both) at least one more time further down.
> 
> ... would likely be applicable for all the other uses.

Agreed.

>>> --- a/xen/include/asm-x86/mm.h
>>> +++ b/xen/include/asm-x86/mm.h
>>> @@ -500,9 +500,10 @@ extern paddr_t mem_hotplug;
>>>    */
>>>   extern bool machine_to_phys_mapping_valid;
>>>   -static inline void set_gpfn_from_mfn(unsigned long mfn, unsigned long 
>>> pfn)
>>> +static inline void set_pfn_from_mfn(mfn_t mfn_, unsigned long pfn)
>>>   {
>>> -    const struct domain *d = page_get_owner(mfn_to_page(_mfn(mfn)));
>>> +    const unsigned long mfn = mfn_x(mfn_);
>>
>> I think it would be better overall if the parameter was named
>> "mfn" and there was no local variable altogether. This would
>> bring things in line with ...
> 
> You asked for this approach on the previous version [1]:
> 
> "Btw, the cheaper (in terms of code churn) change here would seem to
> be to name the function parameter mfn_, and the local variable mfn.
> That'll also reduce the number of uses of the unfortunate trailing-
> underscore-name."
> 
> So can you pick a side and stick with it?

Well, things like this happen when you see the final result, sorry.
And indeed I recalled commenting on this before, but upon searching
I didn't manage to find the earlier reply, to better justify what I
also suspected might have been a change of mind.

Jan