Re: [V7 PATCH 3/7] pvh dom0: implement XENMEM_add_to_physmap_range for x86

[Ian Campbell <Ian.Campbell@citrix.com>]

On Tue, 2013-12-17 at 14:36 +0000, Jan Beulich wrote:
> >>> On 17.12.13 at 14:59, Ian Campbell <Ian.Campbell@citrix.com> wrote:
> > On Tue, 2013-12-17 at 13:07 +0000, Jan Beulich wrote:
> >> >>> On 17.12.13 at 03:38, Mukesh Rathor <mukesh.rathor@oracle.com> wrote:
> >> > --- a/xen/arch/x86/mm.c
> >> > +++ b/xen/arch/x86/mm.c
> >> > @@ -4675,6 +4675,39 @@ static int xenmem_add_to_physmap(struct domain *d,
> >> >      return xenmem_add_to_physmap_once(d, xatp);
> >> >  }
> >> >  
> >> > +static int xenmem_add_to_physmap_range(struct domain *d,
> >> > +                                       struct xen_add_to_physmap_range 
> > *xatpr)
> >> > +{
> >> > +    /* Process entries in reverse order to allow continuations */
> >> > +    while ( xatpr->size > 0 )
> >> > +    {
> >> > +        int rc;
> >> > +        xen_ulong_t idx;
> >> > +        xen_pfn_t gpfn;
> >> > +        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) )
> >> > +            return -EFAULT;
> >> > +
> >> > +        xatp.space = xatpr->space;
> >> > +        xatp.idx = idx;
> >> > +        xatp.gpfn = gpfn;
> >> > +        rc = xenmem_add_to_physmap_once(d, &xatp);
> >> > +
> >> > +        if ( copy_to_guest_offset(xatpr->errs, xatpr->size-1, &rc, 1) )
> >> > +            return -EFAULT;
> >> > +
> >> > +        xatpr->size--;
> >> > +
> >> > +        /* Check for continuation if it's not the last interation */
> >> > +        if ( xatpr->size > 0 && hypercall_preempt_check() )
> >> > +            return -EAGAIN;
> >> > +    }
> >> > +
> >> > +    return 0;
> >> > +}
> >> 
> >> Now that I started looking into creating the compat wrapper for this,
> >> I realize that processing this request backwards is wrong: The effect
> >> of the entire hypercall can end up being different between forward
> >> and reverse processing, if an index or gpfn happens to be twice in
> >> the set. While that's perhaps not the usual thing to do, you never
> >> know how a creative user of the interface may find it useful to do so.
> > 
> > Hrm, the ARM code does things this way as well. But you are of course
> > right.
> > 
> > We could change the code but we could also tighten the interface
> > requirements, either by explicit specifying that the range is handled in
> > reverse order or by mandating that index/gpfn must not be repeated
> > (whether or not we actively try and detect such cases).
> 
> Specifying that this gets processed backwards would be, well,
> backwards. Requiring no duplicates (or else getting undefined
> behavior) would be possible. But processing the operation in the
> conventional order doesn't seem all that hard.

The reason I thought it would be tricky was finding somewhere to stash
the progress over the continuation. Do you have a cunning plan?

In principal the ABI on ARM is still subject to change (until 4.4) and
on x86 it does exist yet but TBH I'd rather find a way not too, either a
clever continuation trick or adding restrictions to the call.

Ian.