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

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

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.