Re: [hybrid]: code review for function mapping pfn to foreign mfn

[Ian Campbell <Ian.Campbell@citrix.com>]

On Sat, 2012-04-14 at 02:29 +0100, Mukesh Rathor wrote:
> Hi,
> 
> I wrote up some code to map/unmap pfn to mfn for hybrid. I wonder if anyone
> can please look at it and give any comments. I tested it and seems to work
> ok.

I'm not all that familiar with x86 p2m stuff but I'll try. (I've also
added Tim, who is familiar with this stuff)

> Basically, the library, xl running in hybrid dom0, needs to map domU pages
> during guest creation. I tried using existing add to physmap, mmu_update,
> but nothing was feasible. So wrote this. Its called from
> privcmd_ioctl_mmap_batch().
> 
> thanks,
> Mukesh
> 
> 
> /* add frames from foreign domain to current domain physmap. Similar to 
>  * XENMEM_add_to_physmap

Why a whole new hypercall rather than a new XENMAPSPACE for the exiting
XENMEM_add_to_physmap.

>  but the mfn frame is foreign, is being mapped into 
>  * current privileged domain, and is not removed from foreign domain. 
>  * Usage: libxl when creating guest in hybrid dom0 doing privcmd_ioctl_mmap
>  * Return: 0 success
>  */
> static long _add_foreign_to_pmap_batch(XEN_GUEST_HANDLE(void) arg)
> {
>     struct xen_add_to_foreign_pmap_batch pmapb;

Ideally we'd have the definition of this (or the equivalent mod to the
XENMEM_add_to_physmap associated struct) for context, but I can probably
guess what the content looks like.

>     unsigned long rc=0, i, prev_mfn, mfn = 0;
>     struct domain *fdom, *currd = current->domain;
>     p2m_type_t p2mt;
> 
>     if ( copy_from_guest(&pmapb, arg, 1) )
>         return -EFAULT;
> 
>     fdom = get_pg_owner(pmapb.foreign_domid);
> 
>     if ( fdom== NULL ) {
>         put_pg_owner(fdom);
>         return -EPERM;
>     }
> 
>     for (i=0; (rc == 0) && (i < pmapb.count); i++) {
>         unsigned long fgmfn = pmapb.gmfn+i, gpfn = pmapb.gpfn+i;
>         mfn = mfn_x(gfn_to_mfn_query(p2m_get_hostp2m(fdom), fgmfn, &p2mt));

I don't see gfn_to_mfn_query anywhere, I presume it's just a pretty
straight forward p2m lookup? Surprised there is no existing API but OK.

> 	if ( !p2m_is_valid(p2mt) )
>             rc = -EINVAL;
> 
>         if ( !rc && !get_page_from_pagenr(mfn, fdom) )
>             rc = -EPERM;
> 
>         if (!rc) 
>             put_page(mfn_to_page(mfn));
>         else 
>             break;
> 
>         /* Remove previously mapped page if it was present. */
>         prev_mfn = gmfn_to_mfn(currd, gpfn);
>         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);

Do you mean "unrefd" here rather than freed? Presumably the remote
domain (which owns the page) is still using it?

>         }
>         /* Map at new location. */
> 	/* Can't use guest_physmap_add_page() because it will update the m2p
> 	 * table so mfn ---> gpfn in dom0 and not gpfn of domU.
> 	 */
>         /* rc = guest_physmap_add_page(currd, gpfn, mfn, 0); */
> 
> 	rc = set_mmio_p2m_entry(p2m_get_hostp2m(currd), gpfn, mfn);

This ends up setting the page type to p2m_mmio_direct, which doesn't
seem likely to be correct. Perhaps you should be calling
set_p2m_entry()? Or adding a set_ram_p2m_entry which does similar checks
etc to set_mmio_p2m_entry (or maybe you could abstract out some generic
bits there)?

>         if (rc==0) {
>             printk("guest_physmap_add_page failed.gpfn:%lx mfn:%lx fgmfn:%lx\n",
>                    gpfn, mfn, fgmfn);
>             rc == -EINVAL;
>         } else 
>             rc = 0;
>     }
> 
>     pmapb.count = i;
>     copy_to_guest(arg, &pmapb, 1);
>     put_pg_owner(fdom);
>     return rc;
> }
> 
> static long noinline _rem_foreign_pmap_batch(XEN_GUEST_HANDLE(void) arg)

Can't XENMEM_remove_from_physmap be used here?

> {
>     xen_pfn_t gmfn;
>     struct xen_rem_foreign_pmap_batch pmapb;
>     p2m_type_t p2mt;
>     int i, rc=0;
>     struct domain *currd = current->domain;
> 
>     if ( copy_from_guest(&pmapb, arg, 1) )
>         return -EFAULT;
> 
>     for ( gmfn=pmapb.s_gpfn, i=0;  i < pmapb.count;  i++, gmfn++ ) {
> 
>         mfn_t mfn = mfn_x(gfn_to_mfn(p2m_get_hostp2m(currd), gmfn, &p2mt));
>         if ( unlikely(!mfn_valid(mfn)) )
>         {
>             gdprintk(XENLOG_INFO, "%s: Domain:%u gmfn:%lx invalid\n",
>                      __FUNCTION__, currd->domain_id, gmfn);
>             rc = -EINVAL;
>             break;
>         }
>         /* guest_physmap_remove_page(currd, gmfn, mfn, 0); */
> 	clear_mmio_p2m_entry(p2m_get_hostp2m(currd), gmfn);
>     }
>     pmapb.count = i;
>     copy_to_guest(arg, &pmapb, 1);
>     return rc;
> }
>