Re: [Xen-devel] [V1 PATCH] dom0 pvh: map foreign pfns in our p2m for toolstack

[Mukesh Rathor <mukesh.rathor@oracle.com>]

On Tue, 27 May 2014 11:59:26 +0100
David Vrabel <david.vrabel@citrix.com> wrote:

> On 27/05/14 11:43, Roger Pau Monné wrote:
> > On 24/05/14 03:33, Mukesh Rathor wrote:
> >> When running as dom0 in pvh mode, foreign pfns that are accessed
> >> must be added to our p2m which is managed by xen. This is done via
> >> XENMEM_add_to_physmap_range hypercall. This is needed for toolstack
> >> building guests and mapping guest memory, xentrace mapping xen
> >> pages, etc..
> 
> Thanks.
> 
> Applied to devel/for-linus-3.16, but see comments below.
> 
> >> +static int xlate_add_to_p2m(unsigned long lpfn, unsigned long
> >> fgmfn,
> >> +			    unsigned int domid)
> 
> The preferred abbreviation is GFN not GMFN.  I fixed this up.
> 
> >> +{
> >> +	int rc, err = 0;
> >> +	xen_pfn_t gpfn = lpfn;
> >> +	xen_ulong_t idx = fgmfn;
> >> +
> >> +	struct xen_add_to_physmap_range xatp = {
> >> +		.domid = DOMID_SELF,
> >> +		.foreign_domid = domid,
> >> +		.size = 1,
> >> +		.space = XENMAPSPACE_gmfn_foreign,
> >> +	};
> >> +	set_xen_guest_handle(xatp.idxs, &idx);
> >> +	set_xen_guest_handle(xatp.gpfns, &gpfn);
> >> +	set_xen_guest_handle(xatp.errs, &err);
> >> +
> >> +	rc = HYPERVISOR_memory_op(XENMEM_add_to_physmap_range,
> >> &xatp);
> >> +	return rc;
> > 
> > Thanks for the patches, I see two problems with this approach, the
> > first one is that you are completely ignoring the error in the
> > variable "err", which means that you can end up with a pfn that
> > Linux thinks it's valid, but it's not mapped to any mfn, so when
> > you try to access it you will trigger the vioapic crash.
> 
> I spotted this and fixed this up by adding:
> 
> +       if (rc < 0)
> +               return rc;
> +       return err;

Thanks a lot.

> > The second one is that this seems extremely inefficient, you are
> > issuing one hypercall for each memory page, when you could instead
> > batch all the pages into a single hypercall and map them in one
> > shot.
> 
> I agree, but the 3.16 merge window is nearly here so I've applied it
> as-is.  Note that the privcmd driver calls this function once per
> page, so the lack of batching doesn't really hurt here.

Thanks again, pleasure working with maintainer like you!

Mukesh