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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 17.12.13 at 14:59, Ian Campbell <Ian.Campbell@xxxxxxxxxx> wrote:
> On Tue, 2013-12-17 at 13:07 +0000, Jan Beulich wrote:
>> >>> On 17.12.13 at 03:38, Mukesh Rathor <mukesh.rathor@xxxxxxxxxx> 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.

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel