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

[Jan Beulich <jbeulich@xxxxxxxx>]

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.

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.

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

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

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

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

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