Re: [Xen-devel] [RFC PATCH 13/16]: PVH xen: introduce p2m_map_foreign

[Tim Deegan <tim@xxxxxxx>]

At 18:09 -0800 on 11 Jan (1357927784), Mukesh Rathor wrote:
> @@ -584,6 +584,11 @@ guest_physmap_add_entry(struct domain *d
>          {
>              ASSERT(mfn_valid(omfn));
>              set_gpfn_from_mfn(mfn_x(omfn), INVALID_M2P_ENTRY);
> +
> +            /* Because PVH domU uses kmalloc for grant pfn, we need to save
> +             * and restore the old mfn */
> +             if (is_pvh_domain(d) && p2m_is_grant(t))
> +                 free_domheap_page(mfn_to_page(omfn));

I think you'll need to explain this in more detail.  The comment assumes
that the guest is running linux, which is worrying.  And in any case you
can't just free_domheap_page() the guest's memory!  What if another
domain has a reference to it?

>          }
>          else if ( ot == p2m_populate_on_demand )
>          {
> @@ -715,7 +720,34 @@ void p2m_change_type_range(struct domain
>      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_unlocked(dp, gfn, &ot);
> +    if (mfn_valid(omfn)) {
> +        gdprintk(XENLOG_ERR, "Already mapped mfn %lx at gfn:%lx\n", 
> +                 mfn_x(omfn), gfn);

Unless you hold a lock here, you can't rely on this check for safety;
two callers could be racing to set the same gfn. 

> +        set_gpfn_from_mfn(mfn_x(omfn), INVALID_M2P_ENTRY);
> +    }
> +
> +    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;
> +}

> diff -r 31a145002453 -r 2c894340b16f xen/arch/x86/physdev.c
> --- a/xen/arch/x86/physdev.c  Fri Jan 11 16:38:07 2013 -0800
> +++ b/xen/arch/x86/physdev.c  Fri Jan 11 16:43:02 2013 -0800
> @@ -485,6 +485,7 @@ ret_t do_physdev_op(int cmd, XEN_GUEST_H
>  
>      case PHYSDEVOP_set_iopl: {
>          struct physdev_set_iopl set_iopl;
> +        NO_PVH_ASSERT_VCPU(current);
>          ret = -EFAULT;
>          if ( copy_from_guest(&set_iopl, arg, 1) != 0 )
>              break;
> @@ -498,6 +499,7 @@ ret_t do_physdev_op(int cmd, XEN_GUEST_H
>  
>      case PHYSDEVOP_set_iobitmap: {
>          struct physdev_set_iobitmap set_iobitmap;
> +        NO_PVH_ASSERT_VCPU(current);
>          ret = -EFAULT;
>          if ( copy_from_guest(&set_iobitmap, arg, 1) != 0 )
>              break;
> @@ -738,7 +740,8 @@ ret_t do_physdev_op(int cmd, XEN_GUEST_H
>          struct domain *d = current->domain;
>  
>          ret = -EPERM;
> -
> +        if ( !IS_PRIV(d) || !is_pvh_domain(d))
> +            break;

That doesn't seem right!  What constraints are you trying to implement
here?  Mapping IO memory seems like it should be an IS_PRIV() thing
regardless of PVH-ness.

Tim.

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