Re: [PATCH 02/20] PVH xen: add XENMEM_add_to_physmap_range

["Jan Beulich" <JBeulich@suse.com>]

>>> On 16.05.13 at 01:05, Mukesh Rathor <mukesh.rathor@oracle.com> wrote:
> On Wed, 15 May 2013 10:58:43 +0100
> "Jan Beulich" <JBeulich@suse.com> wrote:
> 
>> >>> On 15.05.13 at 02:52, Mukesh Rathor <mukesh.rathor@oracle.com>
>> >>> wrote:
>> > --- a/xen/arch/x86/mm.c
>> > +++ b/xen/arch/x86/mm.c
>> > @@ -4519,7 +4519,8 @@ static int handle_iomem_range(unsigned long
>> > s, unsigned long e, void *p) 
>> >  static int xenmem_add_to_physmap_once(
>> >      struct domain *d,
>> > -    const struct xen_add_to_physmap *xatp)
>> > +    const struct xen_add_to_physmap *xatp,
>> > +    domid_t foreign_domid)
>> >  {
>> >      struct page_info *page = NULL;
>> >      unsigned long gfn = 0; /* gcc ... */
>> > @@ -4646,7 +4647,7 @@ static int xenmem_add_to_physmap(struct
>> > domain *d,
>> 
>> I know I said this before: This patch can't be complete, or else the
>> new function parameter would actually get used. With the way
>> things are, if this patch gets applied, a user of the new XENMEM_
>> sub-op would not get the expected behavior.
>> 
> 
> No, the new foreign_domid parameter is meaningful for only the 
> XENMAPSPACE_gmfn_foreign OP which is defined in patch 0018. So we 
> should be OK here.

Mukesh, please. Go look at your own patch again: It adds handling
of XENMEM_add_to_physmap_range to arch_memory_op(), calling
xenmem_add_to_physmap_range(), which in turn calls
xenmem_add_to_physmap_once() passing xatpr->foreign_domid
as the last argument. I don't see anywhere in the patch prevention
of that execution flow for an arbitrary guest. If I'm overlooking
something, please point me to it.

Furthermore, now that you forced me to look at that code yet
another time,

>+        xen_ulong_t idx;
>+        xen_pfn_t gpfn;

Pointless variables, ...

>+        struct xen_add_to_physmap xatp;
>+
>+        if ( copy_from_guest_offset(&idx, xatpr->idxs, xatpr->size-1, 1)  ||
>+             copy_from_guest_offset(&gpfn, xatpr->gpfns, xatpr->size-1, 1) )

... you can read directly into the respective xatp fields here.

>+        {
>+            return -EFAULT;
>+        }

Pointless (and inconsistent with code further down in this same
function) braces.

>+
>+        xatp.space = xatpr->space;
>+        xatp.idx = idx;
>+        xatp.gpfn = gpfn;
>+        rc = xenmem_add_to_physmap_once(d, &xatp, xatpr->foreign_domid);

xatp has a domid field - why don't you use that instead of adding a
new function parameter? I'm unclear anyway why two domain IDs
are useful here at all - Ian, Stefano, for one I still can't spot any use
of xen_add_to_physmap_range in tools and qemu (and hence can't
see a clear use case), and then I doubt there's real use for one
domain mapping GFNs from a second domain into a third one. If it's
really dead code that got added here, shouldn't we drop it now
rather than releasing 4.3 with it baked into the interface?

Jan