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

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

[Mukesh Rathor]

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.

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 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;
    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));
	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);
        }
        /* 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);
        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)
{
    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;
}

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

[Ian Campbell]

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;
> }
> 

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

[Tim Deegan]

At 14:53 +0100 on 16 Apr (1334588002), Ian Campbell wrote:
> 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)

This is on my todo list for later this week. 

> >     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.

Yes, it's a lookup with no PoD/paging/sharing side-effects.
gfn_to_mfn_*() have been replaced by get_gfn()/put_gfn(), so at the
very least this will have to be rebased across that change.

Tim.

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

[Mukesh Rathor]

On Mon, 16 Apr 2012 14:53:22 +0100
Ian Campbell <Ian.Campbell@citrix.com> wrote:

> > Similar to 
> >  * XENMEM_add_to_physmap
> 
> Why a whole new hypercall rather than a new XENMAPSPACE for the
> exiting XENMEM_add_to_physmap.

> 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.

Not a new hcall, just a new subcall. Forgot to include the struct:

#define XENMEM_add_foreign_to_pmap_batch      19
struct xen_add_to_foreign_pmap_batch {
    domid_t foreign_domid;         /* IN: gmfn belongs to this domain */
    int count;                     /* IN/OUT: number of contigous
frames */ unsigned long     gpfn;        /* IN: pfn in the current
domain */ unsigned long     gmfn;        /* IN: from foreign domain */
    int fpmap_flags;               /* future use */
};
typedef struct xen_add_to_foreign_pmap_batch
xen_add_to_foreign_pmap_batch_t;
DEFINE_GUEST_HANDLE_STRUCT(xen_add_to_foreign_pmap_batch_t);


> > 	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)?

well, set_mmio_p2m_entry() calls set_p2m_entry() with a couple checks.
I can add those to my function and just call set_p2m_entry too. It says
mmio, but doesn't seem to do anything mmio specific. 

thanks,
Mukesh

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

[Ian Campbell]

On Tue, 2012-04-17 at 02:53 +0100, Mukesh Rathor wrote:
> On Mon, 16 Apr 2012 14:53:22 +0100
> Ian Campbell <Ian.Campbell@citrix.com> wrote:
> 
> > > Similar to 
> > >  * XENMEM_add_to_physmap
> > 
> > Why a whole new hypercall rather than a new XENMAPSPACE for the
> > exiting XENMEM_add_to_physmap.
> 
> > 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.
> 
> Not a new hcall, just a new subcall.

Sorry, I meant why a whole new subcall instead of a new XENMAPSPACE ;-)

e.g. On ARM I did:

diff --git a/xen/include/public/memory.h b/xen/include/public/memory.h
index 86d02c8..b2adfbe 100644
--- a/xen/include/public/memory.h
+++ b/xen/include/public/memory.h
@@ -212,11 +212,13 @@ struct xen_add_to_physmap {
     uint16_t    size;
 
     /* Source mapping space. */
-#define XENMAPSPACE_shared_info 0 /* shared info page */
-#define XENMAPSPACE_grant_table 1 /* grant table page */
-#define XENMAPSPACE_gmfn        2 /* GMFN */
-#define XENMAPSPACE_gmfn_range  3 /* GMFN range */
-    unsigned int space;
+#define XENMAPSPACE_shared_info  0 /* shared info page */
+#define XENMAPSPACE_grant_table  1 /* grant table page */
+#define XENMAPSPACE_gmfn         2 /* GMFN */
+#define XENMAPSPACE_gmfn_range   3 /* GMFN range */
+#define XENMAPSPACE_gmfn_foreign 4 /* GMFN from another guest */
+    uint16_t space;
+    domid_t foreign_domid; /* IFF gmfn_foreign */
 
 #define XENMAPIDX_grant_table_status 0x80000000
 

Effectively stealing the (currently always zero) top 16 bits of the
space member for the foreign domid.


>  Forgot to include the struct:
> 
> #define XENMEM_add_foreign_to_pmap_batch      19
> struct xen_add_to_foreign_pmap_batch {
>     domid_t foreign_domid;         /* IN: gmfn belongs to this domain */
>     int count;                     /* IN/OUT: number of contigous
> frames */ unsigned long     gpfn;        /* IN: pfn in the current
> domain */ unsigned long     gmfn;        /* IN: from foreign domain */
>     int fpmap_flags;               /* future use */
> };
> typedef struct xen_add_to_foreign_pmap_batch
> xen_add_to_foreign_pmap_batch_t;
> DEFINE_GUEST_HANDLE_STRUCT(xen_add_to_foreign_pmap_batch_t);
> 
> 
> > > 	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)?
> 
> well, set_mmio_p2m_entry() calls set_p2m_entry() with a couple checks.
> I can add those to my function and just call set_p2m_entry too. It says
> mmio, but doesn't seem to do anything mmio specific. 

If that's the case perhaps you could rename it and add the type as a
parameter?

If not then I wouldn't add those checks to your function -- create a new
wrapper (set_ram_p2m_entry or whatever) with the checks.

Ian.

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

[Mukesh Rathor]

On Tue, 17 Apr 2012 10:05:28 +0100
Ian Campbell <Ian.Campbell@citrix.com> wrote:

> On Tue, 2012-04-17 at 02:53 +0100, Mukesh Rathor wrote:
> > On Mon, 16 Apr 2012 14:53:22 +0100
> > Ian Campbell <Ian.Campbell@citrix.com> wrote:
> 
> Sorry, I meant why a whole new subcall instead of a new
> XENMAPSPACE ;-)
> 
> e.g. On ARM I did:
> 
> diff --git a/xen/include/public/memory.h b/xen/include/public/memory.h
> index 86d02c8..b2adfbe 100644
> --- a/xen/include/public/memory.h
> +++ b/xen/include/public/memory.h
> @@ -212,11 +212,13 @@ struct xen_add_to_physmap {
>      uint16_t    size;
>  
>      /* Source mapping space. */
> -#define XENMAPSPACE_shared_info 0 /* shared info page */
> -#define XENMAPSPACE_grant_table 1 /* grant table page */
> -#define XENMAPSPACE_gmfn        2 /* GMFN */
> -#define XENMAPSPACE_gmfn_range  3 /* GMFN range */
> -    unsigned int space;
> +#define XENMAPSPACE_shared_info  0 /* shared info page */
> +#define XENMAPSPACE_grant_table  1 /* grant table page */
> +#define XENMAPSPACE_gmfn         2 /* GMFN */
> +#define XENMAPSPACE_gmfn_range   3 /* GMFN range */
> +#define XENMAPSPACE_gmfn_foreign 4 /* GMFN from another guest */
> +    uint16_t space;
> +    domid_t foreign_domid; /* IFF gmfn_foreign */


Well, for several reasons, I didn't use it, mainly, it doesn't allow
for count. So requests have to come in one frame at a time. Second,
none of the common code can be used by my new request, because:
   - frame is not removed from foreign domain in my case
   - i don't want to update the m2p with new info.

Anyways, I put it there for now. With ballooning change in dom0, I'm
now doing the hcall one frame at a time anyways. We can always enhance
in the future.

        case XENMAPSPACE_gmfn_foreign:
        {
            rc = _add_foreign_to_pmap_batch(&xatp);
            rcu_unlock_domain(d);
            return rc;
        }


>> static long noinline _rem_foreign_pmap_batch(XEN_GUEST_HANDLE(void)
>> arg)  

>Can't XENMEM_remove_from_physmap be used here?

Well, that calls guest_physmap_remove_page() which calls
p2m_remove_page which updates the M2P with INVALID_M2P_ENTRY. Whereas,
i just need to remove from the dom0 p2m and leave M2P as is (mfn to
domU gmfn). I could add a flag to the struct causing it to just call
set_p2m_entry() directly?

thanks,
Mukesh

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

[Ian Campbell]

On Thu, 2012-04-19 at 00:29 +0100, Mukesh Rathor wrote:
> On Tue, 17 Apr 2012 10:05:28 +0100
> Ian Campbell <Ian.Campbell@citrix.com> wrote:
> 
> > On Tue, 2012-04-17 at 02:53 +0100, Mukesh Rathor wrote:
> > > On Mon, 16 Apr 2012 14:53:22 +0100
> > > Ian Campbell <Ian.Campbell@citrix.com> wrote:
> > 
> > Sorry, I meant why a whole new subcall instead of a new
> > XENMAPSPACE ;-)
> > 
> > e.g. On ARM I did:
> > 
> > diff --git a/xen/include/public/memory.h b/xen/include/public/memory.h
> > index 86d02c8..b2adfbe 100644
> > --- a/xen/include/public/memory.h
> > +++ b/xen/include/public/memory.h
> > @@ -212,11 +212,13 @@ struct xen_add_to_physmap {
> >      uint16_t    size;
> >  
> >      /* Source mapping space. */
> > -#define XENMAPSPACE_shared_info 0 /* shared info page */
> > -#define XENMAPSPACE_grant_table 1 /* grant table page */
> > -#define XENMAPSPACE_gmfn        2 /* GMFN */
> > -#define XENMAPSPACE_gmfn_range  3 /* GMFN range */
> > -    unsigned int space;
> > +#define XENMAPSPACE_shared_info  0 /* shared info page */
> > +#define XENMAPSPACE_grant_table  1 /* grant table page */
> > +#define XENMAPSPACE_gmfn         2 /* GMFN */
> > +#define XENMAPSPACE_gmfn_range   3 /* GMFN range */
> > +#define XENMAPSPACE_gmfn_foreign 4 /* GMFN from another guest */
> > +    uint16_t space;
> > +    domid_t foreign_domid; /* IFF gmfn_foreign */
> 
> 
> Well, for several reasons, I didn't use it, mainly, it doesn't allow
> for count. So requests have to come in one frame at a time.

I've got it that way on ARM too at the moment but I was planning to make
it behave like gmfn_range and use the size parameter to map multiple at
a time (unless I've misunderstood what the gmfn_range variant does).

>  Second, none of the common code can be used by my new request,

I don't think that's an impediment to the API, XENMAPSPACE_gmfn_foreign
is in pretty much the same position.

>  because:
>    - frame is not removed from foreign domain in my case
>    - i don't want to update the m2p with new info.
> 
> Anyways, I put it there for now. With ballooning change in dom0, I'm
> now doing the hcall one frame at a time anyways. We can always enhance
> in the future.
> 
>         case XENMAPSPACE_gmfn_foreign:
>         {
>             rc = _add_foreign_to_pmap_batch(&xatp);
>             rcu_unlock_domain(d);
>             return rc;
>         }
> 
> 
> >> static long noinline _rem_foreign_pmap_batch(XEN_GUEST_HANDLE(void)
> >> arg)  
> 
> >Can't XENMEM_remove_from_physmap be used here?
> 
> Well, that calls guest_physmap_remove_page() which calls
> p2m_remove_page which updates the M2P with INVALID_M2P_ENTRY. Whereas,
> i just need to remove from the dom0 p2m and leave M2P as is (mfn to
> domU gmfn). I could add a flag to the struct causing it to just call
> set_p2m_entry() directly?

Would it be useful to track the fact that a p2m entry is foreign
somewhere? That would let you do the appropriate type of teardown etc
without relying on the tools to get it right.

Are there s/w bits available in the p2m entry itself on x86 or do we use
them all already?

Ian.

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

[Tim Deegan]

Hi, 

At 18:29 -0700 on 13 Apr (1334341792), Mukesh Rathor wrote:
> 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 agree with what Ian's already said about this.  In particular: 

 - This should use the existing XENMEM_add_to_physmap interface rather
   than having a new operation.
 - AFAICT you're using set_mmio_p2m_entry and adding a new unmap
   operation just to avoid having the m2p updated.  Since you can't rely
   on the unmap always happening through the new call (and you don't
   enforce it anywhere), it would be better to add a new p2m_type
   just for non-grant foreign mappings.  Then you can gate the m2p
   updates in the existing code on the map being normal RAM, as is
   already done for p2m_is_grant().

Apart from that: 

> struct xen_add_to_foreign_pmap_batch {
>     domid_t foreign_domid;         /* IN: gmfn belongs to this domain */
>     int count;                     /* IN/OUT: number of contigous frames */

Please only add explicitly-sized fields to the public interface.  
(I understand that there's currently no call for a compat VM to make
this call, but even so).

>     unsigned long     gpfn;        /* IN: pfn in the current domain */
>     unsigned long     gmfn;        /* IN: from foreign domain */
>     int fpmap_flags;               /* future use */
> };


> /* add frames from foreign domain to current domain physmap. Similar to 
>  * 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;
>     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);

Best not, if it's NULL. :)

>         return -EPERM;
>     }
> 
>     for (i=0; (rc == 0) && (i < pmapb.count); i++) {

This loop could do nearly 2^31 iterations; it needs to have a preemption
check to stop it locking up the hypervisor.  (If you switch to using
XENMEM_add_to_physmap, you'll get this for free.)

Also, I understand this is early code, but it will eventually have to
follow the coding style about whitespace.  There are hard tabs in a few
places below as well.  Can you train your text editor not to do that?

>         unsigned long fgmfn = pmapb.gmfn+i, gpfn = pmapb.gpfn+i;
>         mfn = mfn_x(gfn_to_mfn_query(p2m_get_hostp2m(fdom), fgmfn, &p2mt));

This will need to use the new get_gfn()/put_gfn() interfaces.

> 	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;

That's a particularly confusing way of putting it.  Also, you'll need to
keep a reference to the foreign page until this mapping goes away;
otherwise the foreign domain could die and its memory be reused while
you still have this mapping.  You should take a PGT_writeable_page
typecount, too, if the foreign domain isn't in paging_mode_external
(like how get_page_from_l1e does for PV mappings).

Cheers,

Tim.

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

[Mukesh Rathor]

On Thu, 19 Apr 2012 15:15:27 +0100
Tim Deegan <tim@xen.org> wrote:

> At 18:29 -0700 on 13 Apr (1334341792), Mukesh Rathor wrote:
>  - AFAICT you're using set_mmio_p2m_entry and adding a new unmap
>    operation just to avoid having the m2p updated.  Since you can't
> rely on the unmap always happening through the new call (and you don't
>    enforce it anywhere), it would be better to add a new p2m_type
>    just for non-grant foreign mappings.  Then you can gate the m2p
>    updates in the existing code on the map being normal RAM, as is
>    already done for p2m_is_grant().
 
Hi Tim,

The variants of get_page* are confusing me, so wanna double check with 
you.  I should be able to do something like following, right?


/* add frames from foreign domain to current domain physmap. Similar to 
 * XENMEM_add_to_physmap but the frame is foreign being mapped into current,
 * and is not removed from foreign domain.  */
static long noinline _add_foreign_to_pmap_batch(struct xen_add_to_physmap *xpmp)
{
    unsigned long rc, prev_mfn, mfn = 0;
    struct domain *fdom, *currd = current->domain;
    unsigned long fgmfn = xpmp->idx, gpfn = xpmp->gpfn;
    p2m_type_t p2mt;

    if ( (fdom = get_pg_owner(xpmp->foreign_domid)) == NULL ) {
        return -EPERM;
    }

    mfn = mfn_x(gfn_to_mfn_query(p2m_get_hostp2m(fdom), fgmfn, &p2mt));
    if ( !p2m_is_valid(p2mt) ) {
        put_pg_owner(fdom);
        return -EINVAL;
    }
    if ( (rc=get_page_and_type_from_pagenr(mfn, PGT_writable_page,fdom,0,0)) ) {
        put_pg_owner(fdom);
        return rc;
    }

    /* 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);
    }
    
    if (set_foreign_p2m_entry(p2m_get_hostp2m(currd), gpfn, mfn) == 0) {
        printk("guest_physmap_add_page failed. gpfn:%lx mfn:%lx fgmfn:%lx\n", 
             gpfn, mfn, fgmfn);
        rc = -EINVAL;      ??????????????
    }

    put_pg_owner(fdom);
    return rc;
}

/* Returns: True for success. 0 for failure */
int
set_foreign_p2m_entry(struct p2m_domain *p2m, unsigned long gfn, mfn_t mfn)
{
    int rc = 0;
    p2m_type_t ot;
    mfn_t omfn;

    if ( !paging_mode_translate(p2m->domain) )
        return 0;

    omfn = gfn_to_mfn_query(p2m, gfn, &ot);
    if (mfn_valid(omfn)) {
        gdprintk(XENLOG_ERR, "Already mapped mfn %lx at gfn:%lx\n", omfn, 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);
    audit_p2m(p2m, 1);
    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(gfn_to_mfn(p2m, gfn, &ot)));
    return rc;
}



thanks,
Mukesh

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

[Tim Deegan]

At 18:37 -0700 on 23 Apr (1335206229), Mukesh Rathor wrote:
> On Thu, 19 Apr 2012 15:15:27 +0100
> Tim Deegan <tim@xen.org> wrote:
> 
> > At 18:29 -0700 on 13 Apr (1334341792), Mukesh Rathor wrote:
> >  - AFAICT you're using set_mmio_p2m_entry and adding a new unmap
> >    operation just to avoid having the m2p updated.  Since you can't
> > rely on the unmap always happening through the new call (and you don't
> >    enforce it anywhere), it would be better to add a new p2m_type
> >    just for non-grant foreign mappings.  Then you can gate the m2p
> >    updates in the existing code on the map being normal RAM, as is
> >    already done for p2m_is_grant().
>  
> Hi Tim,
> 
> The variants of get_page* are confusing me, so wanna double check with 
> you.  I should be able to do something like following, right?
> 

[...]

>     if ( (rc=get_page_and_type_from_pagenr(mfn, PGT_writable_page,fdom,0,0)) ) {
>         put_pg_owner(fdom);
>         return rc;
>     }

Yes, but: 

 - You should use get_page_from_pagenr() if fdom is paging_mode_external()
   and reference/copy the comment in get_page_from_l1e() to explain why:

    /* Foreign mappings into guests in shadow external mode don't       
     * contribute to writeable mapping refcounts.  (This allows the
     * qemu-dm helper process in dom0 to map the domain's memory without
     * messing up the count of "real" writable mappings.) */

 - You should drop the refcount (and typecount, if you took one) if the 
   mapping fails.

 - You need to make sure that _any_ path that removes the mapping drops
   the ref/type (_after_ any TLB flushes have happened, please!)  Maybe
   the best way to do that is in ept_set_entry() for EPT and
   paging_write_p2m_entry() for NPT/shadow. 

 - You need to handle the p2m teardown path as well.  I think the best
   way to do that is to hoist the relinquish_shared_pages() loop 
   up into a new function in p2m.c, and add your put_page[_and_type] 
   calls in there. 

Cheers,

Tim.

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

[Mukesh Rathor]

On Tue, 24 Apr 2012 10:36:26 +0100
Tim Deegan <tim@xen.org> wrote:

> At 18:37 -0700 on 23 Apr (1335206229), Mukesh Rathor wrote:
> > On Thu, 19 Apr 2012 15:15:27 +0100
> > Tim Deegan <tim@xen.org> wrote:

>you still have this mapping.  You should take a PGT_writeable_page
>typecount, too, if the foreign domain isn't in paging_mode_external

Ok, I've it as:
    if (paging_mode_external(fdom)) {
        if (get_page_from_pagenr(mfn, fdom) == 0)
	    failed = 1;
    } else {
        if (get_page_and_type_from_pagenr(mfn, PGT_writable_page, fdom,0,0))
            failed = 1;
    }

But then later fails when it tries to pin the page,
MMUEXT_PIN_L4_TABLE, from the lib at:

        rc = pin_table(dom->xch, pgd_type,
	                xc_dom_p2m_host(dom, dom->pgtables_seg.pfn),
			dom->guest_domid);

(XEN) mm.c:2424:d0 Bad type (saw 7400000000000001 != exp 4000000000000000) for mfn 1bc160 (pfn 2eed)

thanks,
-m

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

[Tim Deegan]

At 16:06 -0700 on 24 Apr (1335283603), Mukesh Rathor wrote:
> On Tue, 24 Apr 2012 10:36:26 +0100
> Tim Deegan <tim@xen.org> wrote:
> 
> > At 18:37 -0700 on 23 Apr (1335206229), Mukesh Rathor wrote:
> > > On Thu, 19 Apr 2012 15:15:27 +0100
> > > Tim Deegan <tim@xen.org> wrote:
> 
> >you still have this mapping.  You should take a PGT_writeable_page
> >typecount, too, if the foreign domain isn't in paging_mode_external
> 
> Ok, I've it as:
>     if (paging_mode_external(fdom)) {
>         if (get_page_from_pagenr(mfn, fdom) == 0)
> 	    failed = 1;
>     } else {
>         if (get_page_and_type_from_pagenr(mfn, PGT_writable_page, fdom,0,0))
>             failed = 1;
>     }
> 
> But then later fails when it tries to pin the page,
> MMUEXT_PIN_L4_TABLE, from the lib at:

What's trying to pin an l4 table?  I thought your hybrid dom0 didn't use
the PV MMU.

Tim.

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

[Mukesh Rathor]

On Thu, 26 Apr 2012 10:08:47 +0100
Tim Deegan <tim@xen.org> wrote:

> At 16:06 -0700 on 24 Apr (1335283603), Mukesh Rathor wrote:
> > On Tue, 24 Apr 2012 10:36:26 +0100
> > Tim Deegan <tim@xen.org> wrote:
> > 
> > > At 18:37 -0700 on 23 Apr (1335206229), Mukesh Rathor wrote:
> > > > On Thu, 19 Apr 2012 15:15:27 +0100
> > > > Tim Deegan <tim@xen.org> wrote:
> > 
> > >you still have this mapping.  You should take a PGT_writeable_page
> > >typecount, too, if the foreign domain isn't in paging_mode_external
> > 
> > Ok, I've it as:
> >     if (paging_mode_external(fdom)) {
> >         if (get_page_from_pagenr(mfn, fdom) == 0)
> > 	    failed = 1;
> >     } else {
> >         if (get_page_and_type_from_pagenr(mfn, PGT_writable_page,
> > fdom,0,0)) failed = 1;
> >     }
> > 
> > But then later fails when it tries to pin the page,
> > MMUEXT_PIN_L4_TABLE, from the lib at:
> 
> What's trying to pin an l4 table?  I thought your hybrid dom0 didn't
> use the PV MMU.
> 
> Tim.

Right, hybrid doesn't use PV mmu. 
Here, xl/xc is building a pv guest while running on *hybrid* dom0. 
As a result, privcmd is calling this function to map
foreign mfns to pfns: 

xc_dom_x86.c:
      arch_setup_bootlate -> pin_table -> xc_mmuext_op().

thanks,
Mukesh

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

[Tim Deegan]

At 11:18 -0700 on 26 Apr (1335439128), Mukesh Rathor wrote:
> On Thu, 26 Apr 2012 10:08:47 +0100
> Tim Deegan <tim@xen.org> wrote:
> 
> > At 16:06 -0700 on 24 Apr (1335283603), Mukesh Rathor wrote:
> > > On Tue, 24 Apr 2012 10:36:26 +0100
> > > Tim Deegan <tim@xen.org> wrote:
> > > 
> > > > At 18:37 -0700 on 23 Apr (1335206229), Mukesh Rathor wrote:
> > > > > On Thu, 19 Apr 2012 15:15:27 +0100
> > > > > Tim Deegan <tim@xen.org> wrote:
> > > 
> > > >you still have this mapping.  You should take a PGT_writeable_page
> > > >typecount, too, if the foreign domain isn't in paging_mode_external
> > > 
> > > Ok, I've it as:
> > >     if (paging_mode_external(fdom)) {
> > >         if (get_page_from_pagenr(mfn, fdom) == 0)
> > > 	    failed = 1;
> > >     } else {
> > >         if (get_page_and_type_from_pagenr(mfn, PGT_writable_page,
> > > fdom,0,0)) failed = 1;
> > >     }
> > > 
> > > But then later fails when it tries to pin the page,
> > > MMUEXT_PIN_L4_TABLE, from the lib at:
> > 
> > What's trying to pin an l4 table?  I thought your hybrid dom0 didn't
> > use the PV MMU.
> > 
> > Tim.
> 
> Right, hybrid doesn't use PV mmu. 
> Here, xl/xc is building a pv guest while running on *hybrid* dom0. 

Oh, I see.  This is the domU's L4.  In that case I suspect that what's
missing is the translation from GFNs to MFNs in the domU. 

To do that, we may finally have to reintroduce the dreaded 'tell me what
MFN backs this domU GFN' call, so the dom0 tools can build proper
pagetables and p2m for the pv domU.  Or have you already taken care of
that?

Tim.

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

[Mukesh Rathor]

On Thu, 26 Apr 2012 20:57:12 +0100
Tim Deegan <tim@xen.org> wrote:

> At 11:18 -0700 on 26 Apr (1335439128), Mukesh Rathor wrote:
> > On Thu, 26 Apr 2012 10:08:47 +0100
> 
> Oh, I see.  This is the domU's L4.  In that case I suspect that what's
> missing is the translation from GFNs to MFNs in the domU. 
> 
> To do that, we may finally have to reintroduce the dreaded 'tell me
> what MFN backs this domU GFN' call, so the dom0 tools can build proper
> pagetables and p2m for the pv domU.  Or have you already taken care of
> that?


The tools get an array of the PV domU mfn's. Next to build pagetables, it
needs to map them. It does via privcmd. I generate needed pfn space (not
virtual address space) via ballooning. Next I need to map each pfn to 
the domU mfns. This is a pv guest being built. So, these are mfns' that
don't need looking up. This mapping is temporary, while the guest is being 
built. For building a hybrid domU, it doesn't need to pin the L4 as the 
hybrid guest doesn't use PV MMU. So, these mfn's that are mapped just 
need temporary page count. 

thanks,
Mukesh

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

[Tim Deegan]

At 18:56 -0700 on 26 Apr (1335466565), Mukesh Rathor wrote:
> The tools get an array of the PV domU mfn's. Next to build pagetables, it
> needs to map them. It does via privcmd. I generate needed pfn space (not
> virtual address space) via ballooning. Next I need to map each pfn to 
> the domU mfns. This is a pv guest being built. So, these are mfns' that
> don't need looking up.

I'm sorry, my brain was addled from staring at spinlock code too long.
Of course, since the domU is PV you already have MFNs. 

In that case, I can't think of any obvious reasons for the failure.
you'll just have to dig a bit further -- see exactly which entry it's
complaining about, whether it matches what the dom0 tools thought they
put there, and what you would expect to be there.

Cheers,

Tim.

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

[Andres Lagar-Cavilla]

> On Thu, 2012-04-19 at 00:29 +0100, Mukesh Rathor wrote:
>> On Tue, 17 Apr 2012 10:05:28 +0100
>> Ian Campbell <Ian.Campbell@citrix.com> wrote:
>>
>> > On Tue, 2012-04-17 at 02:53 +0100, Mukesh Rathor wrote:
>> > > On Mon, 16 Apr 2012 14:53:22 +0100
>> > > Ian Campbell <Ian.Campbell@citrix.com> wrote:
>> >
>> > Sorry, I meant why a whole new subcall instead of a new
>> > XENMAPSPACE ;-)
>> >
>> > e.g. On ARM I did:
>> >
>> > diff --git a/xen/include/public/memory.h b/xen/include/public/memory.h
>> > index 86d02c8..b2adfbe 100644
>> > --- a/xen/include/public/memory.h
>> > +++ b/xen/include/public/memory.h
>> > @@ -212,11 +212,13 @@ struct xen_add_to_physmap {
>> >      uint16_t    size;
>> >
>> >      /* Source mapping space. */
>> > -#define XENMAPSPACE_shared_info 0 /* shared info page */
>> > -#define XENMAPSPACE_grant_table 1 /* grant table page */
>> > -#define XENMAPSPACE_gmfn        2 /* GMFN */
>> > -#define XENMAPSPACE_gmfn_range  3 /* GMFN range */
>> > -    unsigned int space;
>> > +#define XENMAPSPACE_shared_info  0 /* shared info page */
>> > +#define XENMAPSPACE_grant_table  1 /* grant table page */
>> > +#define XENMAPSPACE_gmfn         2 /* GMFN */
>> > +#define XENMAPSPACE_gmfn_range   3 /* GMFN range */
>> > +#define XENMAPSPACE_gmfn_foreign 4 /* GMFN from another guest */
>> > +    uint16_t space;
>> > +    domid_t foreign_domid; /* IFF gmfn_foreign */
>>
>>
>> Well, for several reasons, I didn't use it, mainly, it doesn't allow
>> for count. So requests have to come in one frame at a time.
>
> I've got it that way on ARM too at the moment but I was planning to make
> it behave like gmfn_range and use the size parameter to map multiple at
> a time (unless I've misunderstood what the gmfn_range variant does).
>
>>  Second, none of the common code can be used by my new request,
>
> I don't think that's an impediment to the API, XENMAPSPACE_gmfn_foreign
> is in pretty much the same position.
>
>>  because:
>>    - frame is not removed from foreign domain in my case
>>    - i don't want to update the m2p with new info.
>>
>> Anyways, I put it there for now. With ballooning change in dom0, I'm
>> now doing the hcall one frame at a time anyways. We can always enhance
>> in the future.
>>
>>         case XENMAPSPACE_gmfn_foreign:
>>         {
>>             rc = _add_foreign_to_pmap_batch(&xatp);
>>             rcu_unlock_domain(d);
>>             return rc;
>>         }
>>
>>
>> >> static long noinline _rem_foreign_pmap_batch(XEN_GUEST_HANDLE(void)
>> >> arg)
>>
>> >Can't XENMEM_remove_from_physmap be used here?
>>
>> Well, that calls guest_physmap_remove_page() which calls
>> p2m_remove_page which updates the M2P with INVALID_M2P_ENTRY. Whereas,
>> i just need to remove from the dom0 p2m and leave M2P as is (mfn to
>> domU gmfn). I could add a flag to the struct causing it to just call
>> set_p2m_entry() directly?
>
> Would it be useful to track the fact that a p2m entry is foreign
> somewhere? That would let you do the appropriate type of teardown etc
> without relying on the tools to get it right.
>
> Are there s/w bits available in the p2m entry itself on x86 or do we use
> them all already?

A few still available. We're at 14 types and EPT uses 4 bits.

NPT has even more room, unless it's sharing its p2m tables with iommu
tables -- in which case it can't really do fancy p2m typing. That would be
an unwelcome side-effect for a hybrid dom0 on amd, but I'm not aware of
how bad it would be.

The more general value here is to allow hvm backends arbitrary mapping
capabilities, correct? Right now all hvm backends/driver domains are
supposed to use grant table code (and there are p2m types for those).

A domain builder/checkpointing hvm domain would require equivalent
capabilities to those being discussed here, if I'm not mistaken.

So, a p2m_foreign_map would be the way to go.

My two cents
Andres


>
> Ian.