From mboxrd@z Thu Jan 1 00:00:00 1970 From: Tim Deegan Subject: Re: [hybrid]: code review for function mapping pfn to foreign mfn Date: Thu, 19 Apr 2012 15:15:27 +0100 Message-ID: <20120419141527.GB23663@ocelot.phlegethon.org> References: <20120413182952.504e2775@mantra.us.oracle.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Content-Disposition: inline In-Reply-To: <20120413182952.504e2775@mantra.us.oracle.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Mukesh Rathor Cc: Keir Fraser , "Xen-devel@lists.xensource.com" , Ian Campbell , "stefano.stabellini@eu.citrix.com" List-Id: xen-devel@lists.xenproject.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.