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

From: Julien Grall <julien@xen.org>
To: Jan Beulich <jbeulich@suse.com>
Cc: "Stefano Stabellini" <sstabellini@kernel.org>,
	"Wei Liu" <wl@xen.org>,
	"Andrew Cooper" <andrew.cooper3@citrix.com>,
	"Ian Jackson" <ian.jackson@eu.citrix.com>,
	"George Dunlap" <george.dunlap@citrix.com>,
	"Julien Grall" <julien.grall@arm.com>,
	"Tamas K Lengyel" <tamas@tklengyel.com>,
	xen-devel@lists.xenproject.org,
	"Volodymyr Babchuk" <Volodymyr_Babchuk@epam.com>,
	"Roger Pau Monné" <roger.pau@citrix.com>
Subject: Re: [Xen-devel] [PATCH 16/17] xen/mm: Convert {s, g}et_gpfn_from_mfn() to use typesafe MFN
Date: Sat, 28 Mar 2020 11:14:16 +0000	[thread overview]
Message-ID: <c640eb5b-224f-e99a-daa2-6def00780e54@xen.org> (raw)
In-Reply-To: <a3d120d0-d67f-bed2-4920-0d3a1c3090ea@suse.com>

Hi,

On 27/03/2020 13:15, Jan Beulich wrote:
> On 22.03.2020 17:14, julien@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 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. This...

> 
> Same (for both) at least one more time further down.

... would likely be applicable for all the other uses.

>> @@ -974,7 +974,7 @@ guest_physmap_add_entry(struct domain *d, gfn_t gfn, mfn_t mfn,
>>                   P2M_DEBUG("old gfn=%#lx -> mfn %#lx\n",
>>                             gfn_x(ogfn) , mfn_x(omfn));
>>                   if ( mfn_eq(omfn, mfn_add(mfn, i)) )
>> -                    p2m_remove_page(p2m, gfn_x(ogfn), mfn_x(mfn_add(mfn, i)),
>> +                    p2m_remove_page(p2m, gfn_x(ogfn), mfn_add(mfn, i),
>>                                       0);
> 
> Pull this up then onto the now shorter prior line?

Ok.

> 
>> @@ -2843,53 +2843,53 @@ void audit_p2m(struct domain *d,
>>       spin_lock(&d->page_alloc_lock);
>>       page_list_for_each ( page, &d->page_list )
>>       {
>> -        mfn = mfn_x(page_to_mfn(page));
>> +        mfn = page_to_mfn(page);
>>   
>> -        P2M_PRINTK("auditing guest page, mfn=%#lx\n", mfn);
>> +        P2M_PRINTK("auditing guest page, mfn=%"PRI_mfn"\n", mfn_x(mfn));
>>   
>>           od = page_get_owner(page);
>>   
>>           if ( od != d )
>>           {
>> -            P2M_PRINTK("mfn %"PRI_mfn" owner %pd != %pd\n", mfn, od, d);
>> +            P2M_PRINTK("mfn %"PRI_mfn" owner %pd != %pd\n", mfn_x(mfn), od, d);
>>               continue;
>>           }
>>   
>> -        gfn = get_gpfn_from_mfn(mfn);
>> +        gfn = get_pfn_from_mfn(mfn);
>>           if ( gfn == INVALID_M2P_ENTRY )
>>           {
>>               orphans_count++;
>> -            P2M_PRINTK("orphaned guest page: mfn=%#lx has invalid gfn\n",
>> -                           mfn);
>> +            P2M_PRINTK("orphaned guest page: mfn=%"PRI_mfn" has invalid gfn\n",
>> +                       mfn_x(mfn));
>>               continue;
>>           }
>>   
>>           if ( SHARED_M2P(gfn) )
>>           {
>> -            P2M_PRINTK("shared mfn (%lx) on domain page list!\n",
>> -                    mfn);
>> +            P2M_PRINTK("shared mfn (%"PRI_mfn") on domain page list!\n",
>> +                       mfn_x(mfn));
>>               continue;
>>           }
>>   
>>           p2mfn = get_gfn_type_access(p2m, gfn, &type, &p2ma, 0, NULL);
>> -        if ( mfn_x(p2mfn) != mfn )
>> +        if ( !mfn_eq(p2mfn, mfn) )
>>           {
>>               mpbad++;
>> -            P2M_PRINTK("map mismatch mfn %#lx -> gfn %#lx -> mfn %#lx"
>> +            P2M_PRINTK("map mismatch mfn %"PRI_mfn" -> gfn %#lx -> mfn %"PRI_mfn""
>>                          " (-> gfn %#lx)\n",
>> -                       mfn, gfn, mfn_x(p2mfn),
>> +                       mfn_x(mfn), gfn, mfn_x(p2mfn),
>>                          (mfn_valid(p2mfn)
>> -                        ? get_gpfn_from_mfn(mfn_x(p2mfn))
>> +                        ? get_pfn_from_mfn(p2mfn)
>>                           : -1u));
> 
> I realize this is an entirely unrelated change, but the -1u here
> is standing out too much to not mention it: Could I talk you into
> making this gfn_x(INVALID_GFN) at this occasion?

Hmmm, I am not sure why I missed this one. I will use gfn_x(INVALID_GFN).

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

> 
>> @@ -515,11 +516,14 @@ static inline void set_gpfn_from_mfn(unsigned long mfn, unsigned long pfn)
>>   
>>   extern struct rangeset *mmio_ro_ranges;
>>   
>> -#define get_gpfn_from_mfn(mfn)      (machine_to_phys_mapping[(mfn)])
>> +static inline unsigned long get_pfn_from_mfn(mfn_t mfn)
>> +{
>> +    return machine_to_phys_mapping[mfn_x(mfn)];
>> +}
> 
> ... this.
> 
> Jan
> 

Cheers,

[1] <5CF7A1090200007800235782@prv1-mh.provo.novell.com>

-- 
Julien Grall