Re: [Xen-devel] [PATCH 13/18 V2]: PVH xen: introduce p2m_map_foreign

[Tim Deegan <tim@xxxxxxx>]

At 17:51 -0700 on 15 Mar (1363369869), Mukesh Rathor wrote:
>  In this patch, I introduce  a new type p2m_map_foreign for pages that a
>  dom0 maps from foreign domains its creating. Also, add
>  set_foreign_p2m_entry() to map p2m_map_foreign type pages. Other misc changes
>  related to p2m.

First of all, as Jan and Konrad have pointed out, there seems to be a
lot of unrelated changes in here, which needs to be separated out.

> diff --git a/xen/arch/x86/mm/p2m-ept.c b/xen/arch/x86/mm/p2m-ept.c
> index a2d1591..38ea9ec 100644
> --- a/xen/arch/x86/mm/p2m-ept.c
> +++ b/xen/arch/x86/mm/p2m-ept.c
> @@ -75,6 +75,7 @@ static void ept_p2m_type_to_flags(ept_entry_t *entry, 
> p2m_type_t type, p2m_acces
>              entry->w = 0;
>              break;
>          case p2m_grant_map_rw:
> +        case p2m_map_foreign:
>              entry->r = entry->w = 1;
>              entry->x = 0;
>              break;
> @@ -430,7 +431,7 @@ ept_set_entry(struct p2m_domain *p2m, unsigned long gfn, 
> mfn_t mfn,
>      }
>  
>      /* Track the highest gfn for which we have ever had a valid mapping */
> -    if ( p2mt != p2m_invalid &&
> +    if ( p2mt != p2m_invalid && p2mt != p2m_mmio_dm &&
>           (gfn + (1UL << order) - 1 > p2m->max_mapped_pfn) )
>          p2m->max_mapped_pfn = gfn + (1UL << order) - 1;

Is this relevant to the p2m_map_foreign stuff?

>  
> diff --git a/xen/arch/x86/mm/p2m-pt.c b/xen/arch/x86/mm/p2m-pt.c
> index 302b621..3f46418 100644
> --- a/xen/arch/x86/mm/p2m-pt.c
> +++ b/xen/arch/x86/mm/p2m-pt.c
> @@ -89,6 +89,7 @@ static unsigned long p2m_type_to_flags(p2m_type_t t, mfn_t 
> mfn)
>      case p2m_ram_rw:
>          return flags | P2M_BASE_FLAGS | _PAGE_RW;
>      case p2m_grant_map_rw:
> +    case p2m_map_foreign:
>          return flags | P2M_BASE_FLAGS | _PAGE_RW | _PAGE_NX_BIT;
>      case p2m_mmio_direct:
>          if ( !rangeset_contains_singleton(mmio_ro_ranges, mfn_x(mfn)) )
> @@ -429,7 +430,7 @@ p2m_set_entry(struct p2m_domain *p2m, unsigned long gfn, 
> mfn_t mfn,
>      }
>  
>      /* Track the highest gfn for which we have ever had a valid mapping */
> -    if ( p2mt != p2m_invalid
> +    if ( p2mt != p2m_invalid && p2mt != p2m_mmio_dm

Likewise.

>           && (gfn + (1UL << page_order) - 1 > p2m->max_mapped_pfn) )
>          p2m->max_mapped_pfn = gfn + (1UL << page_order) - 1;
>  
> diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c
> index 4837de3..6888cf1 100644
> --- a/xen/arch/x86/mm/p2m.c
> +++ b/xen/arch/x86/mm/p2m.c
> @@ -523,7 +523,7 @@ p2m_remove_page(struct p2m_domain *p2m, unsigned long 
> gfn, unsigned long mfn,
>          for ( i = 0; i < (1UL << page_order); i++ )
>          {
>              mfn_return = p2m->get_entry(p2m, gfn + i, &t, &a, 0, NULL);
> -            if ( !p2m_is_grant(t) && !p2m_is_shared(t) )
> +            if ( !p2m_is_grant(t) && !p2m_is_shared(t) && !p2m_is_foreign(t) 
> )
>                  set_gpfn_from_mfn(mfn+i, INVALID_M2P_ENTRY);
>              ASSERT( !p2m_is_valid(t) || mfn + i == mfn_x(mfn_return) );
>          }
> @@ -754,7 +754,35 @@ void p2m_change_type_range(struct domain *d,
>      p2m_unlock(p2m);
>  }
>  
> +/* Returns: True for success. 0 for failure */
> +int set_foreign_p2m_entry(struct domain *dp, unsigned long gfn, mfn_t mfn)
> +{
> +    int rc = 0;
> +    p2m_type_t ot;
> +    mfn_t omfn;
> +    struct p2m_domain *p2m = p2m_get_hostp2m(dp);
> +
> +    if ( !paging_mode_translate(dp) )
> +        return 0;
> +
> +    omfn = get_gfn_query(dp, gfn, &ot);
> +    if (mfn_valid(omfn)) {
> +        gdprintk(XENLOG_ERR, "Already mapped mfn %lx at gfn:%lx\n", 
> +                 mfn_x(omfn), gfn);
> +        set_gpfn_from_mfn(mfn_x(omfn), INVALID_M2P_ENTRY);

That needs to be gated on 'ot' -- you should only be doing this for
plain RAM owned by the caller.

> +    }
> +    put_gfn(dp, gfn);

Also, this put_gfn is too early.  Now there's a race where another CPU
could populate that GFN again before your set_p2m_entry().

AFAICT once you've done that this function will look so similar to
set_mmio_p2m_entry that they should be refactored to extract the common
parts.

> +    P2M_DEBUG("set foreign %lx %lx\n", gfn, mfn_x(mfn));
> +    p2m_lock(p2m);
> +    rc = set_p2m_entry(p2m, gfn, mfn, 0, p2m_map_foreign, 
> p2m->default_access);
> +    p2m_unlock(p2m);
> +    if ( rc == 0 )
> +        gdprintk(XENLOG_ERR,
> +            "set_foreign_p2m_entry: set_p2m_entry failed! gfn:%lx 
> mfn=%08lx\n",
> +            gfn, mfn_x(get_gfn_query(dp, gfn, &ot)));
> +    return rc;
> +}

> @@ -510,6 +512,8 @@ p2m_type_t p2m_change_type(struct domain *d, unsigned 
> long gfn,
>  int set_mmio_p2m_entry(struct domain *d, unsigned long gfn, mfn_t mfn);
>  int clear_mmio_p2m_entry(struct domain *d, unsigned long gfn);
>  
> +/* Set foreign mfn in the current guest's p2m table (for pvh dom0) */
> +int set_foreign_p2m_entry(struct domain *domp, unsigned long gfn, mfn_t mfn);

I don't think it's specific to dom 0.  Certainly it doesn't have any
access controls in it.

Cheers,

Tim.

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