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

[Julien Grall <julien@xxxxxxx>]

Hi,

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 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@xxxxxxxxxxxxxxxxxxxxxxxx>

--
Julien Grall