[RFC PATCH 14/16]: PVH xen: add xenmem_add_foreign_to_pmap()

[RFC PATCH 14/16]: PVH xen: add xenmem_add_foreign_to_pmap()

[Mukesh Rathor]

In this patch, I add a new function, xenmem_add_foreign_to_pmap(), to
map pages from foreign gue st into current dom0 for domU creation.
Also, some minor grant changes for PVH.

Signed-off-by: Mukesh Rathor <mukesh.rathor@oracle.com>

diff -r 2c894340b16f -r 47d2e652bd4d xen/arch/x86/mm.c
--- a/xen/arch/x86/mm.c	Fri Jan 11 16:43:02 2013 -0800
+++ b/xen/arch/x86/mm.c	Fri Jan 11 16:45:36 2013 -0800
@@ -2608,7 +2608,7 @@ static struct domain *get_pg_owner(domid
         goto out;
     }
 
-    if ( unlikely(paging_mode_translate(curr)) )
+    if ( !is_pvh_domain(curr) && unlikely(paging_mode_translate(curr)) )
     {
         MEM_LOG("Cannot mix foreign mappings with translated domains");
         goto out;
@@ -3734,16 +3734,34 @@ static int replace_grant_p2m_mapping(
     old_mfn = get_gfn(d, gfn, &type);
     if ( !p2m_is_grant(type) || mfn_x(old_mfn) != frame )
     {
-        put_gfn(d, gfn);
         gdprintk(XENLOG_WARNING,
-                 "replace_grant_p2m_mapping: old mapping invalid (type %d, mfn %lx, frame %lx)\n",
+                 "replace_grant_p2m_mapping: old mapping invalid "
+                 "(type %d, mfn %lx, frame %lx)\n",
                  type, mfn_x(old_mfn), frame);
-        return GNTST_general_error;
+        goto out_err;
     }
     guest_physmap_remove_page(d, gfn, frame, PAGE_ORDER_4K);
 
+    /* PVH: Because we free the existing mfn in XENMEM_add_to_physmap during
+     * grant map, we undo that here so the guest P2M (EPT/NPT) is consistent */
+    if ( is_pvh_domain(d) ) {
+        struct page_info *page = alloc_domheap_page(d, 0);
+
+	if ( page == NULL ) {
+	    gdprintk(XENLOG_ERR, "Unable to alloc domheap page\n");
+            goto out_err;
+	}
+	if ( guest_physmap_add_page(d, gfn, page_to_mfn(page), 0) != 0 ) {
+	    gdprintk(XENLOG_ERR, "Unable to add mfn to replace grant\n");
+            goto out_err;
+        }
+    }
     put_gfn(d, gfn);
     return GNTST_okay;
+
+out_err:
+    put_gfn(d, gfn);
+    return GNTST_general_error;
 }
 
 int replace_grant_host_mapping(
@@ -4143,7 +4161,7 @@ long do_update_descriptor(u64 pa, u64 de
     page = get_page_from_gfn(dom, gmfn, NULL, P2M_ALLOC);
     if ( (((unsigned int)pa % sizeof(struct desc_struct)) != 0) ||
          !page ||
-         !check_descriptor(dom, &d) )
+         (!is_pvh_domain(dom) && !check_descriptor(dom, &d)) )
     {
         if ( page )
             put_page(page);
@@ -4217,7 +4235,81 @@ static int handle_iomem_range(unsigned l
     return 0;
 }
 
-static int xenmem_add_to_physmap_once(
+/* add frames from foreign domain to current domain's physmap. Similar to 
+ * XENMAPSPACE_gmfn but the frame is foreign being mapped into current,
+ * and is not removed from foreign domain. 
+ * Usage: libxl on pvh dom0 creating a guest and doing privcmd_ioctl_mmap
+ * Returns: 0 ==> success
+ */
+static long noinline xenmem_add_foreign_to_pmap( domid_t foreign_domid, 
+                                        unsigned long fgmfn, unsigned long gpfn)
+{
+    unsigned long rc=0, prev_mfn, mfn = 0;
+    struct domain *fdom, *currd = current->domain;
+    p2m_type_t p2mt, p2mt_prev;
+
+    if ( (fdom = get_pg_owner(foreign_domid)) == NULL ) {
+        return -EPERM;
+    }
+
+    mfn = mfn_x(get_gfn_query_unlocked(fdom, fgmfn, &p2mt));
+
+    /* qemu, running on PVH dom0, mapping hvm domain's io pages during domain 
+     * creation, doesn't have mfns in the HAP table */
+    if ( !mfn_valid(mfn) && p2m_is_mmio(p2mt) ) {
+
+        if (!is_hvm_domain(fdom)) {
+            printk("mmio type for non-hvm domain. fd:%d fgmfn:%lx gpfn:%lx\n",
+                   foreign_domid, fgmfn, gpfn);
+            return -EINVAL;
+        }
+        mfn = fgmfn;     /* map 1 to 1 */
+    }
+
+    if ( !p2m_is_valid(p2mt) ) {
+        put_pg_owner(fdom);
+        return -EINVAL;
+    }
+
+    if ( !p2m_is_mmio(p2mt) && !get_page_from_pagenr(mfn, fdom) ) {
+        put_pg_owner(fdom);
+        return -EINVAL;
+    }
+
+    /* Remove previously mapped page if it was present. */
+    prev_mfn = mfn_x(get_gfn_query_unlocked(currd, gpfn, &p2mt_prev));
+    if ( mfn_valid(prev_mfn) )
+    {
+        if ( is_xen_heap_mfn(prev_mfn) )
+            /* Xen heap frames are simply unhooked from this phys slot */
+            guest_physmap_remove_page(currd, gpfn, prev_mfn, 0);
+        else
+            /* Normal domain memory is freed, to avoid leaking memory. */
+            guest_remove_page(currd, gpfn);
+    }
+
+    /* Map at new location.  Can't use guest_physmap_add_page() because it 
+     * will update the m2p table which will result in  mfn --> gpfn of dom0 
+     * and not gpfn of domU.  */
+    if ( p2m_is_mmio(p2mt) ) {
+        if ( set_mmio_p2m_entry(currd, gpfn, _mfn(mfn)) == 0 ) {
+            printk("set_mmio_p2m failed. gpfn:%lx mfn:%lx fgmfn:%lx\n", 
+                   gpfn, mfn, fgmfn);
+            rc = -EINVAL;
+        }
+
+    } else if ( set_foreign_p2m_entry(currd, gpfn, _mfn(mfn)) == 0 ) {
+
+        printk("guest_physmap_add_page failed1. gpfn:%lx mfn:%lx fgmfn:%lx\n", 
+             gpfn, mfn, fgmfn);
+        put_page(mfn_to_page(mfn));
+        rc = -EINVAL;
+    }
+    put_pg_owner(fdom);
+    return rc;
+}
+
+static int noinline xenmem_add_to_physmap_once(
     struct domain *d, uint16_t xatp_space, domid_t foreign_domid,
     unsigned long xatp_idx, unsigned long xatp_gpfn)
 {
@@ -4278,6 +4370,13 @@ static int xenmem_add_to_physmap_once(
             page = mfn_to_page(mfn);
             break;
         }
+
+        case XENMAPSPACE_gmfn_foreign:
+        {
+            rc = xenmem_add_foreign_to_pmap(foreign_domid, xatp_idx, xatp_gpfn);
+            return rc;
+        }
+
         default:
             break;
     }
diff -r 2c894340b16f -r 47d2e652bd4d xen/common/grant_table.c
--- a/xen/common/grant_table.c	Fri Jan 11 16:43:02 2013 -0800
+++ b/xen/common/grant_table.c	Fri Jan 11 16:45:36 2013 -0800
@@ -529,7 +529,7 @@ static void mapcount(
  * addr is _either_ a host virtual address, or the address of the pte to
  * update, as indicated by the GNTMAP_contains_pte flag.
  */
-static void
+static noinline void
 __gnttab_map_grant_ref(
     struct gnttab_map_grant_ref *op)
 {
@@ -745,7 +745,7 @@ __gnttab_map_grant_ref(
 
     double_gt_lock(lgt, rgt);
 
-    if ( !is_hvm_domain(ld) && need_iommu(ld) )
+    if ( !is_hvm_or_pvh_domain(ld) && need_iommu(ld) )
     {
         unsigned int wrc, rdc;
         int err = 0;
@@ -956,7 +956,7 @@ __gnttab_unmap_common(
             act->pin -= GNTPIN_hstw_inc;
     }
 
-    if ( !is_hvm_domain(ld) && need_iommu(ld) )
+    if ( !is_hvm_or_pvh_domain(ld) && need_iommu(ld) )
     {
         unsigned int wrc, rdc;
         int err = 0;

Re: [RFC PATCH 14/16]: PVH xen: add xenmem_add_foreign_to_pmap()

[Tim Deegan]

At 18:11 -0800 on 11 Jan (1357927863), Mukesh Rathor wrote:
> In this patch, I add a new function, xenmem_add_foreign_to_pmap(), to
> map pages from foreign gue st into current dom0 for domU creation.
> Also, some minor grant changes for PVH.
> 
> Signed-off-by: Mukesh Rathor <mukesh.rathor@oracle.com>
> 
> diff -r 2c894340b16f -r 47d2e652bd4d xen/arch/x86/mm.c
> --- a/xen/arch/x86/mm.c	Fri Jan 11 16:43:02 2013 -0800
> +++ b/xen/arch/x86/mm.c	Fri Jan 11 16:45:36 2013 -0800
> @@ -2608,7 +2608,7 @@ static struct domain *get_pg_owner(domid
>          goto out;
>      }
>  
> -    if ( unlikely(paging_mode_translate(curr)) )
> +    if ( !is_pvh_domain(curr) && unlikely(paging_mode_translate(curr)) )
>      {
>          MEM_LOG("Cannot mix foreign mappings with translated domains");
>          goto out;
> @@ -3734,16 +3734,34 @@ static int replace_grant_p2m_mapping(
>      old_mfn = get_gfn(d, gfn, &type);
>      if ( !p2m_is_grant(type) || mfn_x(old_mfn) != frame )
>      {
> -        put_gfn(d, gfn);
>          gdprintk(XENLOG_WARNING,
> -                 "replace_grant_p2m_mapping: old mapping invalid (type %d, mfn %lx, frame %lx)\n",
> +                 "replace_grant_p2m_mapping: old mapping invalid "
> +                 "(type %d, mfn %lx, frame %lx)\n",
>                   type, mfn_x(old_mfn), frame);
> -        return GNTST_general_error;
> +        goto out_err;
>      }
>      guest_physmap_remove_page(d, gfn, frame, PAGE_ORDER_4K);
>  
> +    /* PVH: Because we free the existing mfn in XENMEM_add_to_physmap during
> +     * grant map, we undo that here so the guest P2M (EPT/NPT) is consistent */
> +    if ( is_pvh_domain(d) ) {
> +        struct page_info *page = alloc_domheap_page(d, 0);

I think this belongs in the previous patch, with the other gnttab stuff.

> +
> +	if ( page == NULL ) {
> +	    gdprintk(XENLOG_ERR, "Unable to alloc domheap page\n");
> +            goto out_err;
> +	}

Your indentation is messed up here. 

> +	if ( guest_physmap_add_page(d, gfn, page_to_mfn(page), 0) != 0 ) {
> +	    gdprintk(XENLOG_ERR, "Unable to add mfn to replace grant\n");
> +            goto out_err;
> +        }
> +    }
>      put_gfn(d, gfn);
>      return GNTST_okay;
> +
> +out_err:
> +    put_gfn(d, gfn);
> +    return GNTST_general_error;
>  }
>  
>  int replace_grant_host_mapping(
> @@ -4143,7 +4161,7 @@ long do_update_descriptor(u64 pa, u64 de
>      page = get_page_from_gfn(dom, gmfn, NULL, P2M_ALLOC);
>      if ( (((unsigned int)pa % sizeof(struct desc_struct)) != 0) ||
>           !page ||
> -         !check_descriptor(dom, &d) )
> +         (!is_pvh_domain(dom) && !check_descriptor(dom, &d)) )
>      {
>          if ( page )
>              put_page(page);
> @@ -4217,7 +4235,81 @@ static int handle_iomem_range(unsigned l
>      return 0;
>  }
>  
> -static int xenmem_add_to_physmap_once(
> +/* add frames from foreign domain to current domain's physmap. Similar to 
> + * XENMAPSPACE_gmfn but the frame is foreign being mapped into current,
> + * and is not removed from foreign domain. 
> + * Usage: libxl on pvh dom0 creating a guest and doing privcmd_ioctl_mmap
> + * Returns: 0 ==> success
> + */
> +static long noinline xenmem_add_foreign_to_pmap( domid_t foreign_domid, 
> +                                        unsigned long fgmfn, unsigned long gpfn)
> +{
> +    unsigned long rc=0, prev_mfn, mfn = 0;
> +    struct domain *fdom, *currd = current->domain;
> +    p2m_type_t p2mt, p2mt_prev;
> +
> +    if ( (fdom = get_pg_owner(foreign_domid)) == NULL ) {
> +        return -EPERM;
> +    }
> +
> +    mfn = mfn_x(get_gfn_query_unlocked(fdom, fgmfn, &p2mt));

I don't think you can use _unlocked lookups here - you need to hold on
to the foreign pfn at least until you can get_page() the underlying mfn
to be sure it's not freed underfoot. 

> +    /* qemu, running on PVH dom0, mapping hvm domain's io pages during domain 
> +     * creation, doesn't have mfns in the HAP table */
> +    if ( !mfn_valid(mfn) && p2m_is_mmio(p2mt) ) {

This test should be for == p2m_mmio_direct; we don't want to try mapping
p2m_mmio_dm areas.

> +        if (!is_hvm_domain(fdom)) {
> +            printk("mmio type for non-hvm domain. fd:%d fgmfn:%lx gpfn:%lx\n",
> +                   foreign_domid, fgmfn, gpfn);
> +            return -EINVAL;
> +        }
> +        mfn = fgmfn;     /* map 1 to 1 */

Surely not -- you want to map the _actual_ MMIO range, right, not just
whatever GFN-address the foreigh domain mapped it at?

> diff -r 2c894340b16f -r 47d2e652bd4d xen/common/grant_table.c
> --- a/xen/common/grant_table.c	Fri Jan 11 16:43:02 2013 -0800
> +++ b/xen/common/grant_table.c	Fri Jan 11 16:45:36 2013 -0800
> @@ -529,7 +529,7 @@ static void mapcount(
>   * addr is _either_ a host virtual address, or the address of the pte to
>   * update, as indicated by the GNTMAP_contains_pte flag.
>   */
> -static void
> +static noinline void
>  __gnttab_map_grant_ref(
>      struct gnttab_map_grant_ref *op)
>  {
> @@ -745,7 +745,7 @@ __gnttab_map_grant_ref(
>  
>      double_gt_lock(lgt, rgt);
>  
> -    if ( !is_hvm_domain(ld) && need_iommu(ld) )
> +    if ( !is_hvm_or_pvh_domain(ld) && need_iommu(ld) )

These seem luike they're part of the previous patch too. 

Tim.

>      {
>          unsigned int wrc, rdc;
>          int err = 0;
> @@ -956,7 +956,7 @@ __gnttab_unmap_common(
>              act->pin -= GNTPIN_hstw_inc;
>      }
>  
> -    if ( !is_hvm_domain(ld) && need_iommu(ld) )
> +    if ( !is_hvm_or_pvh_domain(ld) && need_iommu(ld) )
>      {
>          unsigned int wrc, rdc;
>          int err = 0;
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel

Re: [RFC PATCH 14/16]: PVH xen: add xenmem_add_foreign_to_pmap()

[Mukesh Rathor]

On Thu, 24 Jan 2013 17:31:18 +0000
Tim Deegan <tim@xen.org> wrote:

> At 18:11 -0800 on 11 Jan (1357927863), Mukesh Rathor wrote:
> 
> > +    /* qemu, running on PVH dom0, mapping hvm domain's io pages
> > during domain 
> > +     * creation, doesn't have mfns in the HAP table */
> > +    if ( !mfn_valid(mfn) && p2m_is_mmio(p2mt) ) {
> 
> This test should be for == p2m_mmio_direct; we don't want to try
> mapping p2m_mmio_dm areas.

Yup. Done.

> > +        if (!is_hvm_domain(fdom)) {
> > +            printk("mmio type for non-hvm domain. fd:%d fgmfn:%lx
> > gpfn:%lx\n",
> > +                   foreign_domid, fgmfn, gpfn);
> > +            return -EINVAL;
> > +        }
> > +        mfn = fgmfn;     /* map 1 to 1 */
> 
> Surely not -- you want to map the _actual_ MMIO range, right, not just
> whatever GFN-address the foreigh domain mapped it at?

Actually, fgmfn here is the machine address of the mmio page. 
Removed the "map 1 to 1" comment.

thanks,
Mukesh

Re: [RFC PATCH 14/16]: PVH xen: add xenmem_add_foreign_to_pmap()

[Mukesh Rathor]

On Mon, 11 Feb 2013 18:18:24 -0800
Mukesh Rathor <mukesh.rathor@oracle.com> wrote:

> On Thu, 24 Jan 2013 17:31:18 +0000
> Tim Deegan <tim@xen.org> wrote:
> 
> > At 18:11 -0800 on 11 Jan (1357927863), Mukesh Rathor wrote:
> > 
> > > +    /* qemu, running on PVH dom0, mapping hvm domain's io pages
> > > during domain 
> > > +     * creation, doesn't have mfns in the HAP table */
> > > +    if ( !mfn_valid(mfn) && p2m_is_mmio(p2mt) ) {
> > 
> > This test should be for == p2m_mmio_direct; we don't want to try
> > mapping p2m_mmio_dm areas.
> 
> Yup. Done.

No, qemu is changing the mem type for these pages, so I need to dig
into understanding what it's trying to do there. But the above, I don't
think is correct. Xen returns p2m type of dm, but I don't think I'm
looking at right mfn's here for this qemu special case. Basically, it's
during hvm guest creation on PVH dom0 that qemu accesses some addresses
that are not mapped.

Anyways, I hope to figure whats going on in qemu soon.

thanks,
Mukesh

Re: [RFC PATCH 14/16]: PVH xen: add xenmem_add_foreign_to_pmap()

[Tim Deegan]

At 18:34 -0800 on 13 Feb (1360780465), Mukesh Rathor wrote:
> On Mon, 11 Feb 2013 18:18:24 -0800
> Mukesh Rathor <mukesh.rathor@oracle.com> wrote:
> 
> > On Thu, 24 Jan 2013 17:31:18 +0000
> > Tim Deegan <tim@xen.org> wrote:
> > 
> > > At 18:11 -0800 on 11 Jan (1357927863), Mukesh Rathor wrote:
> > > 
> > > > +    /* qemu, running on PVH dom0, mapping hvm domain's io pages
> > > > during domain 
> > > > +     * creation, doesn't have mfns in the HAP table */
> > > > +    if ( !mfn_valid(mfn) && p2m_is_mmio(p2mt) ) {
> > > 
> > > This test should be for == p2m_mmio_direct; we don't want to try
> > > mapping p2m_mmio_dm areas.
> > 
> > Yup. Done.
> 
> No, qemu is changing the mem type for these pages, so I need to dig
> into understanding what it's trying to do there. But the above, I don't
> think is correct. Xen returns p2m type of dm, but I don't think I'm
> looking at right mfn's here for this qemu special case. Basically, it's
> during hvm guest creation on PVH dom0 that qemu accesses some addresses
> that are not mapped.

It may be that the memory is genuinely not there - since qemu doesn't
build the guest itself it doesn't necessarily know exactly where the
builder put all the memory.

If that's the case, then just failing the mapping is the right
response. 

Tim.

Re: [RFC PATCH 14/16]: PVH xen: add xenmem_add_foreign_to_pmap()

[Tim Deegan]

At 18:18 -0800 on 11 Feb (1360606704), Mukesh Rathor wrote:
> > > +        if (!is_hvm_domain(fdom)) {
> > > +            printk("mmio type for non-hvm domain. fd:%d fgmfn:%lx
> > > gpfn:%lx\n",
> > > +                   foreign_domid, fgmfn, gpfn);
> > > +            return -EINVAL;
> > > +        }
> > > +        mfn = fgmfn;     /* map 1 to 1 */
> > 
> > Surely not -- you want to map the _actual_ MMIO range, right, not just
> > whatever GFN-address the foreigh domain mapped it at?
> 
> Actually, fgmfn here is the machine address of the mmio page. 

I hope not!  You've just passed it to a p2m lookup a little earlier, so
it had better be a gfn.  (And in either case it could do with a more
explicit name: maybe fmfn if it's an mfn or fgfn if it's a gfn?

Tim.

Re: [RFC PATCH 14/16]: PVH xen: add xenmem_add_foreign_to_pmap()

[Mukesh Rathor]

On Thu, 14 Feb 2013 10:39:27 +0000
Tim Deegan <tim@xen.org> wrote:

> At 18:18 -0800 on 11 Feb (1360606704), Mukesh Rathor wrote:
> > > > +        if (!is_hvm_domain(fdom)) {
> > > > +            printk("mmio type for non-hvm domain. fd:%d
> > > > fgmfn:%lx gpfn:%lx\n",
> > > > +                   foreign_domid, fgmfn, gpfn);
> > > > +            return -EINVAL;
> > > > +        }
> > > > +        mfn = fgmfn;     /* map 1 to 1 */
> > > 
> > > Surely not -- you want to map the _actual_ MMIO range, right, not
> > > just whatever GFN-address the foreigh domain mapped it at?
> > 
> > Actually, fgmfn here is the machine address of the mmio page. 
> 
> I hope not!  You've just passed it to a p2m lookup a little earlier,
> so it had better be a gfn.  (And in either case it could do with a
> more explicit name: maybe fmfn if it's an mfn or fgfn if it's a gfn?

Yup, already changed the name to fgfn last week.

>It may be that the memory is genuinely not there - since qemu doesn't
>build the guest itself it doesn't necessarily know exactly where the
>builder put all the memory.

>If that's the case, then just failing the mapping is the right
>response. 

Correct, and thats what PV dom0 does. I had it blatantly wrong! Fixed. 

Thanks,
Mukesh