From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ian Campbell Subject: Re: [hybrid]: code review for function mapping pfn to foreign mfn Date: Mon, 16 Apr 2012 14:53:22 +0100 Message-ID: <1334584402.14560.184.camel@zakaz.uk.xensource.com> References: <20120413182952.504e2775@mantra.us.oracle.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: 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: "Xen-devel@lists.xensource.com" , Tim Deegan , Keir Fraser , Stefano Stabellini List-Id: xen-devel@lists.xenproject.org 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; > } >