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

[Tim Deegan <tim@xen.org>]

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.