Re: [Xen-devel] [PATCH v5 06/17] xen: Use a typesafe to define INVALID_MFN

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 28/06/16 17:17, Julien Grall wrote:
> @@ -60,16 +60,17 @@ dbg_hvm_va2mfn(dbgva_t vaddr, struct domain *dp, int 
> toaddr,
>          return INVALID_MFN;
>      }
>  
> -    mfn = mfn_x(get_gfn(dp, *gfn, &gfntype)); 
> +    mfn = get_gfn(dp, *gfn, &gfntype);
>      if ( p2m_is_readonly(gfntype) && toaddr )
>      {
>          DBGP2("kdb:p2m_is_readonly: gfntype:%x\n", gfntype);
>          mfn = INVALID_MFN;
>      }
>      else
> -        DBGP2("X: vaddr:%lx domid:%d mfn:%lx\n", vaddr, dp->domain_id, mfn);
> +        DBGP2("X: vaddr:%lx domid:%d mfn:%lx\n",

This should be PRImfn rather than assuming %lx is the correct format
identifier for mfn_t.

Similarly throughout the patch.

> diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c
> index 6258a5b..6f90510 100644
> @@ -493,7 +493,7 @@ int p2m_set_entry(struct p2m_domain *p2m, unsigned long 
> gfn, mfn_t mfn,
>              rc = set_rc;
>  
>          gfn += 1ul << order;
> -        if ( mfn_x(mfn) != INVALID_MFN )
> +        if ( !mfn_eq(mfn, INVALID_MFN) )
>              mfn = _mfn(mfn_x(mfn) + (1ul << order));

This could turn into mfn_add(mfn, 1ul << order), if you fancy fixing it up.

> @@ -1107,7 +1107,7 @@ int clear_mmio_p2m_entry(struct domain *d, unsigned 
> long gfn, mfn_t mfn,
>      }
>  
>      /* Do not use mfn_valid() here as it will usually fail for MMIO pages. */
> -    if ( (INVALID_MFN == mfn_x(actual_mfn)) || (t != p2m_mmio_direct) )
> +    if ( (mfn_eq(actual_mfn, INVALID_MFN)) || (t != p2m_mmio_direct) )

You can drop the brackets around mfn_eq().

> @@ -838,7 +838,7 @@ mfn_t oos_snapshot_lookup(struct domain *d, mfn_t gmfn)
>  
>      SHADOW_ERROR("gmfn %lx was OOS but not in hash table\n", mfn_x(gmfn));
>      BUG();
> -    return _mfn(INVALID_MFN);
> +    return INVALID_MFN;

You should be able to get away with deleting this return.  Now that
BUG() is properly annotated with unreachable(), the compiler shouldn't
warn about this exit path from the function.

Otherwise, no major concerns.  Reviewed-by: Andrew Cooper
<andrew.cooper3@xxxxxxxxxx>

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel