On 28.03.2020 12:14, Julien Grall wrote:
> On 27/03/2020 13:15, Jan Beulich wrote:
>> On 22.03.2020 17:14, jul...@xen.org 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
